security: complete the 2.4.0 pre-release audit (AUDIT-06 to AUDIT-22)

Makefile

@@ -111,46 +111,101 @@ smoke:
 
 test-postgres: test-cleanup-postgres
 	docker run -d --name authorizer_postgres -p 5434:5432 -e POSTGRES_PASSWORD=postgres -e POSTGRES_DB=postgres postgres
-	sleep 3
-	go clean --testcache && TEST_DBS="postgres" $(GO_TEST_ALL)
-	docker rm -vf authorizer_postgres
+	@# Wait for readiness, then ALWAYS tear the container down — including when
+	@# the tests fail — and exit with the TESTS' status, not the teardown's.
+	@# Previously `sleep N` guessed at readiness and `docker rm` sat on its own
+	@# recipe line, so a failing test aborted the recipe before teardown and left
+	@# the port bound, breaking the NEXT run for an unrelated reason.
+	sh scripts/wait-for-test-dbs.sh postgres && \
+	{ go clean --testcache; TEST_DBS="postgres" $(GO_TEST_ALL); }; \
+	status=$$?; \
+	docker rm -vf authorizer_postgres; \
+	exit $$status
 
 test-sqlite:
 	go clean --testcache && TEST_DBS="sqlite" $(GO_TEST_ALL)
 
 test-mongodb: test-cleanup-mongodb
 	docker run -d --name authorizer_mongodb_db -p 27017:27017 mongo:4.4.15
-	sleep 3
-	go clean --testcache && TEST_DBS="mongodb" $(GO_TEST_ALL)
-	docker rm -vf authorizer_mongodb_db
+	@# Wait for readiness, then ALWAYS tear the container down — including when
+	@# the tests fail — and exit with the TESTS' status, not the teardown's.
+	@# Previously `sleep N` guessed at readiness and `docker rm` sat on its own
+	@# recipe line, so a failing test aborted the recipe before teardown and left
+	@# the port bound, breaking the NEXT run for an unrelated reason.
+	sh scripts/wait-for-test-dbs.sh mongodb && \
+	{ go clean --testcache; TEST_DBS="mongodb" $(GO_TEST_ALL); }; \
+	status=$$?; \
+	docker rm -vf authorizer_mongodb_db; \
+	exit $$status
 
 test-scylladb: test-cleanup-scylladb
 	docker run -d --name authorizer_scylla_db -p 9042:9042 scylladb/scylla
-	sleep 15
-	go clean --testcache && TEST_DBS="scylladb" $(GO_TEST_ALL)
-	docker rm -vf authorizer_scylla_db
+	@# Wait for readiness, then ALWAYS tear the container down — including when
+	@# the tests fail — and exit with the TESTS' status, not the teardown's.
+	@# Previously `sleep N` guessed at readiness and `docker rm` sat on its own
+	@# recipe line, so a failing test aborted the recipe before teardown and left
+	@# the port bound, breaking the NEXT run for an unrelated reason.
+	sh scripts/wait-for-test-dbs.sh scylladb && \
+	{ go clean --testcache; TEST_DBS="scylladb" $(GO_TEST_ALL); }; \
+	status=$$?; \
+	docker rm -vf authorizer_scylla_db; \
+	exit $$status
 
 test-arangodb: test-cleanup-arangodb
 	docker run -d --name authorizer_arangodb -p 8529:8529 -e ARANGO_NO_AUTH=1 arangodb/arangodb:3.10.3
-	sleep 5
-	go clean --testcache && TEST_DBS="arangodb" $(GO_TEST_ALL)
-	docker rm -vf authorizer_arangodb
+	@# Wait for readiness, then ALWAYS tear the container down — including when
+	@# the tests fail — and exit with the TESTS' status, not the teardown's.
+	@# Previously `sleep N` guessed at readiness and `docker rm` sat on its own
+	@# recipe line, so a failing test aborted the recipe before teardown and left
+	@# the port bound, breaking the NEXT run for an unrelated reason.
+	sh scripts/wait-for-test-dbs.sh arangodb && \
+	{ go clean --testcache; TEST_DBS="arangodb" $(GO_TEST_ALL); }; \
+	status=$$?; \
+	docker rm -vf authorizer_arangodb; \
+	exit $$status
 
 test-dynamodb: test-cleanup-dynamodb
 	docker run -d --name authorizer_dynamodb -p 8000:8000 amazon/dynamodb-local:latest
-	sleep 3
-	go clean --testcache && TEST_DBS="dynamodb" $(GO_TEST_ALL)
-	docker rm -vf authorizer_dynamodb
+	@# Wait for readiness, then ALWAYS tear the container down — including when
+	@# the tests fail — and exit with the TESTS' status, not the teardown's.
+	@# Previously `sleep N` guessed at readiness and `docker rm` sat on its own
+	@# recipe line, so a failing test aborted the recipe before teardown and left
+	@# the port bound, breaking the NEXT run for an unrelated reason.
+	sh scripts/wait-for-test-dbs.sh dynamodb && \
+	{ go clean --testcache; TEST_DBS="dynamodb" $(GO_TEST_ALL); }; \
+	status=$$?; \
+	docker rm -vf authorizer_dynamodb; \
+	exit $$status
 
 test-couchbase: test-cleanup-couchbase
 	docker run -d --name authorizer_couchbase -p 8091-8097:8091-8097 -p 11210:11210 -p 11207:11207 -p 18091-18095:18091-18095 -p 18096:18096 -p 18097:18097 couchbase:latest
-	sh scripts/couchbase-test.sh
-	go clean --testcache && TEST_DBS="couchbase" $(GO_TEST_ALL)
-	docker rm -vf authorizer_couchbase
+	@# couchbase-test.sh already provisions and waits, so no readiness poll
+	@# here — but the teardown still has to survive a failing test run.
+	sh scripts/couchbase-test.sh && \
+	{ go clean --testcache; TEST_DBS="couchbase" $(GO_TEST_ALL); }; \
+	status=$$?; \
+	docker rm -vf authorizer_couchbase; \
+	exit $$status
 
-test-all-db: test-cleanup test-docker-up test-cleanup
-	go clean --testcache && TEST_DBS="couchbase,postgres,sqlite,mongodb,arangodb,scylladb,dynamodb" $(GO_TEST_ALL)
-	$(MAKE) test-cleanup
+# Prerequisites are `test-cleanup test-docker-up`, NOT
+# `test-cleanup test-docker-up test-cleanup`. The trailing duplicate looked like
+# "tear down afterwards" but never did anything: make deduplicates prerequisites,
+# so it silently collapsed into the leading one. Teardown belongs in the recipe,
+# below, where it can also run when the tests FAIL.
+test-all-db: test-cleanup test-docker-up
+	@# Always tear the containers down, including on failure, and exit with the
+	@# TESTS' status rather than the teardown's.
+	@#
+	@# `go test ... ; make test-cleanup` (two recipe lines) aborted the recipe on
+	@# a failing test and never reached cleanup, leaking seven containers and
+	@# leaving ports 5434/27017/9042/8529/8000/8091 bound — so the NEXT run
+	@# failed to start them and produced a second, misleading failure. Same
+	@# capture-status-then-clean shape the e2e-playground target already uses.
+	go clean --testcache; \
+	TEST_DBS="couchbase,postgres,sqlite,mongodb,arangodb,scylladb,dynamodb" $(GO_TEST_ALL); \
+	status=$$?; \
+	$(MAKE) test-cleanup; \
+	exit $$status
 
 # Start all test database containers
 test-docker-up:
@@ -162,7 +217,7 @@ test-docker-up:
 	docker run -d --name authorizer_dynamodb -p 8000:8000 amazon/dynamodb-local:latest
 	docker run -d --name authorizer_couchbase -p 8091-8097:8091-8097 -p 11210:11210 -p 11207:11207 -p 18091-18095:18091-18095 -p 18096:18096 -p 18097:18097 couchbase:latest
 	sh scripts/couchbase-test.sh
-	sleep 5
+	sh scripts/wait-for-test-dbs.sh
 
 # Remove all test database containers
 test-cleanup:

cmd/cookie_defaults_test.go

@@ -0,0 +1,76 @@
+package cmd
+
+import (
+	"net/http"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/authorizerdev/authorizer/internal/config"
+	"github.com/authorizerdev/authorizer/internal/cookie"
+)
+
+// TestAppCookieSameSiteDefaultIsNone is a decision guard for the other half of
+// the session-cookie topology (the first half is pinned in
+// internal/cookie.TestSessionCookieTopologyIsDeliberate).
+//
+// "none" looks like an obviously-wrong default to a scanner or a security
+// review, and the 2.4.0 pre-release audit duly flagged it. It was consciously
+// kept: Authorizer targets an auth server on a subdomain serving apps on other
+// sites, and Lax withholds the session cookie on exactly those cross-site
+// requests, breaking the browser-session half of the SDK. Auth0 takes the same
+// position — it recommends SameSite=None for cross-origin authentication and
+// ships fallback cookies for browsers that cannot do it.
+//
+// CSRF middleware, HttpOnly, and Authorization-header auth are what actually
+// carry the security here; SameSite is defense-in-depth. See
+// cookie.BuildSessionCookies before changing this.
+func TestAppCookieSameSiteDefaultIsNone(t *testing.T) {
+	f := RootCmd.PersistentFlags().Lookup("app-cookie-same-site")
+	require.NotNil(t, f, "the --app-cookie-same-site flag must exist")
+	assert.Equal(t, "none", f.DefValue,
+		"changing this default breaks cross-site apps; read cookie.BuildSessionCookies first")
+}
+
+// TestAppCookieSameSiteIsValidated pins that a mistyped value stops the process
+// instead of silently becoming lax.
+//
+// cookie.ParseSameSite falls back to Lax for anything unrecognised. That is a
+// safe default but a silent one: an operator who asks for `strict` and mistypes
+// it gets Lax — a real downgrade from what they requested, with nothing
+// anywhere to say so — and a mistyped `none` withholds the session cookie from
+// cross-site apps, which presents as "login randomly doesn't stick".
+func TestAppCookieSameSiteIsValidated(t *testing.T) {
+	t.Parallel()
+
+	for _, valid := range []string{"lax", "strict", "none", "STRICT", " none ", ""} {
+		cfg := config.Config{AppCookieSameSite: valid}
+		assert.NoError(t, cfg.ValidateAppCookieSameSite(), "%q is a supported value", valid)
+	}
+
+	for _, invalid := range []string{"strct", "nonw", "same-site", "true", "0"} {
+		cfg := config.Config{AppCookieSameSite: invalid}
+		err := cfg.ValidateAppCookieSameSite()
+		require.Error(t, err, "%q must be rejected, not silently downgraded to lax", invalid)
+		assert.Contains(t, err.Error(), "app-cookie-same-site")
+	}
+}
+
+// TestEverySameSiteValueRoundTrips pins that each accepted CLI value maps to the
+// SameSite mode it names — the validator and the parser must agree, or a value
+// passes validation and then means something else.
+func TestEverySameSiteValueRoundTrips(t *testing.T) {
+	t.Parallel()
+	want := map[string]http.SameSite{
+		"lax":    http.SameSiteLaxMode,
+		"strict": http.SameSiteStrictMode,
+		"none":   http.SameSiteNoneMode,
+	}
+	for _, v := range config.ValidAppCookieSameSiteValues {
+		cfg := config.Config{AppCookieSameSite: v}
+		require.NoError(t, cfg.ValidateAppCookieSameSite())
+		assert.Equal(t, want[v], cookie.ParseSameSite(v),
+			"%q passes validation, so it must parse to the mode it names", v)
+	}
+}

cmd/root.go

@@ -82,7 +82,11 @@ var (
 			rootArgs.config.Finalize()
 			// Fail closed rather than silently encrypting TOTP seeds with a
 			// publicly computable key derived from an empty secret.
-			return rootArgs.config.ValidateEncryptionKey()
+			if err := rootArgs.config.ValidateEncryptionKey(); err != nil {
+				return err
+			}
+			// A mistyped SameSite silently becomes lax — see the doc comment.
+			return rootArgs.config.ValidateAppCookieSameSite()
 		},
 		Run: runRoot,
 	}
@@ -204,7 +208,12 @@ func init() {
 
 	// Cookies flags
 	f.BoolVar(&rootArgs.config.AppCookieSecure, "app-cookie-secure", true, "Application secure cookie flag")
-	f.StringVar(&rootArgs.config.AppCookieSameSite, "app-cookie-same-site", "none", "SameSite attribute for session cookies (lax, strict, none)")
+	// Default "none" is deliberate and audit-reviewed, not an oversight: the
+	// product targets an auth server on a subdomain serving apps on other
+	// sites, and Lax withholds the session cookie on exactly those cross-site
+	// requests. Same position Auth0 takes. See cookie.BuildSessionCookies for
+	// the full reasoning before changing it.
+	f.StringVar(&rootArgs.config.AppCookieSameSite, "app-cookie-same-site", "none", "SameSite attribute for session cookies (lax, strict, none). Default none supports apps on other domains; set lax if every app shares this host")
 	f.BoolVar(&rootArgs.config.AdminCookieSecure, "admin-cookie-secure", true, "Admin secure cookie flag")
 	f.BoolVar(&rootArgs.config.DisableAdminHeaderAuth, "disable-admin-header-auth", false, "Disable admin authentication via X-Authorizer-Admin-Secret header")
 
@@ -249,6 +258,7 @@ func init() {
 	f.StringVar(&rootArgs.config.MicrosoftTenantID, "microsoft-tenant-id", defaultMicrosoftTenantID, "Tenant ID for Microsoft")
 	f.StringSliceVar(&rootArgs.config.MicrosoftScopes, "microsoft-scopes", defaultMicrosoftScopes, "Scopes for Microsoft")
 	f.StringSliceVar(&rootArgs.config.MicrosoftAllowedTenants, "microsoft-allowed-tenants", nil, "Entra tenant IDs allowed to sign in when --microsoft-tenant-id is a multi-tenant alias (common/organizations/consumers). Empty allows any tenant, but an untrusted tenant's email will not link to an existing account")
+	f.BoolVar(&rootArgs.config.FgaAllowUnconstrainedAgents, "fga-allow-unconstrained-agents", false, "When a delegated (agent-acting-for-user) FGA check runs against an authorization model with no `type agent`, authorize as the delegating user alone instead of denying. Discards the agent half of the permission intersection; add `type agent` to your model instead")
 	f.BoolVar(&rootArgs.config.OAuthAllowUnverifiedProviderEmail, "oauth-allow-unverified-provider-email", false, "Compatibility escape hatch: let a social login whose provider did not attest the email address sign up or return to an account that same provider already owns. It still cannot cross into an account another credential owns. Prefer pinning --microsoft-tenant-id or enabling the xms_edov claim; see docs/email-verification-contract.md")
 	f.StringVar(&rootArgs.config.TwitchClientID, "twitch-client-id", "", "Client ID for Twitch")
 	f.StringVar(&rootArgs.config.TwitchClientSecret, "twitch-client-secret", "", "Client secret for Twitch")

e2e-playground/tests/social/apple.spec.ts

@@ -76,6 +76,7 @@ test.describe('Social login — Apple', () => {
     await configureProviderProfile(request, 'apple', {
       sub: `apple-${crypto.randomUUID()}`,
       email,
+      email_verified: true,
       omit_user_field: true,
     });
     await page.getByRole('button', { name: /apple/i }).click();
@@ -84,3 +85,40 @@ test.describe('Social login — Apple', () => {
     await expect(page.locator(`a[href="mailto:${email}"]`)).toBeVisible();
   });
 });
+
+// --- Email-attestation contract (nOAuth defence, AUDIT-01/AUDIT-02) ---------
+//
+// These live in this provider's own spec file on purpose. mock-oauth stores ONE
+// profile per provider globally, so two spec FILES driving the same provider
+// race under parallel workers. One provider per file is the convention that
+// keeps the suite order-independent; see docs/email-verification-contract.md
+// for what the contract itself says.
+
+test.describe('Social login — Apple — email-attestation contract', () => {
+  test('the string form of email_verified is honoured, not silently dropped', async ({
+    page,
+    request,
+  }) => {
+    // Apple documents email_verified as "a string or Boolean value". Decoding
+    // it into a plain Go bool fails the whole claim set, which would downgrade
+    // a genuinely verified address to unverified and lock the user out — so the
+    // quoted form has to work end to end.
+    const email = `evc-apple-${crypto.randomUUID()}@example.com`;
+    await runSocialLoginHappyPath(page, request, {
+      provider: 'apple',
+      buttonName: /apple/i,
+      profile: {
+        sub: `apple-${crypto.randomUUID()}`,
+        email,
+        email_verified: 'true',
+        given_name: 'Alan',
+        family_name: 'Turing',
+      },
+      expectedEmail: email,
+    });
+
+    const user = await getUserByEmail(email);
+    expect(user.signup_methods).toContain('apple');
+    expect(user.email_verified).toBe(true);
+  });
+});

e2e-playground/tests/social/discord.spec.ts

@@ -50,7 +50,7 @@ test.describe('Social login — Discord', () => {
     // correctly with no synthetic-email machinery needed (unlike Twitter,
     // which never gets a real email at all).
     const email = `discord-repeat-${crypto.randomUUID()}@example.com`;
-    const profile = { id: `discord-stable-${crypto.randomUUID()}`, username: 'gracehopper', avatar: 'def456', email };
+    const profile = { id: `discord-stable-${crypto.randomUUID()}`, username: 'gracehopper', avatar: 'def456', email, verified: true };
 
     // First login (fresh browser context = `page`/`request` from the test
     // fixture): creates the account.