-
Notifications
You must be signed in to change notification settings - Fork 237
fix: add secure context checks for navigator.mediaDevices #1769
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Prevents crashes in non-HTTPS environments by checking window.isSecureContext before accessing navigator.mediaDevices API.
|
- Add window.isSecureContext checks before accessing navigator.mediaDevices - Prevent crashes in non-HTTPS environments when mediaDevices is undefined - Add checks for devicechange event listeners (add/remove) - Add checks for getUserMedia calls (video/audio) - Gracefully fallback to empty MediaStream in non-secure contexts
src/room/Room.ts
Outdated
| signal: abortController.signal, | ||
| }); | ||
| if (window.isSecureContext && navigator.mediaDevices?.addEventListener) { | ||
| navigator.mediaDevices.addEventListener('devicechange', this.handleDeviceChange, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we use optional chaining for this in other places already, I think this could be used here as well with the same effect as explicitly checking for secureContext?
| navigator.mediaDevices.addEventListener('devicechange', this.handleDeviceChange, { | |
| navigator.mediaDevices?.addEventListener('devicechange', this.handleDeviceChange, { |
you mention
addEventListener('devicechange') fails even if mediaDevices exists
under which circumstances is this the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Thanks for the question — here’s the context in which addEventListener('devicechange') fails even when navigator.mediaDevices exists.
In my case:
The page was running over HTTP (insecure context).
In this environment, navigator.mediaDevices does exist, but it only exposes methods like getUserMedia.

However, mediaDevices does not implement addEventListener, so calling it results in:
TypeError: navigator.mediaDevices.addEventListener is not a function
This matches the spec behavior: devicechange and event handling on mediaDevices are only guaranteed in secure contexts (HTTPS). On HTTP, the object may be present but not fully functional.
So the failure happens specifically when:
- navigator.mediaDevices exists
- but the browser/environment doesn’t provide the event interface (e.g., insecure context)
I tested this in:
Chrome Version 142.0.7444.176 (Official Build) (64-bit)
React 18.2.0 (though the framework isn't relevant but who knows; it's a browser API behavior)
Let me know if you'd like me to test in other environments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the change suggestion, I got a safer approach would be:
navigator.mediaDevices?.addEventListener?.('devicechange', this.handleDeviceChange,
This ensures we only call the handler when both the object and the method are available, which matches Chrome browser behavior in non-secure contexts.
Happy to make a PR if this approach sounds reasonable!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that approach sounds great, thanks!
lukasIO
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the PR and the detailed description!
…ecureContext checks
| ? ( | ||
| await window.navigator.mediaDevices.getUserMedia({ video: true }) | ||
| ? (await window.navigator.mediaDevices?.getUserMedia?.({ video: true }) ?? new MediaStream() | ||
| ).getVideoTracks()[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: The getUserMedia calls with ?? new MediaStream() fallback (lines 2502, 2529)
ensure we don't get undefined when calling .getVideoTracks()[0] in non-secure contexts
where mediaDevices?.getUserMedia?.() returns undefined due to optional chaining.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of falling back to new MediaStream() we could simply check for publishOptions.useRealTracks && window.navigator.mediaDevices?.getUserMedia above
…ecureContext checks
…ecureContext checks
…ecureContext checks
lukasIO
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you, just one more thing and could you run pnpm format locally to ensure the prettier tests aren't failing
| ? ( | ||
| await window.navigator.mediaDevices.getUserMedia({ video: true }) | ||
| ? (await window.navigator.mediaDevices?.getUserMedia?.({ video: true }) ?? new MediaStream() | ||
| ).getVideoTracks()[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of falling back to new MediaStream() we could simply check for publishOptions.useRealTracks && window.navigator.mediaDevices?.getUserMedia above
src/room/Room.ts
Outdated
| ? (window.isSecureContext && navigator.mediaDevices | ||
| ? await navigator.mediaDevices.getUserMedia({ audio: true }) | ||
| : new MediaStream() | ||
| ? (await navigator.mediaDevices?.getUserMedia?.({ audio: true }) ?? new MediaStream() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
Fix: Support LiveKit Client in Non-Secure Context (HTTP Environment)
Problem
The original code directly calls
navigator.mediaDevicesAPIs without checking for secure context. In HTTP environments (non-HTTPS), these APIs are restricted by browser security policies:navigator.mediaDevicesmay beundefinedaddEventListener('devicechange')fails even ifmediaDevicesexistsgetUserMedia()calls throw errorsThis causes the WebRTC video player to fail in HTTP environments, breaking the live video streaming functionality.
Solution
navigator.mediaDevicesAPIsChanges
1. Code Changes in Fixed PR
The fixed PR adds security context checks in
src/room/Room.ts:Before:
After:
Fixed locations:
getUserMedia()calls for video tracksgetUserMedia()calls for audio tracksTesting
navigator.mediaDevicesRelated
This fix ensures compatibility with HTTP environments while maintaining full functionality in HTTPS environments. The changes are backward compatible and do not affect existing HTTPS deployments.
Notes
livekit-clientgetUserMedia()calls fall back to dummy tracks when not available