fix(todo-gate): refuse redirects, bound reads and git probes#368

merged1
main
fix/todo-gate-harden
opened29d agobyLlux
View on GitHubGitHub import enabledPR last imported 3d ago

Summary

  • The TODO gate POSTed to the Decisions endpoint with plain urlopen: a 302 was followed with the Authorization header, leaking OPENROUTER_API_KEY to the redirect target, and the redirected body was accepted as the verdict. Reproduced against a local server (302 to /evil received the bearer key).
  • ask() now refuses redirects with a NoRedirect opener (same pattern as the AI Insights helper) and caps response reads at 64 KiB; the git probes carry a 120 s timeout, and a git timeout skips the gate (exit 0) instead of crashing it.
  • Observable result: a redirecting or oversized gate response now reports "TODO gate unavailable" (exit 2); a hung git now skips instead of hanging or tracebacking.

Related issue

None

Documentation impact

  • No maintained guide documents the TODO gate helper (no hits in docs/); the contributor workflow is unchanged (same make todo-gate command, same success/skip exit codes).
  • TODO.md is unaffected: no parity entry covers helper hardening.
  • Changelog: updated Unreleased.

Verification

  • New tests in tests/test_todo_gate.py (AskTests local-server redirect/oversize/happy-path, ReadChangeTests timeout kwargs + timeout skip): observed red before the fix (3 failures + 1 error), green after.
  • make check: full suite green locally on 90c8730. Skipped locally (tools absent, CI covers them): actionlint, pyflakes. QML hardening noted partial import/type checks locally ("CI requires full checks").
  • CI: green on 90c8730 โ€” check, smoke, smoke-runtime, CodeQL, pr-title all pass (run).

Visual changes

None

Review notes

  • The refused redirect surfaces as HTTPError, which main() already catches via URLError and reports as unavailable (exit 2, fail-closed).
  • TimeoutExpired is a SubprocessError, not a CalledProcessError, hence the widened except in main().

Checks

All checks passed

Added 1 commit

  • fix(todo-gate): refuse redirects, bound reads and git probes90c8730
CCopilotBotreviewed29d ago
Synced from GitHub

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Synced from GitHub

Review in Change Stack โ†’

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Important

  • ๐Ÿ” Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

โš™๏ธ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: c6ec7ce3-342f-4e2c-8a61-21d36f692ad6

No actionable comments were generated in the recent review. ๐ŸŽ‰

โ„น๏ธ Recent review info
โš™๏ธ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 790c3dba-a142-49f3-bab0-de443630fc5c

๐Ÿ“ฅ Commits

Reviewing files that changed from the base of the PR and between 6e93105bdab859b335b55ff4fe3cffdc28a45bf8 and 90c873030bd4694e86a8252037e55b6333bbff25.

๐Ÿ“’ Files selected for processing (3)
  • CHANGELOG.md
  • scripts/todo_gate.py
  • tests/test_todo_gate.py

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


๐Ÿ“ Walkthrough

Walkthrough

The TODO gate now refuses HTTP redirects, limits Decisions response reads, and applies a timeout to git subprocess calls. Tests cover the HTTP response limits and git timeout handling.

Changes

TODO gate safeguards

Layer / File(s)Summary
Bound Decisions responses
scripts/todo_gate.py, tests/test_todo_gate.py, CHANGELOG.md
The helper rejects redirects and responses larger than 65,536 bytes. Tests cover valid, redirect, and oversized responses. The changelog records these changes.
Bound git probes
scripts/todo_gate.py, tests/test_todo_gate.py
Git commands in read_change use a 120-second timeout. The error handler catches subprocess.SubprocessError. Tests check the timeout argument and verify that main returns 0 after a timeout.

Priority: โž– Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: โšช Minimal ยท up to 90c87

No actionable merge-blocking issue is established; the change is mergeable after normal checks.

๐Ÿšฅ Pre-merge checks | โœ… 4 | โŒ 1

โŒ Failed checks (1 warning)

Check nameStatusExplanationResolution
Docstring Coverageโš ๏ธ WarningDocstring coverage is 10.53% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 2 files. (1 skipped: โ€ฆWrite docstrings for the functions missing them to satisfy the coverage threshold.
โœ… Passed checks (4 passed)
Check nameStatusExplanation
Linked Issues checkโœ… PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes checkโœ… PassedCheck skipped because no linked issues were found for this pull request.
Title checkโœ… PassedThe title clearly summarizes the main changes: refusing redirects and bounding response reads and git probes.
Description checkโœ… PassedThe description covers the required sections, explains the change and its impact, addresses documentation, and reports verification results and limitations.
Full details: Docstring Coverage

Explanation

Docstring coverage is 10.53% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 2 files. (1 skipped: 1 unsupported.)

โœจ Finishing Touches ๐Ÿ’ก 1
๐Ÿ“ Generate docstrings ๐Ÿ’ก
  • Commit to this branch
  • Create a new PR
๐Ÿงช Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

โค๏ธ Share

Comment @coderabbitai help to get the list of available commands.