Conversation
…e mappable SAML login no longer overwrites a stored name with an empty or synthesized assertion value, updates email symmetrically on strong (provider-id or linked-identity) matches, sets LastLogin like the OAuth path, and given_name/family_name become operator-mappable per SAML provider via SAML_PROVIDERS_<PROVIDER_ID>_GIVEN_NAME_ATTRIBUTE / SAML_PROVIDERS_<PROVIDER_ID>_FAMILY_NAME_ATTRIBUTE. Explicit config defaults for email/name are unnecessary: the well-known OID/WS-Fed fallbacks already cover them. Refs #648 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PhifvpsfXEJcxhYS2No4pD
…rt vocabulary Every generated ORDER BY in the threat store (semantic, plain, and default/fallback paths) and the fixed orderings in the asset, document, note, and repository stores now carry a unique 'id ASC' tiebreaker, so rows tied on the sort key cannot shuffle between LIMIT/OFFSET windows. The status rank map now covers the real status vocabulary (open, confirmed, deferred, mitigation_planned, mitigation_in_progress, verification_pending, resolved, accepted, closed, false_positive), keeping legacy values at nearby ranks. Refs #645 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PhifvpsfXEJcxhYS2No4pD
…lt-order constant Review follow-up on #645: the SQLite-backed TestSortPaginationStability was a vacuous guard, since SQLite breaks ties on rowid deterministically with or without the tiebreaker. It's kept as an intent-documenting smoke test, and TestSortPaginationStability_Integration (PostgreSQL, make test-integration) now exercises the same scenario against a backend that makes no such promise, so a regressed tiebreaker can actually be caught. Also removes DefaultSortOrderCreatedAtDesc (api/auth_utils.go), which became dead code once the threat store switched to defaultOrder() -- and was specifically the un-tiebroken order string, a footgun left lying around. Refs #645 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PhifvpsfXEJcxhYS2No4pD
…efaults in quota list responses UserAPIQuota.MaxRequestsPerMinute gorm default 100 -> 1000 and AddonInvocationQuota.MaxActiveInvocations 1 -> 3, matching DefaultMaxRequestsPerMinute and DefaultMaxActiveInvocations. The three admin quota list responses now carry a required 'defaults' block so clients stop transcribing Go constants. dbtool setQuotas no longer lets a zero rpm fall through to the DB column default. Refs #649 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PhifvpsfXEJcxhYS2No4pD
The markers added for #649 were stamped with the pre-commit HEAD sha (the tool's documented fallback for uncommitted entities); refresh them to the actual commit that landed the behavior change. Refs #649 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PhifvpsfXEJcxhYS2No4pD
Loads the embedded OpenAPI spec and asserts every constant in api/bulk_limits.go equals the maxItems of the operation(s) it guards, in both directions for the per-entity metadata map, so handler/spec drift fails unit tests instead of shipping. Closes #663 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PhifvpsfXEJcxhYS2No4pD
…tions Review follow-up on #663: TestBulkThreatDeletesMatchSpec's assertions were bare, so a failure wouldn't name the operation or the two mismatched values. Also hardens requestBodyMaxItems against a missing application/json media type and entityForMetadataPath against a too-short path. Related to #663
Adds features.timmy_enabled to the public client config, true only when Timmy is enabled AND fully configured — the same two-stage gate the Timmy middleware enforces — so clients can gate the launcher UI. Refs #644 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PhifvpsfXEJcxhYS2No4pD
is_builtin derives from the fixed seeded-group UUID allowlist (validation.IsBuiltInGroup) — the same predicate the GORM protection hooks enforce — rather than a persisted flag or name matching, so it cannot drift or collide with admin-created groups (which share the "tmi" provider). GET /admin/groups gains a built_in boolean filter. Refs #656 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PhifvpsfXEJcxhYS2No4pD
Refs #656 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PhifvpsfXEJcxhYS2No4pD
GET /admin/users/{user_id}/identities mirrors /me/identities (primary
from the user record, linked rows from the linked-identity store);
DELETE /admin/users/{user_id}/identities/{identity_id} unlinks with a
real 404 for unknown ids. The primary identity lives on the user record
and is structurally not unlinkable; content-provider tokens are
deliberately untouched (admins revoke those via the existing
content_tokens endpoints). No admin link endpoint by design.
The DELETE operation defaults to step-up-required (no opt-out set),
consistent with the other user-credential-adjacent admin writes
(client_credentials, user delete); a matching audit descriptor was
added to satisfy TestAdminAuditDescriptors_OpenAPISpecCoverageGate.
Refs #646
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PhifvpsfXEJcxhYS2No4pD
GET /admin/users/{user_id}/identities and DELETE
/admin/users/{user_id}/identities/{identity_id} both route store errors
through StoreErrorToRequestError, which returns 503 on transient DB
faults — and every other admin operation already documents 503. The
brief's "skip 503" instruction for these two ops was in error; add the
response back so the spec matches actual server behavior per the
Documented-Status-Code policy.
Refs #646
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PhifvpsfXEJcxhYS2No4pD
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PhifvpsfXEJcxhYS2No4pD
…reat_type Oracle review follow-up on #645, two blocking findings: - buildSemanticOrderExpr iterated the rank map with a bare range, so the generated CASE WHEN clause order was random per call. Combined with GORM's PrepareStmt cache (keyed on exact SQL text) and status/severity/priority sorts not qualifying for the query-plan cache bypass, every such request on Oracle produced a fresh prepared statement and open cursor, eventually exhausting the session's cursor limit (ORA-01000). Keys are now sorted before emission, so a given (column, orderMap) always renders identical SQL text. - threat_type was listed as a sortable column, but ThreatType is a StringArray, which maps to CLOB on Oracle, and Oracle rejects ORDER BY on LOB columns (ORA-00932) -- a documented, spec-legal sort parameter would 500 only on the Oracle backend. Removed it from validColumns; unknown columns already fall back to defaultOrder(), so the sort degrades gracefully instead of erroring. Also adds Oracle-dialect unit coverage via a minimal fake gorm.Dialector (reports "oracle", never opens a real connection) plus a subtest pinning threat_type's fallback behavior. Refs #645 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PhifvpsfXEJcxhYS2No4pD
Oracle re-review's only note on the #645 sort fixes (APPROVED WITH NOTES): the slices.Sort determinism fix had no regression guard -- nothing asserted buildSemanticOrderExpr's output was stable, so reverting the sort would have gone green through the whole suite. Adds a subtest to TestBuildSemanticOrderExpr pinning the exact canonical CASE WHEN string for severityOrder (also documents the expected SQL text) and calling buildSemanticOrderExpr 50 times to catch any residual non-determinism. Verified by hand-reverting the slices.Sort fix locally: the new subtest fails as expected, then restored the fix. Test-only change, no production code. Refs #645 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PhifvpsfXEJcxhYS2No4pD
The nested integration module drifted when #676 and #680 bumped root dependencies without the companion tidy; make test-integration could not build the workflow tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PhifvpsfXEJcxhYS2No4pD
54 markers re-anchored/rewritten across 17 files for entities whose behavior changed in the #644-#663 batch, including the stale group-repository List/Count descriptions flagged in review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PhifvpsfXEJcxhYS2No4pD
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PhifvpsfXEJcxhYS2No4pD
…n claim Final #645 verification: mutation-tested the PostgreSQL pagination test by hand -- temporarily dropped orderTiebreaker's ", id ASC" suffix and reran TestSortPaginationStability_Integration through make test-integration-pg (GOFLAGS='-count=1' so nothing came from cache). It passed anyway. PostgreSQL's tuplesort was stable enough on this fresh, sequentially inserted 25-row fixture that removing the tiebreaker produced no visible duplicate or missing row, so neither this test nor its SQLite counterpart (which never claimed otherwise) actually catches a reverted tiebreaker. Corrects both tests' doc comments to stop calling the PostgreSQL variant "the behavioral regression guard" -- it's real end-to-end coverage of paginated semantic sorting, just not proof against this specific regression. The property that IS proven to catch a reverted tiebreaker is the exact ORDER BY / CASE WHEN text pinned in TestBuildOrderBy and TestBuildSemanticOrderExpr; both comments now point there instead. Test-only change, no production code. Refs #645 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PhifvpsfXEJcxhYS2No4pD
updateSAMLUserOnLogin treated samlMatchLinkedIdentity as a "strong" match tier eligible to update the stored user's email, on par with samlMatchProviderID. That is wrong: a linked (secondary) identity's asserted email is display-cache only, not authoritative, per the #383 identity-link design — the primary account's identity is anchored to its provider_id, and a linked identity exists to let a user log in through an additional IdP without becoming a second identity. Left unguarded, this let an attacker link a secondary SAML identity whose IdP-asserted email they control, set it to a victim's email, and log in: the linked-identity match tier would rewrite the primary row's email to the attacker-chosen value. users.email carries no unique index and GetUserByEmail uses a first-match lookup, so a later email-addressed grant or invite could then bind to the attacker instead of the victim. The fix restricts the email-update guard to samlMatchProviderID only, matching the OAuth path: handlers_oauth_user.go defines a userMatchLinkedIdentity constant, but its update switch (updateUserOnLogin) has cases only for ProviderID, ProviderEmail, and EmailOnly — it never had an update case for linked identities either. Name updates on a linked-identity match are unaffected and remain intended, since the display name carries no such trust implication. Refs #648 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PhifvpsfXEJcxhYS2No4pD
Closed
5 tasks
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
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.
Seven milestone issues, each implemented as its own commit with a spec-and-quality review pass, plus the 1.8.0 version bump. Reviewed end to end by an Oracle DBA gate (2 blocking findings, fixed and re-approved), a whole-branch review (READY), and a security review (1 Medium, fixed).
Closes #644
Closes #645
Closes #646
Closes #648
Closes #649
Closes #656
Closes #663
What's in it
#648 — SAML login no longer corrupts stored profiles.
updateSAMLUserOnLoginwas overwriting the stored name on every login with no guard, and never updating email. A bare!= ""check would not have fixed it: the attribute pipeline synthesizes<subject>@<provider>.saml.tmiand derives a name from that local-part, so the value is never empty — just garbage.saml.UserInfonow carriesEmailSynthesized/NameSynthesizedflags, and updates are tier-aware, mirroring the OAuth path.given_name/family_namebecome operator-mappable per provider viaSAML_PROVIDERS_<ID>_GIVEN_NAME_ATTRIBUTE/_FAMILY_NAME_ATTRIBUTE.#645 — paginated list sorts are stable. Every generated ORDER BY in the threat store, plus the fixed orderings in the asset/document/note/repository stores, now carries a unique
idtiebreaker, so rows tied on the sort key can no longer shuffle between LIMIT/OFFSET windows (duplicated and dropped rows while paging). The status rank map now covers the real vocabulary, keeping legacy values at nearby ranks.#649 — quota defaults agree with themselves.
UserAPIQuota.MaxRequestsPerMinuteGORM default 100 → 1000 andAddonInvocationQuota.MaxActiveInvocations1 → 3, matching the app constants (the addon mismatch was found during the work, not in the issue). The three admin quota list responses now carry a requireddefaultsblock so clients stop transcribing Go constants.#663 — bulk limits are pinned to the spec. A unit test loads the embedded OpenAPI document and asserts every constant in
api/bulk_limits.goequals themaxItemsof the operations it guards, bidirectionally for the per-entity metadata map. Both drift directions had already happened in this repo.#644 —
features.timmy_enabledonGET /config, true only when Timmy is both enabled and fully configured — the same two-stage gate the middleware enforces, so the flag never advertises an endpoint that would 503.#656 —
is_builtinon AdminGroup + abuilt_infilter. Derived from the fixed seeded-group UUID allowlist (validation.IsBuiltInGroup, the same predicate the GORM protection hooks use) rather than a persisted column or name matching, so it cannot drift or collide with admin-created groups — which share thetmiprovider, the reason the client's name-matching workaround existed.#646 — admin endpoints for a user's linked sign-in identities.
GET /admin/users/{user_id}/identitiesandDELETE .../identities/{identity_id}. The primary identity lives on the user record and so is structurally not unlinkable; content-provider tokens are deliberately untouched (admins revoke those via the existingcontent_tokensendpoints); no admin link endpoint by design, since linking needs the target user's own consent.Findings from the review gates
Oracle (BLOCKING, fixed):
buildSemanticOrderExpriterated its rank map with a barerange, so CASE WHEN clause order was random per call. With GORM's uncappedPrepareStmtcache and sorted queries bypassing the query cache, every status-sorted request on Oracle minted a fresh prepared statement and open cursor — ORA-01000 on a hot endpoint, invisible on PostgreSQL. Keys are now sorted before emission. Separately,threat_typewas a documented sortable column but maps to CLOB on Oracle, which rejects ORDER BY on LOBs (ORA-00932 → an Oracle-only 500); it was removed from the whitelist and now degrades to the default order.Security (Medium, fixed): a linked (secondary) SAML identity could rewrite the authoritative
users.email. The OAuth path deliberately has no linked-identity update case — a linked identity's email is display cache only — sostrongMatchnow admits the provider-id tier alone.Release notes
sort=threat_typenow falls back to the default order instead of sorting by threat type. It was only ever sortable on PostgreSQL and errored on Oracle; the parameter is a free-form string with no enum, so this is spec-legal, but a saved client query using it changes behavior silently.DELETE /admin/users/{user_id}/identities/{identity_id}is step-up gated (consistent with other user-credential-adjacent admin writes). An admin whose token is older than the step-up window gets a401with aWWW-Authenticatechallenge; the client needs to handle that.Verification
make lintclean,make build-serverprints 1.8.0,make test-unit2487 passed / 0 failed / 10 skipped.make test-integrationgreen, including a newTestSortPaginationStability_Integration. Getting there required tidying thetest/integrationnested module, which had drifted since chore(deps): bump the go-minor-patch group with 5 updates #676/chore(deps): bump dependencies #680 and left the workflow tests unbuildable — that drift had been masking two pre-existing webhook test failures, now filed as fix(test): TestWebhookDelivery shared-fixture race — subtests intermittently see prior subtest's delivery status #691.TestBuildOrderBy/TestBuildSemanticOrderExpr, verified by reverting the fix and watching it fail.Follow-ups filed
#682 (Oracle column-default backfill), #684 (
PrepareStmtMaxSizecap — the Oracle SME asked for this first), #685 (ORA-00932/01000/04031 classification), #686 (id DESC+ composite-index optimization), #687 (EmailVerifieddowngrade on token mint), #688 (narrow the Timmy settings read on public/config), #689 (compile gate for build-tagged test files), #690 (nested-module tidy automation), #691 (webhook test flake).🤖 Generated with Claude Code
https://claude.ai/code/session_01PhifvpsfXEJcxhYS2No4pD