Conversation
Two commits in this same PR chain drifted apart: d42addb added a workflow guard requiring '### ' in the checker's stdout to treat exit code 1 as a normal detection, but 5efd794 rewrote the checker in Python, which prints "PG version issue: ..." with no '###' anywhere. As a result the workflow hard-fails (exit 2, raw stdout dump) instead of posting the intended PR comment on every genuine detection - the one case this tooling exists for. Verified by running the workflow's own guard logic against the checker's real "issue found" output before and after this fix. Same fix applied to the PR comment step's JS output filter, which keyed on the same stale '###' marker. Also decode git subprocess output with errors='replace' instead of the default strict UTF-8, since a single non-ASCII byte (e.g. a smart quote in a comment) anywhere in a touched .c/.h file would otherwise raise an uncaught UnicodeDecodeError and hard-fail the check with a message that doesn't name the offending file. Added a regression fixture that runs the workflow's actual guard logic (read from the workflow YAML, not duplicated) against a real detected issue, so the two can't silently diverge again.
git diff --name-only collapses renames to the new path, so a persisted struct whose header was renamed and its layout changed never loaded its old definition and was silently excluded from the version-bump check. Use --no-renames so both old and new paths are collected, and add a rename regression fixture.
The pull_request workflow's GITHUB_TOKEN is read-only for fork-originated runs, so its github-script comment step failed with 403 and hard-failed the job instead of warning. Move comment posting to a privileged workflow_run consumer: the check workflow uploads the checker output and PR number as artifacts, and the consumer downloads them and posts/updates the comment (mirroring pr-test-builds.yml).
Contributor
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
PR Summary by QodoReport PG Version Findings and Support Fork PR Comments
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
1. Attackers can redirect bot comments
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| with: | ||
| script: | | ||
| const fs = require('fs'); | ||
| const prNumber = Number(fs.readFileSync('pr_number.txt', 'utf8').trim()); |
There was a problem hiding this comment.
1. Attackers can redirect bot comments 🐞 Bug ⛨ Security
The comment workflow reads prNumber from the downloaded pr_number.txt artifact instead of deriving it from the trusted workflow_run event. Because the producing pull-request workflow and its executed scripts are controlled by the contributor, a malicious fork can supply another pull request number and use the write-enabled workflow to create or update a bot-authored comment there.
Agent Prompt
## Issue description
The privileged comment workflow trusts `pr_number.txt` from an artifact produced by pull-request-controlled code, allowing a contributor to redirect the bot's write access to another pull request.
## Fix Focus Areas
- .github/workflows/pg-version-check-comment.yml[34-40]
- .github/workflows/pg-version-check.yml[61-71]
## Recommended Fix
Derive and validate the pull request number from `github.event.workflow_run.pull_requests` or another trusted GitHub API association for the completed run. Reject runs with no unique associated pull request, and stop using an artifact-provided value to select the API write target.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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.
Problem
maintenance-10.xships a Parameter Group check that cannot report a finding. The workflow guard at.github/workflows/pg-version-check.yml:45requires^###in the checker's stdout, and the github-script filter at:77keys on###, but.github/scripts/check-pg-versions.py:187printsPG version issue: .... Exit code 1 therefore always takes theexit 2path: raw stdout dump, no PR comment, failed job, in exactly the case the tooling exists for.Introduced in #11885:
d42addb88fadded the###guard for the old shell checker's Markdown headings,5efd794951then rewrote the checker in Python with no marker in its output.Reproduction
Throwaway repo, one struct changed without a version bump: the checker exits 1 and prints the finding, the workflow step exits 2, the comment step never runs. With this PR the step exits 0 and records
exit_code=1for the comment step.Changes
Three commits by @sensei-hacker, cherry-picked unmodified from #11951 (the release/9.1 backport):
e2762ab638- both markers keyed onPG version issue:;check-pg-versions.py:17decodes git output witherrors='replace'; new fixture that runs the workflow's own guard against the checker's real output, so the two cannot drift apart againf874de624a-check-pg-versions.py:149gains--no-renames; without it a struct whose header was renamed and whose layout changed was silently excluded from the checke47bf801ae- comment posting moves to aworkflow_runconsumer; thepull_requesttoken is read-only for fork PRs, so the inline github-script step fails with 403 there172174158bis deliberately left out, it addsrelease/9.1to the trigger list and is release/9.1 only.Test
python3 .github/scripts/test-check-pg-versions.pypasses 18/18, including the two new fixtures. Run under Linux; on Windows the harness passes a Windows path tobashand the test cannot run.Related
This duplicates work already in #11951. If that merges first, the next release/9.1 to maintenance-10.x sync carries these commits over and this PR should be closed. Opened because maintenance-10.x ships the broken guard today.