security: harden auth surface and add cross-backend store conformance suite - #39
Merged
Merged
Conversation
The refresh replay branch re-ran the family cascade and re-emitted the refresh_token_replayed audit + hook event on every presentation of a revoked token, with no idempotency. A client stuck re-presenting an already-rotated token (e.g. a frontend that never persists the rotated token) produced one alert per attempt -> a runaway audit stream. Add an atomic store method MarkRefreshTokenReplayed(hash) that conditionally upgrades a revoked token's reason rotated->replay_detected and reports whether it made the transition. The engine now cascade-revokes the family and emits the security alert only on that first transition; repeat presentations are refused quietly. The conditional UPDATE is race-safe, so concurrent double-submits also resolve to a single alert. - session.Store: +MarkRefreshTokenReplayed, implemented in postgres, sqlite, mongo, memory - engine Refresh: alert + cascade gated on firstReplay - tests: engine "alert fires once across 5 replays" + store idempotency
… suite Fixes the findings from a full security review plus adds test infrastructure to prevent backend divergence going forward. All changes are test-first and the full suite passes (go vet, -race, and memory+sqlite conformance locally; postgres+mongo conformance in CI). Access control - Add authentication + ownership/tenant scoping to previously unauthenticated endpoint groups: sessions, devices, webhooks, environments, and the organization plugin routes (membership/role authz + self-escalation and cross-org guards). - Fix the confused-deputy class in the config, RBAC, and admin/impersonation handlers: every handler now verifies the target resource belongs to the caller's tenant (scopedAppID / roleInCallerApp / userInCallerApp). Sessions & tokens - Refresh no longer downgrades a JWT access token to opaque. - Refresh rotation uses a compare-and-swap (RotateSession) to close a TOCTOU race; a concurrent family-revoke that deletes the session is treated as a benign lost race rather than a 500. - Allocate the MFA ceremony store eagerly to remove a request-time data race. - settings.Get falls back to the registered default (not the zero value) on a store error, so cookie Secure/HttpOnly can't silently flip off. Auth strategies - MFA verification endpoints are rate limited; TOTP codes are single-use within their window; TOTP/SMS secrets are encrypted at rest; SMS OTP compare is constant-time. - WebAuthn clone detection (CloneWarning) now rejects the login. - Implement passwordless (discoverable) passkey login: per-ceremony cookie correlation, FinishDiscoverableLogin with a user-handle resolver, and real session issuance. - SSO refuses to link a login to a pre-existing unverified local account. - Magic-link resolves the real user by email (was bound to a random id, so every verification 500'd); unknown emails get a uniform response. Network - Trusted-proxy-aware client IP resolution: X-Forwarded-For / X-Real-IP are honored only when the peer is a trusted proxy, closing spoofing of rate-limit keys, session IP-binding, and IP allow/block lists. Storage - Mongo: DeleteApp now cascades to all app-scoped collections in a MongoTx; ListUsers sets Total; the email filter is escaped (regexp.QuoteMeta) to stop regex/ReDoS injection. - memory: ListUsers now applies the email filter (previously ignored, so admin search returned all users); DeleteApp idempotency unified across backends. Store conformance suite - Add store/storetest: a backend-agnostic contract suite (app/user/session/org CRUD, delete cascade, tenant scoping, ListUsers total/filter, RotateSession CAS, refresh replay). Runs against memory + sqlite in normal CI (no Docker) and postgres + mongo under -tags integration. - CI now runs the integration tests against real Postgres (testcontainers) and a single-node Mongo replica set; previously integration tests never ran.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Clears all 14 Go Dependabot alerts (7 critical + 2 high + 4 medium on golang.org/x/crypto <0.52.0, and one medium on quic-go <=0.59.0). Build and the crypto-dependent test paths (account/apikey/tokenformat/passkey/mfa) pass.
…p, picomatch) Bumps docs/ next 16.1.6 -> 16.2.6 and pins path-to-regexp>=8.4.0 / picomatch>=4.0.4 via pnpm overrides. docs build passes.
Pins the vulnerable transitive deps in the ui monorepo to their patched versions: next 15.5.16, vite 6.4.3, ws 8.21.0, brace-expansion 5.0.7, minimatch 9.0.7, picomatch 4.0.4, postcss 8.5.10, @babel/core 7.29.6, turbo 2.9.14, uuid 11.1.1. lodash resolves to 4.18.0 (dev-only, via @storybook/test). esbuild left to vite's own bump (low severity). ui build and typecheck pass.
…gration job Lint (32 issues from the new security/conformance code): - name multi-result returns (ValidateTOTPStep, resolveDiscoverableUser) - set Secure on the passkey ceremony cookies (from request scheme) + #nosec G124 - rename shadowing/reassigned err vars flagged by govet shadow + gocritic sloppyReassign across api/plugins/service - use httptest.NewRequestWithContext (noctx); drop unused runner param; avoid the id-package import shadow in storetest; //nolint:unparam on the general passkey audit/relay/hook helpers CI: - Scope the Postgres/Mongo conformance job to `-run TestConformance` so it no longer runs the older per-backend store_test.go suites (pre-existing postgres users.env_id FK failures) or the hanging mongo migration-lock test. - Mark that job advisory (continue-on-error): the blocking cross-backend coverage is memory + sqlite in the main `go` job (no Docker); pg + mongo run here for visibility until the pre-existing per-backend issues are resolved.
The cross-backend conformance suite caught a real mongo bug: refresh-token
revocation/replay lookups always failed on the mongo backend.
Root cause: grove's mongodriver builds documents from grove column names and
maps a primary key to `_id` ONLY when the column is named "id" (see
mongodriver structToDoc / jsonschema). revokedRefreshTokenModel's pk column is
"token_hash", so on insert the hash is stored under a "token_hash" field while
MongoDB auto-generates an ObjectID `_id`. But IsRefreshTokenRevoked,
GetRevokedRefreshTokenFamily, and MarkRefreshTokenReplayed all filtered by
`{_id: tokenHash}` — which never matched. Result: IsRefreshTokenRevoked always
returned false and replay detection never fired on mongo.
Fix: filter by "token_hash" (where the value is actually stored) and align the
bson decode tag. This also makes any pre-existing (previously unqueryable)
revocation rows work. Documented the grove pk→_id quirk on the model so the
filters aren't "simplified" back to _id. Memory + sqlite conformance unchanged.
All four backends (memory + sqlite in the go job, postgres + mongo here) now pass the conformance suite against real databases in CI (verified on run 30109099743: ok store/mongo, ok store/postgres). Remove continue-on-error so cross-backend drift fails the build.
The per-backend postgres store_test.go suite created users/sessions/orgs and other env-scoped rows without an env_id, violating the NOT NULL + FK constraint added in migration 7 (fk_authsome_users_env et al.). Memory and sqlite don't enforce this, so the failures were postgres-only and pre-existing. createTestApp now also creates a default Production environment and records its id (testEnvByApp / testEnvID helper); every env-scoped literal in store_test.go and refresh_replay_test.go now sets EnvID so the rows reference a real environment. Also collapses a shadowed err in refresh_replay_test.go. Separately, fixes two noctx lint failures in plugins/passkey/discoverable_test.go by switching httptest.NewRequest to NewRequestWithContext.
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
2 participants
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.
Fixes the findings from a full security review (five parallel review agents across the engine, stores, HTTP surface, auth strategies, and multi-tenancy) plus adds test infrastructure to prevent backend divergence going forward. Every change is test-first;
go vet ./...,-race, and the memory+sqlite conformance suite pass locally, with postgres+mongo conformance running in CI.Access control
scopedAppID/roleInCallerApp/userInCallerApp).Sessions & tokens
RotateSession) to close a TOCTOU race; a concurrent family-revoke that deletes the session is treated as a benign lost race, not a 500.settings.Getfalls back to the registered default (not the zero value) on a store error, so cookieSecure/HttpOnlycan't silently flip off.Auth strategies
CloneWarning) now rejects the login.FinishDiscoverableLoginwith a user-handle resolver, real session issuance.Network
X-Forwarded-For/X-Real-IPhonored only when the peer is a trusted proxy — closing spoofing of rate-limit keys, session IP-binding, and IP allow/block lists.Storage
DeleteAppcascades to all app-scoped collections in aMongoTx;ListUserssetsTotal; email filter escaped (regexp.QuoteMeta) to stop regex/ReDoS injection.memory.ListUsersnow applies the email filter (previously ignored → admin search returned all users);DeleteAppidempotency unified across backends.Store conformance suite (new)
store/storetest: a backend-agnostic contract suite (CRUD, delete cascade, tenant scoping,ListUserstotal/filter,RotateSessionCAS, refresh replay). Runs against memory + sqlite in normal CI (no Docker) and postgres + mongo under-tags integration.memory.ListUsersfilter bug and aDeleteAppidempotency divergence, both fixed here.68 files changed, +4,487 / −179.