Repository navigation
ci(security-gate): allow escape sequences when fetching changed .py blobs - #126
Merged
tarekziade merged 2 commits intoOct 8, 2026
Merged
Conversation
…lobs gh refuses to emit a response body containing terminal escape sequences unless --allow-escape-sequences is passed. It exits 1 having written nothing, and the step runs under `bash -e`, so get-changed-files aborts and approve-pr-ci never runs. A .py file may legitimately contain an ANSI escape byte: transformers has one in tests/utils/test_modeling_utils.py, in a test asserting on colourised output. Any fork PR touching that file is blocked, even when its own diff is nowhere near the line. The flag is safe here — the body is redirected to a file and parsed by Bandit's AST, never rendered to a terminal.
6 tasks
Collaborator
|
@askserge review this PR. focus on security |
tarekziade
self-requested a review
October 8, 2026 09:28
![sergereview[bot]](https://avatars.githubusercontent.com/in/3673521?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤗 Serge says:
Verdict
Clean, minimal, well-reasoned security fix; adding --allow-escape-sequences to the gh api raw-blob fetch is safe and does not weaken existing controls.
Security
- Flag is correctly scoped:
gh's escape-sequence guard prevents TTY injection, but output is redirected to> "/tmp/pr_py/$f"and parsed by Bandit's AST, never rendered, so the flag only enables the file write. - No other control is weakened: allowlist (rules 1–3) and protected-path checks run before the fetch; fetch still reads via contents API at head SHA, not a fork checkout, preserving "pwn request" hardening.
- No new attack surface: fetched content is already attacker-controlled fork PR head blob; escape sequences in Python source are inert bytes for Bandit's static AST parser.
- Omitted
[ -s ]guard is correct: underbash -eo pipefail, failedgh apiaborts the step, preventing a 0-byte file from scanning clean; only genuinely empty files (e.g.__init__.py) are 0-byte. The separate empty-blob-clean hole would need size comparison, not a blocker.
Comment accurately describes behavior and safety rationale; no inline issues.
serge v0.1.0 · model: Qwen/Qwen3.8-27B:novita · 7 LLM turns · 7 tool calls · 356.8s · 66609 in / 16475 out tokens
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
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.
Fixes #125.
What
get-changed-filesaborts on any fork PR that touches a.pyfile containing an ANSI escape byte, whether or not the PR's own diff introduces one.The fetch step runs
gh api -H "Accept: application/vnd.github.raw" ... > /tmp/pr_py/$f.ghrefuses to emit a body containing terminal escape sequences unless--allow-escape-sequencesis passed:It exits 1 having written nothing, the step is
bash -e, so the job fails,approve-pr-ciis skipped, and the PR's CI is never approved.Live instance
huggingface/transformers #48543 is approved and cannot merge.
tests/utils/test_modeling_utils.pycarries a literal ESC onmainat line 1754, in a test asserting on colourised output. That line predates the PR and the diff does not touch it.Since the caller gates on
author_association, this only ever fires for outside contributors — a maintainer editing the same file never sees it.Fix
One flag. Safe here because the body is redirected to a file and parsed by Bandit's AST, never rendered to a terminal — which is the case the protection exists for.
Verified against the real blob:
Deliberately left out
The issue floated a
[ -s ... ] || exit 1guard so an empty blob fails loudly instead of being scanned clean by Bandit. I did not include it: an empty__init__.pyis a perfectly ordinary changed file, and the guard would reject legitimate PRs. If you want that hole closed, it needs to compare against the blob size the API reports rather than testing for non-empty — happy to do it in a separate PR.🤖 Generated with Claude Code