fix(security): v0.40.0 — path-boundary scope, more secret redaction, ingestion bounds, run-executor truth, provider errors#126
Conversation
…ingestion bounds, run-executor truth, provider errors Adversarial mining round hardening across the security floor, ingestion, run executors, and provider-credential handling. Security: - check_scope matches prefixes at a path boundary (logs never admits logs-private/…) and drops empty-string prefix entries. - Redact Azure SAS sig=, temporary session tokens (FQoG/FwoG/IQoJ), and private_key = <value> assignments. - Route the agent's appended provider name/endpoint through redact_text. Ingestion: - Bounded readline loop + cumulative-bytes ceiling in _nonempty_lines. - Clip free-text group-by labels; HTML-escape finding cells in both report writers; future-dated objects → unknown age bucket. - DuckDB memory_limit/threads ceiling; cumulative decompression-output budget across all combine parts. Run executors: - recent_run_ids_for_provider returns only completed surveys; per-bucket persistence rolls back on failure with a warning finding; report generation wrapped in require_success; region-mismatch HeadBucket surfaces a distinct region_mismatch status. Provider credentials: - fchmod the vault .tmp to 0600 before writing ciphertext; malformed credential refs raise CredentialResolutionError instead of a raw 500. Tests: tests/test_v0400_fixes.py (16 cases). Full suite 726 passed, ruff clean, frontend tsc/build/vitest green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3YojFPzjkfMj6YYSwvzdx
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 721f85c405
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # degrades to bounded junk rows (the parsed-fraction truth guard flags them), | ||
| # never an OOM. | ||
| _MAX_LINE_CHARS = 1 << 20 # 1 MiB per line | ||
| _MAX_TOTAL_CHARS = 3 << 30 # ~3 GiB total (above the 2 GiB upload cap, below OOM) |
There was a problem hiding this comment.
Because _nonempty_lines() still returns a materialized list[str], this 3 GiB ceiling allows a max-size access-log upload to be accumulated in RAM before parsing; for example, a 2 GiB single-line file becomes ~2048 one-MiB chunks appended to out in import_access_logs, so the sidecar can still OOM despite the new bounded readline() loop. The cap needs to be a real in-memory budget or the parser needs to stream chunks instead of collecting them all.
Useful? React with 👍 / 👎.
| except Exception as exc: # noqa: BLE001 - per-bucket persistence isolation | ||
| conn.rollback() | ||
| bus.publish(run_id, {"type": "finding", "severity": "warning", | ||
| "title": f"Bucket {name}: persistence error", | ||
| "detail": redact_text(str(exc))}) |
There was a problem hiding this comment.
When one of these inserts fails, the rollback removes that bucket from the persisted snapshot but the run continues, later marks the survey completed, and builds the summary/report from per_bucket as if the bucket were saved. In that scenario recent_run_ids_for_provider() will treat this completed but partially persisted survey as authoritative, so query_account_profile and diffs can report missing buckets as removed or omit posture findings; either the snapshot counts should reflect only successfully persisted buckets or the run should not complete as a valid survey.
Useful? React with 👍 / 👎.
| -- A future last_modified (clock skew, or a garbage 9999 date) yields a NEGATIVE | ||
| -- age; without this guard it satisfies '<= 7' and is mis-bucketed as freshly | ||
| -- modified ('0-7d'). Treat future-dated objects as 'unknown', not recent. | ||
| WHEN datediff('day', try_cast(last_modified AS TIMESTAMP), current_timestamp) < 0 THEN 'unknown' |
There was a problem hiding this comment.
This branch now classifies future last_modified values as the unknown age bucket, but the unknown_age_ratio query below still counts only unparseable timestamps. For inventories where many rows have future dates, the distribution shows unknown while the warning threshold sees 0%, so the report can say age analysis is fine even though those rows were explicitly treated as unknown; include the same future-date predicate in the unknown_age count.
Useful? React with 👍 / 👎.
2 participants
Adversarial mining-round hardening across the security floor, ingestion, run executors, and provider-credential handling.
Security
check_scopematched prefixes with a rawstartswith, soallowed_prefixes=["logs"]admittedlogs-private/secret(a different top-level path). It now matches only an exact equal or a/-delimited child, and drops empty-string prefix entries so a stray""can't silently unrestrict the bucket.sig=HMAC params, temporary-credential session tokens (FQoG/FwoG/IQoJ…), andprivate_key = <value>assignments — labels kept, values masked.build_session_contextand thus outsideassert_no_secrets_in_context) now routes both fields throughredact_text.Ingestion bounds
_nonempty_linesreads via a boundedreadline(_MAX_LINE_CHARS)loop with a cumulative-bytes ceiling — a single multi-GiB line can no longer be slurped whole into one Python string._LABEL_LEN; finding cells HTML-escaped in both report writers; future-dated objects land inunknownage (not0-7d).memory_limit/threadsceiling; a managed evidence combine enforces a cumulative decompressed-output budget across all parts (decompression-bomb defense in depth).Run executors
recent_run_ids_for_providerjoins onruns.status = 'completed'so a crashed/partial survey isn't read back as newest.require_success.301/PermanentRedirect) surfaces a distinctregion_mismatchstatus.Provider credentials
.tmpisfchmod'd to0600before ciphertext is written (a leftover.tmpcan't be briefly group/world-readable).CredentialResolutionErrorinstead of a raw 500.How to run
cd sidecar && pytest -qcd frontend && npm run build && npx tsc --noEmit && npx vitest runChecks run
ruff check appclean (newtests/test_v0400_fixes.py, 16 cases).tsc --noEmitclean,npm run buildclean,vitest17 passed.Security invariants
No generic shell, no plaintext secrets, no destructive/mutating S3 op; secrets resolved only server-side and never placed in an LLM prompt; migration SQL append-only. This PR tightens the redaction and scope floor rather than relaxing it.
🤖 Generated with Claude Code
https://claude.ai/code/session_01M3YojFPzjkfMj6YYSwvzdx
Generated by Claude Code