fix: authorize room invite previews through socket hosts - #25
Conversation
|
Warning Review limit reached
Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThe client sends validated room previews with media updates. Socket handlers track previews by socket and host. The server stores revisioned poster metadata, serves poster proxies, removes stale snapshots, and updates invite metadata. Tests cover lifecycle, authorization, caching, and routes. ChangesRoom preview synchronization
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/syncloungeserver/src/socketserver/validation.js`:
- Around line 46-62: Update assertOptionalMetadataScalar and
assertOptionalMetadataIndex to enforce bounded numeric metadata: validate year
as a non-negative safe integer within the allowed year range, and require
numeric indices to use the same maximum bound as the existing 1–12 digit string
validation. Preserve null/undefined handling and existing rejection messages or
schema behavior.
- Line 98: Update the room preview validation around assertOptionalString and
fetchPoster so posterUrl is resolved only through the configured Plex origin
associated with roomPreview.machineIdentifier; reject URLs whose origin does not
match that machine before fetching. Preserve valid matching-origin posters and
add a Socket.IO regression test covering a mismatched public origin.
In `@server.js`:
- Around line 449-464: Update onRoomMediaUpdate to bound retained room preview
revision snapshots independently from the shared metadata cache, while
preserving the immediately previous revision for authorization. Track recent
revision keys per room, delete the oldest snapshot when the configured small
limit is exceeded, clean up tracking when previews are cleared, and add a
regression test emitting many updates that verifies a previously cached
/api/metadata entry still resolves.
In `@src/__tests__/plexclients-room-preview.test.js`:
- Around line 20-23: Update the 'plexservers/GET_MEDIA_IMAGE_URL' mock to record
the getter arguments, then assert that the request includes both width: 600 and
height: 900. Preserve the existing URL behavior while ensuring the test verifies
the externally requested image dimensions.
In `@src/store/modules/plexclients/getters.js`:
- Around line 42-53: The Plex metadata preview copies unbounded fields that can
violate the socket validation schema. In
src/store/modules/plexclients/getters.js lines 42-53, update the room-preview
getter to truncate or omit every preview field whose server schema type or
length limit may be exceeded, especially the summary, while preserving valid
metadata. In src/__tests__/plexclients-room-preview.test.js lines 5-38, add
boundary cases with over-limit metadata and assert the generated preview remains
valid against the socket schema.
In `@test/room-preview-authorization.test.js`:
- Around line 203-216: Drain the child process stdout created by the
serverProcess spawn configuration. Update the stdio handling adjacent to
serverProcess.stderr.on so stdout is either forwarded to process.stdout or
explicitly discarded, ensuring server.js output cannot fill the pipe and block
the test suite.
In `@test/server.test.js`:
- Around line 754-756: Add a failure-path test in the existing HTTP server test
suite for the `/share/room-poster/:room/:revision` route, requesting an unknown
room and revision and asserting that the response status is 404.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f5a1bf8a-953a-4082-b289-2062555d6d44
📒 Files selected for processing (15)
cache.jspackages/syncloungeserver/src/socketserver/handlers.jspackages/syncloungeserver/src/socketserver/index.jspackages/syncloungeserver/src/socketserver/state.jspackages/syncloungeserver/src/socketserver/validation.jsserver.jssrc/__tests__/plexclients-room-preview.test.jssrc/__tests__/synclounge-actions.test.jssrc/store/modules/plexclients/actions.jssrc/store/modules/plexclients/getters.jssrc/store/modules/synclounge/actions.jssrc/views/PlexMedia.vuetest/cache.test.jstest/room-preview-authorization.test.jstest/server.test.js
💤 Files with no reviewable changes (1)
- src/views/PlexMedia.vue
696b012 to
16d74f0
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
src/__tests__/plexclients-room-preview.test.js (1)
20-23: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the requested poster height.
The mock output uses
options.widthonly. A change toheight: 900would still pass this test. Record the getter arguments and assert both requested dimensions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/plexclients-room-preview.test.js` around lines 20 - 23, Update the GET_MEDIA_IMAGE_URL mock in the room preview test to record its received options, then assert that both width and height match the requested poster dimensions, including height 900. Ensure the mock output remains based on the recorded arguments.Source: Path instructions
packages/syncloungeserver/src/socketserver/validation.js (1)
55-70: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winApply the same numeric bounds to both representations.
Line 58 accepts negative and fractional
yearvalues. Line 66 accepts numeric indices above the twelve-digit string limit. Require a bounded non-negative integer year. Cap numeric indices at999999999999.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/syncloungeserver/src/socketserver/validation.js` around lines 55 - 70, Update assertOptionalMetadataScalar to validate year values as bounded non-negative integers, applying the same constraints to numeric and string representations as appropriate. Update assertOptionalMetadataIndex so numeric values are non-negative safe integers no greater than 999999999999, matching the existing twelve-digit string limit.Source: Path instructions
src/store/modules/plexclients/getters.js (1)
42-53: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep room previews within the Socket.IO schema limits.
The getter copies raw Plex metadata. Server validation rejects over-limit fields and disconnects the socket. Long valid Plex metadata can therefore disconnect a client during
joinormediaUpdate.
src/store/modules/plexclients/getters.js#L42-L53: bound optional display fields before emission. If required identifiers or the generated URL exceed protocol limits, omit the preview instead of changing identifiers independently frommedia.src/__tests__/plexclients-room-preview.test.js#L5-L38: add 500-character and over-limit cases. Assert that generated previews remain within the socket schema or becomenull.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/store/modules/plexclients/getters.js` around lines 42 - 53, Bound optional display fields in the room-preview getter around the returned metadata object, while preserving required identifiers and the generated poster URL consistently with media; if any required identifier or URL exceeds the Socket.IO schema limits, return null for the entire preview rather than partially changing identifiers. In src/store/modules/plexclients/getters.js lines 42-53, apply the schema limits before emission. In src/__tests__/plexclients-room-preview.test.js lines 5-38, add 500-character and over-limit cases asserting previews remain schema-valid or become null.Source: Path instructions
test/room-preview-authorization.test.js (1)
203-216: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDrain or discard the child stdout pipe.
Line 214 sets
stdio: 'pipe', so the child receives a pipe for stdout. Line 216 consumes onlystderr. Nothing readsserverProcess.stdoutin the current code.
server.jslogspublicAppConfigat startup, and the socket handlers log on every join, room creation, and host transfer. This suite creates several rooms and many sockets. If the child fills the pipe buffer, its next write blocks and the suite hangs until the test timeout.Forward stdout like stderr, or set stdout to
ignore.🛠️ Proposed fix
- stdio: 'pipe', + stdio: ['ignore', 'ignore', 'pipe'], }); serverProcess.stderr.on('data', (data) => process.stderr.write(data));As per path instructions, tests must "cleanup spawned resources" and "avoid timing flakiness".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/room-preview-authorization.test.js` around lines 203 - 216, Update the child-process setup around serverProcess and its stdio configuration to consume or discard stdout, preventing the pipe from filling during room and socket activity. Either forward serverProcess.stdout through process.stdout alongside the existing stderr handler or set stdout to ignore, while preserving the test’s spawned-resource cleanup and timing behavior.Source: Path instructions
test/server.test.js (1)
754-756: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a failure-path test for an unknown room poster revision.
The
/share/room-poster/:room/:revisionroute still has no cache-miss coverage in this suite.proxyPosterreturns 404 whenmetais null. Request an arbitrary room and revision and assert 404. This suite already runs the server over HTTP, so the test needs no socket setup.💚 Suggested test
it('returns 404 for an unknown room poster revision', async () => { const res = await request('/share/room-poster/no-such-room/no-such-revision'); assert.equal(res.status, 404); });As per path instructions,
server.jsrequires "tests for failure paths".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/server.test.js` around lines 754 - 756, Add a failure-path test in the existing HTTP server test suite that requests an arbitrary unknown room and revision through `/share/room-poster/:room/:revision`, then assert the response status is 404. Use the suite’s existing `request` helper and test style; no socket setup or production changes are needed.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server.js`:
- Around line 47-63: Update the createCache call that initializes
setRoomPosterMetadata and getRoomPosterMetadata to include an explicit shorter
ttlMs of a few hours while preserving ROOM_POSTER_CACHE_MAX_SIZE. Leave the
general metadata cache and poster path behavior unchanged.
In `@test/cache.test.js`:
- Around line 122-133: Add a test near the existing createCache size coverage
that asserts createCache throws TypeError for invalid maxSize values of 0 and
1.5, and invalid ttlMs values of -1 and NaN, covering malformed and boundary
inputs.
In `@test/room-preview-authorization.test.js`:
- Around line 293-308: Replace the immediate getJoinHtml call after awaiting
guestSawTransfer with the existing waitForJoinHtml helper, using the room
identifier and expected transferred-host preview state. Keep the subsequent
guest HTML, poster, and retained host poster assertions unchanged.
---
Duplicate comments:
In `@packages/syncloungeserver/src/socketserver/validation.js`:
- Around line 55-70: Update assertOptionalMetadataScalar to validate year values
as bounded non-negative integers, applying the same constraints to numeric and
string representations as appropriate. Update assertOptionalMetadataIndex so
numeric values are non-negative safe integers no greater than 999999999999,
matching the existing twelve-digit string limit.
In `@src/__tests__/plexclients-room-preview.test.js`:
- Around line 20-23: Update the GET_MEDIA_IMAGE_URL mock in the room preview
test to record its received options, then assert that both width and height
match the requested poster dimensions, including height 900. Ensure the mock
output remains based on the recorded arguments.
In `@src/store/modules/plexclients/getters.js`:
- Around line 42-53: Bound optional display fields in the room-preview getter
around the returned metadata object, while preserving required identifiers and
the generated poster URL consistently with media; if any required identifier or
URL exceeds the Socket.IO schema limits, return null for the entire preview
rather than partially changing identifiers. In
src/store/modules/plexclients/getters.js lines 42-53, apply the schema limits
before emission. In src/__tests__/plexclients-room-preview.test.js lines 5-38,
add 500-character and over-limit cases asserting previews remain schema-valid or
become null.
In `@test/room-preview-authorization.test.js`:
- Around line 203-216: Update the child-process setup around serverProcess and
its stdio configuration to consume or discard stdout, preventing the pipe from
filling during room and socket activity. Either forward serverProcess.stdout
through process.stdout alongside the existing stderr handler or set stdout to
ignore, while preserving the test’s spawned-resource cleanup and timing
behavior.
In `@test/server.test.js`:
- Around line 754-756: Add a failure-path test in the existing HTTP server test
suite that requests an arbitrary unknown room and revision through
`/share/room-poster/:room/:revision`, then assert the response status is 404.
Use the suite’s existing `request` helper and test style; no socket setup or
production changes are needed.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8d3677aa-0fcd-4948-9587-6ff233e0aa20
📒 Files selected for processing (15)
cache.jspackages/syncloungeserver/src/socketserver/handlers.jspackages/syncloungeserver/src/socketserver/index.jspackages/syncloungeserver/src/socketserver/state.jspackages/syncloungeserver/src/socketserver/validation.jsserver.jssrc/__tests__/plexclients-room-preview.test.jssrc/__tests__/synclounge-actions.test.jssrc/store/modules/plexclients/actions.jssrc/store/modules/plexclients/getters.jssrc/store/modules/synclounge/actions.jssrc/views/PlexMedia.vuetest/cache.test.jstest/room-preview-authorization.test.jstest/server.test.js
💤 Files with no reviewable changes (1)
- src/views/PlexMedia.vue
3b49af4 to
a026959
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/__tests__/plexclients-room-preview.test.js`:
- Around line 104-117: Add a test alongside the existing schema-length case
using an unpaired UTF-16 surrogate in either metadata.machineIdentifier or
metadata.ratingKey, then assert GET_ACTIVE_MEDIA_ROOM_PREVIEW returns null with
the same getter setup. Keep the test focused on malformed Unicode metadata
reaching neither join nor mediaUpdate.
In `@test/room-preview-authorization.test.js`:
- Around line 429-455: Rename the test case around its existing
socket.emit('mediaUpdate', playerUpdate(...)) flow to state that it disconnects
clients submitting unknown room preview properties, replacing the inaccurate
“unbounded room preview fields” wording. Leave the test behavior and assertions
unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 17dd3ba8-8cbc-4dd8-8bbd-61a175567f87
📒 Files selected for processing (15)
cache.jspackages/syncloungeserver/src/socketserver/handlers.jspackages/syncloungeserver/src/socketserver/index.jspackages/syncloungeserver/src/socketserver/state.jspackages/syncloungeserver/src/socketserver/validation.jsserver.jssrc/__tests__/plexclients-room-preview.test.jssrc/__tests__/synclounge-actions.test.jssrc/store/modules/plexclients/actions.jssrc/store/modules/plexclients/getters.jssrc/store/modules/synclounge/actions.jssrc/views/PlexMedia.vuetest/cache.test.jstest/room-preview-authorization.test.jstest/server.test.js
💤 Files with no reviewable changes (1)
- src/views/PlexMedia.vue
a026959 to
140e832
Compare
1 participant
Summary
Verification
npm cinpm run lintnpm test— 162 Node tests and 242 Vitest tests passednpm audit --audit-level=high— 0 vulnerabilitiesReview notes
This is intentionally based on
dev, after the Babel 8 and ESLint 9 migrations. The previous HTTProomfield is ignored for room binding; only the server callback from the current socket host can select invite metadata.CodeRabbit is installed on the repository. Its recent checks have been rate-limited, so a green status without review text should not be interpreted as a substantive review; the PR will remain draft until required CI and an independent review complete.
Summary by CodeRabbit