fix(auth): drop popup.closed check in Plex pin poll#2941
fix(auth): drop popup.closed check in Plex pin poll#29410xSysR3ll merged 3 commits intoseerr-team:developfrom
Conversation
`popup.closed` is inconsistent when used after the OAuth popup makes a cross-origin navigation when deployed under `Cross-Origin-Opener-Policy: same-origin-allow-popups`. This results in the opener becoming undefined, and `closed` evaluating to `true`, even though the user is still authenticating with app.plex.tv. The previous `pinPoll()` considered this situation a cancelled login attempt after the first iteration and therefore failed. The result was that signing into Plex was impossible through any reverse proxy configured to include this header (e.g., Caddy/nginx/Traefik "secure defaults" blocks). Continue polling until the PIN is either authorised by Plex or invalidated; the latter condition manifests through the catch. Normal operation is unaffected.
📝 WalkthroughWalkthrough
Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Parent as Parent Window
participant Popup as Plex Popup
participant Plex as Plex API
Note over Parent,Popup: Login flow with PIN polling
User->>Parent: Click "Sign in with Plex"
Parent->>Popup: Open popup
User->>Popup: Authenticate on plex.tv
Popup->>Parent: (postMessage) AUTH_COMPLETE with pin id
Parent->>Plex: GET /api/v2/pins/{id} (poll)
Plex-->>Parent: { data: { authToken: ... } } OR { data: { expiresAt: ... } }
alt authToken present
Parent->>Popup: close popup
Parent->>Parent: resolve poll with token -> proceed to POST /api/v1/auth/plex
else no authToken yet
Parent->>Parent: compute expiresAt (or use deadline)
alt now >= min(expiresAt, deadline)
Parent->>Popup: close popup
Parent->>Parent: reject poll (PIN expired)
else
Parent->>Parent: schedule next poll after 1s
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/plex.ts`:
- Around line 143-147: The polling loop in executePoll (src/utils/plex.ts) can
run forever because popup.closed was removed and Plex returns 200 with null
authToken for expired pins; update executePoll to read the pin response's
expiresAt or expiresIn and compute a client-side deadline (fallback to ~15
minutes), then stop scheduling further setTimeout calls and reject the original
Promise (via reject) once the deadline is reached; ensure the check runs before
scheduling setTimeout and that any in-flight schedule is not re-queued after
expiry so the Promise always settles.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
fallenbagel
left a comment
fallenbagel
left a comment
There was a problem hiding this comment.
@Finchow Could you also update #2939 to make it clear this is specifically a COOP same-origin-allow-popups issue? Right now the title/description read like a generic "Plex login broken behind reverse proxy," which it isn't.
Without that framing, anyone hitting an unrelated Plex OAuth problem (popup blocker, network, misconfigured reverse proxy, whatever) is going to pile onto this issue with "+1 same here" and create unrelated noise.
A title like "Plex login fails when reverse proxy sets Cross-Origin-Opener-Policy: same-origin-allow-popups" plus a description of the COOP/popup.closed interaction would be fine.
|
@fallenbagel. Hi, sorry for the delay I have added the changes. I can confirm it works in a normal env and in my COOP env. Please let me know if you would like anything else done |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/utils/plex.ts (1)
148-155: Minor: guard againstDate.parsereturningNaN.If
response.data.expiresAtis ever present but in a formatDate.parsecan't handle,expiresAtbecomesNaN,Math.min(NaN, deadline)isNaN, andDate.now() >= NaNis alwaysfalse— so the loop would run until tab close. Plex currently returns ISO 8601, so this is defensive only.🛡️ Defensive fallback to
deadlineon parse failure- const expiresAt = response.data?.expiresAt - ? Date.parse(response.data.expiresAt) - : deadline; - if (Date.now() >= Math.min(expiresAt, deadline)) { + const parsedExpiresAt = response.data?.expiresAt + ? Date.parse(response.data.expiresAt) + : NaN; + const expiresAt = Number.isFinite(parsedExpiresAt) + ? parsedExpiresAt + : deadline; + if (Date.now() >= Math.min(expiresAt, deadline)) { this.closePopup(); reject(new Error('Plex PIN expired before login completed.')); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/plex.ts` around lines 148 - 155, The expiresAt computation uses Date.parse(response.data.expiresAt) which may return NaN; update the logic around the expiresAt variable (the code that sets expiresAt from response.data.expiresAt) to detect NaN (Number.isNaN) and fall back to the existing deadline when parsing fails so Math.min(expiresAt, deadline) never becomes NaN; this change should be made where expiresAt is assigned and before the Date.now() >= Math.min(...) check in the same function/method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/utils/plex.ts`:
- Around line 148-155: The expiresAt computation uses
Date.parse(response.data.expiresAt) which may return NaN; update the logic
around the expiresAt variable (the code that sets expiresAt from
response.data.expiresAt) to detect NaN (Number.isNaN) and fall back to the
existing deadline when parsing fails so Math.min(expiresAt, deadline) never
becomes NaN; this change should be made where expiresAt is assigned and before
the Date.now() >= Math.min(...) check in the same function/method.
5 participants
Description
popup.closedis inconsistent when used after the OAuth popup makes a cross-origin navigation when deployed underCross-Origin-Opener-Policy: same-origin-allow-popups.This results in the opener becoming undefined, and
closedevaluating totrue, even though the user is still authenticating with app.plex.tv.The previous
pinPoll()considered this situation a cancelled login attempt after the first iteration and therefore failed.The result was that signing into Plex was impossible through any reverse proxy configured to include this header (e.g., Caddy/nginx/Traefik "secure defaults" blocks).
Continue polling until the PIN is either authorised by Plex or invalidated; the latter condition manifests through the catch. Normal operation is unaffected.
AI Disclosure:
How Has This Been Tested?
Built and deployed onto my prod server where I first encountered the issue. Tested after fix applied and works.
Ran all tests in project & checked for issues in the dev console - none.__
Checklist:
pnpm buildpnpm i18n:extractSummary by CodeRabbit