feat(auth): multi-origin viewport OAuth (localhost tunnel + reverse-proxied host) #11

Merged

hibryda merged 3 commits from feat/viewport-multi-origin-auth into main 2026-06-03 12:52:50 +02:00

Conversation 3 Commits 3 Files changed 10 +405 -23

hibryda commented 2026-06-03 12:32:16 +02:00

Owner

Copy link

Prerequisite for exposing the viewport at reviewer.hemoglobina.store while keeping the existing localhost tunnel as an independent login path (operator chose 'multi-origin' over single-origin).

Problem

The viewport ties OAuth (redirect_uri, session-cookie host, SSE Origin check) to a single STATUS_PUBLIC_URL. Flipping it to the public host breaks localhost-tunnel login; keeping localhost breaks remote login.

Approach

Select the public origin per request from an operator-configured allow-list.

• config: STATUS_PUBLIC_URLS (comma-separated allow-list); back-compat — a lone STATUS_PUBLIC_URL → single-entry list. statusPublicUrl stays as primary/fallback ([0]).
• src/auth/select-origin.ts (pure, security-critical): derive scheme (X-Forwarded-Proto only when trustProxy) + host (X-Forwarded-Host/Host), match exactly against the allow-list; unmatched/forged → primary fallback (no host-header injection / open redirect). Chosen origin carried in the OAuth state.
• routes: login builds redirect_uri from the selected origin + stores it in state; callback rebuilds the token-exchange redirect_uri from the state origin, re-validated against the allow-list (tamper-safe).
• sse-producer: originAllowed → allow-list membership.
• __Host- cookie unchanged (host-scoped, works per-origin).

Security notes

The allow-list match is the load-bearing boundary — a forged Host/X-Forwarded-Host can only ever resolve to a pre-registered origin or fall back to the primary. X-Forwarded-* is honoured only under TRUST_PROXY. Tested: forged-Host fallback, spoofed-forwarded-when-untrusted fallback, scheme-mismatch fallback, state-tamper fallback.

Tests

+17 (origin selection, login→callback origin carry, tamper fallback, config parse + back-compat, SSE multi-origin); full suite 2168; typecheck clean.

Not in this PR

The infra (DNS reviewer A-record, DO Caddy block, viewport dual-bind + STATUS_PUBLIC_URLS/TRUST_PROXY env, OAuth-app callback URI) lands after this merges + deploys.

Prerequisite for exposing the viewport at `reviewer.hemoglobina.store` while keeping the existing localhost tunnel as an independent login path (operator chose 'multi-origin' over single-origin). ## Problem The viewport ties OAuth (`redirect_uri`, session-cookie host, SSE Origin check) to a **single** `STATUS_PUBLIC_URL`. Flipping it to the public host breaks localhost-tunnel login; keeping localhost breaks remote login. ## Approach Select the public origin **per request** from an operator-configured allow-list. - **config**: `STATUS_PUBLIC_URLS` (comma-separated allow-list); back-compat — a lone `STATUS_PUBLIC_URL` → single-entry list. `statusPublicUrl` stays as primary/fallback (`[0]`). - **`src/auth/select-origin.ts`** (pure, security-critical): derive scheme (`X-Forwarded-Proto` only when `trustProxy`) + host (`X-Forwarded-Host`/`Host`), match **exactly** against the allow-list; unmatched/forged → primary fallback (no host-header injection / open redirect). Chosen origin carried in the OAuth `state`. - **routes**: login builds `redirect_uri` from the selected origin + stores it in state; callback rebuilds the token-exchange `redirect_uri` from the **state** origin, re-validated against the allow-list (tamper-safe). - **sse-producer**: `originAllowed` → allow-list membership. - `__Host-` cookie unchanged (host-scoped, works per-origin). ## Security notes The allow-list match is the load-bearing boundary — a forged `Host`/`X-Forwarded-Host` can only ever resolve to a pre-registered origin or fall back to the primary. `X-Forwarded-*` is honoured only under `TRUST_PROXY`. Tested: forged-Host fallback, spoofed-forwarded-when-untrusted fallback, scheme-mismatch fallback, state-tamper fallback. ## Tests +17 (origin selection, login→callback origin carry, tamper fallback, config parse + back-compat, SSE multi-origin); full suite **2168**; typecheck clean. ## Not in this PR The infra (DNS `reviewer` A-record, DO Caddy block, viewport dual-bind + `STATUS_PUBLIC_URLS`/`TRUST_PROXY` env, OAuth-app callback URI) lands after this merges + deploys.

hibryda added 1 commit 2026-06-03 12:32:16 +02:00

feat(auth): multi-origin viewport OAuth (localhost tunnel + reverse-proxied host) ... a8c1c51e09

The viewport ties OAuth (redirect_uri, cookie host, CSRF/SSE origin) to a SINGLE
STATUS_PUBLIC_URL, so a deployment can only fully log in on one origin. To expose
the viewport publicly via a reverse proxy (e.g. reviewer.hemoglobina.store) while
KEEPING the existing direct localhost tunnel as an independent login path, the
viewport now selects its public origin per request from an allow-list.

- config: new STATUS_PUBLIC_URLS (comma-separated allow-list); back-compat —
  a lone STATUS_PUBLIC_URL yields a single-entry list. statusPublicUrl stays as
  the primary (= statusPublicUrls[0]) + the fallback.
- select-origin.ts: pure selectPublicOrigin(req, allowList, {trustProxy}) —
  derives scheme (X-Forwarded-Proto only when trustProxy) + host
  (X-Forwarded-Host/Host), matched EXACTLY against the allow-list; any
  unmatched/forged value falls back to the primary (no host-header injection /
  open redirect). The chosen origin is carried in the OAuth state.
- routes: handleLogin builds redirect_uri from the selected origin + stores it
  in state; handleCallback rebuilds the token-exchange redirect_uri from the
  state origin, re-validated against the allow-list (tamper-safe fallback).
- sse-producer: originAllowed now checks membership of the origin allow-list
  (the viewport may answer on several origins at once).
- viewport wires selectOrigin into the auth ctx + expectedOrigins into the SSE
  producer. __Host- cookie is unchanged (host-scoped, works per-origin).

+17 tests (origin selection incl. forged-Host/spoofed-forwarded fallbacks,
login→callback origin carry, tamper fallback, config parsing, SSE allow-list);
full suite 2168.

pr-reviewer-bot commented 2026-06-03 12:37:39 +02:00

Member

Copy link

Superseded by round 2.

Show previous round

hib-pr-reviewer review — PR #11 (hib/hib-pr-reviewer)

Round 1 — head a8c1c51e09d9, base main, trigger opened

TL;DR: CONDITIONAL_APPROVE — kept 2 agreed findings + 1 verified unique-to-A + 1 verified unique-to-B; all minor/info, no blocking issues.

Summary

Arbitration summary

3 total findings evaluated (2 agreed, 1 unique-to-A, 1 unique-to-B).

Agreed (A ∩ B)

• routes.ts:1117 as URL cast — both reviewers independently flagged the same line. Read at offset 1090 confirms return config.statusPublicUrl as URL; at line 1117 with no local null assertion. Kept as minor.

Unique-to-A — verified KEEP

• config.ts:529 dead null-guard. Read of parseStatusPublicUrl (line 456-508) confirms: for a non-empty raw the function either throws (all validation paths) or returns a URL; null is only returned for !raw (line 457). The loop already .filters out empty strings (line 527), so u is never null there. The u && guard is dead code and misleads readers. Kept as info.

Unique-to-B — verified KEEP

• routes.ts:263 origin.toString() stores the full URL string (e.g. https://example.com/), not the bare origin string. Confirmed at line 263: origin: origin.toString(). resolveAllowedOrigin at line 1108 then re-parses it with new URL(stored).origin inside a try/catch — a round-trip that would be unnecessary if origin.origin (the bare scheme+host+port) were stored instead. The code is functionally correct but the extra parse + try/catch is avoidable churn. Kept as minor.

No findings were dropped. Total kept: 3.

Blast Radius

The diff spans the exported auth surface (src/auth/config.ts, src/auth/routes.ts), a new module (src/auth/select-origin.ts), the viewport entry-point (bin/svelte-viewport.ts), and the SSE producer. Changes to AuthConfig and AuthContext interfaces propagate to every consumer that constructs or reads those types. Impact is bounded to the auth subsystem but the interface changes are exported.

BLAST_SCORE: 5/10

Risk Indicators

Indicator	Value
Sensitive functions	resolveAllowedOrigin, selectPublicOrigin, handleCallback, handleLogin, buildCallbackUri, parseStatusPublicUrls, parseStatusPublicUrl
Migration touched	—
Test delta	+204 / -6 lines in test files
Dependency changes	—

CI status (head a8c1c51e09d9)

Overall: ✓ success

3 checks: 3 pending

Check	State	Link
pr-reviewer CI / tsc --noEmit (pull_request)	⏳ pending	details
pr-reviewer CI / vitest (pull_request)	⏳ pending	details
pr-reviewer CI / tsc build (pull_request)	⏳ pending	details

Findings (3)

[MINOR] Unchecked as URL cast in resolveAllowedOrigin — relies on undocumented caller invariant

src/auth/routes.ts:1117

return config.statusPublicUrl as URL;

config.statusPublicUrl is typed URL | null. The cast is safe at the current callsite because handleCallback guards statusPublicUrl !== null upstream, but resolveAllowedOrigin carries no local assertion, so TypeScript will type-check cleanly even if a future caller omits that guard — the runtime would then return null and cause a TypeError inside buildCallbackUri. Fix with an explicit guard:

if (!config.statusPublicUrl) throw new Error("[auth] resolveAllowedOrigin: statusPublicUrl unset — caller must guard");
return config.statusPublicUrl;

This removes the cast and makes TypeScript enforce the invariant rather than suppress it.

[MINOR] State field origin stores a full URL string, not a bare origin string — unnecessary parse round-trip in callback

src/auth/routes.ts:263

origin: origin.toString(), // stores e.g. "https://reviewer.hemoglobina.store/"

The field is named origin but holds the full URL (trailing slash included). resolveAllowedOrigin must therefore recover the origin with a try/catch new URL(stored).origin call before the allow-list lookup. Storing the bare origin string eliminates the round-trip and makes the intent unambiguous:

origin: origin.origin, // stores "https://reviewer.hemoglobina.store"

With this change resolveAllowedOrigin simplifies to a direct === comparison:

const match = config.statusPublicUrls.find((u) => u.origin === stored);

No URL re-parse, no try/catch needed.

[INFO] Dead null-guard on u in parseStatusPublicUrls loop

src/auth/config.ts:529

const u = parseStatusPublicUrl(part);
if (u && !seen.has(u.origin)) {

part is guaranteed non-empty by the preceding .filter((s) => s !== ""). For non-empty inputs, parseStatusPublicUrl either throws (invalid URL, wrong scheme, userinfo, query/fragment present) or returns a URL — the only return null path is if (!raw) return null at line 457, which is unreachable here. The u && truthiness guard is therefore dead code and suggests to future readers that invalid entries might be silently skipped rather than surfacing a startup error. Remove it:

const u = parseStatusPublicUrl(part); // throws on invalid; always returns URL for non-empty input
if (!seen.has(u.origin)) {

Verdict

CONDITIONAL_APPROVE

---

_{hib-pr-reviewer • round 1 • 3 findings (2m/1i) • 2026-06-03T10:32:17.326Z → 2026-06-03T10:37:39.523Z • posted-as: pr-reviewer-bot • model: auto}

<!-- hib-pr-reviewer collapsed --> > _Superseded by round 2._ <details> <summary>Show previous round</summary> <!-- hib-pr-reviewer round:1 --> ## hib-pr-reviewer review — PR #11 (hib/hib-pr-reviewer) **Round 1** — head `a8c1c51e09d9`, base `main`, trigger `opened` **TL;DR:** CONDITIONAL_APPROVE — kept 2 agreed findings + 1 verified unique-to-A + 1 verified unique-to-B; all minor/info, no blocking issues. ### Summary ## Arbitration summary **3 total findings evaluated (2 agreed, 1 unique-to-A, 1 unique-to-B).** ### Agreed (A ∩ B) - `routes.ts:1117` `as URL` cast — both reviewers independently flagged the same line. `Read` at offset 1090 confirms `return config.statusPublicUrl as URL;` at line 1117 with no local null assertion. Kept as minor. ### Unique-to-A — verified KEEP - `config.ts:529` dead null-guard. `Read` of `parseStatusPublicUrl` (line 456-508) confirms: for a non-empty `raw` the function either **throws** (all validation paths) or **returns a `URL`**; `null` is only returned for `!raw` (line 457). The loop already `.filter`s out empty strings (line 527), so `u` is never `null` there. The `u &&` guard is dead code and misleads readers. Kept as info. ### Unique-to-B — verified KEEP - `routes.ts:263` `origin.toString()` stores the full URL string (e.g. `https://example.com/`), not the bare origin string. Confirmed at line 263: `origin: origin.toString()`. `resolveAllowedOrigin` at line 1108 then re-parses it with `new URL(stored).origin` inside a try/catch — a round-trip that would be unnecessary if `origin.origin` (the bare scheme+host+port) were stored instead. The code is functionally correct but the extra parse + try/catch is avoidable churn. Kept as minor. No findings were dropped. Total kept: 3. ### Blast Radius The diff spans the exported auth surface (`src/auth/config.ts`, `src/auth/routes.ts`), a new module (`src/auth/select-origin.ts`), the viewport entry-point (`bin/svelte-viewport.ts`), and the SSE producer. Changes to `AuthConfig` and `AuthContext` interfaces propagate to every consumer that constructs or reads those types. Impact is bounded to the auth subsystem but the interface changes are exported. **BLAST_SCORE: 5/10** ### Risk Indicators | Indicator | Value | |---|---| | Sensitive functions | `resolveAllowedOrigin`, `selectPublicOrigin`, `handleCallback`, `handleLogin`, `buildCallbackUri`, `parseStatusPublicUrls`, `parseStatusPublicUrl` | | Migration touched | — | | Test delta | +204 / -6 lines in test files | | Dependency changes | — | ### CI status (head `a8c1c51e09d9`) **Overall: ✓ success** 3 checks: 3 pending | Check | State | Link | |---|---|---| | pr-reviewer CI / tsc --noEmit (pull_request) | ⏳ pending | [details](/hib/hib-pr-reviewer/actions/runs/81/jobs/0) | | pr-reviewer CI / vitest (pull_request) | ⏳ pending | [details](/hib/hib-pr-reviewer/actions/runs/81/jobs/1) | | pr-reviewer CI / tsc build (pull_request) | ⏳ pending | [details](/hib/hib-pr-reviewer/actions/runs/81/jobs/2) | ### Findings (3) #### **[MINOR]** Unchecked `as URL` cast in `resolveAllowedOrigin` — relies on undocumented caller invariant _src/auth/routes.ts:1117_ ```typescript return config.statusPublicUrl as URL; ``` `config.statusPublicUrl` is typed `URL | null`. The cast is safe at the current callsite because `handleCallback` guards `statusPublicUrl !== null` upstream, but `resolveAllowedOrigin` carries no local assertion, so TypeScript will type-check cleanly even if a future caller omits that guard — the runtime would then return `null` and cause a `TypeError` inside `buildCallbackUri`. Fix with an explicit guard: ```typescript if (!config.statusPublicUrl) throw new Error("[auth] resolveAllowedOrigin: statusPublicUrl unset — caller must guard"); return config.statusPublicUrl; ``` This removes the cast and makes TypeScript enforce the invariant rather than suppress it. #### **[MINOR]** State field `origin` stores a full URL string, not a bare origin string — unnecessary parse round-trip in callback _src/auth/routes.ts:263_ ```typescript origin: origin.toString(), // stores e.g. "https://reviewer.hemoglobina.store/" ``` The field is named `origin` but holds the full URL (trailing slash included). `resolveAllowedOrigin` must therefore recover the origin with a try/catch `new URL(stored).origin` call before the allow-list lookup. Storing the bare origin string eliminates the round-trip and makes the intent unambiguous: ```typescript origin: origin.origin, // stores "https://reviewer.hemoglobina.store" ``` With this change `resolveAllowedOrigin` simplifies to a direct `===` comparison: ```typescript const match = config.statusPublicUrls.find((u) => u.origin === stored); ``` No URL re-parse, no try/catch needed. #### **[INFO]** Dead null-guard on `u` in `parseStatusPublicUrls` loop _src/auth/config.ts:529_ ```typescript const u = parseStatusPublicUrl(part); if (u && !seen.has(u.origin)) { ``` `part` is guaranteed non-empty by the preceding `.filter((s) => s !== "")`. For non-empty inputs, `parseStatusPublicUrl` either **throws** (invalid URL, wrong scheme, userinfo, query/fragment present) or **returns a `URL`** — the only `return null` path is `if (!raw) return null` at line 457, which is unreachable here. The `u &&` truthiness guard is therefore dead code and suggests to future readers that invalid entries might be silently skipped rather than surfacing a startup error. Remove it: ```typescript const u = parseStatusPublicUrl(part); // throws on invalid; always returns URL for non-empty input if (!seen.has(u.origin)) { ``` ### Verdict **CONDITIONAL_APPROVE** --- _{hib-pr-reviewer • round 1 • 3 findings (2m/1i) • 2026-06-03T10:32:17.326Z → 2026-06-03T10:37:39.523Z • posted-as: pr-reviewer-bot • model: auto} </details>

hibryda referenced this pull request from a commit 2026-06-03 12:41:20 +02:00

fix(auth): address PR #11 review — cleaner origin storage + no unchecked cast

hibryda added 1 commit 2026-06-03 12:41:20 +02:00

fix(auth): address PR #11 review — cleaner origin storage + no unchecked cast ... 3ca1bbc35a

- resolveAllowedOrigin takes the (non-null) primary origin explicitly instead of
  casting `config.statusPublicUrl as URL` — removes the undocumented-invariant
  cast (the caller already guards null and now passes the narrowed local).
- State carries the BARE origin (publicUrl.origin) instead of the full URL
  string, so the callback matches it directly against the allow-list with no
  new URL() re-parse round-trip; the field name now matches its contents.
- parseStatusPublicUrl → parsePublicUrl: non-nullable (takes a non-empty entry,
  returns URL or throws), dropping the dead `u &&` null-guard in the loop.

full suite 2168.

pr-reviewer-bot commented 2026-06-03 12:49:26 +02:00

Member

Copy link

hib-pr-reviewer review — PR #11 (hib/hib-pr-reviewer)

Round 2 — head 3ca1bbc35a90, base main, trigger synchronize

TL;DR: CONDITIONAL_APPROVE — verified 1 unique-to-A minor finding (property-narrowing loss across await at line 257); Reviewer B returned 0 findings. No blocking issues.

Summary

Arbitration — Round 2

Reviewer A: 1 minor finding (null-guard pattern inconsistency in handleLogin across await boundary).
Reviewer B: 0 findings (empty JSON array, though prose notes two minor issues — the JSON governs).

Verification

Reviewer A's finding is unique-to-A and requires independent verification via Read.

What I checked (lines 219–282 of src/auth/routes.ts at HEAD):

1. Line 219 — if (ctx.config.statusPublicUrl === null) guard confirmed. TypeScript narrows ctx.config.statusPublicUrl to URL within this branch of control flow.
2. Lines 230–237 — await ctx.redis.eval(…) call confirmed between the guard and the later use. TypeScript does not preserve property-access narrowing across await suspension points; only const-local variables retain CFA narrowing past an await. This is documented TypeScript behavior.
3. Line 257 — const publicUrl = ctx.selectOrigin?.(req) ?? ctx.config.statusPublicUrl; confirmed. ctx.config.statusPublicUrl is read from the property after the await. In strict TypeScript, the RHS of ?? is typed as URL | null, so publicUrl is URL | null.
4. Line 264 — origin: publicUrl.origin confirmed. Under strictNullChecks, accessing .origin on URL | null without a guard is a type error.
5. handleCallback (lines 294–296) — confirmed uses the safe local-variable pattern: const primaryOrigin = ctx.config.statusPublicUrl; if (primaryOrigin === null) return;, so primaryOrigin is URL everywhere downstream.

Conclusion: Finding verified. At runtime, the null guard at line 219 does prevent execution when statusPublicUrl is null, so there is no crash risk in the current call flow. However, TypeScript strict mode would flag publicUrl.origin as a potential null-dereference because property narrowing is lost across the await. The inconsistency with handleCallback's established pattern also creates a maintenance hazard — a future refactor that moves the guard or the await could silently introduce a real NPE. Keeping as minor.

Blast Radius

Changes are contained to src/auth/ (config parsing, routes, new select-origin module) plus wiring in bin/svelte-viewport.ts. The multi-origin allow-list is additive and back-compatible; existing single-origin deployments are unaffected. The blast radius is limited to the OAuth login/callback flow.

BLAST_SCORE: 3/10

Risk Indicators

Indicator	Value
Sensitive functions	handleLogin, handleCallback, resolveAllowedOrigin, selectPublicOrigin, parsePublicUrl, parseStatusPublicUrls
Migration touched	—
Test delta	+204 / -6 lines in test files
Dependency changes	—

CI status (head 3ca1bbc35a90)

Overall: ✓ success

3 checks: 3 pending

Check	State	Link
pr-reviewer CI / tsc --noEmit (pull_request)	⏳ pending	details
pr-reviewer CI / vitest (pull_request)	⏳ pending	details
pr-reviewer CI / tsc build (pull_request)	⏳ pending	details

Findings (1)

[MINOR] Property re-read after await loses TypeScript narrowing; use local-variable pattern (matches handleCallback)

src/auth/routes.ts:257

handleLogin checks ctx.config.statusPublicUrl === null at line 219 (returning early on null), but then re-reads the same property at line 257 — after the await ctx.redis.eval(…) suspension at lines 230-237:

const publicUrl = ctx.selectOrigin?.(req) ?? ctx.config.statusPublicUrl;
// ^^^^ re-read from property after await

TypeScript's control-flow narrowing for property accesses is not preserved across await boundaries (only const-local variables retain narrowing past a suspension point). This means the RHS of ?? is still typed as URL | null, so publicUrl is URL | null. publicUrl.origin at line 264 is therefore a strictNullChecks violation.

handleCallback already uses the correct pattern (lines 294-296):

const primaryOrigin = ctx.config.statusPublicUrl; // captured before any await
if (primaryOrigin === null) { writeStatus(res, 500, …); return; }

Apply the same fix in handleLogin, capturing before the rate-limit await:

// At the top of handleLogin, before the rate-limit block:
const primaryStatusUrl = ctx.config.statusPublicUrl;
if (primaryStatusUrl === null) {
  writeStatus(res, 500, "auth misconfigured: STATUS_PUBLIC_URL unset");
  return;
}
// …then at line 257:
const publicUrl = ctx.selectOrigin?.(req) ?? primaryStatusUrl;

Runtime behaviour is unchanged (the line-219 guard still prevents null execution), but TypeScript now correctly infers publicUrl: URL, and the code matches the established handleCallback pattern, making future refactors safe.

Verdict

CONDITIONAL_APPROVE

---

_{hib-pr-reviewer • round 2 • 1 finding (1m) • 2026-06-03T10:41:20.937Z → 2026-06-03T10:49:26.159Z • posted-as: pr-reviewer-bot • model: auto}

<!-- hib-pr-reviewer round:2 --> ## hib-pr-reviewer review — PR #11 (hib/hib-pr-reviewer) **Round 2** — head `3ca1bbc35a90`, base `main`, trigger `synchronize` **TL;DR:** CONDITIONAL_APPROVE — verified 1 unique-to-A minor finding (property-narrowing loss across await at line 257); Reviewer B returned 0 findings. No blocking issues. ### Summary ## Arbitration — Round 2 **Reviewer A**: 1 minor finding (null-guard pattern inconsistency in `handleLogin` across `await` boundary). **Reviewer B**: 0 findings (empty JSON array, though prose notes two minor issues — the JSON governs). ### Verification Reviewer A's finding is unique-to-A and requires independent verification via `Read`. **What I checked (lines 219–282 of `src/auth/routes.ts` at HEAD):** 1. **Line 219** — `if (ctx.config.statusPublicUrl === null)` guard confirmed. TypeScript narrows `ctx.config.statusPublicUrl` to `URL` within this branch of control flow. 2. **Lines 230–237** — `await ctx.redis.eval(…)` call confirmed between the guard and the later use. TypeScript does **not** preserve property-access narrowing across `await` suspension points; only `const`-local variables retain CFA narrowing past an `await`. This is documented TypeScript behavior. 3. **Line 257** — `const publicUrl = ctx.selectOrigin?.(req) ?? ctx.config.statusPublicUrl;` confirmed. `ctx.config.statusPublicUrl` is read from the property after the `await`. In strict TypeScript, the RHS of `??` is typed as `URL | null`, so `publicUrl` is `URL | null`. 4. **Line 264** — `origin: publicUrl.origin` confirmed. Under `strictNullChecks`, accessing `.origin` on `URL | null` without a guard is a type error. 5. **`handleCallback` (lines 294–296)** — confirmed uses the safe local-variable pattern: `const primaryOrigin = ctx.config.statusPublicUrl; if (primaryOrigin === null) return;`, so `primaryOrigin` is `URL` everywhere downstream. **Conclusion**: Finding verified. At runtime, the null guard at line 219 does prevent execution when `statusPublicUrl` is null, so there is no crash risk in the current call flow. However, TypeScript strict mode **would** flag `publicUrl.origin` as a potential null-dereference because property narrowing is lost across the `await`. The inconsistency with `handleCallback`'s established pattern also creates a maintenance hazard — a future refactor that moves the guard or the `await` could silently introduce a real NPE. Keeping as **minor**. ### Blast Radius Changes are contained to `src/auth/` (config parsing, routes, new select-origin module) plus wiring in `bin/svelte-viewport.ts`. The multi-origin allow-list is additive and back-compatible; existing single-origin deployments are unaffected. The blast radius is limited to the OAuth login/callback flow. **BLAST_SCORE: 3/10** ### Risk Indicators | Indicator | Value | |---|---| | Sensitive functions | `handleLogin`, `handleCallback`, `resolveAllowedOrigin`, `selectPublicOrigin`, `parsePublicUrl`, `parseStatusPublicUrls` | | Migration touched | — | | Test delta | +204 / -6 lines in test files | | Dependency changes | — | ### CI status (head `3ca1bbc35a90`) **Overall: ✓ success** 3 checks: 3 pending | Check | State | Link | |---|---|---| | pr-reviewer CI / tsc --noEmit (pull_request) | ⏳ pending | [details](/hib/hib-pr-reviewer/actions/runs/82/jobs/0) | | pr-reviewer CI / vitest (pull_request) | ⏳ pending | [details](/hib/hib-pr-reviewer/actions/runs/82/jobs/1) | | pr-reviewer CI / tsc build (pull_request) | ⏳ pending | [details](/hib/hib-pr-reviewer/actions/runs/82/jobs/2) | ### Findings (1) #### **[MINOR]** Property re-read after `await` loses TypeScript narrowing; use local-variable pattern (matches `handleCallback`) _src/auth/routes.ts:257_ `handleLogin` checks `ctx.config.statusPublicUrl === null` at line 219 (returning early on null), but then re-reads the same property at line 257 — after the `await ctx.redis.eval(…)` suspension at lines 230-237: ```typescript const publicUrl = ctx.selectOrigin?.(req) ?? ctx.config.statusPublicUrl; // ^^^^ re-read from property after await ``` TypeScript's control-flow narrowing for **property accesses** is not preserved across `await` boundaries (only `const`-local variables retain narrowing past a suspension point). This means the RHS of `??` is still typed as `URL | null`, so `publicUrl` is `URL | null`. `publicUrl.origin` at line 264 is therefore a `strictNullChecks` violation. `handleCallback` already uses the correct pattern (lines 294-296): ```typescript const primaryOrigin = ctx.config.statusPublicUrl; // captured before any await if (primaryOrigin === null) { writeStatus(res, 500, …); return; } ``` Apply the same fix in `handleLogin`, capturing before the rate-limit `await`: ```typescript // At the top of handleLogin, before the rate-limit block: const primaryStatusUrl = ctx.config.statusPublicUrl; if (primaryStatusUrl === null) { writeStatus(res, 500, "auth misconfigured: STATUS_PUBLIC_URL unset"); return; } // …then at line 257: const publicUrl = ctx.selectOrigin?.(req) ?? primaryStatusUrl; ``` Runtime behaviour is unchanged (the line-219 guard still prevents null execution), but TypeScript now correctly infers `publicUrl: URL`, and the code matches the established `handleCallback` pattern, making future refactors safe. ### Verdict **CONDITIONAL_APPROVE** --- _{hib-pr-reviewer • round 2 • 1 finding (1m) • 2026-06-03T10:41:20.937Z → 2026-06-03T10:49:26.159Z • posted-as: pr-reviewer-bot • model: auto}

hibryda added 1 commit 2026-06-03 12:50:26 +02:00

fix(auth): capture primaryOrigin local in handleLogin (narrowing across await) ... b10603d936

Round-2 review: handleLogin relied on TS narrowing ctx.config.statusPublicUrl
non-null across the rate-limit await — TS drops property narrowing across await,
so the invariant was fragile. Capture it in a local const right after the guard
(mirrors handleCallback's primaryOrigin), and build publicUrl from that local.
No behaviour change. full suite 2168.

hibryda merged commit 33e6532b27 into main 2026-06-03 12:52:50 +02:00

hibryda deleted branch feat/viewport-multi-origin-auth 2026-06-03 12:52:50 +02:00

hibryda referenced this pull request from a commit 2026-06-03 12:52:50 +02:00

feat(auth): multi-origin viewport OAuth (localhost tunnel + reverse-proxied host) (#11)

pr-reviewer-bot commented 2026-06-03 12:52:55 +02:00

Member

Copy link

Review cancelled by operator at 2026-06-03T10:52:55.453Z.

Review cancelled by operator at 2026-06-03T10:52:55.453Z.

hibryda referenced this pull request 2026-06-03 17:55:11 +02:00

docs: document public reverse-proxy exposure (reviewer.hemoglobina.store) #12

hibryda referenced this pull request from a commit 2026-06-03 18:46:56 +02:00

fix(health): cancel/restart return 202 (not 503) when only the publish ack times out

hibryda referenced this pull request 2026-06-03 18:47:00 +02:00

fix(health): cancel/restart 202 (not 503) when only the publish ack times out #13

hibryda referenced this pull request from a commit 2026-06-03 18:58:48 +02:00

fix(health): cancel/restart 202 (not 503) when only the publish ack times out (#13)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

hib/hib-pr-reviewer!11

No description provided.