Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Audit invalid versions #15936

Merged
merged 1 commit into from
Sep 5, 2023
Merged

Audit invalid versions #15936

merged 1 commit into from
Sep 5, 2023

Conversation

MikeMcQuaid
Copy link
Member

@MikeMcQuaid MikeMcQuaid commented Sep 2, 2023

We have a bunch of versions we've been meaning to adjust to not use invalid GitHub Packages characters for a while. Let's audit for them and plan for deprecating their use in future.

Companion to Homebrew/homebrew-core#141202

@Bo98
Copy link
Member

Bo98 commented Sep 2, 2023

A problem here is auto version parsing should be updated too, but that comes with a risk of breaking existing formulae immediately.

@Bo98
Copy link
Member

Bo98 commented Sep 2, 2023

This "bad" list is very much homebrew-core specific.

Broadly, it probably should be a whitelist like [a-zA-Z0-9_][a-zA-Z0-9._-]{0,127}

@MikeMcQuaid
Copy link
Member Author

A problem here is auto version parsing should be updated too, but that comes with a risk of breaking existing formulae immediately.

Ideally yes but I think it's fine to just have the audit require a specific version.

This "bad" list is very much homebrew-core specific.

Sure but: I don't think we need to handle more than that?

If we fix all the issues in homebrew/core and have CI in both places to prevent it regressing: that seems sufficient.

@MikeMcQuaid MikeMcQuaid marked this pull request as draft September 3, 2023 01:32
@Bo98
Copy link
Member

Bo98 commented Sep 4, 2023

Sure but: I don't think we need to handle more than that?

If we fix all the issues in homebrew/core and have CI in both places to prevent it regressing: that seems sufficient.

To be clear: the whitelist I suggested shouldn't break more things (i.e. there should be nothing not already covered by your PR that is broken - but I haven't checked). It's more to cover future things.

It's what I believe to be the correct regex based on https://github.com/opencontainers/distribution-spec/blob/main/spec.md (though GHP's implementation could vary).

Can be a separate PR though if preferred.

@MikeMcQuaid
Copy link
Member Author

To be clear: the whitelist I suggested shouldn't break more things (i.e. there should be nothing not already covered by your PR that is broken - but I haven't checked). It's more to cover future things.

It's what I believe to be the correct regex based on https://github.com/opencontainers/distribution-spec/blob/main/spec.md (though GHP's implementation could vary).

Ah, I see, sorry for misunderstanding! This is great, thanks for digging it up. Having updated the regexes accordingly.

We have a bunch of versions we've been meaning to adjust to not use
invalid GitHub Packages characters for a while. Let's audit for them
and plan for deprecating their use in future.
@MikeMcQuaid MikeMcQuaid marked this pull request as ready for review September 5, 2023 20:43
@MikeMcQuaid MikeMcQuaid added the critical Critical change which should be shipped as soon as possible. label Sep 5, 2023
@MikeMcQuaid MikeMcQuaid merged commit 9677a9a into Homebrew:master Sep 5, 2023
25 checks passed
@MikeMcQuaid MikeMcQuaid deleted the audit_invalid_versions branch September 5, 2023 20:58
@scpeters
Copy link
Member

scpeters commented Sep 6, 2023

we are using ~ in version strings in the osrf/simulation tap for formulae that are currently in a prerelease version:

these formulae are now failing the audit. I will see if changing the ~ characters to - allows these formulae to pass this audit, while still following the same version comparison scheme (since 1.0.0~pre1 < 1.0.0, then hopefully also 1.0.0-pre1 < 1.0.0)

@scpeters
Copy link
Member

scpeters commented Sep 6, 2023

we are using ~ in version strings in the osrf/simulation tap for formulae that are currently in a prerelease version:

these formulae are now failing the audit. I will see if changing the ~ characters to - allows these formulae to pass this audit, while still following the same version comparison scheme (since 1.0.0~pre1 < 1.0.0, then hopefully also 1.0.0-pre1 < 1.0.0)

unfortunately, changing our explicit version strings will make our bottles inaccessible (since the version is encoded in the bottle filename) and possibly require them to be rebuilt.

can this be considered an audit requirement for homebrew/core only for the time being? I can work on complying with this for future pre-releases and bottles, but it's causing audit failures for a bunch of our current formulae at the moment, and I don't have a trivial way of fixing them

@scpeters
Copy link
Member

scpeters commented Sep 6, 2023

I've proposed relaxing the new audit for 3rd-party taps in #15972

@github-actions github-actions bot added the outdated PR was locked due to age label Oct 7, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants