Conversation
…or session salt get_authenticated_user resolved the session subject with a bare id lookup, so a blocked or deactivated account holding a live session cookie could still complete the consent flow and mint an access token. The legacy warden branch also discarded the session salt, so salt based session revocation (password reset, sign out everywhere) had no effect on this flow. Resolve the identity the way Guard.GrpcServers.AuthServer.get_user/1 does: an OIDC session resolves through FrontRepo.User.active_user_by_id/1, and a warden session threads its salt through active_user_by_id_and_salt/2. Both filter blocked and deactivated accounts, and the salt variant secure compares the stored salt. Anything that does not resolve is treated as unauthenticated and redirected to login. Also add id host ingress mappings for /mcp/oauth/authorize and /mcp/oauth/grant-selection mirroring the existing mcp host ones, so those paths are not served through the bypass_auth catch all on that host. The resolution above no longer trusts the edge, so this is defense in depth. Tests cover both directions: active OIDC and warden sessions still complete the flow and mint a token bound to the session user, while blocked, deactivated and stale salt sessions get no consent page and no code.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75629b3654
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| prefix: /mcp/oauth/authorize | ||
| rewrite: /mcp/oauth/authorize | ||
| service: guard-id-http-api.{{ .Release.Namespace }}:80 | ||
| bypass_auth: false |
There was a problem hiding this comment.
For an unauthenticated request to this mapping, Emissary forwards /exauth/mcp/oauth/authorize with the id. host, so the strict MCP handlers in auth/lib/auth.ex:248-265 do not match; the request instead reaches the generic id-host handler at auth/lib/auth.ex:60-68, which deliberately returns 200 even when set_user_headers reports an authentication error. Consequently, bypass_auth: false here does not actually require authentication or provide the claimed edge defense-in-depth; corresponding strict id.-host handlers need to reject or redirect missing sessions.
Useful? React with 👍 / 👎.
active_user_by_id_and_salt/2 passed the resolved user's salt straight into Plug.Crypto.secure_compare/2, which raises a FunctionClauseError (surfacing as a 500) when that salt is nil, as it can be for a legacy or OIDC only account that never had one. A salt bearing warden session for such an account should be rejected, not crash. Guard that both the supplied salt and the stored salt are non nil binaries before comparing, and return the same not-found result used for a salt mismatch otherwise. These are data shape checks rather than secret comparisons, so they do not leak timing; a present but wrong salt still goes through the constant time secure_compare. This fixes both callers of the shared function at once: Guard.McpOAuth.Server and Guard.GrpcServers.AuthServer, for neither of which a nil salt account authenticating is a legitimate flow.
oidc_session_identity/1 accepted any session that was not expired, so it missed the revocation that Guard.GrpcServers.AuthServer.process_session/3 enforces: a session whose refresh_token_enc has been nulled is revoked, and AuthServer rejects it. A session reaches that state, with expires_at still in the future, when the refresh token is cleared without expiring the session, the sign-out-everywhere and refresh-resolved-to-a-different-user paths. Because the MCP authorize flow only mirrored get_user/1 and OIDC revocation lives in process_session/3, a revoked-everywhere session could still mint an MCP token. Add a shared Guard.Store.OIDCSession.valid_for_auth?/1 predicate (not expired and refresh token present) and gate oidc_session_identity/1 on it, so a caller that consumes a session as identity without refreshing it cannot drift from the revocation rules again. AuthServer's equivalent checks are left in place, since its expired-but-refreshable branch refreshes rather than rejecting, with a comment tying them to the shared predicate. An invalid session fails closed to the login redirect. The audit of process_session/3 found these are the only two accept-path rejections for a non-expired session: session missing (already handled) and refresh_token_enc nil.
1 participant
No description provided.