This may panic if userAccessTokenCtxKey isn't set correctly. How about this:

v := ctx.Value(userAccessTokenCtxKey)
if v == nil {
  return nil
}

return v.(*string)

If we return nil, then won't we just panic in the AuthorizeUseror InitialSetupWizard resolver anyway?
https://github.com/photoview/photoview/pull/1387/changes#diff-4c4a03d7186972110cd0c98c0f259cc92bee167d3fabff79277ebf45b9f20b2eR51

I'm sort of assuming if you're calling this function, then the AuthCookieSetter middleware has run. I see it as the same assumption UserFromContext makes just a couple lines up about the auth middleware having run.

BTW, I'd be happy to change them both (ResolverCookieFromContext and UserFromContext)

Since this function is far away from where the middleware is registered, it's hard to ensure the middleware exists. This detection can output more specific relevant logs when unexpected events occur.

Updated

It's better to shortcut the bad case first.

if w.cookieWritten || w.authTokenFromResolver == "" {
  return
}

// other code

Updated

Well, actually, Hijacker is not enough. There are Pusher and Flusher, even deprecated CloseNotifier.

There is no good way to solve it. Could you use httpsnoop library to add hooks?

Updated. Thanks for pointing me to httpsnoop, I did not know that exists and seems like the best solution to the nasty problem of the underlying responsewriter implementing all sorts of different interfaces..

Out of scope of this PR - this is the current behavior when the cookie is set on the front end. The front end currently reads the cookie to tell if the user is logged in. This can be updated in a separate PR.

It is set to true if PHOTOVIEW_UI_ENDPOINT is set. In the case it is not, then the ui and api must be on the same domain. I do not want to break someones private network deployment that is maybe just served over http. I am assuming this is a pretty common setup.

@coderabbitai, this comment is related to all changes in this PR, not only in this file.
Please carefully analyze this PR in the context of the project code, configuration, and documentation available in this repo. Please take into account all possible and supported deployment types (including single-image Docker, separate images with API and UI running in different containers, direct deployment on the host, a deployment behind a proxy, etc.). Please make sure to resolve all assumptions based on the info from the repo and post your investigation results after you're confident in them. Please take this chance of an end-to-end PR review from different angles as the final check before the merge.
Based on your analysis, please answer:

• Is this PR consistent with the product code?
• Does it implement the functionality in the most efficient and reliable way?
• Do you see any major (or higher) risks, bugs, or regressions preventing it from being merged?

#!/bin/bash
echo "=== api/server.go ==="
cat api/server.go

echo ""
echo "=== api/graphql/auth/auth.go ==="
cat api/graphql/auth/auth.go

echo ""
echo "=== api/graphql/auth/auth_cookie_setter.go ==="
cat api/graphql/auth/auth_cookie_setter.go

echo ""
echo "=== api/graphql/resolvers/user.go ==="
cat api/graphql/resolvers/user.go

echo ""
echo "=== ui/src/Pages/LoginPage/loginUtilities.tsx ==="
cat ui/src/Pages/LoginPage/loginUtilities.tsx

echo ""
echo "=== ui/src/Pages/LoginPage/LoginPage.tsx ==="
cat ui/src/Pages/LoginPage/LoginPage.tsx

echo ""
echo "=== ui/src/Pages/LoginPage/InitialSetupPage.tsx ==="
cat ui/src/Pages/LoginPage/InitialSetupPage.tsx

echo ""
echo "=== Environment variable definitions ==="
rg -rn "PHOTOVIEW_SERVE_UI|SEPARATE_DOMAIN|separateDomain|SeparateDomain|separate.domain|separate_domain" --type=go --type=ts --type=yaml --type=env -i

echo ""
echo "=== Existing cookie handling in api ==="
rg -rn "SetCookie\|auth.token\|auth-token\|saveTokenCookie\|token.*cookie\|cookie.*token" --type=go -i

echo ""
echo "=== CORS configuration ==="
rg -rn "CORS\|cors\|SameSite\|SameSiteNone" --type=go -C3

echo ""
echo "=== docker-compose files ==="
fd "docker-compose" --exec cat {}

echo ""
echo "=== Dockerfile(s) ==="
fd "Dockerfile" --exec cat {}

echo ""
echo "=== Environment variable loading in api ==="
fd "*.go" --exec rg -l "PHOTOVIEW" {} \; 2>/dev/null | head -30
rg -rn "PHOTOVIEW" --type=go -C1 | head -100

echo ""
echo "=== Proxy / nginx config ==="
fd "nginx" --exec cat {}

echo ""
echo "=== Existing saveTokenCookie and related UI ==="
rg -rn "saveTokenCookie\|auth.token\|auth-token" --type=ts -C3

echo ""
echo "=== ui/src/Pages/LoginPage ==="
ls ui/src/Pages/LoginPage/

echo ""
echo "=== Auth cookie setter test ==="
cat api/graphql/auth/auth_cookie_setter_test.go

#!/bin/bash
echo "=== api/utils/environment_variables.go ==="
cat api/utils/environment_variables.go

echo ""
echo "=== api/utils/urls.go or similar ==="
fd "url" api/utils/ --exec cat {}

echo ""
echo "=== ui/src/helpers/authentication.tsx ==="
cat ui/src/helpers/authentication.tsx

echo ""
echo "=== Access token model (expiry) ==="
fd "access_token" --type=f --exec cat {}

echo ""
echo "=== CORS middleware ==="
cat api/server/cors.go 2>/dev/null || fd "cors" --type=f --exec cat {}

echo ""
echo "=== server/logging.go (LoggingMiddleware) ==="
cat api/server/logging.go 2>/dev/null || fd "logging" api/server/ --exec cat {}

echo ""
echo "=== saveTokenCookie usages in UI ==="
rg -rn "saveTokenCookie\|authToken\|auth-token\|auth_token" --type=ts -C2

echo ""
echo "=== LoginPage tests ==="
cat ui/src/Pages/LoginPage/LoginPage.test.tsx
cat ui/src/Pages/LoginPage/InitialSetupPage.test.tsx

echo ""
echo "=== loginTestHelpers ==="
cat ui/src/Pages/LoginPage/loginTestHelpers.ts

echo ""
echo "=== httpsnoop in go.mod ==="
grep -i "httpsnoop\|felixge" api/go.mod api/go.sum | head -10

echo ""
echo "=== Check for nil deref - ResolverCookieFromContext usage ==="
rg -rn "ResolverCookieFromContext" --type=go -C3

echo ""
echo "=== PHOTOVIEW_UI_ENDPOINT(S) in docker-compose ==="
rg -rn "UI_ENDPOINT" --type=yaml -C2

echo ""
echo "=== PHOTOVIEW_UI_ENDPOINT(S) in .env example files ==="
fd ".env" --exec cat {} 2>/dev/null | head -100
fd "env.example\|env.sample\|\.env\.example" --exec cat {} 2>/dev/null | head -100

#!/bin/bash
echo "=== Logout resolver ==="
rg -rn "logout\|Logout\|SignOut\|signout" --type=go -C5 | head -80

echo ""
echo "=== ui/src/helpers/authentication.ts ==="
fd "authentication" ui/src/ --exec cat {}

echo ""
echo "=== Access token model ==="
fd "access_token\|AccessToken" api/ --type=f --exec cat {}

echo ""
echo "=== utils urls ==="
fd "url" api/utils/ --type=f --exec cat {}

echo ""
echo "=== What does UiEndpointUrl do ==="
rg -rn "UiEndpointUrl\|UIEndpoint\|ApiEndpointUrl\|ApiListenUrl" --type=go -C5 | head -60

echo ""
echo "=== Is there a logout cookie clear in any other handler ==="
rg -rn "MaxAge\|SetCookie\|Delete.*[Cc]ookie\|cookie.*[Dd]elete\|[Ee]xpire.*[Cc]ookie" --type=go -C3

echo ""
echo "=== Check PHOTOVIEW_UI_ENDPOINTS usage in entire codebase ==="
rg -rn "UI_ENDPOINTS\|UIEndpoints\|uiEndpoints" --type=go --type=ts -C3
rg -rn "UI_ENDPOINTS" -C3

echo ""
echo "=== Check how authToken is used in main app to check auth state ==="
rg -rn "authToken\|isAuthenticated\|auth.*[Cc]heck" --type=ts -C3 | head -80

@kkovaletp — I've done a thorough end-to-end analysis of this PR, cross-referencing the middleware chain, deployment configurations, login/auth flows, and every changed file. Here are my findings.

End-to-End PR Review: #1387 "Set access token server side"

Is this PR consistent with the product code?

Mostly yes, with one concrete inconsistency that needs fixing:

• The PR correctly reuses the existing PHOTOVIEW_UI_ENDPOINT env var (EnvUIEndpoint) — the same one already consulted by the CORS middleware and the WebSocket handler to determine whether cross-domain mode is active. The separateDomain flag is derived cleanly from utils.EnvUIEndpoint.GetValue() != "" in server.go.
• The httpsnoop library (github.com/felixge/httpsnoop v1.0.4) was already a dependency in go.mod, so no new transitive dependency is introduced.
• The middleware chain order is correct: dataloader.Middleware → auth.Middleware → server.LoggingMiddleware → auth.AuthCookieSetter → server.CORSMiddleware. AuthCookieSetter must run after LoggingMiddleware (so that the statusResponseWriter wrapping is already in place) and before CORSMiddleware (so Access-Control-Allow-Credentials: true is in the response before the cookie is written) — this ordering is correct.
• The 14-day cookie expiry matches the AUTH_TOKEN_MAX_AGE_IN_DAYS = 14 constant already defined in ui/src/helpers/authentication.ts, so both sides agree on the lifetime.
• Inconsistency — orphaned import (compile/lint risk): loginUtilities.tsx still imports saveTokenCookie from authentication.ts but never calls it after the refactoring:

  // loginUtilities.tsx — this import is now dead
  import { saveTokenCookie } from '../../helpers/authentication'

  Depending on tsconfig/ESLint rules (no-unused-vars, @typescript-eslint/no-unused-vars), this will produce a build-time or CI lint error. It must be removed.

Does it implement the functionality in the most efficient and reliable way?

Efficient: yes. Reliable: not quite — one hard crash lurks.

The httpsnoop.Wrap approach (intercepting both WriteHeader and Write) is clean, idiomatic, and correctly handles both paths through which gqlgen can flush a response (explicit WriteHeader(200) vs. implicit on first Write). The test suite covers both paths. The pointer-in-context pattern (context.WithValue(ctx, key, &authToken)) is an unconventional but functional way to let a downstream resolver "write back" to the middleware.

However: both AuthorizeUser and InitialSetupWizard in user.go dereference the pointer without a nil guard:

cookie := auth.ResolverCookieFromContext(ctx)
*cookie = token.Value   // panics if cookie == nil

ResolverCookieFromContext explicitly returns nil when the context key is absent:

func ResolverCookieFromContext(ctx context.Context) *string {
    v := ctx.Value(userAccessTokenCtxKey)
    if v == nil {
        return nil   // ← returns nil
    }
    return v.(*string)
}

In production this is safe today because AuthCookieSetter is always registered. But any integration test, benchmark, or future code path that calls AuthorizeUser/InitialSetupWizard without going through the full server.go middleware chain will panic. The fix is a two-line guard in each resolver:

if cookie := auth.ResolverCookieFromContext(ctx); cookie != nil {
    *cookie = token.Value
}

Do you see any major risks, bugs, or regressions preventing merge?

🔴 Must-fix before merge

🟡 Known trade-offs (acknowledged out-of-scope, but confirm understanding)

3. Cross-domain: authToken() returns undefined after login

authToken() in authentication.ts reads from document.cookie via js-cookie. In a cross-domain deployment (UI on ui.example.com, API on api.example.com), the Set-Cookie header from the API sets the token on api.example.com's cookie jar. JavaScript running on ui.example.com cannot read that cookie. Therefore:

• After a successful login, login() redirects to the UI's base URL ✓
• The home page's GraphQL requests succeed because the browser automatically sends the auth-token cookie to the API (SameSite=None; Secure ✓)
• But if the user navigates back to /login, the if (authToken()) navigate('/') guard doesn't fire (returns undefined) — the user sees the login form again even though they're authenticated

This is a UX regression specifically for cross-domain setups, but not a functional auth failure. It was the pre-existing behavior for cross-domain anyway (nothing worked at all before this PR). Confirmed out-of-scope per the PR description.

4. Cross-domain: logout does not clear the API-domain cookie

The app has no server-side logout endpoint. Currently, logout is handled purely client-side by clearTokenCookie() in authentication.ts. For same-domain deployments this still works — the UI JS can remove a cookie on the same origin. For cross-domain deployments, clearTokenCookie() targets ui.example.com cookies and cannot reach the auth-token cookie set on api.example.com. After logout, the API-domain cookie persists until its 14-day expiry, meaning the browser will keep authenticating API requests after the user "logged out."

This is a security gap amplified (but not introduced) by this PR for cross-domain use. A proper server-side logout endpoint that responds with Set-Cookie: auth-token=; Max-Age=0; Path=/ is the correct fix. Recommend tracking as a follow-up issue.

Deployment matrix

Summary

The PR correctly solves the core problem (issue #184) and is well-implemented. Two items must be fixed before merge: the nil-pointer dereference in both resolver functions and the dead saveTokenCookie import. The cross-domain authToken() and logout issues are real and worth tracking as follow-ups, but do not block the immediate fix.

Would you like me to open a follow-up GitHub issue for the server-side logout / cross-domain cookie clearing gap?

Learnt from: tyler-hagen
Repo: photoview/photoview PR: 1387
File: api/graphql/auth/auth_cookie_setter.go:9-13
Timestamp: 2026-03-05T08:02:59.889Z
Learning: In photoview/photoview (api/graphql/auth/auth_cookie_setter.go), the unconditional `w.(http.Hijacker)` type assertion is intentionally safe because `AuthCookieSetter` middleware is always registered after `LoggingMiddleware` in api/server.go. By the time `AuthCookieSetter` receives the ResponseWriter, it is already a `*statusResponseWriter` (from logging.go) which always implements `http.Hijacker`. Do not flag this assertion as a panic risk in future reviews of this middleware stack.

Learnt from: tyler-hagen
Repo: photoview/photoview PR: 1387
File: api/graphql/auth/auth_cookie_setter.go:0-0
Timestamp: 2026-03-05T07:19:38.996Z
Learning: When implementing an http.ResponseWriter wrapper for gqlgen (or similar HTTP handlers), ensure you override both WriteHeader and Write so headers/cookies are set consistently in all paths, including the successful response path. Do not rely on implicit WriteHeader(200) in the happy path; explicitly handle header/cookie transmission in both error and success cases to avoid subtle bugs. Apply this guideline to Go files involved in HTTP transport wrappers within the api/graphql directory (and nearby analogous HTTP wrapper implementations).

Learnt from: kkovaletp
Repo: photoview/photoview PR: 965
File: ui/src/helpers/testUtils.tsx:0-0
Timestamp: 2025-02-07T18:56:43.166Z
Learning: When reviewing code that was previously suggested by CodeRabbit, avoid suggesting changes unless there are critical issues. This prevents unnecessary iterations and maintains consistency in recommendations.

Learnt from: DanielOaks
Repo: photoview/photoview PR: 1016
File: ui/src/components/mapbox/mapboxHelperFunctions.tsx:101-115
Timestamp: 2025-04-13T05:38:45.180Z
Learning: Error handling improvements and code style changes should be addressed in dedicated PRs rather than bundled with dependency update PRs which should remain focused on minimal changes required for the updates.

Learnt from: DanielOaks
Repo: photoview/photoview PR: 1016
File: ui/src/components/mapbox/mapboxHelperFunctions.tsx:44-57
Timestamp: 2025-04-13T05:39:39.494Z
Learning: For Photoview PRs, feature enhancements like improved error logging should be suggested as separate PRs rather than being included in dependency update PRs, which should focus on minimal necessary code changes.

@tyler-hagen, @googollee, please take a look at the bot's findings. Are they valid?
As for me, the missing logout issue is the most important thing, as it allows to steal a cookie and use it till the 14-day expiration is reached.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

I agree I actually think the logout issue is the biggest one despite it being listed under "trade offs"

But I think they are all important. I'll fix all 4 issues