Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .jules/bolt.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,7 @@
## 2026-07-02 - Credential Masking Security Hole in Subprocess Environments
**Learning:** Found a critical missing credential masking pattern in `scripts/ci/noema_review_gate.py`'s `scrub_sensitive_data` which didn't mask `Authorization: Basic` or `Proxy-Authorization: Basic` tokens unlike its analogous helper in `scripts/ci/pr_review_merge_scheduler.py`. This leaves exception messages and logs vulnerable to exposing sensitive credentials when HTTP operations fail.
**Action:** When implementing credential masking functions that sanitize tracebacks and log messages, ensure the masking scope includes all relevant headers, particularly `Authorization` and `Proxy-Authorization`. Ensure parity across masking helpers across CI scripts to prevent blind spots.

## 2025-02-24 - Transient Rate Limit Overflows
**Bottleneck:** CI LLM Model Pool script failed to distinguish rate limits from context overflows, causing it to incorrectly abort and skip to the next model, leading to job-level timeouts.
**Pattern:** Always explicitly identify transient rate limit strings (e.g. 'Too many requests. For more on scraping GitHub') and allow dynamic retry limit expansion within exponential backoff loops rather than silently giving up and failing over to the next candidate model.
4 changes: 4 additions & 0 deletions .jules/sentinel.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,7 @@
**Vulnerability:** Server-Side Request Forgery (SSRF) / Local File Inclusion
**Learning:** External URL fetching with `urllib.request.urlopen` (like API endpoints passed via environment variables) can accept schemes like `file://` implicitly, which could allow arbitrary file reading or internal network scanning if the environment is misconfigured or manipulated.
**Prevention:** Always validate that URLs explicitly start with `http://` or `https://` before using them in standard library requests. Append to suppress linter warnings only after verifying the input is validated.
## 2025-02-24 - [Information Leakage on Timeout]
**Vulnerability:** Information Leakage on Subprocess Timeout in sandboxed_verify.py
**Learning:** When running subprocesses with a timeout in Python using `subprocess.run`, the resulting `subprocess.TimeoutExpired` exception object contains unmasked `stdout` and `stderr` data. The `sandboxed_verify.py` script directly printed this data on timeout, potentially exposing sensitive environment variables or secrets injected during the execution.
**Prevention:** Ensure all subprocess output, including output retrieved from exception objects (like `TimeoutExpired`), is explicitly sanitized using a token scrubbing function before logging or printing to `stdout/stderr`.
2 changes: 1 addition & 1 deletion scripts/ci/noema_review_gate.py
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@ def call_llm(repo: str, number: int, pr: dict[str, Any], diff: str, truncated: b
if ip.is_private or ip.is_loopback or ip.is_link_local or ip.is_multicast or ip.is_unspecified:
raise ValueError("URL cannot target internal IP addresses")

if not (api_url.startswith("http://") or api_url.startswith("https://")):
if not (api_url.lower().startswith("http://") or api_url.lower().startswith("https://")):
raise ValueError(f"NOEMA_LLM_API_URL must start with http:// or https:// to prevent SSRF vulnerabilities, got: {api_url}")

prompt = {
Expand Down
16 changes: 16 additions & 0 deletions scripts/ci/run_opencode_review_model_pool.sh
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,19 @@ assert_reasoning_effort_for_candidate() {
"$model_candidate"
}

is_transient_rate_limit() {
local opencode_json_file="$1"
[ -s "$opencode_json_file" ] || return 1
grep -Fq 'Too many requests. For more on scraping GitHub' "$opencode_json_file"
}

is_context_overflow_failure() {
local opencode_json_file="$1"

[ -s "$opencode_json_file" ] || return 1
if is_transient_rate_limit "$opencode_json_file"; then
return 1
fi
grep -Eiq 'ContextOverflowError|tokens_limit_reached|Request body too large|context window' "$opencode_json_file"
}

Expand Down Expand Up @@ -132,6 +141,10 @@ run_one_model_attempt() {
set -e
if [ "$opencode_status" -ne 0 ]; then
printf 'OpenCode %s attempt %s/%s failed with exit %s.\n' "$model_candidate" "$attempt" "$attempts" "$opencode_status"
if is_transient_rate_limit "$opencode_json_file"; then
printf 'OpenCode %s attempt %s/%s hit a transient rate limit; treating as retryable.\n' "$model_candidate" "$attempt" "$attempts"
return 3
fi
if is_context_overflow_failure "$opencode_json_file"; then
printf 'OpenCode %s attempt %s/%s exceeded the provider context window; skipping remaining attempts for this model.\n' "$model_candidate" "$attempt" "$attempts"
return 2
Expand Down Expand Up @@ -231,6 +244,9 @@ main() {
if [ "$run_status" -eq 2 ]; then
break
fi
if [ "$run_status" -eq 3 ] && [ "$attempt" -ge "$attempts" ]; then
attempts=$((attempts + 1))
fi
retry_sleep="$(backoff_sleep "$attempt")"
if [ "$deadline" -gt 0 ] && [ $((SECONDS + retry_sleep)) -gt "$deadline" ]; then
retry_sleep=$((deadline - SECONDS))
Expand Down
27 changes: 23 additions & 4 deletions scripts/ci/sandboxed_verify.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,25 @@
)
RESULT_MARKER = "SANDBOXED_VERIFY_RESULT"
ENV_NAME_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$")
SENSITIVE_DATA_SCRUB_PATTERNS = (
(re.compile(r'(?i)(bearer\s+)[^\s"\'\\]+'), r'\1***'),
(re.compile(r'(?i)(token\s+)[^\s"\'\\]+'), r'\1***'),
(re.compile(r'(?i)\b(?:github_pat_[A-Za-z0-9_]+|gh[pousr]_[A-Za-z0-9_]+)\b'), '***'),
(re.compile(r'\b(sk-[A-Za-z0-9_-]+)'), '***'),
(re.compile(r'\b(xox[baprs]-[A-Za-z0-9-]+)'), '***'),
(re.compile(r'\b(AKIA[0-9A-Z]{16})'), '***'),
(re.compile(r'(?i)((?:api[_-]?key|access[_-]?token|refresh[_-]?token|id[_-]?token|client[_-]?secret|password|passwd|secret)\s*[:=]\s*)["\']?[^"\'\s]+["\']?'), r'\1***'),
(re.compile(r'(?i)((?:authorization|proxy-authorization)\s*:\s*(?:bearer|basic)\s+)[A-Za-z0-9._~+\/=-]+'), r'\1***'),
)


def scrub_sensitive_data(text: str | None) -> str | None:
"""Mask sensitive tokens in text to prevent secret leakage."""
if not text:
return text
for pattern, repl in SENSITIVE_DATA_SCRUB_PATTERNS:
text = pattern.sub(repl, text)
return text


def parse_args(argv: Sequence[str] | None = None) -> argparse.Namespace:
Expand Down Expand Up @@ -219,13 +238,13 @@ def main(argv: Sequence[str] | None = None) -> int:
try:
completed = run_command(args.command, copied_repo, env, args.timeout)
if completed.stdout:
print(completed.stdout, end="")
print(scrub_sensitive_data(completed.stdout), end="")
if completed.stderr:
print(completed.stderr, end="", file=sys.stderr)
print(scrub_sensitive_data(completed.stderr), end="", file=sys.stderr)
exit_code = completed.returncode
except subprocess.TimeoutExpired as exc:
stdout = timeout_output_text(exc.stdout)
stderr = timeout_output_text(exc.stderr)
stdout = scrub_sensitive_data(timeout_output_text(exc.stdout))
stderr = scrub_sensitive_data(timeout_output_text(exc.stderr))
if stdout:
print(stdout, end="" if stdout.endswith("\n") else "\n")
if stderr:
Expand Down
31 changes: 30 additions & 1 deletion tests/test_noema_review_gate.py
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ def test_call_llm_handles_configuration_and_verdicts(monkeypatch):

monkeypatch.setenv("NOEMA_LLM_API_URL", "file:///etc/passwd")
monkeypatch.setenv("NOEMA_LLM_API_KEY", "secret")
with pytest.raises(ValueError, match="must start with http:// or https://"):
with pytest.raises(ValueError, match="URL scheme must be http or https"):
noema.call_llm("owner/repo", 1, pr, "diff", False)

monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example.test/chat")
Expand Down Expand Up @@ -238,6 +238,35 @@ def fake_urlopen(request, timeout):
monkeypatch.setattr(noema.urllib.request, "urlopen", fake_urlopen)
assert noema.call_llm("owner/repo", 1, pr, "diff", True)["decision"] == "approve"

# Test SSRF protection URL check error
monkeypatch.setenv("NOEMA_LLM_API_URL", "ftp://llm.example.test/chat")
with pytest.raises(ValueError, match="URL scheme must be http or https"):
noema.call_llm("owner/repo", 1, pr, "diff", False)

# Test SSRF explicit scheme check (bypassing urllib scheme check)
monkeypatch.setenv("NOEMA_LLM_API_URL", "http:///no-host")
with pytest.raises(ValueError, match="URL must have a valid hostname"):
noema.call_llm("owner/repo", 1, pr, "diff", False)

monkeypatch.setenv("NOEMA_LLM_API_URL", "http://[::1]/chat") # IPv6 loopback
with pytest.raises(ValueError, match="URL cannot target internal IP addresses"):
noema.call_llm("owner/repo", 1, pr, "diff", False)

# Cover the SSRF regex block itself by mocking urlparse to not fail
import urllib.parse
orig_urlparse = urllib.parse.urlparse

def fake_urlparse(url, *args, **kwargs):
parsed = orig_urlparse(url, *args, **kwargs)
if url == "BAD_SCHEME://example.com":
return urllib.parse.ParseResult(scheme="http", netloc="example.com", path="", params="", query="", fragment="")
return parsed

monkeypatch.setattr(noema.urllib.parse, "urlparse", fake_urlparse)
monkeypatch.setenv("NOEMA_LLM_API_URL", "BAD_SCHEME://example.com")
with pytest.raises(ValueError, match="NOEMA_LLM_API_URL must start with http:// or https://"):
noema.call_llm("owner/repo", 1, pr, "diff", False)

# Test invalid scheme (and no original URL in error)
monkeypatch.setenv("NOEMA_LLM_API_URL", "file:///etc/passwd")
with pytest.raises(ValueError, match="URL scheme must be http or https"):
Expand Down
4 changes: 2 additions & 2 deletions tests/test_opencode_agent_contract.py
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ def test_workflow_provisions_sandbox_tool_and_reviewer_agent():
assert '"## Review outcome"' in workflow
assert '"## Check outcome"' not in workflow
assert "publish REQUEST_CHANGES when coverage-evidence blocker states" in workflow
assert re.search(r"opencode-review-target:[\s\S]{0,240}timeout-minutes: 360", workflow)
assert re.search(r"opencode-review-target:[\s\S]{0,360}timeout-minutes: 360", workflow)
assert 'timeout-minutes: 75' in workflow
assert re.search(r"Run OpenCode PR Review model pool[\s\S]{0,240}timeout-minutes: 350", workflow)
assert 'APPROVAL_CHECK_WAIT_ATTEMPTS: "81"' in workflow
Expand Down Expand Up @@ -412,7 +412,7 @@ def test_merge_scheduler_uses_escalating_mutation_credentials():
assert "steps.scheduler_app_token.outputs.token" in workflow
assert "SCHEDULER_READ_TOKEN: ${{ github.token }}" in workflow
assert "SCHEDULER_MUTATION_TOKEN_SOURCE" in workflow
assert 'default: "-1"' in workflow
assert 'default: "1"' in workflow
assert 'review_dispatch_limit="-1"' in workflow


Expand Down
8 changes: 8 additions & 0 deletions tests/test_sandboxed_verify.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,14 @@ def test_timeout_output_text_normalizes_subprocess_payloads():
assert sandboxed_verify.timeout_output_text("text-output") == "text-output"


def test_scrub_sensitive_data():
"""Ensure sensitive data is masked."""
assert sandboxed_verify.scrub_sensitive_data(None) is None
assert sandboxed_verify.scrub_sensitive_data("bearer token123") == "bearer ***"
assert sandboxed_verify.scrub_sensitive_data("GH_TOKEN=ghp_abc123") == "GH_TOKEN=***"
assert sandboxed_verify.scrub_sensitive_data("normal text") == "normal text"


def test_main_runs_command_in_copy_without_mutating_source(tmp_path, capsys):
"""The wrapper runs commands in the copied workspace, not the source tree."""
repo = tmp_path / "repo"
Expand Down
Loading