feat(playback): secure remote casting controls (DLNA + Cast)#1069
feat(playback): secure remote casting controls (DLNA + Cast)#1069SalemOurabi wants to merge 9 commits into
Conversation
Greptile SummaryThis PR adds secure remote casting (DLNA, Google Cast, AirPlay, W3C Remote Playback) to the inline web player, with companion SSRF hardening, a new Electron dev-server coordination script, and
Confidence Score: 4/5Safe to merge with low risk; all findings are non-blocking quality and security-posture items. The DLNA service's SSRF protections (request-pinning, private-only SSDP validation, receiver URL allow/block list, payload validation) are thorough and well-tested. The Google Cast integration correctly restricts to secure PWA contexts. The main concerns are: the CSP relaxation that permits gstatic.com scripts in the Electron renderer where Cast never runs; the absolute request timeout that is redundant at the same duration as the socket inactivity timeout; and the toCastPlayback plain-method binding that produces a new object reference on every change-detection cycle under OnPush. None of these affect correctness or security critically. apps/web/src/index.html (CSP applies to Electron renderer), apps/electron-backend/src/app/services/dlna-protocol.ts (redundant absolute timeout), and libs/workspace/shell/feature/src/lib/workspace-shell/workspace-shell.component.ts (plain method in template binding under OnPush).
|
| Filename | Overview |
|---|---|
| apps/electron-backend/src/app/services/dlna-renderer.service.ts | Core DLNA renderer discovery and playback control. Security measures (request-pinning, receiver-URL allow/block lists, IPC payload validation) are well-applied; absolute request timeout is redundant at current configuration. |
| apps/electron-backend/src/app/services/dlna-protocol.ts | SSDP parsing, SSRF guards, and pinned HTTP helper. The enforceAbsoluteRequestTimeout is called with the same 4000ms as req.setTimeout, making it redundant against trickle-data attacks. |
| libs/ui/playback/src/lib/casting/cast.service.ts | Google Cast SDK lazy-loading, AirPlay, Remote Playback, and DLNA orchestration. SDK load promise is properly deduplicated and resets on failure; canUseGoogleCast correctly restricts to secure PWA contexts. |
| apps/web/src/index.html | CSP extended to allow scripts from https://www.gstatic.com for the Google Cast SDK. Relaxation is necessary for PWA Cast support but also applies to the Electron renderer where Cast is never used. |
| apps/electron-backend/src/app/events/casting.events.ts | Registers two IPC handlers for DLNA discovery and playback; module-level DlnaRendererService singleton is appropriate and each handler delegates validation to the service. |
| libs/ui/playback/src/lib/casting/cast-media.utils.ts | findCastMediaElement traverses the DOM using hardcoded class selectors (.web-player-view, .radio-hero) which creates implicit coupling to component class names. |
| libs/workspace/shell/feature/src/lib/workspace-shell/workspace-shell.component.ts | toCastPlayback() is a plain method called in the template, producing a new object reference on every change-detection cycle; with OnPush the cast-control child re-evaluates its signal-computed properties unnecessarily each cycle. |
| apps/electron-backend/src/app/events/mpv-session.service.ts | Refactors header resolution to use buildMpvReusePropertyCommands; now actively clears user-agent/referrer/header-fields when switching streams, which is intentional and tested. |
| apps/electron-backend/src/app/events/external-player-session-registry.ts | Fixes a bug where return inside a finally block silently swallowed close() exceptions; now returns getSession(id) after the try-finally, propagating errors correctly. |
| tools/electron/serve-electron-dev.runtime.mjs | Dev-server coordination: port-availability check, IPTVnator-specific readiness probe, and bidirectional child-process lifecycle management; well-tested with node:test. |
| apps/electron-backend/src/app/services/dlna-xml.ts | SAX-based XML parsing for renderer device descriptions; XML escaping in buildUpnpActionBody is complete and correct. |
Sequence Diagram
sequenceDiagram
participant U as User (UI)
participant CC as CastControlComponent
participant CS as CastService
participant EP as ElectronBridge (IPC)
participant CE as CastingEvents (main)
participant DR as DlnaRendererService
participant DV as DLNA Device
U->>CC: opens cast menu
CC->>CS: discoverDlnaDevices()
CS->>EP: window.electron.discoverDlnaRenderers()
EP->>CE: IPC CAST:DLNA_DISCOVER
CE->>DR: discover(2200ms)
DR-->>DV: SSDP M-SEARCH (UDP multicast 239.255.255.250)
DV-->>DR: SSDP 200 OK (location, USN)
DR->>DR: isTrustedSsdpLocation (private IP only)
DR->>DV: requestPinnedText(location, responderIP)
DR->>DR: parseRendererDescription (SAX)
DR->>DR: cache CachedRenderer (5 min TTL)
DR-->>CE: DlnaRendererDevice[]
CE-->>EP: IPC result
EP-->>CS: DlnaRendererDevice[]
CS-->>CC: device list
U->>CC: selects DLNA device
CC->>CS: startDlnaPlayback(deviceId, playback)
CS->>EP: window.electron.startDlnaPlayback(deviceId, playback)
EP->>CE: IPC CAST:DLNA_START
CE->>DR: startPlayback(deviceId, playback)
DR->>DR: validate payload + isReceiverFetchableUrl + hasPlaybackHeaders
DR->>DV: SetAVTransportURI (SOAP POST, pinned to device IP)
DR->>DV: Play (SOAP POST, pinned to device IP)
DR-->>CE: success: true
CE-->>EP: IPC result
EP-->>CC: success
Prompt To Fix All With AI
Fix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
apps/web/src/index.html:9-12
**CSP relaxation applies to both PWA and Electron renderer**
`https://www.gstatic.com` has been added to `script-src`, which is required to load the Google Cast SDK in the PWA. However, this same `index.html` is served in the Electron renderer, where `canUseGoogleCast` is always `false` (guarded by `isPwa`) and the SDK is never fetched. A script-origin that is intentionally unused in one build still widens the CSP surface for that build. Consider applying the gstatic allowance only in the PWA configuration — for example via an Nx fileReplacement or a build-time `<meta>` injection — so the Electron renderer retains the tighter `script-src 'self'`.
### Issue 2 of 4
apps/electron-backend/src/app/services/dlna-protocol.ts:126-135
**`enforceAbsoluteRequestTimeout` provides no additional protection at the same deadline as `req.setTimeout`**
Both `req.setTimeout(DLNA_REQUEST_TIMEOUT_MS, …)` and `enforceAbsoluteRequestTimeout(req)` (which defaults to `DLNA_REQUEST_TIMEOUT_MS = 4000 ms`) fire at the same wall-clock moment. The absolute timeout exists to guard against trickle-data attacks where a server sends a byte every few milliseconds to keep the socket inactivity timer from expiring. With both set to 4000 ms they fire simultaneously and the absolute timeout adds no protection. The absolute deadline should be set to a longer value — e.g., `enforceAbsoluteRequestTimeout(req, 10_000)` — while the socket inactivity timeout stays short, so a slow sender is still terminated within a bounded wall-clock window.
### Issue 3 of 4
libs/ui/playback/src/lib/casting/cast-media.utils.ts:12-20
**`findCastMediaElement` is implicitly coupled to component CSS class names**
The DOM traversal relies on the hardcoded selectors `.web-player-view` and `.radio-hero`. If either class name is ever renamed or the layout is restructured, Cast silently stops finding the `<video>`/`<audio>` element — no type error, no lint warning. Consider using a `data-cast-scope` attribute set explicitly on the host elements, or alternatively accept the container element as a typed parameter from the component (which already has a reference via `ElementRef`), so the coupling is explicit and survives CSS refactors.
### Issue 4 of 4
libs/workspace/shell/feature/src/lib/workspace-shell/workspace-shell.component.ts:53-62
**`toCastPlayback` creates a new object reference on every change-detection cycle**
`CastControlComponent` uses `ChangeDetectionStrategy.OnPush`. Since `toCastPlayback(session)` is a plain method called in the template binding, Angular evaluates it on every CD run and passes a fresh object reference each time. OnPush compares inputs by reference, so the cast-control child always appears to have a changed `playback` input and will re-evaluate all its signal-computed properties on every cycle. Converting this to a `computed()` signal in the component (keyed off `facade.activeExternalSession()`) would produce stable references and avoid redundant re-evaluation.
Reviews (1): Last reviewed commit: "fix(playback): hide inactive cast contro..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| <meta | ||
| http-equiv="Content-Security-Policy" | ||
| content="default-src 'self'; base-uri 'self'; object-src 'none'; frame-src 'none'; form-action 'self'; script-src 'self'; style-src 'self' 'unsafe-inline'; img-src 'self' http: https: file: data: blob:; font-src 'self' data:; media-src 'self' http: https: file: data: blob:; connect-src 'self' http: https: ws: wss: blob: data:; worker-src 'self' blob:" | ||
| content="default-src 'self'; base-uri 'self'; object-src 'none'; frame-src 'none'; form-action 'self'; script-src 'self' https://www.gstatic.com; style-src 'self' 'unsafe-inline'; img-src 'self' http: https: file: data: blob:; font-src 'self' data:; media-src 'self' http: https: file: data: blob:; connect-src 'self' http: https: ws: wss: blob: data:; worker-src 'self' blob:" | ||
| /> |
There was a problem hiding this comment.
https://www.gstatic.com has been added to script-src, which is required to load the Google Cast SDK in the PWA. However, this same index.html is served in the Electron renderer, where canUseGoogleCast is always false (guarded by isPwa) and the SDK is never fetched. A script-origin that is intentionally unused in one build still widens the CSP surface for that build. Consider applying the gstatic allowance only in the PWA configuration — for example via an Nx fileReplacement or a build-time <meta> injection — so the Electron renderer retains the tighter script-src 'self'.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/src/index.html
Line: 9-12
Comment:
**CSP relaxation applies to both PWA and Electron renderer**
`https://www.gstatic.com` has been added to `script-src`, which is required to load the Google Cast SDK in the PWA. However, this same `index.html` is served in the Electron renderer, where `canUseGoogleCast` is always `false` (guarded by `isPwa`) and the SDK is never fetched. A script-origin that is intentionally unused in one build still widens the CSP surface for that build. Consider applying the gstatic allowance only in the PWA configuration — for example via an Nx fileReplacement or a build-time `<meta>` injection — so the Electron renderer retains the tighter `script-src 'self'`.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| let size = 0; | ||
| response.on('data', (chunk: Buffer) => { | ||
| size += chunk.length; | ||
| if (size > MAX_DLNA_RESPONSE_BYTES) { | ||
| req.destroy( | ||
| new Error('DLNA response exceeded the size limit.') | ||
| ); | ||
| return; | ||
| } | ||
| chunks.push(chunk); |
There was a problem hiding this comment.
enforceAbsoluteRequestTimeout provides no additional protection at the same deadline as req.setTimeout
Both req.setTimeout(DLNA_REQUEST_TIMEOUT_MS, …) and enforceAbsoluteRequestTimeout(req) (which defaults to DLNA_REQUEST_TIMEOUT_MS = 4000 ms) fire at the same wall-clock moment. The absolute timeout exists to guard against trickle-data attacks where a server sends a byte every few milliseconds to keep the socket inactivity timer from expiring. With both set to 4000 ms they fire simultaneously and the absolute timeout adds no protection. The absolute deadline should be set to a longer value — e.g., enforceAbsoluteRequestTimeout(req, 10_000) — while the socket inactivity timeout stays short, so a slow sender is still terminated within a bounded wall-clock window.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/electron-backend/src/app/services/dlna-protocol.ts
Line: 126-135
Comment:
**`enforceAbsoluteRequestTimeout` provides no additional protection at the same deadline as `req.setTimeout`**
Both `req.setTimeout(DLNA_REQUEST_TIMEOUT_MS, …)` and `enforceAbsoluteRequestTimeout(req)` (which defaults to `DLNA_REQUEST_TIMEOUT_MS = 4000 ms`) fire at the same wall-clock moment. The absolute timeout exists to guard against trickle-data attacks where a server sends a byte every few milliseconds to keep the socket inactivity timer from expiring. With both set to 4000 ms they fire simultaneously and the absolute timeout adds no protection. The absolute deadline should be set to a longer value — e.g., `enforceAbsoluteRequestTimeout(req, 10_000)` — while the socket inactivity timeout stays short, so a slow sender is still terminated within a bounded wall-clock window.
How can I resolve this? If you propose a fix, please make it concise.| controlElement: Element | ||
| ): RemotePlaybackMediaElement | null { | ||
| const host = | ||
| controlElement.closest('.web-player-view, .radio-hero') ?? | ||
| controlElement.parentElement; | ||
|
|
||
| return ( | ||
| host?.querySelector<RemotePlaybackMediaElement>('video, audio') ?? null | ||
| ); |
There was a problem hiding this comment.
findCastMediaElement is implicitly coupled to component CSS class names
The DOM traversal relies on the hardcoded selectors .web-player-view and .radio-hero. If either class name is ever renamed or the layout is restructured, Cast silently stops finding the <video>/<audio> element — no type error, no lint warning. Consider using a data-cast-scope attribute set explicitly on the host elements, or alternatively accept the container element as a typed parameter from the component (which already has a reference via ElementRef), so the coupling is explicit and survives CSS refactors.
Prompt To Fix With AI
This is a comment left during a code review.
Path: libs/ui/playback/src/lib/casting/cast-media.utils.ts
Line: 12-20
Comment:
**`findCastMediaElement` is implicitly coupled to component CSS class names**
The DOM traversal relies on the hardcoded selectors `.web-player-view` and `.radio-hero`. If either class name is ever renamed or the layout is restructured, Cast silently stops finding the `<video>`/`<audio>` element — no type error, no lint warning. Consider using a `data-cast-scope` attribute set explicitly on the host elements, or alternatively accept the container element as a typed parameter from the component (which already has a reference via `ElementRef`), so the coupling is explicit and survives CSS refactors.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| readonly facade = inject(WorkspaceShellFacade); | ||
| readonly keyboardShortcuts = inject(WorkspaceKeyboardShortcutsService); | ||
|
|
||
| toCastPlayback(session: ExternalPlayerSession): ResolvedPortalPlayback { | ||
| return { | ||
| streamUrl: session.streamUrl, | ||
| title: session.title, | ||
| thumbnail: session.thumbnail, | ||
| isLive: !session.contentInfo, | ||
| requiresRequestHeaders: session.requiresRequestHeaders, |
There was a problem hiding this comment.
toCastPlayback creates a new object reference on every change-detection cycle
CastControlComponent uses ChangeDetectionStrategy.OnPush. Since toCastPlayback(session) is a plain method called in the template binding, Angular evaluates it on every CD run and passes a fresh object reference each time. OnPush compares inputs by reference, so the cast-control child always appears to have a changed playback input and will re-evaluate all its signal-computed properties on every cycle. Converting this to a computed() signal in the component (keyed off facade.activeExternalSession()) would produce stable references and avoid redundant re-evaluation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: libs/workspace/shell/feature/src/lib/workspace-shell/workspace-shell.component.ts
Line: 53-62
Comment:
**`toCastPlayback` creates a new object reference on every change-detection cycle**
`CastControlComponent` uses `ChangeDetectionStrategy.OnPush`. Since `toCastPlayback(session)` is a plain method called in the template binding, Angular evaluates it on every CD run and passes a fresh object reference each time. OnPush compares inputs by reference, so the cast-control child always appears to have a changed `playback` input and will re-evaluate all its signal-computed properties on every cycle. Converting this to a `computed()` signal in the component (keyed off `facade.activeExternalSession()`) would produce stable references and avoid redundant re-evaluation.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
The casting change to video-player.component.html binds [playback] on the inline player child; the spec's mock needs the matching input() declared or the template binding fails. (Carried over from the 4gray#1056 merge resolution that the topic cherry-pick missed.)
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1069 +/- ##
===========================================
- Coverage 71.05% 54.73% -16.32%
===========================================
Files 40 549 +509
Lines 691 30424 +29733
Branches 87 6502 +6415
===========================================
+ Hits 491 16654 +16163
- Misses 176 11265 +11089
- Partials 24 2505 +2481
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Closing — consolidating this split back into #1056 at the author's request. Superseded by the combined PR. |
2 participants
Summary
Secure remote casting controls for the inline web player, split out of #1056 (casting topic only).
libs/ui/playback/src/lib/casting/):cast-controlcomponent,cast.service,cast-media.utils, Google Cast types, and acast-control-visibilityhelper that auto-hides the overlay when idle and removes it from the focus order / a11y tree while inactive.apps/electron-backend/src/app/services/dlna-*): protocol + XML helpers and the renderer service, exposed viacasting.eventsIPC.runtime-capabilities.service) so the control only renders where casting is supported.Tests
New unit specs for cast-control, cast.service, cast-media.utils, cast-control-visibility, casting.events, dlna-renderer.service, external-player-session-registry, mpv-session.service, and web-player-view overlay/accessibility behavior.
Verified locally on top of
upstream/master:nx build electron-backendcompiles; affectedlintclean (no new errors — only pre-existing upstream warnings); affectedtestgreen.Context
Part of splitting the oversized #1056 into reviewable, topic-scoped PRs. Companion PRs: workspace navigation rail and xtream IPC hardening.