Add git:// protocol check

[ffaf1]

Include the following checklist in your PR:

• Patches conform to the coding conventions.
• Any changes that could be relevant to users have been recorded in the changelog.
• The documentation has been updated, if necessary.
• Manual QA notes have been included.
• Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)

Manual QA

1. Go to a local cabal package that has a source-repository stanza.
2. In location: substitute https:// (or what you have) with git://.
3. New cabal should display a git:// is not secure warning.

[ffaf1]

Review ready.

Note:

1. “Hackage would reject this package”; and
2. the suggestion to use https:// or ssh:// instead (should we push something else?).

[ulysses4ever]

I'd expect it to say something about "popular platforms, like GitHub, don't support it" but maybe it's too much detail...

[Kleidukos]

I'd expect it to say something about "popular platforms, like GitHub, don't support it" but maybe it's too much detail...

We can absolutely check that it's git://github.com and display something specific to GitHub

[ffaf1]

We can absolutely check that it's git://github.com and display something specific to GitHub.

I left the warning vague (“Furthermore, popular forges like GitHub do not support it.”) and general (displayed always) because I other platforms choke on git://, e.g. GHC GitLab.

Shall the message be more generic? Shall we tailor the “Furthermore” to specific forges?

[ulysses4ever]

I think the current version is the perfect balance between specific and general. I am not a fan of making the code more complex than it needs to be to analyze the URI and tailor the message to the specific platform dynamically.

[mpickering]

There are many packages on hackage which use git:// for the source repo.

Perhaps it's worth considering a softer migration so that uploads of new versions of these packages are not blocked? I'm not sure on this one but wanted to point out it could affect a lot of users.

[ffaf1]

Perhaps it's worth considering a softer migration so that uploads of new versions of these packages are not blocked? I'm not sure on this one but wanted to point out it could affect a lot of users.

I believe with a changelog entry and the possibility of running cabal check locally, we are fine from a UX perspective.

Also let's keep in mind that first this will land in cabal, and after a while in Hackage, so it will take some time before Hackage refuse such packages, if users run cabal check now and then — they should! — they will get the message.

From the relevant Git Book section (which Brandon provided), there seems to be no sensible reason to still use git://, so downgrading this to a mere warning feels wrong.

[mpickering]

Perhaps it's worth considering a softer migration so that uploads of new versions of these packages are not blocked? I'm not sure on this one but wanted to point out it could affect a lot of users.

I believe with a changelog entry and the possibility of running cabal check locally, we are fine from a UX perspective.

Also let's keep in mind that first this will land in cabal, and after a while in Hackage, so it will take some time before Hackage refuse such packages, if users run cabal check now and then — they should! — they will get the message.

From the relevant Git Book section (which Brandon provided), there seems to be no sensible reason to still use git://, so downgrading this to a mere warning feels wrong.

Sounds good!

[mouse07410]

Please remind me - what is the problem this PR is supposed to fix?

And why is it wrong to reduce the level to "warning"?

[mpickering]

@mouse07410 The issue which was reported is that many of the source repository links on hackage are dead as they use the git:// protocol with github links. This means that cabal get -s doesn't work reliably.

So this PR doesn't fix that issue directly but would achieve something similar (getting people to update their links to https variants).

[mouse07410]

A lot of my repos are addressed via ssh://github.com/...... Is this PR going to force them to https://?

[ffaf1]

A lot of my repos are addressed via ssh://github.com/...... Is this PR going to force them to https://?

No, just to avoid git://….

[Kleidukos]

@mouse07410 On the contrary you are using the SSH protocol which is good! Could you perhaps take a look at the suggested message in this PR and tell us if it could have been phrased better?

[geekosaur]

Specifically, any URL using git://github.com/… has failed to connect since 2021, so is useless. [ssh:][email protected]/… is the GitHub-recommended alternative. (Note that you can't push to https: URLs; you must use ssh.)

[mouse07410]

@mouse07410 On the contrary you are using the SSH protocol which is good! Could you perhaps take a look at the suggested message in this PR and tell us if it could have been phrased better?

Frankly, yes - especially since with this PR Cabal will error out such packages. I'll post a suggestion here, or in a Review when I get to my desktop computer tomorrow or day after.

[geekosaur]

I'm confused. This check is fine with ssh; it's the old git server protocol that is rejected, which hasn't been accepted by GitHub in 3 years.

[ffaf1]

@mouse07410 I did not remove the merge me label and now this is merged.

If you could illustrate what your problems are with the wording (or the logic), I can quickly write a fixup patch.

To be clear, if you have this stanza in your .cabal

source-repository head
    type:     git
    location: ssh://example.org/prova

cabal check will not complain. If you have this stanza in your .cabal:

source-repository head
    type:     git
    location: git://example.org/prova

cabal check will return will output:

Error: [git-protocol] Cloning over git:// might lead to an arbitrary code
execution vulnerability. Furthermore, popular forges like GitHub do not
support it. Use https:// or ssh:// instead.
Error: Hackage would reject this package.

Apologies for the messing-up, I was under the impression that further comments reset the 48-hours delay window for automatic merging.