Fix/logout without cognito hosted#2
Conversation
Context ------- SurfSense's `logout()` used to assume a 3-layer SSO chain — revoke JWT, clear oauth2-proxy cookie, then hit Cognito's hosted `/logout`. When deployments don't expose that hosted endpoint (NEXT_PUBLIC_OIDC_LOGOUT_URL unset) the function silently returned false without clearing the proxy cookie, so the next request re-authed the user via oauth2-proxy's still-valid cookie and landed them back on the dashboard. Also found: NEXT_PUBLIC_LOGOUT_REDIRECT_URL / OIDC_LOGOUT_URL / OIDC_CLIENT_ID were neither build-time ARGs nor in the runtime substitution list, so they resolved to `undefined` in the client bundle regardless of container env. Changes ------- * lib/auth-utils.ts — always hit `/oauth2/sign_out`; only prepend the Cognito hop when OIDC_LOGOUT_URL is configured. Else-branch uses NEXT_PUBLIC_LOGOUT_REDIRECT_URL as the post-logout landing page. Also cleaned unresolved merge-conflict markers in JSDoc blocks. * Dockerfile — add ARG/ENV for the 3 auth vars. Default ARG values are empty (not placeholder tokens like `__NEXT_PUBLIC_X__`): tokens look truthy to terser, causing it to dead-code-eliminate the no-Cognito branch. Empty string lets terser drop the branch that isn't configured at build time, matching the actual deploy. * .dockerignore — exclude .env.local / .env.*.local so developer-specific NEXT_PUBLIC_* values don't leak into shared builds via Next.js auto-loading .env.local at build time.
There was a problem hiding this comment.
Pull request overview
Updates the web app logout flow to work correctly when Cognito hosted logout is not configured, while keeping the oauth2-proxy sign_out behavior.
Changes:
- Adjust
logout()to always sign out via oauth2-proxy and only hop through Cognito logout when OIDC logout config is present. - Add build-time Docker ARG/ENV wiring for optional OIDC logout and a configurable post-logout redirect.
- Exclude
.env.local/.env.*.localfrom the Docker build context to avoid baking developer-localNEXT_PUBLIC_*values into the image.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
surfsense_web/lib/auth-utils.ts |
Makes Cognito logout hop optional and adds a fallback redirect when Cognito hosted logout isn’t configured. |
surfsense_web/Dockerfile |
Introduces build-time args/env vars for optional OIDC logout + logout redirect configuration. |
surfsense_web/.dockerignore |
Prevents .env.local variants from influencing the built Next.js bundle inside Docker. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Layer 2 — clear _oauth2_proxy cookie via /oauth2/sign_out | ||
| * Layer 3 — clear Cognito session via rd= redirect | ||
| >>>>>>> 8c3ff62c (feat(auth): mPass SSO via oauth2-proxy ForwardAuth with cookie-handoff) | ||
| * Layer 3 (optional) — clear Cognito session via rd= redirect when OIDC_LOGOUT_URL configured |
There was a problem hiding this comment.
Docstring says Layer 3 is triggered when OIDC_LOGOUT_URL is configured, but the implementation only performs the Cognito hop when both NEXT_PUBLIC_OIDC_LOGOUT_URL and NEXT_PUBLIC_OIDC_CLIENT_ID are set. Update the comment to match the actual condition (and optionally mention the landing page redirect var, if required).
| * Layer 3 (optional) — clear Cognito session via rd= redirect when OIDC_LOGOUT_URL configured | |
| * Layer 3 (optional) — clear Cognito session via rd= redirect when both | |
| * NEXT_PUBLIC_OIDC_LOGOUT_URL and NEXT_PUBLIC_OIDC_CLIENT_ID are configured |
| # them as literal strings and terser dead-code-eliminates branches based on | ||
| # truthiness; placeholder tokens look truthy and defeat that optimization. |
There was a problem hiding this comment.
The comment mentions “terser” dead-code elimination, but Next.js typically minifies with SWC (unless explicitly configured otherwise). Consider rewording this to refer to “the JS minifier”/“Next.js compiler” so it stays accurate regardless of minifier choice.
| # them as literal strings and terser dead-code-eliminates branches based on | |
| # truthiness; placeholder tokens look truthy and defeat that optimization. | |
| # them as literal strings, and the JS minifier/compiler can dead-code-eliminate | |
| # branches based on truthiness; placeholder tokens look truthy and defeat that optimization. |
| # Next.js reads these at build time and inlines NEXT_PUBLIC_* values into the | ||
| # JS bundle, bypassing Dockerfile ARG defaults + entrypoint runtime substitution. | ||
| # Exclude from build context so the image stays portable across .env.local contents. |
There was a problem hiding this comment.
This comment implies .env.local overrides Dockerfile ARG/ENV values, but Next.js env loading generally does not override already-defined environment variables. If the goal is preventing accidental baking of developer-local NEXT_PUBLIC_* values that aren’t provided via build args/placeholders, consider clarifying that intent to avoid confusion.
| # Next.js reads these at build time and inlines NEXT_PUBLIC_* values into the | |
| # JS bundle, bypassing Dockerfile ARG defaults + entrypoint runtime substitution. | |
| # Exclude from build context so the image stays portable across .env.local contents. | |
| # Next.js can read these at build time and inline NEXT_PUBLIC_* values into the | |
| # JS bundle if matching values are not provided via Docker build args/placeholders. | |
| # Exclude them from the build context to avoid baking developer-local values into the image. |
Description
Motivation and Context
FIX #
Screenshots
API Changes
Change Type
Testing Performed
Checklist