Repository navigation
Audit fixes: the 4 LOW findings (credential uniqueness, TLS subject, report org bound, SSO error) - #28
Open
Krishcalin wants to merge 4 commits into
Open
Audit fixes: the 4 LOW findings (credential uniqueness, TLS subject, report org bound, SSO error)#28Krishcalin wants to merge 4 commits into
Krishcalin wants to merge 4 commits into
Conversation
uq_credential_assignment_target spanned (credential_id, device_id, group_id), but exactly one of device_id/group_id is ever set. Under Postgres' default NULLS DISTINCT two device-level rows (cred, dev, NULL) compare unequal, so the constraint never fired — it could only ever match an impossible all-non-NULL row — and the SELECT-then-INSERT in CredentialService.assign had no DB backstop against two concurrent callers both inserting the same (credential, device), which then appeared twice in the fallback list. Replaced with two partial unique indexes keyed on the target column each level actually sets (migration 0018_credential_assignment, distinct revision id to avoid colliding with the DR-sets branch's 0018 — they merge as a normal multi-head). assign() now runs the INSERT in a savepoint and, on IntegrityError, falls back to updating the winner's priority, preserving the upsert contract instead of surfacing a 500 to the loser. Regression tests: the DB rejects a duplicate device-level assignment; assign() twice on one target updates rather than duplicates. alembic check shows no new operations for these indexes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…signals send_https_certificate joined the certificate subject and the HTTP header block with a newline into one payload, and _read_certificate split it back on the first newline. When the certificate was unreadable (subject empty), the split made the HTTP status line the "TLS subject" — a bogus TLS_SUBJECT signal shown verbatim to the reviewer, and a vendor string in the true first header line credited to the wrong, heavier signal type. The early `if not outcome.payload: return` also dropped the header evidence and the CN hostname whenever the subject was empty. ProbeOutcome now carries subject (payload) and headers in separate fields; _read_certificate reads each from its own field and gates on responded (so an error `detail` is never read as a subject or hostname). Regression test: an unreadable certificate followed by a header block records an HTTP_HEADER signal with the whole block and never a TLS_SUBJECT. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
_active_findings started from select(Finding).where(status.in_(active)) with no org bound; only the restricted branch narrowed by visible group, so an unrestricted (super-admin) caller aggregated active findings across every org into the executive summary. Added Finding.org_id == self.org_id, matching every other report query. This is the still-open sibling of the _require_group org-leak (audit finding on reporting.py); _require_group itself is already fixed on PR #25 (audit-correctness-highs) and is left to that PR to avoid a merge conflict. Regression test: the executive summary excludes a finding created in another org. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e member SSO_ERRORS was an object literal indexed by the attacker-controlled sso_error query param. ?sso_error=toString (or constructor / hasOwnProperty) resolved to the inherited Object.prototype member — a function — which is truthy so the ?? fallback never fired; the function reached React as a child, which throws, and with no ErrorBoundary anywhere it unmounts the whole app. Converted SSO_ERRORS to a Map so .get() only ever returns an own entry or undefined. Regression test: crafted toString/constructor/hasOwnProperty reasons take the general fallback message instead of crashing the render. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes out the audit's LOW tier — all 4 findings, each with the regression test the audit noted was missing. Every finding was re-verified against current
mainby a per-finding investigation pass before implementation.Commits
d4a3377) —uq_credential_assignment_targetspanned three columns but one of device_id/group_id is always NULL, so under Postgres NULLS DISTINCT it never fired andassign()'s SELECT-then-INSERT had no DB backstop against concurrent duplicate assignments. Replaced with two partial unique indexes (migration0018_credential_assignment);assign()runs the INSERT in a savepoint and, on IntegrityError, falls back to updating the winner. Regression tests: DB rejects a duplicate; re-assign updates rather than duplicates.fd1119f) — the cert probe joined TLS subject + HTTP headers then split on the first newline, so an unreadable cert made the HTTP status line a bogus TLS_SUBJECT (and the early return dropped the header + CN hostname).ProbeOutcomenow carries the two in separate fields;_read_certificategates onresponded. Regression test: an unreadable cert never fakes a subject.6782832) —_active_findingshad no org bound, so an unrestricted caller aggregated active findings across every org. AddedFinding.org_id. (This is the open sibling of the_require_grouporg-leak, which PR Audit fixes: correctness highs (topology cache-staleness + reporting scope/org leaks) #25 already fixes — left there to avoid a conflict.) Regression test: exec summary excludes another org's finding.1a2a740) — reflectedsso_errorindexed a plain object, so?sso_error=toStringreturnedObject.prototype.toString(a function), bypassed the?? fallback, and crashed the render (no ErrorBoundary exists). ConvertedSSO_ERRORSto aMap. Regression test:toString/constructor/hasOwnPropertytake the fallback.Testing
test_inventory.py,test_discovery_transport.py,test_reports.py,test_migration_upgrade.pyall green.LoginPage.test.tsx(14 tests) green;tsc --noEmitclean.alembic checkshows no new operations for the credential indexes. It still reports theix_oidc_login_states_org_iddiff, which is PR Audit fixes: 15 MEDIUM findings (vuln engine, parsers, discovery, auth, analysis) #27's 0017 fix (not on this branch) and clears once Audit fixes: 15 MEDIUM findings (vuln engine, parsers, discovery, auth, analysis) #27 lands on main.🤖 Generated with Claude Code