-
Notifications
You must be signed in to change notification settings - Fork 0
π‘οΈ Sentinel: [HIGH] Fix information disclosure in GitHubClient errors #1211
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
74b05d3
e64a63e
b20d423
865329f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,9 +9,17 @@ | |
| import os | ||
| import re | ||
| import subprocess | ||
| import sys | ||
| from dataclasses import dataclass | ||
| from pathlib import Path | ||
| from typing import Any, Sequence | ||
|
|
||
| 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 | ||
|
|
||
|
|
||
| CENTRAL_AUTOMATION_REPOSITORY = "ContextualWisdomLab/.github" | ||
| TRUSTED_ASSOCIATIONS = frozenset({"OWNER", "MEMBER", "COLLABORATOR"}) | ||
| MENTION_PATTERNS = { | ||
|
|
@@ -86,9 +94,10 @@ def request( | |
| timeout=GITHUB_API_TIMEOUT_SECONDS, | ||
| ) | ||
| 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 | ||
|
Comment on lines
96
to
101
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π Info: Timeout diagnostic exposes only non-sensitive args The timeout branch appends the redacted command via Was this helpful? React with π or π to provide feedback. |
||
| return_code = int(getattr(completed, "returncode", 0)) | ||
| if return_code: | ||
|
|
@@ -98,7 +107,7 @@ def request( | |
| if not diagnostic: | ||
| diagnostic = "no stderr output" | ||
| raise RuntimeError( | ||
| f"gh api failed with exit code {return_code}: {diagnostic[:2000]}" | ||
| f"gh api failed with exit code {return_code}: {redact_text(diagnostic)[:2000]}" | ||
| ) | ||
| output = completed.stdout.strip() | ||
| return None if not output else json.loads(output) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,7 +17,7 @@ | |
| def load_module() -> ModuleType: | ||
| """Load the router module from its script path.""" | ||
|
|
||
| module_name = "agent_mention_router" | ||
| module_name = "router" | ||
| spec = importlib.util.spec_from_file_location(module_name, MODULE_PATH) | ||
| assert spec and spec.loader | ||
| module = importlib.util.module_from_spec(spec) | ||
|
|
@@ -371,3 +371,54 @@ | |
| ) | ||
| 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: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π 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 Was this helpful? React with π or π to provide feedback. |
||
| module = load_module() | ||
| client = module.GitHubClient("gh" + "p_123456789012345678901234567890123456") | ||
|
|
||
|
|
||
| class Completed: | ||
| returncode = 1 | ||
| stderr = "gh: command failed. token " + "gh" + "p_123456789012345678901234567890123456" + " is invalid" | ||
|
|
||
| stdout = "" | ||
|
|
||
| import subprocess | ||
| monkeypatch.setattr(subprocess, "run", lambda *args, **kwargs: Completed()) | ||
|
|
||
| with pytest.raises(RuntimeError) as excinfo: | ||
| client.request(["--help"]) | ||
|
|
||
| assert "gh" + "p_123456789012345678901234567890123456" not in str(excinfo.value) | ||
|
|
||
| assert "[REDACTED]" 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) | ||
|
|
||
|
|
||
| monkeypatch.setattr(subprocess, "run", raise_timeout) | ||
|
|
||
| with pytest.raises(RuntimeError) as excinfo: | ||
| client.request(["--help"]) | ||
|
|
||
| assert "gh" + "p_123456789012345678901234567890123456" not in str(excinfo.value) | ||
|
|
||
| assert "[REDACTED]" 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") | ||
|
|
||
|
|
||
| class Completed: | ||
| returncode = 1 | ||
| stderr = "" | ||
| stdout = "" | ||
|
|
||
| import subprocess | ||
| monkeypatch.setattr(subprocess, "run", lambda *args, **kwargs: Completed()) | ||
|
|
||
| with pytest.raises(RuntimeError) as excinfo: | ||
| client.request(["--help"]) | ||
|
|
||
| assert "no stderr output" in str(excinfo.value) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
π Info: Redaction import resolves on all invocation paths
The
redact_textimport at agent_mention_router.py runs after the conditionalsys.path.insert(lines 17-18). It resolves in every entry path: direct script run and top-levelagent_mention_routerimport both hit the insert because__package__is None/empty, and thescripts.cipackage 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.