fix(auth): resolve Plex OAuth client ID mismatch#2746
Open
fix(auth): resolve Plex OAuth client ID mismatch#2746
Conversation
📝 WalkthroughWalkthroughAdds a persistent server-side Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Server
participant Plex
participant LinkedAPI as Server:LinkedAccounts
Browser->>Server: GET /api/v1/settings/public
Server-->>Browser: PublicSettings (includes plexClientIdentifier)
Browser->>Plex: POST /pins (X-Plex-Client-Identifier header)
Plex-->>Browser: PIN + clientIdentifier
Browser->>Browser: Open Plex auth popup (user approves)
Browser->>Plex: Poll /pins/:id for authToken
Plex-->>Browser: authToken
Browser->>LinkedAPI: POST /api/v1/user/linked/plex (authToken)
LinkedAPI-->>Server: validate/store token
LinkedAPI-->>Browser: 200 OK
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@seerr-api.yml`:
- Around line 713-717: Update the OpenAPI schema to make plexClientIdentifier
required: in the PublicSettings schema add "plexClientIdentifier" to its
required array (and ensure the plexClientIdentifier property remains defined
with type: string and format: uuid). Locate the PublicSettings definition and
modify the required list so the contract enforces presence of
plexClientIdentifier used by the Plex OAuth client.
In `@server/interfaces/api/settingsInterfaces.ts`:
- Line 51: The FullPublicSettings type and the fullPublicSettings getter are
missing the required plexClientIdentifier property, causing
/api/v1/settings/public to omit the Plex OAuth client id; update the
FullPublicSettings type to include plexClientIdentifier: string and modify the
fullPublicSettings getter in server/lib/settings/index.ts (function/getter
fullPublicSettings) to read the stored Plex client identifier (e.g., from the
same settings source used for other public fields) and include
plexClientIdentifier in the returned object so the public settings endpoint
always contains the identifier.
In `@server/lib/settings/index.ts`:
- Around line 392-393: Replace uses of randomUUID() for sessionSecret with a
32-byte high-entropy hex string: use crypto.randomBytes(32).toString('hex')
wherever sessionSecret is generated (e.g., the constructor default, the load()
fallback that sets sessionSecret, and the migration that seeds sessionSecret).
Add or ensure an import of randomBytes from 'crypto' (or use
require('crypto').randomBytes) in the modules where you change it so the new
32-byte hex secret is produced instead of a UUID.
In `@src/utils/plex.ts`:
- Around line 82-84: Wrap the call to initializeHeaders inside login in a
try/catch so that if header initialization throws (e.g., missing
plexClientIdentifier) you catch the error, call the same popup cleanup/close
routine used by preparePopup to close the pre-opened popup, then rethrow or
handle the error before returning; specifically update the login method (which
currently calls this.initializeHeaders and this.getPin) to catch initialization
failures, invoke the popup close/cleanup method used elsewhere in this class
(the one that undoes preparePopup), and only proceed to this.getPin on success.
- Around line 31-35: Replace unsafe global checks that use "if (!window)" with a
typeof check to avoid ReferenceError: in initializeHeaders (and the other
similar check in this file) change the condition to use "typeof window ===
'undefined'" (or the negated form) so the code safely detects non-browser
environments; update both the initializeHeaders function and the second
occurrence at line 162 to use this typeof check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ccb23e80-37d8-49bc-9bcb-7d989a4cc2a4
📒 Files selected for processing (11)
cypress/config/settings.cypress.jsonseerr-api.ymlserver/index.tsserver/interfaces/api/settingsInterfaces.tsserver/lib/settings/index.tsserver/lib/settings/migrations/0009_migrate_session_secret.tssrc/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsxsrc/context/SettingsContext.tsxsrc/hooks/usePlexLogin.tssrc/pages/_app.tsxsrc/utils/plex.ts
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes Plex OAuth failures caused by mismatched client identifiers between browser and server, and improves session security by moving cookie signing to a dedicated secret.
Changes:
- Browser-side Plex OAuth now uses a server-provided
plexClientIdentifierinstead of generating/storing its own localStorage ID. - Introduces
sessionSecretin settings and uses it for express-session signing (replacingclientId). - Updates public settings API/types/OpenAPI spec and Cypress settings fixture to include the new fields.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/plex.ts | Removes per-browser generated Plex client ID; requires identifier to be passed in and uses it in headers/login. |
| src/hooks/usePlexLogin.ts | Retrieves plexClientIdentifier from settings and passes it into plexOAuth.login. |
| src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsx | Passes settings-provided plexClientIdentifier into Plex OAuth when linking accounts. |
| src/context/SettingsContext.tsx | Extends default settings to include plexClientIdentifier. |
| src/pages/_app.tsx | Extends initial public settings shape to include plexClientIdentifier. |
| server/lib/settings/index.ts | Adds sessionSecret, exposes plexClientIdentifier via public settings, and ensures secrets are generated if missing. |
| server/lib/settings/migrations/0009_migrate_session_secret.ts | Adds a settings migration to populate sessionSecret for existing installs. |
| server/index.ts | Switches session signing secret from clientId to sessionSecret. |
| server/interfaces/api/settingsInterfaces.ts | Updates PublicSettingsResponse to include plexClientIdentifier. |
| seerr-api.yml | Documents plexClientIdentifier in the public settings schema. |
| cypress/config/settings.cypress.json | Adds sessionSecret to test settings fixture. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsx
Show resolved
Hide resolved
0xSysR3ll
force-pushed
the
fix/0xsysr3ll/plex-oauth-client-id
branch
from
401563c to
c8756e5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/utils/plex.ts (1)
82-89:⚠️ Potential issue | 🟡 MinorClose the pre-opened popup when
getPin()fails too.
preparePopup()runs beforelogin(), but the newtryonly coversinitializeHeaders(). If the PIN request rejects, the popup stays stuck on/login/plex/loading.Suggested fix
public async login(plexClientIdentifier: string): Promise<string> { try { this.initializeHeaders(plexClientIdentifier); + await this.getPin(); } catch (e) { this.closePopup(); throw e; } - await this.getPin();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/plex.ts` around lines 82 - 89, The login method currently only closes the pre-opened popup if initializeHeaders throws; wrap the await this.getPin() call (or add a try/catch around it) so that if getPin() rejects you call this.closePopup() and rethrow the error; update the login function (referencing login, initializeHeaders, getPin, and closePopup) to ensure the popup is closed on getPin failure while leaving successful flows unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/lib/settings/index.ts`:
- Around line 759-760: The getter get sessionSecret currently returns
this.data.sessionSecret! which can be undefined when load(overrideSettings)
returned early; update the override-path in load(overrideSettings) to
normalize/backfill the sessionSecret before returning (e.g., set
this.data.sessionSecret = this.data.sessionSecret ?? generateSessionSecret() or
call the same backfill/normalize helper used for the normal load path), and
ensure the same fix is applied for the other affected getters around 798-800 so
get sessionSecret and related getters never return undefined.
---
Duplicate comments:
In `@src/utils/plex.ts`:
- Around line 82-89: The login method currently only closes the pre-opened popup
if initializeHeaders throws; wrap the await this.getPin() call (or add a
try/catch around it) so that if getPin() rejects you call this.closePopup() and
rethrow the error; update the login function (referencing login,
initializeHeaders, getPin, and closePopup) to ensure the popup is closed on
getPin failure while leaving successful flows unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3507b71f-f311-47e7-bd7e-8b95da132343
📒 Files selected for processing (11)
cypress/config/settings.cypress.jsonseerr-api.ymlserver/index.tsserver/interfaces/api/settingsInterfaces.tsserver/lib/settings/index.tsserver/lib/settings/migrations/0009_migrate_session_secret.tssrc/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsxsrc/context/SettingsContext.tsxsrc/hooks/usePlexLogin.tssrc/pages/_app.tsxsrc/utils/plex.ts
✅ Files skipped from review due to trivial changes (5)
- src/pages/_app.tsx
- server/index.ts
- cypress/config/settings.cypress.json
- src/context/SettingsContext.tsx
- server/interfaces/api/settingsInterfaces.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/hooks/usePlexLogin.ts
- server/lib/settings/migrations/0009_migrate_session_secret.ts
- seerr-api.yml
0xSysR3ll
force-pushed
the
fix/0xsysr3ll/plex-oauth-client-id
branch
from
c8756e5 to
e2626eb
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/plex.ts (1)
82-117:⚠️ Potential issue | 🟠 MajorFail fast when the popup never opens.
If
window.open()is blocked orpreparePopup()was skipped, Lines 111-117 still fall through topinPoll().pinPoll()treatsthis.popup === undefinedas “not closed”, so the login flow polls forever instead of returning an actionable error.💡 Suggested fix
if (!this.plexHeaders || !this.pin) { throw new Error('Unable to call login if class is not initialized.'); } @@ - if (this.popup) { - this.popup.location.href = `https://app.plex.tv/auth/#!?${this.encodeData( - params - )}`; - } + if (!this.popup || this.popup.closed) { + this.closePopup(); + throw new Error( + 'Unable to open Plex login popup. Please allow popups and try again.' + ); + } + + this.popup.location.href = `https://app.plex.tv/auth/#!?${this.encodeData( + params + )}`; return this.pinPoll();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/plex.ts` around lines 82 - 117, The login method can hang because if the popup wasn't created (this.popup is undefined due to window.open being blocked or preparePopup skipped), the method proceeds to call pinPoll which treats undefined popup as "not closed" and polls forever; update login (after getPin and before calling pinPoll) to check that this.popup exists and is not closed, and if it doesn't exist or is blocked throw a descriptive error (e.g., "Popup blocked or not opened") and call closePopup as needed so the caller gets an actionable failure instead of an infinite poll; reference the login, getPin, pinPoll, this.popup and closePopup symbols when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/utils/plex.ts`:
- Around line 82-117: The login method can hang because if the popup wasn't
created (this.popup is undefined due to window.open being blocked or
preparePopup skipped), the method proceeds to call pinPoll which treats
undefined popup as "not closed" and polls forever; update login (after getPin
and before calling pinPoll) to check that this.popup exists and is not closed,
and if it doesn't exist or is blocked throw a descriptive error (e.g., "Popup
blocked or not opened") and call closePopup as needed so the caller gets an
actionable failure instead of an infinite poll; reference the login, getPin,
pinPoll, this.popup and closePopup symbols when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1421cd12-3b1f-4140-82b3-ac73c86447f6
📒 Files selected for processing (11)
cypress/config/settings.cypress.jsonseerr-api.ymlserver/index.tsserver/interfaces/api/settingsInterfaces.tsserver/lib/settings/index.tsserver/lib/settings/migrations/0009_migrate_session_secret.tssrc/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsxsrc/context/SettingsContext.tsxsrc/hooks/usePlexLogin.tssrc/pages/_app.tsxsrc/utils/plex.ts
✅ Files skipped from review due to trivial changes (5)
- server/interfaces/api/settingsInterfaces.ts
- src/pages/_app.tsx
- src/hooks/usePlexLogin.ts
- src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsx
- server/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- server/lib/settings/migrations/0009_migrate_session_secret.ts
- src/context/SettingsContext.tsx
- seerr-api.yml
fallenbagel
requested changes
Mar 29, 2026
Collaborator
fallenbagel
left a comment
fallenbagel
left a comment
There was a problem hiding this comment.
Overall looks good. A few comments
0xSysR3ll
force-pushed
the
fix/0xsysr3ll/plex-oauth-client-id
branch
from
e2626eb to
a289811
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
server/lib/settings/index.ts (1)
759-761:⚠️ Potential issue | 🟡 Minor
sessionSecretcan still be undefined on the override load path.
load(overrideSettings)returns before the missing-secret backfill runs, so the non-null assertion inget sessionSecret()can still returnundefinedat runtime for override payloads withoutsessionSecret.Suggested fix
public async load( overrideSettings?: AllSettings, raw = false ): Promise<Settings> { if (overrideSettings) { this.data = overrideSettings; + if (!this.data.sessionSecret) { + this.data.sessionSecret = randomBytes(32).toString('hex'); + } return this; }Also applies to: 798-800
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/settings/index.ts` around lines 759 - 761, The sessionSecret getter uses a non-null assertion but load(overrideSettings) can return before the backfill runs, so change get sessionSecret() to safely handle undefined: either throw a clear error if this.data.sessionSecret is missing or synchronously invoke the existing backfill/ensure routine (e.g., call an ensureSessionSecret()/backfillSessionSecret() helper or run the same logic used after load) and then return the value; do the same fix for the other affected getter referenced in the comment (replace the `!` with a guarded check and recovery/throw). Ensure you reference the sessionSecret getter and load(overrideSettings) behavior when implementing the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@server/lib/settings/index.ts`:
- Around line 759-761: The sessionSecret getter uses a non-null assertion but
load(overrideSettings) can return before the backfill runs, so change get
sessionSecret() to safely handle undefined: either throw a clear error if
this.data.sessionSecret is missing or synchronously invoke the existing
backfill/ensure routine (e.g., call an
ensureSessionSecret()/backfillSessionSecret() helper or run the same logic used
after load) and then return the value; do the same fix for the other affected
getter referenced in the comment (replace the `!` with a guarded check and
recovery/throw). Ensure you reference the sessionSecret getter and
load(overrideSettings) behavior when implementing the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 85c9e0ee-0d24-4c5d-8375-68c1a5f1dd65
📒 Files selected for processing (11)
cypress/config/settings.cypress.jsonseerr-api.ymlserver/index.tsserver/interfaces/api/settingsInterfaces.tsserver/lib/settings/index.tsserver/lib/settings/migrations/0009_migrate_session_secret.tssrc/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsxsrc/context/SettingsContext.tsxsrc/hooks/usePlexLogin.tssrc/pages/_app.tsxsrc/utils/plex.ts
✅ Files skipped from review due to trivial changes (3)
- src/pages/_app.tsx
- server/index.ts
- server/lib/settings/migrations/0009_migrate_session_secret.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- server/interfaces/api/settingsInterfaces.ts
- src/hooks/usePlexLogin.ts
- src/context/SettingsContext.tsx
fallenbagel
requested changes
Mar 30, 2026
Collaborator
There was a problem hiding this comment.
Isn't this migration unnecessary? The load() method already has an established pattern for generating missing secrets (clientId, apiKey, and VAPID keys all use the same approach)
Here is the example:
if (!this.data.clientId) {
this.data.clientId = randomUUID();
change = true;
}And you also have added same approach for sessionSecret below that already:
if (!this.data.sessionSecret) {
this.data.sessionSecret = randomBytes(32).toString('hex');
change = true;
}in load(). This is all that's needed and will be generated on first run and persisted via save(). The mergeSettings (lodash mergeWith) already handles new default values from the constructor, and the explicit check covers generating a unique value.
Settings migrations are only needed for transforming existing data (like renaming keys, restructuring objects, removing obsolete fields). It is not needed for adding new fields with generated values.
Contributor
Author
There was a problem hiding this comment.
Yep, that's on me my bad
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes an issue where Plex Oauth login could fail because the browser and server were using different client identifiers.
The browser generated its own ID (
plex-client-id, stored in localStorage), while the server usedclientId(stored insettings.json. Now both use the same value:plexClientIdentifierAs part of this PR, I also discovered a security issue:
clientIdwas being used to sign sessions even though it is a public value.To address this, this PR introduces a new
sessionSecretdedicated to session signing.Warning
Introducing this new session secret will obviously invalidate all existing sessions, meaning all users will be logged out.
How Has This Been Tested?
Screenshots / Logs (if applicable)
Checklist:
pnpm buildpnpm i18n:extractSummary by CodeRabbit
New Features
Bug Fixes