Skip to content

Fix: [for cherry-picking] Removed the allowlist filter pipeline's fail- - #23

Closed
qodo-code-review[bot] wants to merge 1 commit into
ci/1747-public-repo-guard-body-scanfrom
fix/remediation-d4809750-0d22cb
Closed

Fix: [for cherry-picking] Removed the allowlist filter pipeline's fail-#23
qodo-code-review[bot] wants to merge 1 commit into
ci/1747-public-repo-guard-body-scanfrom
fix/remediation-d4809750-0d22cb

Conversation

@qodo-code-review

@qodo-code-review qodo-code-review Bot commented Aug 6, 2026

Copy link
Copy Markdown

Fixed Findings

  • Fail closed on allowlist filtering errors

Automated fix from agentic review of #22

Qodo Logo


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Open in Devin Review

Review in cubic

Note

Fix allowlist filter in check to fail closed on ripgrep errors

Previously, the allowlist ripgrep pipeline in body-policy.sh used || true, silently swallowing errors. Now, the exit code is captured and any ripgrep exit code ≥ 2 emits a GitHub Actions ::error annotation and exits with status 2. Exit codes 0 and 1 (match/no-match) continue to behave as before.

Macroscope summarized a271dd8.

@greptile-apps

greptile-apps Bot commented Aug 6, 2026

Copy link
Copy Markdown

PR author is in the excluded authors list.

@macroscopeapp

macroscopeapp Bot commented Aug 6, 2026

Copy link
Copy Markdown

Approvability

Verdict: Needs human review

Unable to check for correctness in a271dd8. Changes to security-related scripts (public-repo-guard) that modify error handling behavior should be reviewed by the designated code owners (wave-av/core-team).

You can customize Macroscope's approvability policy. Learn more.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 3 potential issues.

Open in Devin Review

Comment on lines 53 to +60
matches="$(printf '%s' "$raw" \
| rg -vN -- 'guard:allow[[:space:]]+[^[:space:]]' \
| rg -vNiP -- "$ABOUT_THE_CONTROL" || true)"
| rg -vNiP -- "$ABOUT_THE_CONTROL")"
filter_rc=$?
if (( filter_rc >= 2 )); then
echo "::error title=public-repo-guard ($name)::ripgrep failed (exit $filter_rc) while applying allowlists — failing closed."
exit 2
fi

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Scanner failure in the first allowlist step can still go unnoticed and let text pass unchecked

The new failure check reads only the combined result of the two-step allowlist filter (filter_rc=$? at scripts/public-repo-guard/body-policy.sh:56) instead of each step's own result, so a crash in the first step is hidden by the second step's normal "nothing left" result and the text is declared clean.
Impact: If part of the text scanner breaks, the gate can silently report success instead of blocking, allowing leaked internal text to be published.

How bash pipefail masks the earlier command's exit code

With set -o pipefail, the pipeline status is the exit status of the rightmost command that exited non-zero, not the maximum. In the pipeline at scripts/public-repo-guard/body-policy.sh:53-55, if the first rg -vN (guard:allow filter) fails with exit 2, it emits no output; the second rg -vNiP then reads empty input and exits 1 (no lines selected). The pipeline status is therefore 1, filter_rc is 1, the >= 2 guard at line 57 does not trigger, matches is empty, and check returns 0 as clean — exactly the fail-open the change intends to prevent. Only a failure in the last stage is currently caught. Using ${PIPESTATUS[@]} (captured immediately after the assignment) and failing if any element is >= 2 closes the gap.

Suggested change
matches="$(printf '%s' "$raw" \
| rg -vN -- 'guard:allow[[:space:]]+[^[:space:]]' \
| rg -vNiP -- "$ABOUT_THE_CONTROL" || true)"
| rg -vNiP -- "$ABOUT_THE_CONTROL")"
filter_rc=$?
if (( filter_rc >= 2 )); then
echo "::error title=public-repo-guard ($name)::ripgrep failed (exit $filter_rc) while applying allowlists — failing closed."
exit 2
fi
matches="$(printf '%s' "$raw" \
| rg -vN -- 'guard:allow[[:space:]]+[^[:space:]]' \
| rg -vNiP -- "$ABOUT_THE_CONTROL")"
local -a filter_st=( "${PIPESTATUS[@]}" )
for filter_rc in "${filter_st[@]}"; do
if (( filter_rc >= 2 )); then
echo "::error title=public-repo-guard ($name)::ripgrep failed (exit $filter_rc) while applying allowlists — failing closed."
exit 2
fi
done
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +55 to +60
| rg -vNiP -- "$ABOUT_THE_CONTROL")"
filter_rc=$?
if (( filter_rc >= 2 )); then
echo "::error title=public-repo-guard ($name)::ripgrep failed (exit $filter_rc) while applying allowlists — failing closed."
exit 2
fi

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Sibling scanner content-policy.sh still swallows allowlist-filter errors

The same fail-open pattern the PR fixes here still exists in the companion script: scripts/public-repo-guard/content-policy.sh:60 uses grep -vE ... || true for its guard:allow filter, so a grep failure there yields empty matches and a clean result. Worth applying the same treatment for consistency between the two gates.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines 53 to +60
matches="$(printf '%s' "$raw" \
| rg -vN -- 'guard:allow[[:space:]]+[^[:space:]]' \
| rg -vNiP -- "$ABOUT_THE_CONTROL" || true)"
| rg -vNiP -- "$ABOUT_THE_CONTROL")"
filter_rc=$?
if (( filter_rc >= 2 )); then
echo "::error title=public-repo-guard ($name)::ripgrep failed (exit $filter_rc) while applying allowlists — failing closed."
exit 2
fi

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟨 Leak gate can silently pass when the first allowlist filter errors

The new fail-closed check inspects only the pipeline's aggregate status (filter_rc=$? at scripts/public-repo-guard/body-policy.sh:56). Under bash pipefail the status is the rightmost non-zero exit, so an error (exit 2) in the first rg -vN allowlist filter (scripts/public-repo-guard/body-policy.sh:54) is masked by the second filter's normal exit 1, leaving matches empty and the body reported as clean. This is a fail-open in a security gate whose purpose is to block secrets and internal detail from being published.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants