fix(controls): trustedUrls bare-hostname matching and partial semver false positives#215
Open
12122J wants to merge 1 commit into
Open
fix(controls): trustedUrls bare-hostname matching and partial semver false positives#21512122J wants to merge 1 commit into
12122J wants to merge 1 commit into
Conversation
…false positives Fixes two regression bugs in Rego policies: 1. ISSUE-411 (pipelineMustNotExecuteUnverifiedScripts): trustedUrls patterns were never matched against bare-hostname URLs (e.g. `curl -sL firebase.tools | bash`). The extractor only found https?:// URLs, so any curl/wget command using a bare hostname bypassed the allowlist entirely and was always reported. Fixed by adding two additional _line_is_trusted rules that extract bare hostname tokens and try matching them against each pattern both directly and with an https:// prefix. 2. ISSUE-403 (includes_outdated): a component ref like "@1" or "@1.2" was incorrectly flagged as outdated when the latest version was "1.2.4". In GitLab component semantics "@1" tracks the latest 1.x.x, so it is never out of date within that major prefix. Fixed by adding a _ref_is_partial_semver_prefix helper that checks whether ref is a proper version prefix of current and suppresses the finding when it is. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
12122J
requested review from
Joseph94m,
Totara-thib,
coconux3 and
thomasboni
as code owners
Contributor
|
Hello @12122J , thanks for this second PR :) We will review soon too |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes two independent Rego policy bugs:
Fix 1 — ISSUE-411
trustedUrlsbare-hostname matching (#214)pipelineMustNotExecuteUnverifiedScripts.trustedUrlshad no effect on curl/wget commands that used a bare hostname without anhttp(s)://scheme, such as:curl -sL firebase.tools | bashThe extractor regex
https?://[^\s|;)'"]+only captured schemed URLs, so bare-hostname tokens were never compared againsttrustedUrlspatterns. The finding was always emitted regardless of the allowlist.Root cause:
_line_is_trustedextracted onlyhttps?://URLs; bare hostnames likefirebase.toolswere invisible to the matcher.Fix: Two additional
_line_is_trustedrules extract bare hostname tokens (requiring at least one dot, nohttpprefix) and try matching them against each pattern — once directly ("firebase.tools") and once normalised tohttps://("https://firebase.tools").Fix 2 — ISSUE-403 partial semver false positive (#157)
A component ref like
@1or@1.2was incorrectly flagged as outdated when the latest version was1.2.4. In GitLab component versioning,@1means "latest 1.x.x" — it is always current within that major prefix and should never trigger ISSUE-403.Root cause: The deny rule compared
inc.ref != inc.currentdirectly, so"1" != "1.2.4"always fired.Fix: New
_ref_is_partial_semver_prefixhelper checks whetherrefis a proper version prefix ofcurrent(e.g."1"is a prefix of"1.2.4","1.2"is a prefix of"1.2.4"). The deny rule now skips the finding when the prefix check passes.Test plan
TestIssue411_UnverifiedScripts_TrustedBareHostname— new regression test, bare hostname in trustedUrls does not fire; untrusted URL still firesTestIssue403_IncludesOutdated_PartialSemver— new regression test covers@1,@v1,@1.2(no fire), and@1 / current=2.0.0(fires)TestIssue411andTestIssue403cases continue to passmake testpasses cleanAI disclosure
This PR was written with Claude Code (claude-sonnet-4-6). All code was reviewed by the submitter; tests were run and confirmed passing before submission.
Closes #214
Closes #157