Fully anchor package duplicate exception regexes

What does this MR do and why?

Namespace::PackageSetting.duplicates_allowed? and the Terraform module CreatePackageService#duplicates_not_allowed? anchor the group's duplicate exception regex as "\\A#{regex}\\z". With a top-level alternation the anchors bind only to the first and the last alternative, so 9\.9\.9|8\.8\.8 matches 9.9.9-rc and 7.8.8.8. With duplicates not allowed, a duplicate of such a version is accepted; with duplicates allowed, it is refused. This affects Maven, generic, NuGet, RubyGems and Terraform module packages.

This MR adds Gitlab::UntrustedRegexp#full_match?, which calls RE2::Regexp#full_match? and returns false for nil as match? does, and uses it in both checks. Wrapping the setting as "\\A(?:#{regex})\\z" would not do: RE2 reads an unclosed \Q as quoting the rest of the pattern, so a valid setting such as \Q1.0.0-beta would no longer compile and the check would raise.

Behavior change: each alternative of a regex with a top-level | now has to match the whole name or version, as a regex without | already does. A setting that ends inside an unclosed \Q, which never matched, now matches the quoted text. No API, GraphQL or database change.

lib/gitlab/untrusted_regexp.rb is in the [Application Security] section of .gitlab/CODEOWNERS, so this MR also needs an approval from that group.

References

Closes #636345 (closed)

  • !165467 (merged) wrote the current form of both checks.
  • !263936 (merged) moves Gitlab::UntrustedRegexp into gems/gitlab-utils. If it merges first, I will rebase and move full_match? and its specs into the gem.

I found this while working on gitlab-mcp-server.

Screenshots or screen recordings

Not applicable: backend change with no UI.

How to set up and validate locally

  1. In a group, turn off Allow duplicates for generic packages and set Exceptions to 9\.9\.9|8\.8\.8.

  2. In a project of that group, upload the same file twice as version 9.9.9-rc:

    curl --request PUT --upload-file f.txt --header "PRIVATE-TOKEN: <your_access_token>" \
      --url "http://gdk.test:3000/api/v4/projects/<project_id>/packages/generic/dup/9.9.9-rc/f.txt"
  3. On master both uploads answer 201 Created. On this branch, under a new package name, the second answers 400 Bad Request, as it does on master with 9\.9\.9 alone.

Specs I ran (Ruby 3.3.11, on master 331323ee)
  • The three changed specs with master's three code files: 460 examples, 43 failures: 42 new examples (8 full_match? examples, 30 rows of the duplicates_allowed? table across the five formats, 4 Terraform module examples) and the Terraform module flake below.
  • On this branch, those specs plus the generic, Maven, NuGet, RubyGems and package settings service specs: 667 examples, 0 failures. The generic, Terraform module and Maven request specs with -e duplicate: 19 examples, 0 failures.
  • The Terraform module spec's creating a package and its metadatum example is flaky in contexts where the regex does not decide the result. It failed once in an earlier run of the set above and also fails on master's own spec and code (1 of 5 seeds), as in #565713 (closed) and #576471 (moved).
  • RuboCop on the six files: no offenses. The commit linter and Danger's commit and title linters: no problems.
  • Left to the pipeline: the NuGet and RubyGems request specs and the rest of the three request specs.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by José M. Requena Plens

Merge request reports

Loading
Loading