chore: config coherence + constant-time auth (GLM review P0s)#20
Merged
Conversation
A security engineer cloning the repo saw ~30 settings about Qdrant, Ollama, Gemini, Postgres, Telegram and Gmail interleaved with the audit config, and reasonably concluded the project was half-abandoned or hiding something. That is a credibility tax on exactly the audience the recorder is for. Pure reorder: the SIEM flight recorder settings (identity, trail, sinks, DLP, tool policy, detection) now come first under a clear header, and the optional governed runtime (vault, model providers, channel drivers) is grouped below with a comment pointing at docs/advanced-governed-runtime.md. Nothing is deleted, so the runtime keeps working; a recorder-only deployment can ignore the second block entirely. No behaviour change: all 64 fields, defaults, and validation aliases verified byte-identical to before via model_dump + model_fields comparison. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
require_api_key and verify_ws_token compared the shared key with plain `==`, which short-circuits on the first differing byte and leaks the key's prefix through response timing to anyone who can reach the endpoint. Low risk on loopback, real the moment the dashboard is bound to a LAN. Compare with secrets.compare_digest via a small helper, coercing a missing token to "" so a None header cannot raise. Behaviour is otherwise unchanged: unset key still skips auth, valid key passes, invalid rejects. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The two GLM-review findings that verified as true and still open on current master. Both small, both hit the credibility point the review nailed.
1. Config coherence (the review's sharpest finding)
GLM: "config.py is a graveyard ... any competent security engineer cloning the repo will see ~30 settings that have nothing to do with a SIEM and assume the project is half-abandoned or secretly back-doored."
Verified true: 31 settings for Qdrant, Ollama, Gemini, Postgres, Telegram, Gmail, sandbox, kernel were interleaved with the audit config.
The fix is not deletion — that config is the surface of the optional governed runtime you deliberately keep (
docs/advanced-governed-runtime.md). It is under-signposted, not dead. So this is a pure reorder: SIEM settings first under a clear header, the optional runtime grouped below with a pointer to its doc. A recorder-only cloner can now ignore the second block.No behaviour change, proven: all 64 fields, defaults, and validation aliases are byte-identical before and after (checked via
model_dump()+model_fieldsalias comparison).2. Constant-time auth compare
GLM: "auth.py ... does a plain string compare." True —
token == settings.api_keyshort-circuits on the first differing byte and leaks the key prefix through timing. Nowsecrets.compare_digest, with a missing token coerced to""so a None header can't raise. Unset key still skips auth; valid passes; invalid rejects.314 passed, ruff clean, existing
test_auth.pygreen.Not in this PR (stale or larger): the review's "durable detection state" P0 is already shipped (
live_store.pySQLite), its DLP-count and rule-count "inconsistencies" are stale (13 DLP rules, 9 sequence rules consistent everywhere), and "single-route dashboard, no detections view" is stale (a Detections tab + triage panel ship today). A point-by-point reply to the review is inagentmetry-notes/if useful.