fix(migrate): refuse a bundle that claims another app's audience (H15)

[alghanim]

Fourth of six. Stacked on #61.

The bug

Apply carried the bundle's audience onto the target app unchecked:

if b.App.Audience != "" { app.Audience = b.App.Audience }

The audience is a security principal, not a label: appAudience() feeds claims.Audience on every issuance path, and SimpleAuth deliberately does not pin aud centrally — F51 records that issuer/audience are enforced at the SDK/RP layer. The string in aud, verified offline against the shared JWKS, is the only thing between a token and a victim resource server.

And the migration-token holder is not a master admin: guardMigrationCall authenticates a single-use token scoped to ONE target app. Nothing restricted the bundle to naming that app's own identity.

Attack: the standalone team POSTs a bundle with app.audience = "billing-api", catalog.role_permissions = {"admin":["billing:write"]}, and a local user with a self-chosen password hash and roles:["admin"]. They log in at their own migrated app and receive aud: ["billing-api"], roles:["admin"] signed by the central's key. The real billing service verifies signature + audience offline and admits them as a billing admin.

The no-effort variant: Package sets aud = app.AppID when the source home app has no explicit audience, and ensureDefaultApp creates it with Audience: appID — "simpleauth" on a stock standalone. Migrating a stock standalone therefore stamped the target with the central's own default-app audience. The pre-fix integration tests encoded this as expected behavior.

Approach

resolveCarriedAudience refuses a claim on anyone else's audience: empty carries nothing; the target's current audience is allowed (idempotent re-run); the central's default app id is refused explicitly; anything matching another registered app's audience or app_id is refused. Trimmed, so whitespace cannot smuggle a near-collision.

The default-app id is checked explicitly rather than through ListApps() because ensureDefaultApp is called only from main.go — on an embedded (pkg/server) deployment the row can be absent while resolveApp still synthesizes it, so a ListApps-only check would miss the highest-value target.

Enforced in both Classify (as a Blocked entry, so the preflight UI renders it) and Apply (before its first write). Fails closed if ListApps errors.

A bypass an adversarial review caught in the first cut

The self-allow was originally aud == target.Audience || aud == target.AppID. The app_id half was a hole: nothing enforces audience uniqueness at app creation, so a central can hold App{AppID:"reports", Audience:"analytics"} while the operator registers the target as App{AppID:"analytics", Audience:"analytics-migrated"}. Self-allowing the app_id there hands the bundle "analytics" — a third-party RP's live audience. Now only the target's current audience is self-allowed; TestApplyRejectsAudienceMatchingTargetAppID pins it.

Caveats

• Integration tests were not executed — six migration subtests moved to a new newMigTargetFrom helper (which gives the source a deliberate audience, as a real migration must). They compile (go vet -tags integration) but this repo's harness needs Docker. Please run make -C test/integration test.
• Already-poisoned targets are not remediated — a central that already took a colliding bundle keeps that audience, and the idempotency branch will re-bless it. Worth an audit query.
• Not fixed here: RedirectURIs and CORSOrigins are carried the same unchecked way (pre-existing). Lesser, because it only affects the app the token already scopes the holder to — but worth review.

🤖 Generated with Claude Code

[alghanim]

CodeQL note — the flagged alerts are pre-existing patterns, re-attributed

CodeQL reports "new alerts in code changed by this pull request". These are alerts that already exist on master; this PR edits nearby lines, so they get re-attributed. Same phenomenon as the go/reflected-xss alerts on #58.

Baseline on master today:

gh api "repos/bodaay/SimpleAuth/code-scanning/alerts?ref=refs/heads/master&state=open&per_page=100" \
  --jq '[.[]|select(.rule.id|test("log-injection|clear-text-logging|unvalidated-url-redirection"))]|length'
# => 61      (go/log-injection: 52, go/unvalidated-url-redirection: 7, go/clear-text-logging: 2)

Per rule:

• go/unvalidated-url-redirection — oidc.go:325. This is issueOIDCCodeRedirect's final http.Redirect, pre-existing code this branch does not modify. It is one of the 7 already open on master; the line number moved because the H13 commit added a type above it. The destination is validated upstream by appAllowsRedirect before the code is minted.

• go/log-injection. 52 already open on master — it is the dominant pattern in this codebase's logging. Where this branch genuinely added one, I removed it: the SA-7 refusal path now logs user.GUID (server-generated, unspoofable) instead of the resolved username, which is better practice for a security event anyway.

• go/clear-text-logging — admin.go:272, on ForcePasswordChange. A boolean flag, not a secret. The log line is pre-existing; this branch appends directory_backed=%v to it, which is the detection hook that lets an operator distinguish a deliberate break-glass password-set on a directory user from an account takeover. Dropping it to silence the alert would remove the point of that hunk.

Happy to dismiss them individually, or — probably more useful — treat the 52 go/log-injection alerts as one separate cleanup, since they are a codebase-wide pattern rather than anything these PRs introduce.

🤖 Generated with Claude Code