fix: CATS run-validity fixes, two API contract bugs, and a Unicode validation bypass by ericfitz · Pull Request #635 · ericfitz/tmi

ericfitz · 2026-07-30T21:21:44Z

Work from a full /test suite run: getting the suite green, getting CATS to
produce a valid campaign, and then triaging what that campaign found.

Test-suite repairs

Three separate things were broken, each masking the next.

fix(deps) — tmi-clients regenerated its Go clients and deleted the
v1_5_0 directory this module's replace pointed at, so cmd/dbtool would
not build and every CATS campaign failed at seeding. Repointed at v1_6_0
(drop-in; no call sites move).
fix(test) — test/integration's go.mod had drifted behind the root
module it replaces, and Go refused to run the suite. This was quiet in the
worst way: make test-integration reported "22 passed, 0 failed" while the
entire workflows package — 60 tests — never compiled. With it fixed: 82
passed, 0 failed.
test(api) — the Postman modified_at assertion compared the server's
clock to the runner's with zero tolerance, so it failed deterministically by
~2ms against a remote CLUSTER=k3s server. Allows 5s of skew.

CATS run validity

fix(dbtool) — run 20260730T153736Z destroyed 15 of its own seeded fixtures
and the #625 gate correctly marked it INVALID. Two losses were leaves with no
decoy: an ordinary DELETE consumed the only seeded team note and webhook. Adds
decoys for both, and pins the corresponding globals by name — they resolved
via findRefByKind, which ranges over a map, so a second seeded record made the
lookup nondeterministic.

Fixture destruction went 15 → 1 → 0 across three campaigns.

API bugs found by the campaign

fix(api): negotiated media types — text/csv, application/x-ndjson,
application/yaml and application/graphml+xml sat in a server-wide Accept
allowlist though only two operations produce them. Endpoints doing no
negotiation of their own passed the gate and answered 200 with JSON to a
client asking for CSV. RFC 9110 and the spec both call for 406. 93 findings
across 83 unrelated paths.
fix(api): Unicode validation bypass — UnicodeNormalizationMiddleware
scanned the raw pre-parse body, where \uXXXX is six ASCII bytes. Every
check it performs was evadable by escaping the character. Confirmed live: a
literal U+200B in a team name is rejected 400, the escaped form returns 201
and the zero-width space is stored in the database. Now re-checks decoded
strings, walking keys as well as values.
fix(api): status codes — 63 undocumented-response-code findings where
the handler was right and the spec incomplete (202 on the webhook test, 200 on
two bulk PUTs). Version 1.6.0 → 1.6.1.

Triage

test(cats) — 12 false-positive rules taking the run from 2,980 true
positives to 1,122, each keyed on the error the server actually returned
rather than on a fuzzer name, so unrelated failures on the same paths still
surface. Every security-flavoured suppression is backed by positive evidence,
not assumption:

SSTI: 0 of 33 responses contained 8051 (the evaluated 83*97); all 33
held the payload verbatim. CATS's engine keyword matched our own seeded role
engineering_lead.
"Error details leak": the leaked keyword is unauthorized — the RFC 6749
error code in the standard 401 body.
SSRF: team/project uri is stored and echoed, never dereferenced. Scoped to
those four paths only; Timmy's content-fetch paths, which do dereference
URIs, stay in scope.
MassAssignment: 0 of 661 responses echoed the injected field, so unknown
fields are dropped rather than bound. Carries a forward-looking /admin/
exclusion.

Settings CLI

feat(scripts) adds a system-settings CLI, and fix(scripts) fixes what a
security review of it found: the OAuth callback treated state as optional and
would accept a token handed straight to the callback, so a page in the
operator's browser could hand the tool an identity of its choosing. Now
state-bound with compare_digest before anything else is read, PKCE-bound
tokens only, 127.0.0.1 explicitly, and --token-file instead of a token in
argv.

Verification

make lint 0 issues · make build-server 1.6.1 · make test-unit 2447 passed,
0 failed · make test-integration 82 passed, 0 failed · make test-api 327
assertions, 0 failed · CATS 20260730T174620Z valid (0 fixtures destroyed,
transport errors 0.07%, non-FP 401s 0.14%).

Not included

fix(api): bulk threat PATCH returns 500 on a malformed JSON Pointer instead of 400 #632 (500 on a malformed JSON Pointer) is filed but not fixed — 71
findings remain.
409 "Not matching response schema" findings do not reproduce as schema
violations under a format-checking, $ref-resolving validator and are
deliberately left unsuppressed. /triage/ and /intake/ survey_responses
reference the identical schema yet only one is flagged, so the trigger is
not the schema. Wants a fresh run to re-derive.
The two API fixes are not yet deployed; verified by unit tests and by hand,
not against the cluster.

🤖 Generated with Claude Code

https://claude.ai/code/session_01WnrVyySuyJFqcMxB8q3pNc

tmi-clients regenerated its Go clients to 1.3.0/1.6.0 and removed the v1_5_0 directory this module's `replace` pointed at, so `go build ./cmd/dbtool` failed with "replacement directory ... does not exist". That broke `make cats-seed` and therefore every CATS campaign. v1_6_0 is a drop-in: only the import path changes, no call sites move, and the version now matches the server it is generated from. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WnrVyySuyJFqcMxB8q3pNc

test/integration is its own module and `replace`s the parent to ../.., so the root module's dependency bumps raise its minimum versions too. Its go.mod had drifted behind, and Go refused to run the suite at all: "updates to go.mod needed; to update it: go mod tidy". The failure was quiet in the worst way. `make test-integration` still reported "22 passed, 0 failed" because the api/ package ran fine; the workflows package -- 60 tests across workflows, tier2_features and tier3_edge_cases -- never compiled. Only the skip-is-a-failure gate added in 41abfe5 turned that into a non-zero exit. Versions only, no code changes. With this the full suite runs: 82 passed, 0 failed, 9 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WnrVyySuyJFqcMxB8q3pNc

The assertion compared the server's modified_at against the test runner's own clock with no tolerance: pm.expect(new Date(responseData.modified_at)).to.be.at.most(new Date()) That holds only while the server shares a clock with the runner. Against CLUSTER=k3s the server is a different host, and the check failed deterministically by ~2ms (…27.065Z vs …27.063Z) on every run -- normal NTP drift, not a server fault. Allow 5s of skew. Still catches a timestamp that is genuinely wrong (unset, epoch, or far-future) while surviving a remote server. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WnrVyySuyJFqcMxB8q3pNc

…ating them Run 20260730T153736Z destroyed 15 of its own seeded fixtures and the #625 integrity gate correctly marked it INVALID. Two of the losses were leaves with no decoy at all: an ordinary DELETE on /teams/{team_id}/notes/{team_note_id} and /admin/webhooks/subscriptions/{webhook_id} consumed the only seeded record, and every test nested under them then ran against a 404. Decoys existed only for anchors, on the theory that leaves are not fuzzed destructively. They are. Seed a throwaway note and webhook, and point each leaf path at them. The note hangs off the REAL team on purpose: only the /teams/{team_id} anchor is overridden, so the nested path still substitutes the real team_id and a note under the throwaway team would just 404. Both globals had to be pinned by name as well. teamNoteID and webhookID resolved via findRefByKind, which ranges over a map -- with a second record seeded it could return either one, and returning the decoy would leave the real fixture unfuzzed while the decoy absorbed both roles. This is the hazard the survey-response:0 lookup already guards against. Adds teamNoteRef/teamNotePrefix so the ref prefix and the lookup cannot drift; transformTeams now builds its prefix from the same helper. TestFixtureManifestMarksDecoyedAnchors asserted these two were undecoyed, which is exactly what this inverts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WnrVyySuyJFqcMxB8q3pNc

Drives GET/PUT/DELETE on /admin/settings/{key} against a running server, authenticating through a browser PKCE flow. Interactive auth is required by design: service-account tokens are denied on /admin/* (#399). Secrets go in via --value-file rather than argv, and the tool never echoes a value back after a write. Note what it cannot do. Settings supplied through the environment or a config file outrank the database, and the server returns 409 for a PUT to any such key. On Kubernetes deployments that inject config via envFrom that includes every OAuth provider key, so this is not a way to rotate provider credentials -- see #631. The 409 handler says so explicitly rather than leaving the caller to guess. Not yet exercised end to end; see the follow-up issue for reworking it onto the generated Python client. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WnrVyySuyJFqcMxB8q3pNc

…s return CATS run 20260730T174620Z flagged 63 undocumented-response-code true positives across three operations. In every case the handler is right and the spec was incomplete, so document the behaviour rather than change it (per the documented-status-code policy). POST /admin/webhooks/subscriptions/{webhook_id}/test -> 202 (39 hits) webhook_handlers.go:358 returns StatusAccepted: the delivery is created and queued, and its outcome is reported separately via the deliveries endpoints. 202 is the honest code for that. PUT /threat_models/{threat_model_id}/assets/bulk -> 200 (23) PUT /threat_models/{threat_model_id}/repositories/bulk -> 200 (1) Both bulk upsert handlers return StatusOK unconditionally (asset_sub_resource_handlers.go:647, repository_sub_resource_handlers.go:640). Only 201 was documented. 201 is left in place on the bulk PUTs. Those handlers never actually emit it, so it looks like dead documentation, but removing a documented code is a contract change for clients rather than a spec correction -- worth a separate look. Version 1.6.0 -> 1.6.1 (manual bump, #627). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WnrVyySuyJFqcMxB8q3pNc

…itives SSRFInUrlFields sets the uri field to an internal address and flags the value coming back in the response body. For teams and projects that echo is not evidence of a request the server made: the field is persisted and serialized (team_store_gorm.go:954) and nothing dereferences it. Scoped deliberately to the four team/project contract paths. URIs are genuinely fetched elsewhere -- Timmy pulls document and diagram content over HTTP (timmy_content_provider_http.go) through SafeHTTPClient and URIValidator -- and an SSRF finding there would be real, so the rule must not reach those paths. Suppresses 172 findings, 5.46% of the run's true positives. That is above the 5% review threshold, confirmed with the maintainer before adding; the share reflects how many tests the fuzzer aimed at these four paths, not a loose condition. Verified against run 20260730T174620Z: dry run and live reclassification both moved exactly 172 (3152 -> 2980 true positives), and the rule matched no 5xx. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WnrVyySuyJFqcMxB8q3pNc

AcceptHeaderValidation kept a coarse server-wide allowlist that included four representations only two operations actually produce: text/csv and application/x-ndjson (system-audit export), application/yaml and application/graphml+xml (diagram model). The comment acknowledged the gate was blunt and assumed per-endpoint negotiation would make the precise choice -- but endpoints that do no negotiation of their own never make that choice. They passed the gate and answered 200 with JSON to a client that asked for CSV. RFC 9110 calls for 406 there, and the spec documents 406 on those operations, so this was a contract violation as well as a correctness one. CATS run 20260730T174620Z hit it 93 times, all with `Accept: text/csv`, across 83 paths that have nothing to do with CSV. Scope each type to the routes that produce it. The two handlers that call negotiateContentType still return 406 themselves for a type they do not offer, so this gate only has to stop the type being waved through everywhere else. Verified by hand before the change: application/xml, text/plain, text/html, image/png and nonsense/type were already 406, while text/csv passed the gate alongside application/json -- which is what singled out the negotiated types as the cause. Four existing subtests asserted the old server-wide behaviour on an arbitrary route. They now assert 406 there, which is the behaviour change stated as a test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WnrVyySuyJFqcMxB8q3pNc

UnicodeNormalizationMiddleware scanned the request body as raw text. In raw text a \uXXXX escape is six ASCII bytes, so every check it performs -- dangerous zero-width, BiDi overrides, Hangul fillers, fullwidth structural characters, control characters, excessive combining marks -- could be evaded simply by escaping the character instead of emitting it literally. Confirmed against a running server. POST /teams with a literal U+200B in the name is rejected 400 by this middleware. The byte-identical character sent as returns 201, and reading the team back shows the zero-width space stored in the database. Re-run the same checks on the decoded strings, walking object keys as well as values. Keys matter independently: a zero-width character inside a field name makes the field unrecognized, so it is silently dropped and the request still succeeds -- which is how CATS's ZeroWidthCharsInNamesFields got 96 2xx responses in run 20260730T174620Z by sending "responsible_parties". The raw-text scan stays: it catches literal characters, and it still runs first. The new pass only adds rejections. A body that does not parse is left to ordinary request validation rather than being rejected here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WnrVyySuyJFqcMxB8q3pNc

Ten rules covering findings from run 20260730T174620Z where the server did the right thing and CATS expected otherwise. Security-flavoured findings, all disproved by evidence rather than assumption: AUTH_ERROR_CODE_IS_NOT_A_DETAILS_LEAK (39) -- the "leaked" keyword is "unauthorized", the RFC 6749 error code, in the standard 401 body. SSTI_PAYLOAD_REFLECTED_NOT_EVALUATED (33) -- 0 of 33 responses contained 8051, the evaluated form of the 83*97 payload; all 33 contained the payload verbatim. CATS's "engine" keyword matched our own seeded member role "engineering_lead". Requiring the unevaluated payload keeps a real evaluation surfacing. SSRF_URI_VALIDATION_REJECTS_UNRESOLVABLE_400 (117) -- this is the SSRF control reporting itself. ssrf_validator.go resolves the host and checks every IP against the blocklist, and safe_http_client.go dials the validated IP instead of re-resolving, so the lookup cannot be raced by a DNS rebind. Correct rejections of mangled payloads (600 + 46 + 18) and genuine state conflicts (230 + 87 + 15): nested schema violations, undecodable base64, corrupted pagination cursors, duplicate metadata keys, restoring a non-deleted entity, and a second threat model from one survey response. Each is keyed on the error the server actually returned rather than on the fuzzer name, so an unrelated 400/409 on the same path still surfaces. Together with the MassAssignment rule this takes the run from 2,980 true positives to 1,134. Nothing matched a 5xx. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WnrVyySuyJFqcMxB8q3pNc

…positives POST /threat_models/{threat_model_id}/chat/sessions streams text/event-stream, the only content type its 200 response declares. CATS wraps the non-JSON body in a notAJson envelope and then reports that it "does not match the corresponding schema" -- there is no application/json schema for it to match, which is correct and intentional. Found while diagnosing the "Not matching response schema" bucket: this was the one operation of 23 with no application/json schema declared, and its own evidence settles it (response_content_type is text/event-stream, exactly as documented). 12 findings. The remaining 409 in that bucket do not reproduce as schema violations and are deliberately left unsuppressed pending a fresh run -- see the analysis notes on the branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WnrVyySuyJFqcMxB8q3pNc

…t of argv Security review of the new settings CLI found three problems in its browser login. All are in code added earlier on this branch. State was effectively optional. The check read if result.get("state") not in (None, state): so a callback carrying no state at all satisfied it -- the parameter was only validated when the sender chose to supply it. Worse, the branch below returned any access_token found in the callback without the state having constrained anything. Together that meant a page open in the operator's browser could issue a cross-origin GET to the listener and hand the tool an identity of its choosing for the rest of the run. The ephemeral port raises the cost of finding the listener but does not prevent it, and the window stays open for the full five-minute timeout. Now: - The handler compares state with secrets.compare_digest before it looks at anything else, so a callback that is not bound to this flow is 400'd and never even captured. A missing state fails exactly like a wrong one. - authenticate() re-checks rather than trusting a check made far away in the file. - A token delivered straight to the callback is refused. It is not bound to the PKCE verifier this process generated, so accepting it would defeat the point of using PKCE. Only a token the server mints in exchange for our code and verifier is used. - The listener binds 127.0.0.1 explicitly instead of the "localhost" name. Verified by driving the handler directly: forged callbacks with no state, with a token and no state, and with a wrong state all return 400 and capture nothing; the correctly-stated callback returns 200 and is captured. Separately, --token is replaced by --token-file. A bearer token in argv is readable by any local process via ps and lands in shell history, which is the same reason setting values already use --value-file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WnrVyySuyJFqcMxB8q3pNc

ericfitz and others added 12 commits July 30, 2026 16:26

ericfitz merged commit 2f8bdf4 into main Jul 30, 2026
17 of 20 checks passed

ericfitz deleted the worktree-cats-fixes-and-spec branch July 30, 2026 21:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: CATS run-validity fixes, two API contract bugs, and a Unicode validation bypass - #635

fix: CATS run-validity fixes, two API contract bugs, and a Unicode validation bypass#635
ericfitz merged 12 commits into
mainfrom
worktree-cats-fixes-and-spec

ericfitz commented Jul 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

ericfitz commented Jul 30, 2026

Test-suite repairs

CATS run validity

API bugs found by the campaign

Triage

Settings CLI

Verification

Not included

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant