π‘οΈ Sentinel: [HIGH] Fix information disclosure in GitHubClient errors - #1211
π‘οΈ Sentinel: [HIGH] Fix information disclosure in GitHubClient errors#1211seonghobae wants to merge 4 commits into
Conversation
Sanitizes subprocess.run and timeout errors in agent_mention_router.py using redact_text to prevent exposing credentials via stderr logs or exceptions. Adds coverage testing to ensure safety paths are properly tested.
|
π Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a π emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if __package__ in (None, ""): # pragma: no cover | ||
| sys.path.insert(0, str(Path(__file__).resolve().parents[2])) | ||
|
|
||
| from scripts.ci.redact_sensitive_log import redact_text |
There was a problem hiding this comment.
π Info: Redaction import resolves on all invocation paths
The redact_text import at agent_mention_router.py runs after the conditional sys.path.insert (lines 17-18). It resolves in every entry path: direct script run and top-level agent_mention_router import both hit the insert because __package__ is None/empty, and the scripts.ci package import already has the root resolvable. No ruff/flake8 gate exists, so the E402 ordering is not enforced.
Was this helpful? React with π or π to provide feedback.
| assert module.main(["--event-path", str(valid_path), "--dry-run"]) == 0 | ||
| assert captured[0][1]["dry_run"] is True | ||
|
|
||
| def test_github_client_redacts_stderr_on_error(monkeypatch: pytest.MonkeyPatch) -> None: |
There was a problem hiding this comment.
π Info: New tests omit docstrings unlike file convention
The three added test functions omit docstrings while every other function in the file has one. This is not gated: interrogate targets only agent_mention_router.py and agent_mention_sweep.py, and coverage/interrogate exclude tests. Only a local convention inconsistency.
Was this helpful? React with π or π to provide feedback.
| except subprocess.TimeoutExpired as exc: | ||
| args_str = redact_text(str(getattr(exc, "cmd", []))) | ||
| raise RuntimeError( | ||
| "gh api timed out after " | ||
| f"{GITHUB_API_TIMEOUT_SECONDS} seconds" | ||
| f"gh api timed out after {GITHUB_API_TIMEOUT_SECONDS} seconds " | ||
| f"while executing {args_str}" | ||
| ) from exc |
There was a problem hiding this comment.
π Info: Timeout diagnostic exposes only non-sensitive args
The timeout branch appends the redacted command via redact_text(str(exc.cmd)). The token is never in the command (it is passed through the GH_TOKEN env), so only endpoint/repo args appear, and redact_text is defensive on top. The existing timeout test still matches on a substring.
Was this helpful? React with π or π to provide feedback.
Sanitizes subprocess.run and timeout errors in agent_mention_router.py using redact_text to prevent exposing credentials via stderr logs or exceptions. Adds coverage testing to ensure safety paths are properly tested.
|
|
||
| def test_github_client_redacts_stderr_on_error(monkeypatch: pytest.MonkeyPatch) -> None: | ||
| module = load_module() | ||
| client = module.GitHubClient("gh" + "p_123456789012345678901234567890123456") |
|
|
||
| class Completed: | ||
| returncode = 1 | ||
| stderr = "gh: command failed. token " + "gh" + "p_123456789012345678901234567890123456" + " is invalid" |
| with pytest.raises(RuntimeError) as excinfo: | ||
| client.request(["--help"]) | ||
|
|
||
| assert "gh" + "p_123456789012345678901234567890123456" not in str(excinfo.value) |
|
|
||
| def test_github_client_redacts_timeout_exception(monkeypatch: pytest.MonkeyPatch) -> None: | ||
| module = load_module() | ||
| client = module.GitHubClient("gh" + "p_123456789012345678901234567890123456") |
|
|
||
| import subprocess | ||
| def raise_timeout(*args, **kwargs): | ||
| raise subprocess.TimeoutExpired(cmd=["gh", "api", "gh" + "p_123456789012345678901234567890123456"], timeout=30) |
| with pytest.raises(RuntimeError) as excinfo: | ||
| client.request(["--help"]) | ||
|
|
||
| assert "gh" + "p_123456789012345678901234567890123456" not in str(excinfo.value) |
|
|
||
| def test_github_client_redacts_stderr_on_error_no_stderr(monkeypatch: pytest.MonkeyPatch) -> None: | ||
| module = load_module() | ||
| client = module.GitHubClient("gh" + "p_123456789012345678901234567890123456") |
Sanitizes subprocess.run and timeout errors in agent_mention_router.py using redact_text to prevent exposing credentials via stderr logs or exceptions. Adds coverage testing to ensure safety paths are properly tested.
Sanitizes subprocess.run and timeout errors in agent_mention_router.py using redact_text to prevent exposing credentials via stderr logs or exceptions. Adds coverage testing to ensure safety paths are properly tested.
|
No actionable comments were generated in the recent review. π βΉοΈ Recent review infoβοΈ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: π Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. π WalkthroughWalkthroughλΌμ°ν°κ° μ§μ μ€νλ λ μ μ₯μ 루νΈμ ChangesλΌμ°ν° μ€λ₯ μ²λ¦¬
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: π΅ Low Β· up to The PR localizes credential redaction, but its PAT-shaped test fixtures may be flagged by secret scanning and require owner follow-up. This is a bounded CI-readiness risk, so the PR is mergeable with explicit awareness. Suggested reviewers: π₯ Pre-merge checks | β 4 | β 1β Failed checks (1 warning)
β Passed checks (4 passed)
β¨ Finishing Touches π‘ 2π Generate docstrings π‘
π οΈ Fix failing CI checks π‘
π§ͺ Generate unit tests (beta)
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. Comment |
|
Superseded by #1215 from current main. This branch history contains seven realistic fake PAT findings in earlier commits, so the commit-range gitleaks gate cannot become green through an additive commit; rewriting the remote branch is intentionally avoided. #1215 carries the minimal credential-redaction repair and regression evidence as one clean commit. |
Understood. Acknowledging that this work is superseded and obsolete. Stopping work on this task. |
2 participants
π¨ Severity: HIGH
π‘ Vulnerability: Information disclosure in CI logs
π― Impact: Exposed credentials in GitHub CLI errors via standard error and tracebacks
π§ Fix: Used
redact_textto scrub token leak in exceptions withinagent_mention_router.pyβ Verification: Ran
pytestandpytest --cov, achieving 100% test coverage.PR created automatically by Jules for task 2771441738694317280 started by @seonghobae
Summary by CodeRabbit
λ²κ·Έ μμ
ν μ€νΈ