[AAASM-5217] 🐛 (dashboard): Validate or document fetch-boundary casts - #1776
Conversation
…efore use `GET /api/v1/capability/matrix` is cast wholesale to `CapabilityMatrix` at the API boundary, so a cell's Decision carries an unenforced annotation over raw wire data. DECISIONS[decision] resolved that value directly, so a hostile payload keyed "__proto__"/"constructor" could resolve an inherited Object.prototype member instead of falling through to a real decision tone. decisionMeta()/isDecision() validate against the Decision union first and fold anything else to the na metadata. Corrects capability.ts's doc comment, which cited structural type identity as making the boundary cast outright "safe" — a compile-time claim, not a runtime guarantee (AAASM-5217).
Regression coverage for AAASM-5217's decisionMeta()/isDecision() guard:
every real Decision resolves its own metadata, and inherited-prototype keys
("__proto__", "constructor") plus unrecognised strings and non-strings all
fold to the na metadata rather than throwing or resolving an inherited
Object.prototype member.
…enum
sortAgents compares two cells' decisions via DECISION_WEIGHT[da] -
DECISION_WEIGHT[db], where da/db are raw wire values wearing an unenforced
Decision annotation (the matrix is cast wholesale at the API boundary,
AAASM-5217). An inherited-prototype key ("__proto__") resolved to an
inherited Object.prototype member rather than a real weight, and an
unrecognised string resolved to undefined, making the comparison NaN.
decisionWeight() validates against the Decision union first and folds
anything else to na's weight.
Covers AAASM-5217's decisionWeight() guard: a cell whose decision is the inherited "__proto__" key must not throw, produce a NaN comparison, or sort ahead of a real "deny" decision.
…wire enum The /alerts list path canonicalises severity via parseAlert.ts before an Alert exists (AAASM-5149), but the single-alert-detail and rules paths (useAlertQuery, useAlertRulesQuery) reach this shared badge through the bare `as T` cast in alertsFetch() with no equivalent normalisation (AAASM-5217). SEVERITY_BG[severity] on an unrecognised or prototype-inherited value resolved undefined or an inherited member, producing a silently blank badge. isSeverity() validates first and falls back to a real background.
Covers AAASM-5217's isSeverity() guard: every real Severity renders its own
badge, and an unrecognised or inherited-prototype value ("__proto__",
"constructor") renders the unknown badge with a real background rather than
a blank one.
… enum Same hazard and rationale as SeverityBadge (AAASM-5217): the single-alert- detail path reaches this component through a bare cast with no canonicalisation. STATUS_STYLE[status] on an unrecognised or prototype-inherited key threw on destructuring `undefined`, crashing the alert detail drawer. isAlertStatus() validates first and falls back to a neutral style instead of throwing.
Covers AAASM-5217's isAlertStatus() guard: every real AlertStatus renders its own badge, and an unrecognised or inherited-prototype value renders the unknown badge instead of throwing on the destructure.
Extracts the allow-list membership check parseScopesFromJwt already applied inline into an exported isScope() guard, so AuthProvider's login() can apply the same validation to the login response's scopes field (AAASM-5217) — that field previously trusted a bare cast unfiltered, unlike this JWT-claim path.
Regression coverage for AAASM-5217's isScope() guard: every real Scope is accepted, and inherited-prototype keys, unrecognised strings, and non-string values are all rejected without throwing. parseScopesFromJwt drops a hostile scope claim rather than trusting it.
login() cast the /api/v1/auth/token response's scopes field straight to Scope[] and used it unfiltered, unlike the JWT-claim fallback path which already validates via VALID_SCOPES. Every granted scope is later used as a SCOPE_RANK lookup key (usePermissions.ts::scopesSatisfy) — a hostile scope like "__proto__" could resolve an inherited Object.prototype member there instead of failing a permission check (AAASM-5217). Filters through the same isScope() allow-list parseScopesFromJwt applies.
Covers AAASM-5217: a login response carrying a hostile "__proto__" scope alongside a real one must have the hostile entry dropped, not carried into context state.
AAASM-5217 audit: res.json() as Promise<T> is a bare cast, but the only fields ever used as a lookup key downstream (CostSegment.key, ActionVolumeSeries.key) already build their row objects on Object.create(null) specifically because that key is unvalidated wire data — so a segment/series keyed "__proto__" becomes an ordinary own property rather than reassigning a prototype.
AAASM-5217 audit. onboarding/api.ts's HealthResponse casts: `checks` is only ever iterated via Object.entries and rendered as opaque text, never used as a lookup key. audit/api.ts's LogEntry.payload parse: only ever probed with two fixed string literals, never a dynamic key from the wire.
AAASM-5217 audit. useLiveOpsStream.ts's GovernanceEvent cast: every field mapEvent uses as a lookup key (status, call-stack kind) is already allow-list-validated via coerceStatus/opStateToStatus/coerceCallStackKind before the lookup. useApprovalsStream.ts's GovernanceEvent/ApprovalPayload casts: event_type/status are only compared with ===, and action is rendered as opaque text / added to a Set, never indexed into an object.
AAASM-5217 audit. useTopologyQuery's TopologyGraphResponse cast: every node's status/mode is allow-list-validated by mapGraph.ts's toStatus/toMode before becoming part of the rendered TopologyNode. useTopologyNodeRecentEvents's RecentEvent[] cast: `type` is rendered as opaque display text, never a lookup key.
AAASM-5217 audit: none of ApiKey's fields (status, role, scopes, recent_activity[].action) are ever used as an object/Map lookup key — ApiKeyList.tsx compares status/role with === and renders them as text; GenerateKeyDialog.tsx checks scopes membership with Array.includes.
AAASM-5217 audit: no field of TopologyOverview/CostSummary/TeamTopology/ AgentLineage (team/agent ids, spend figures, member status, lineage tier) is ever used as an object/Map lookup key in features/teams — joinTeamRows keys a real Map by team_id (opaque, .get() treats every key as ordinary), and every status/tier field is rendered as display text or compared with ===.
AAASM-5217 audit: ApiResponse.nodes[].agent_id is used as a React list key and a d3 hierarchy id (ViolationHeatmap.tsx), never as an object/Map lookup key; violation_count/window_secs are plain numbers consumed arithmetically.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Chisanan232
left a comment
Chisanan232
left a comment
There was a problem hiding this comment.
✅ LGTM — reviewed and approved by Claude Code
(Recorded as a review comment because GitHub blocks self-approval on an own-authored PR; this is the sign-off of record.)
Requirement correctness
Enumerated 20 bare-cast sites at dashboard fetch/JWT/WebSocket boundaries; 5 needed runtime validation (lookup-key hazard), 15 correctly documented as accepted-risk (value-only or already-guarded downstream). Confirmed both sites the ticket explicitly flagged as possibly missed by the AAASM-5208 sweep were real gaps and fixed correctly:
features/capability/types.ts'sDECISIONS— addedisDecision()/decisionMeta(), an allow-listSet<Decision>check before the lookup, with a comment correctly tracing the hazard back toapi/capability.ts's wholesale cast.features/capability/sort.ts'sDECISION_WEIGHT— same pattern viadecisionWeight(), correctly noting the un-guarded version would have producedNaNcomparisons, not just a wrong sort.
Two new findings beyond the ticket's named list, both real: SeverityBadge.tsx/StatusBadge.tsx share a lookup boundary reached by both a validated path (/alerts list, canonicalized via parseAlert.ts) and an unvalidated path (single-alert detail/rules endpoints) — correctly fixed at the shared boundary rather than chasing every caller. AuthProvider.tsx's login-response scopes field was unfiltered unlike the JWT-claim fallback — fixed by reusing a new shared isScope() guard rather than duplicating the allow-list, and correctly changed the fallback condition to "when the response carried none at all" rather than "always OR", which is a meaningful behavior distinction (preferring a validated empty array from the server over silently falling back to the JWT claim).
Also correctly caught and fixed a pre-existing doc-comment inaccuracy: api/capability.ts's cast was called "safe" based on compile-time structural type identity, which doesn't address runtime content — now corrected.
All 5 fixes have regression tests proving inherited-prototype values (__proto__, constructor) are rejected rather than resolving an inherited member. Accepted-risk sites (e.g. jwtScopes.ts's getSubject) are justified with a specific, checkable reason (fixed literal key probes, not dynamic wire-derived keys) rather than a blanket "trust me."
CI — fully green
17/17 checks pass, including Dashboard e2e (Playwright), Dashboard tests + coverage, aa-cli build compat, SonarCloud ×2, CodeQL, and CI Success.
Security
This is exactly the class of fix the whole Epic has been building toward — closing the fetch-boundary root cause that a purely structural lint rule (AAASM-5245) cannot see. No new risk introduced.
Merging with a merge commit as org admin.
🤖 Reviewed with Claude Code
1 participant
Description
AAASM-5217 audit: enumerates every bare
as T/as Promise<T>cast at a dashboard fetch/WebSocket/JWT deserialization boundary, and for each site either (a) adds runtime validation where a cast-derived value is used as an object/Map lookup key, or (b) documents why the cast is accepted-risk when it isn't.This closes the gap AAASM-5245's AST lint rule cannot catch: a lookup can be declared with a correctly-constrained union key type and still receive raw, unvalidated wire data if the fetch boundary feeding it uses a bare cast. Two of the fixes below are genuine defects the original 18-site AAASM-5208 sweep missed because they're one hop removed from the cast site itself (
capability/types.ts'sDECISIONS,capability/sort.ts'sDECISION_WEIGHT) — both named explicitly in the ticket. Two more (SeverityBadge/StatusBadge,AuthProvider's login scopes) are new findings surfaced during this audit's enumeration pass.features/alerts/api.ts's cast site itself was re-examined per the ticket: AAASM-5215'sisAlertMetric()fix (merged via PR #1748) already guardsalertCategory.ts'sMETRIC_CATEGORYMap downstream of that cast using aMap(not object), so that specific lookup is validated. However, the audit found the same cast feedsSeverityBadge/StatusBadgethrough the unvalidated single-alert-detail and rules paths (useAlertQuery,useAlertRulesQuery), which bypass the list path'sparseAlertList/canonicalSeverity/canonicalStatusnormalisation — fixed at the shared badge boundary since both paths render through the same components.api/capability.ts's doc comment claimed the boundary cast was "safe" because the hand-written and codegen'd types are "structurally identical" — that's a compile-time claim about shape, not a runtime guarantee about content, and was corrected to state the actual (now-enforced) justification.Enumeration (AAASM-5247)
features/capability/types.ts:133(DECISIONS)Decision(one hop fromapi/capability.ts:31)DECISIONS[decision]isDecision/decisionMeta()features/capability/sort.ts:12(DECISION_WEIGHT)Decision(one hop fromapi/capability.ts:31)DECISION_WEIGHT[da/db]decisionWeight()features/alerts/SeverityBadge.tsx(SEVERITY_BG)Severity(one hop fromalerts/api.ts:57via unvalidated detail/rules paths)SEVERITY_BG[severity]isSeverity()features/alerts/StatusBadge.tsx(STATUS_STYLE)AlertStatus(same paths)STATUS_STYLE[status]isAlertStatus()auth/AuthProvider.tsx:29(login())Scope[](feedsSCOPE_RANK[s]inusePermissions.ts)isScope()features/alerts/api.ts:57(alertsFetch)Tfeatures/analytics/analyticsFetch.ts:23TObject.create(null)api/capability.ts:31(getMatrix)CapabilityMatrixfeatures/onboarding/api.ts:47,54GatewayHealthfeatures/audit/api.ts:74Record<string, unknown>auth/jwtScopes.ts:43(getSubject)Record<string, unknown>features/liveOps/useLiveOpsStream.ts:274GovernanceEventmapEventallow-list-validates before lookupfeatures/alerts/useAlertsStream.ts:111AlertsStreamFrameframe.alertgoes throughnormaliseAlertfeatures/approvals/useApprovalsStream.ts:64GovernanceEvent/ApprovalPayload===comparisons / text / Set membershipfeatures/topology/api.ts:64TopologyGraphResponsemapGraph.tsallow-list-validatesfeatures/topology/api.ts:94RecentEvent[]features/iam/apiKeys.ts:83,94,115ApiKey[]/GeneratedApiKey===/text/Array.includesonlyfeatures/teams/api.ts:42,89,144,165TopologyOverview/CostSummary/TeamTopology/AgentLineagecomponents/ViolationHeatmap.tsx:172ViolationNode(d3 datum, not wire)pages/ViolationHeatmapPage.tsx:64ApiResponseagent_idis a React/d3 key, not object index5 sites required runtime validation; 15 documented as accepted-risk.
Type of Change
Breaking Changes
Related Issues
Testing
capability/__tests__/types.test.ts,capability/__tests__/sort.test.ts,alerts/SeverityBadge.test.tsx,alerts/StatusBadge.test.tsx,auth/jwtScopes.test.ts,auth/AuthProvider.test.tsx), each proving an inherited-prototype-member wire value ("__proto__","constructor") is rejected or safely handled.pnpm exec tsc --noEmit,pnpm exec eslint .,pnpm exec vitest runall clean (275 test files, 3053 tests passing).Checklist