Skip to content

feat: redirect to AUTH login URL on 401 from manager - #695

Open
birme wants to merge 1 commit into
mainfrom
frontend/166-osc-login-redirect
Open

birme wants to merge 1 commit into
mainfrom
frontend/166-osc-login-redirect

Conversation

@birme

@birme birme commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds an optional build-time env var AUTH (an OSC login URL). When set, any HTTP 401 from the manager navigates the browser to that URL so the user can (re)authenticate — implements the OSC-hosted deployment flow described in Redirect to OSC login on 401 auth from intercom-manager #166.
  • The check lives in the central handleFetchRequest, so every API.* endpoint behaves uniformly; a small maybeRedirectToAuth(status) helper does the redirect (with a loop guard) and is a pure no-op when AUTH is unset, leaving the existing OSC reauth flow and all current behavior unchanged.
  • Exposes the literally-named AUTH var (no VITE_ prefix, as the issue specifies) via envPrefix: ["VITE_", "AUTH"] in vite.config.ts, types it in vite-env.d.ts, and passes it into the container build in scripts/entrypoint.sh.
  • Documents the new env var in README.md and .env.local.sample.

Test plan

  • Tests pass (npm test) — added 4 focused tests for the redirect helper; existing 401 regression suite unchanged (151 tests / 19 files green locally)
  • TypeScript compiles (npm run typecheck)
  • Lint clean (npm run lint)
  • With AUTH set at build time, a 401 from the manager redirects the browser to the configured URL
  • With AUTH unset, behavior is unchanged (existing reauth flow still runs)

Closes #166

🤖 Generated with Claude Code

Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com

When built with the AUTH env var set to an OSC login URL, redirect the
browser there whenever the manager returns HTTP 401 so the user can
re-authenticate. Wired into the central handleFetchRequest choke point so
all endpoints behave uniformly; no-op when AUTH is unset.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@birme

birme commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

daily-backlog-pr automated code review (independent reviewer invocation, separate from the implementer). A state-bearing GitHub approval could not be posted because this PR was opened by the same account the automation runs as (birme); main requires 1 approving review and enforce_admins is false, so no admin bypass was used. This PR is LGTM and CI is green — it needs a human maintainer's approving review to merge. Leaving the board item in In review.

Code Review

Verdict: LGTM

Summary: Clean, well-scoped implementation. The redirect fires on exactly 401 only when AUTH is a non-empty string, is wired before the throw so the existing 401→reauth path is untouched when AUTH is undefined, and is covered by focused tests. Typecheck, lint, and both new + regression suites pass green.

Blocking

  • None.

Warnings

  • vite.config.ts:11envPrefix: ["VITE_", "AUTH"] is a prefix match, not exact. Any build-time env var beginning with AUTH (e.g. AUTHORIZATION, AUTH_TOKEN) would also be inlined into the client bundle. Today only AUTH is intended, but this is a latent secret-leak footgun. Consider documenting the prefix semantics, or using define/a VITE_-prefixed name instead of widening the prefix allowlist.
  • src/api/handle-fetch-request.ts:26 — When AUTH is set and a 401 occurs, the redirect is triggered but the function still throws, so callers reacting to 401 (use-fetch-production-list.tsAPI.reauth(), use-heartbeat.ts) may fire a stray reauth in the brief window before navigation completes (location.assign is async). Redundant, not breaking.

Suggestions

  • src/api/redirect-on-auth-failure.ts:18 — the exact-string loop guard is effectively dead code in production (AUTH is a different origin/path); harmless.
  • Open-redirect is not a concern — AUTH is a build-time trusted value, never user input.
  • entrypoint.sh / Dockerfile passthrough verified correct (build runs at container start, so runtime AUTH reaches the build).

@birme

birme commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Automated code review (daily-backlog-pr) — LGTM

Verdict recorded as a comment: the pipeline account authored this PR, so it cannot post a formal Approve or self-merge. A human maintainer needs to approve + merge.

Clean, well-contained change. The redirect is a genuine no-op when AUTH is unset (returns false without touching location), the loop guard is correct with no infinite-redirect risk, all fetches still route through handleFetchRequest/the API object, and there's no open-redirect concern since AUTH is build-time config, not user input. The 4 tests are meaningful and pass. One thing to confirm: when AUTH is set, a 401 (including from /reauth) now hard-navigates to the OSC login URL, pre-empting the existing silent cookie-refresh flow — please confirm that's the intended product behavior.

@birme

birme commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

code-reviewer verdict: NEEDS CHANGES (PR #695, closes #166)

Implementation is clean and correctly fixes #166 with good tests; no open-redirect (target is a build-time constant). One blocking item:

  • vite.config.ts: envPrefix: ["VITE_", "AUTH"] is a prefix match — any env var starting with AUTH (e.g. AUTH_TOKEN, AUTHORIZATION) gets baked into the public client bundle. Secret-exposure regression. Narrow to the exact var via define (e.g. import.meta.env.AUTH) or rename to VITE_AUTH.
  • Warnings: loop guard is dead code for external URLs; consider a new URL() sanity check on the redirect target.

Note: recorded as a comment, not a state-bearing --request-changes review, because this PR's author and the automation account are both birme and GitHub blocks self-reviews. This does NOT increment the branch-protection review count or the escalation guard.

@birme

birme commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Automated code-review verdict: NEEDS CHANGES — implements #166.
Blocking: the new maybeRedirectToAuth fires on every 401 inside handle-fetch-request.ts before the throw, which collides with the existing silent-reauth flow (use-fetch-production-list.ts, hourly use-reauth.tsx). A transient/expected 401 — including one from the reauth endpoint itself, which also routes through handleFetchRequest — now navigates away to OSC login instead of letting in-app reauth renew the cookie. Needs a deliberate interaction fix (only redirect after reauth has failed, or exclude the reauth/list paths) plus a test.
Warnings: loop guard compares window.location.href to an external authUrl (different origin) so it's a no-op in production — real redirect-loop risk; no import.meta.env.DEV guard unlike the existing reauth flow.

Posted as a comment, not a state-bearing GitHub review: the daily-backlog-pr review identity (birme) is also this PR's author, so GitHub blocks a self request-changes. Board item left at In review (#48) and flagged for a human to action the changes below.

@birme

birme commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

code-reviewer verdict: Needs Changes (self-authored PR — marker in lieu of a state-bearing review, which GitHub blocks for the automation's own account).

Three Warnings, no single Blocking, but 3+ Warnings with no mitigations → Needs Changes:

  • envPrefix footgun (vite.config.ts:11): envPrefix: ["AUTH"] is a prefix match, so it exposes every AUTH* env var to the client bundle, not just the intended one. Scope it to the exact var(s) needed.
  • Redirect short-circuits reauth: the redirect path returns before the reauth flow runs, so a redirected response silently skips re-authentication.
  • Brittle loop guard: the guard condition is fragile and can mis-fire; tighten it to the actual terminating condition.

Board item moved back to Ready for the implementer to address.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Redirect to OSC login on 401 auth from intercom-manager

2 participants