Audit fixes: 15 MEDIUM findings (vuln engine, parsers, discovery, auth, analysis) - #27
Open
Krishcalin wants to merge 5 commits into
Open
Krishcalin wants to merge 5 commits into
Krishcalin wants to merge 5 commits into
Conversation
…aths Three medium findings in the vulnerability views, all "confident number over the wrong population" defects: - vuln_view._enrich ranked and displayed the headline CVE on CVSS v3.1 alone. A CVE scored only under CVSS 4.0 (increasingly common post-2024; both NVD and CSAF ingest it) has a NULL cvss31, so it sorted to the bottom at 0.0 and its row cell was blank. Rank across every scored version (_score) and display whichever version scores highest (_headline_cvss). - vuln_view.summary() derived devices_unassessed from an unscoped, org-unfiltered, archived-inclusive device count while total/by_severity were scoped and org-filtered. A group-scoped analyst was told hundreds of devices they cannot see are unassessed. Numerator and denominator now describe one population: this org, this principal's visible groups, non-archived (matching assess_all). - matcher._fixed_versions read only advisory.fixed (the CSAF form). NVD never populates it and states the fix only as the affected range's exclusive upper bound (versionEndExcluding, constraint.fixed), so the upgrade-path view was blind to every NVD vulnerability. Harvest the range fix bound too; last_affected (still-vulnerable) is deliberately not read. Regression tests: a v4-only CVE still shows its score; an archived device does not inflate the unassessed count; an NVD-shape range fix feeds the upgrade view while an inclusive last_affected bound is never offered as a fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…s, NSX id key Four parser findings, each one dropping or misclassifying a rule member so the firewall/path analysis reads a wrong answer as complete: - arista/eos: `ip route <prefix> <intf> <gw>` captured only the first token, dropping the trailing gateway. The route stored as interface-only, so the path walk treated it as directly-attached and lost the forwarding hop. Regex now captures an optional trailing gateway address (a bare administrative distance or name/tag keyword does not capture); an interface-only route still has no next hop. - cloud/aws: a rule referencing a peered/cross-account group (sg-... absent from the export) got no NetworkObject and fell to the unresolved/MISSING bucket, advising a re-collection that can never produce it. Backfilled as type security-group (externally resolved), alongside the existing pl- prefix-list handling. - cloud/azure: the service-tag guard `any(ch in member for ch in '.:/')` treated the dot in a regional tag (Storage.EastUS, Sql.WestEurope) as proof of an address literal, so it was never typed service-tag and resolved to nothing. Replaced with an actual ip_network parse; only a value that parses as an address/network is a literal. - vmware/nsx: the group (and service) catalogue keyed on display_name while rules refer to groups by path id (reduced via _leaf). Any API/Terraform object whose display_name differs from its id (id=grp-4821, display_name='Web Servers') was catalogued under a name no rule uses, so every referencing rule lost its members. Keyed by id now, per the module's own docstring. Regression tests cover each: interface+gateway route, cross-account SG typing, regional tag vs real CIDR, and id-keyed group resolution. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ive silence Two discovery findings that made a partial-coverage run read as complete: - transport.send_tcp_connect caught ConnectionRefusedError under the generic OSError and returned responded=False, indistinguishable from a timeout. A refusal is an RST from the host itself — the port is closed but the host is provably up. A hardened device that drops ICMP and closes every scanned port was therefore reported absent. ProbeOutcome gains a host_alive flag (distinct from responded, so a refused port is not mistaken for an open one); HostProber marks the host responsive on a refusal without opening the port or attempting a follow-up read. - executor._record_batch collected per-host notes only after the `if not responded: continue` short-circuit. The ICMP-degradation note rides on whichever host was being probed when echo stopped, and that host usually does not respond — so the very caveat that makes the run partial was the one dropped, and "found N hosts" read as complete. Notes are now collected before the liveness short-circuit. Regression tests: an injected refusal proves liveness at both the probe and prober level without opening the port; a caveat on a non-responding host still reaches the run row. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… org_id index - services/auth.complete_mfa recorded last_used_step as the CURRENT time-step, not the step the accepted code belongs to. A code for step S stays valid through S+window, so the current-step guard left it replayable: an intercepted code yielded a second session one step later. Added core.security.matched_totp_step, which returns the step the code actually matched; the guard now stores and compares that, so a used code is refused for the rest of its window while a genuinely newer code still advances. - core/config: the production guard checked debug, cookie_secure and CORS but never that SECRET_KEY was explicitly set. Unset, it fell to a default_factory random per-process key — tokens signed by one worker failing on another, every restart invalidating every session, all silently. Production now fails at boot when secret_key is not in model_fields_set (i.e. came only from the factory). - migration 0017: oidc_login_states inherits OrgMixin (org_id index=True) but the migration never created ix_oidc_login_states_org_id, so `alembic check` saw drift (C-5) and the tenancy column was unindexed. Added to the head migration directly (it is the head, unlike 0009 which needed corrective 0011), avoiding a multi-head collision. Verified: `alembic check` now reports no new operations. Regression tests: matched step is the code's own step not the current one and advances for a fresh code; a TOTP code cannot be completed twice; an unset SECRET_KEY is rejected in production while an explicit one is accepted. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…AA parse
Three findings where an analysis reported a confident answer over the wrong basis:
- topology/path: the range-destination arrival check `serves(dst)` tested only the
representative (low) address, short-circuiting before the routes_subdividing guard. A
range whose low half is directly connected and high half is routed away was reported
fully arrived/ROUTED. Moved the subdividing guard ahead of the arrival and routing
branches, so a range any prefix cuts across returns UNKNOWN with the prefixes named.
- services/reporting._trend: `get()` enforces only org_id, so a group-scoped caller
could fetch a whole-estate report and subtract its frozen counts from a scoped current
summary, fabricating deltas ("960 findings closed"). Reports now stamp a
scope_fingerprint (unrestricted vs the sorted visible group ids) at generation; a trend
refuses when the earlier report's fingerprint differs from the caller's, including
pre-fingerprint reports that cannot be shown comparable.
- services/aaa_correlation: `product not in _SERVER_PRODUCTS or not config.clients`
conflated "not an AAA server" with "an AAA server whose client list was empty/unparsed"
and silently dropped both. A device registered only on the unparsed server was then
flagged unregistered/HIGH on the strength of the other servers. Split the conditions:
a known server with no readable clients is recorded, registration_reliable goes false,
the unregistered verdict is withheld, and a limitation names the servers to re-collect.
Regression tests: a part-connected/part-routed range returns UNKNOWN; a trend across
mismatched scopes is refused while same-scope still compares; an unreadable client list
withholds the unregistered verdict and states the gap.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Krishcalin
added a commit
that referenced
this pull request
Sep 30, 2026
Both dr_sets and dr_set_members inherit OrgMixin (org_id index=True, DATA-04), so the ORM declares ix_dr_sets_org_id and ix_dr_set_members_org_id, but migration 0018 created neither — an autogenerate diff that fails the C-5 alembic-check gate, and an unindexed tenancy column on tables every scoped query filters. Created both in the migration, mirroring every other OrgMixin table in the chain. Regression test asserts both indexes exist in pg_indexes after migration. (The sibling ix_oidc_login_states_org_id omission on migration 0017 is the same finding on a different table and is fixed on the audit-mediums branch / PR #27; once that lands on main this branch's alembic check is fully clean.) 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.
Resolves the 15 MEDIUM audit findings that live on
main, in five reviewable commits, each fix paired with the regression test the audit noted was missing.Three of the 18 mediums are not here — they live in this session's own new code on unmerged branches and will be fixed there:
db/migrations/0018_dr_sets.pyorg_id index → belongs ondr-sets(PR DR sets (Slice A, PR1): data layer, service and API #19)services/matrix.pyempty/degenerate + IPv6 zones → belong onmatrix-discovery(PR Slice B: group-scoped connectivity discovery matrix #21)Commits
Vuln engine (
24dc769)summary().devices_unassessedmixed an unscoped/org-unfiltered/archived-inclusive denominator with scoped totals → both sides now describe one population (this org, visible groups, non-archived).advisory.fixed(CSAF); NVD states them as the range'sversionEndExcluding→ harvest the range fix bound too (last_affecteddeliberately not read).Parsers (
a662f1f)ip route <prefix> <intf> <gw>dropped the gateway → capture the optional trailing gateway.sg-...refs left untyped (MISSING) → backfilled assecurity-group(externally resolved).Storage.EastUS) misclassified as literals by a dot test → realip_networkparse.display_namewhile rules reference the pathid→ keyed byid.Discovery (
06f42e6)host_aliveflag marks the host found without opening the port.Auth / config / migration (
37b9d0d)matched_totp_stepstores the matched step; a used code is refused for the rest of its window.SECRET_KEY→ fails at boot when it comes only from the random default_factory.ix_oidc_login_states_org_id→ added to the head migration;alembic checknow reports no drift.Analysis (
a1dd434)Testing
Each group's suite runs green locally (PowerShell venv).
alembic checkconfirms no migration drift. Adjacent suites (auth flow, vuln wiring, report delivery) also pass; 4 report-delivery skips are an unrelated environmental pycryptodomex issue.🤖 Generated with Claude Code