Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1284193003:
Removal of getUserMedia() on insecure origins (Closed)
|
Created:
5 years, 4 months ago by jww Modified:
5 years, 4 months ago CC:
blink-reviews, tommyw+watchlist_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/remotes/origin/master Project:
blink Visibility:
Public. |
DescriptionRemoval of getUserMedia() on insecure origins
This disallows getUserMedia() from being used on insecure origins. Adds
a console warning message that the API call has failed because of this.
BUG=520765
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200986
Patch Set 1 #
Total comments: 9
Patch Set 2 : Fixes from comments #Patch Set 3 : Rebase on ToT #Patch Set 4 : Remove redundant test #
Total comments: 4
Patch Set 5 : Test nits #Patch Set 6 : Dedupped secure scheme registry. #
Messages
Total messages: 29 (10 generated)
jww@chromium.org changed reviewers: + philipj@opera.com
Received lgtm for remove getUserMedia() on insecure origins: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/Dsoq5xPdzyw
mkwst@chromium.org changed reviewers: + mkwst@chromium.org
https://codereview.chromium.org/1284193003/diff/1/LayoutTests/http/tests/secu... File LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html (right): https://codereview.chromium.org/1284193003/diff/1/LayoutTests/http/tests/secu... LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html:105: assert_unreached('getUserMedia should throw an exception, but called the "fail" callback.'); You probably don't need both of these. In fact, I'd suggest rewriting this test in terms of assert_throws to verify that a) it throws, and b) the exception is a SecurityError. https://codereview.chromium.org/1284193003/diff/1/Source/modules/mediastream/... File Source/modules/mediastream/NavigatorMediaStream.cpp (right): https://codereview.chromium.org/1284193003/diff/1/Source/modules/mediastream/... Source/modules/mediastream/NavigatorMediaStream.cpp:70: exceptionState.throwSecurityError(errorMessage); Why throw? Why not go through the error callback like the other rejection types?
https://codereview.chromium.org/1284193003/diff/1/LayoutTests/http/tests/secu... File LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html (right): https://codereview.chromium.org/1284193003/diff/1/LayoutTests/http/tests/secu... LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html:105: assert_unreached('getUserMedia should throw an exception, but called the "fail" callback.'); On 2015/08/18 00:28:38, Mike West (traveling) wrote: > You probably don't need both of these. In fact, I'd suggest rewriting this test > in terms of assert_throws to verify that a) it throws, and b) the exception is a > SecurityError. Agreed, that would also get rid of the wrapping try-catch blocks. The form is assert_throws(new TypeError, codeThatThrows()), but I'm not sure what the type of exception should be. https://codereview.chromium.org/1284193003/diff/1/Source/modules/mediastream/... File Source/modules/mediastream/NavigatorMediaStream.cpp (right): https://codereview.chromium.org/1284193003/diff/1/Source/modules/mediastream/... Source/modules/mediastream/NavigatorMediaStream.cpp:70: exceptionState.throwSecurityError(errorMessage); On 2015/08/18 00:28:38, Mike West (traveling) wrote: > Why throw? Why not go through the error callback like the other rejection types? Looking at http://w3c.github.io/mediacapture-main/getusermedia.html#dom-mediadevices-get... I don't see it described how the rejection should happen. Is there an open spec bug for that? Even if it's an optional step, it's still better to have it in the main spec than in a supplement spec or something.
https://codereview.chromium.org/1284193003/diff/1/LayoutTests/http/tests/secu... File LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html (right): https://codereview.chromium.org/1284193003/diff/1/LayoutTests/http/tests/secu... LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html:105: assert_unreached('getUserMedia should throw an exception, but called the "fail" callback.'); On 2015/08/18 08:35:09, philipj wrote: > On 2015/08/18 00:28:38, Mike West (traveling) wrote: > > You probably don't need both of these. In fact, I'd suggest rewriting this > test > > in terms of assert_throws to verify that a) it throws, and b) the exception is > a > > SecurityError. > > Agreed, that would also get rid of the wrapping try-catch blocks. The form is > assert_throws(new TypeError, codeThatThrows()), but I'm not sure what the type > of exception should be. Assuming we change this failure to an error callback, then this should simply revert to the succeed case being assert_unreached, the error callback being a call to done(), and no catch block at all. https://codereview.chromium.org/1284193003/diff/1/Source/modules/mediastream/... File Source/modules/mediastream/NavigatorMediaStream.cpp (right): https://codereview.chromium.org/1284193003/diff/1/Source/modules/mediastream/... Source/modules/mediastream/NavigatorMediaStream.cpp:70: exceptionState.throwSecurityError(errorMessage); On 2015/08/18 08:35:10, philipj wrote: > On 2015/08/18 00:28:38, Mike West (traveling) wrote: > > Why throw? Why not go through the error callback like the other rejection > types? > > Looking at > http://w3c.github.io/mediacapture-main/getusermedia.html#dom-mediadevices-get... > I don't see it described how the rejection should happen. Is there an open spec > bug for that? Even if it's an optional step, it's still better to have it in the > main spec than in a supplement spec or something. Looking at https://w3c.github.io/mediacapture-main/getusermedia.html#methods-4, it seems to me that this is rather similar to the "Permission Failure" and should be a promise rejection (which, for now, would remain an error callback I believe). This also maps to what slightlyoff@ said to me in conversation yesterday. However, it's not clear to me what type of error the callback should be made with. Should we just make up a new one, and file a bug with the spec suggesting that a new one is needed? Or should we just reuse "PermissionDeniedError" for now?
On 2015/08/18 at 16:27:12, jww wrote: > https://codereview.chromium.org/1284193003/diff/1/LayoutTests/http/tests/secu... > File LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html (right): > > https://codereview.chromium.org/1284193003/diff/1/LayoutTests/http/tests/secu... > LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html:105: assert_unreached('getUserMedia should throw an exception, but called the "fail" callback.'); > On 2015/08/18 08:35:09, philipj wrote: > > On 2015/08/18 00:28:38, Mike West (traveling) wrote: > > > You probably don't need both of these. In fact, I'd suggest rewriting this > > test > > > in terms of assert_throws to verify that a) it throws, and b) the exception is > > a > > > SecurityError. > > > > Agreed, that would also get rid of the wrapping try-catch blocks. The form is > > assert_throws(new TypeError, codeThatThrows()), but I'm not sure what the type > > of exception should be. > > Assuming we change this failure to an error callback, then this should simply revert to the succeed case being assert_unreached, the error callback being a call to done(), and no catch block at all. > > https://codereview.chromium.org/1284193003/diff/1/Source/modules/mediastream/... > File Source/modules/mediastream/NavigatorMediaStream.cpp (right): > > https://codereview.chromium.org/1284193003/diff/1/Source/modules/mediastream/... > Source/modules/mediastream/NavigatorMediaStream.cpp:70: exceptionState.throwSecurityError(errorMessage); > On 2015/08/18 08:35:10, philipj wrote: > > On 2015/08/18 00:28:38, Mike West (traveling) wrote: > > > Why throw? Why not go through the error callback like the other rejection > > types? > > > > Looking at > > http://w3c.github.io/mediacapture-main/getusermedia.html#dom-mediadevices-get... > > I don't see it described how the rejection should happen. Is there an open spec > > bug for that? Even if it's an optional step, it's still better to have it in the > > main spec than in a supplement spec or something. > > Looking at https://w3c.github.io/mediacapture-main/getusermedia.html#methods-4, it seems to me that this is rather similar to the "Permission Failure" and should be a promise rejection (which, for now, would remain an error callback I believe). This also maps to what slightlyoff@ said to me in conversation yesterday. > > However, it's not clear to me what type of error the callback should be made with. Should we just make up a new one, and file a bug with the spec suggesting that a new one is needed? Or should we just reuse "PermissionDeniedError" for now? This should (hopefully) cause a few `MediaStreamPermissionTest`s to fail. I hope you don't mind if I trigger a dry run to check.
The CQ bit was checked by lgarron@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1284193003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1284193003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/1284193003/diff/1/Source/modules/mediastream/... File Source/modules/mediastream/NavigatorMediaStream.cpp (right): https://codereview.chromium.org/1284193003/diff/1/Source/modules/mediastream/... Source/modules/mediastream/NavigatorMediaStream.cpp:70: exceptionState.throwSecurityError(errorMessage); On 2015/08/18 16:27:12, jww wrote: > On 2015/08/18 08:35:10, philipj wrote: > > On 2015/08/18 00:28:38, Mike West (traveling) wrote: > > > Why throw? Why not go through the error callback like the other rejection > > types? > > > > Looking at > > > http://w3c.github.io/mediacapture-main/getusermedia.html#dom-mediadevices-get... > > I don't see it described how the rejection should happen. Is there an open > spec > > bug for that? Even if it's an optional step, it's still better to have it in > the > > main spec than in a supplement spec or something. > > Looking at https://w3c.github.io/mediacapture-main/getusermedia.html#methods-4, > it seems to me that this is rather similar to the "Permission Failure" and > should be a promise rejection (which, for now, would remain an error callback I > believe). This also maps to what slightlyoff@ said to me in conversation > yesterday. > > However, it's not clear to me what type of error the callback should be made > with. Should we just make up a new one, and file a bug with the spec suggesting > that a new one is needed? Or should we just reuse "PermissionDeniedError" for > now? Oh, I didn't see the "Permission Failure" bit in the spec. It's pretty clear, it says to use a DOMException of the SecurityError flavor.
https://codereview.chromium.org/1284193003/diff/1/Source/modules/mediastream/... File Source/modules/mediastream/NavigatorMediaStream.cpp (right): https://codereview.chromium.org/1284193003/diff/1/Source/modules/mediastream/... Source/modules/mediastream/NavigatorMediaStream.cpp:70: exceptionState.throwSecurityError(errorMessage); On 2015/08/19 12:26:43, philipj wrote: > On 2015/08/18 16:27:12, jww wrote: > > On 2015/08/18 08:35:10, philipj wrote: > > > On 2015/08/18 00:28:38, Mike West (traveling) wrote: > > > > Why throw? Why not go through the error callback like the other rejection > > > types? > > > > > > Looking at > > > > > > http://w3c.github.io/mediacapture-main/getusermedia.html#dom-mediadevices-get... > > > I don't see it described how the rejection should happen. Is there an open > > spec > > > bug for that? Even if it's an optional step, it's still better to have it in > > the > > > main spec than in a supplement spec or something. > > > > Looking at > https://w3c.github.io/mediacapture-main/getusermedia.html#methods-4, > > it seems to me that this is rather similar to the "Permission Failure" and > > should be a promise rejection (which, for now, would remain an error callback > I > > believe). This also maps to what slightlyoff@ said to me in conversation > > yesterday. > > > > However, it's not clear to me what type of error the callback should be made > > with. Should we just make up a new one, and file a bug with the spec > suggesting > > that a new one is needed? Or should we just reuse "PermissionDeniedError" for > > now? > > Oh, I didn't see the "Permission Failure" bit in the spec. It's pretty clear, it > says to use a DOMException of the SecurityError flavor. Oh, I see the difficulty now, this is webkitGetUserMedia() which doesn't return a promise. The only guidance would be what the spec used to say before promisification: https://github.com/w3c/mediacapture-main/commit/f6c4e9085568a81d5e9d36ca32eea... And that was: 10. Optionally, e.g., based on a previously-established user preference, for security reasons, or due to platform limitations, jump to the step labeled Permission Failure below. 15. Permission Failure: Let error be a new MediaStreamError object whose name attribute has the value PermissionDeniedError and jump to the step labeled Error Task below. 19. Error Task: Queue a task to invoke errorCallback with error as its argument. So using the error callback seems good, it also slightly increases the chances of the problem being handled correctly (or like a user rejection), I think.
https://codereview.chromium.org/1284193003/diff/1/Source/modules/mediastream/... File Source/modules/mediastream/NavigatorMediaStream.cpp (right): https://codereview.chromium.org/1284193003/diff/1/Source/modules/mediastream/... Source/modules/mediastream/NavigatorMediaStream.cpp:70: exceptionState.throwSecurityError(errorMessage); On 2015/08/19 12:39:24, philipj wrote: > On 2015/08/19 12:26:43, philipj wrote: > > On 2015/08/18 16:27:12, jww wrote: > > > On 2015/08/18 08:35:10, philipj wrote: > > > > On 2015/08/18 00:28:38, Mike West (traveling) wrote: > > > > > Why throw? Why not go through the error callback like the other > rejection > > > > types? > > > > > > > > Looking at > > > > > > > > > > http://w3c.github.io/mediacapture-main/getusermedia.html#dom-mediadevices-get... > > > > I don't see it described how the rejection should happen. Is there an open > > > spec > > > > bug for that? Even if it's an optional step, it's still better to have it > in > > > the > > > > main spec than in a supplement spec or something. > > > > > > Looking at > > https://w3c.github.io/mediacapture-main/getusermedia.html#methods-4, > > > it seems to me that this is rather similar to the "Permission Failure" and > > > should be a promise rejection (which, for now, would remain an error > callback > > I > > > believe). This also maps to what slightlyoff@ said to me in conversation > > > yesterday. > > > > > > However, it's not clear to me what type of error the callback should be made > > > with. Should we just make up a new one, and file a bug with the spec > > suggesting > > > that a new one is needed? Or should we just reuse "PermissionDeniedError" > for > > > now? > > > > Oh, I didn't see the "Permission Failure" bit in the spec. It's pretty clear, > it > > says to use a DOMException of the SecurityError flavor. > > Oh, I see the difficulty now, this is webkitGetUserMedia() which doesn't return > a promise. The only guidance would be what the spec used to say before > promisification: > https://github.com/w3c/mediacapture-main/commit/f6c4e9085568a81d5e9d36ca32eea... > > And that was: > > 10. Optionally, e.g., based on a previously-established user preference, for > security reasons, or due to platform limitations, jump to the step labeled > Permission Failure below. > 15. Permission Failure: Let error be a new MediaStreamError object whose name > attribute has the value PermissionDeniedError and jump to the step labeled Error > Task below. > 19. Error Task: Queue a task to invoke errorCallback with error as its argument. > > So using the error callback seems good, it also slightly increases the chances > of the problem being handled correctly (or like a user rejection), I think. Okay, I've modified to call the error callback with a PermissionDenied error.
LGTM, thanks for taking a second pass.
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1284193003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1284193003/20001
lgarron@chromium.org changed reviewers: - lgarron@chromium.org
On 2015/08/19 at 22:58:36, commit-bot wrote: > Dry run: CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1284193003/20001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1284193003/20001 jww@: You should remove the three MediaStreamPermissionTest.TestNonSecureOrigin* tests before landing this. Ideally, they should be replaced with a single test (that sanity checks for rejection) after this lands. Independently, http/tests/security/powerfulFeatureRestrictions/getUserMedia-on-insecure-origin.html also seems to be failing.
LGTM with test nits https://codereview.chromium.org/1284193003/diff/60001/LayoutTests/http/tests/... File LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html (right): https://codereview.chromium.org/1284193003/diff/60001/LayoutTests/http/tests/... LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html:101: assert_unreached('getUserMedia should throw an exception, but called the "success" callback.'); Actually it should call the error callback :) Also, this could be written as this.unreached_func(description) to save some characters. https://codereview.chromium.org/1284193003/diff/60001/LayoutTests/http/tests/... LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html:104: this.done(); Can be written as this.step_func_done().
https://codereview.chromium.org/1284193003/diff/60001/LayoutTests/http/tests/... File LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html (right): https://codereview.chromium.org/1284193003/diff/60001/LayoutTests/http/tests/... LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html:101: assert_unreached('getUserMedia should throw an exception, but called the "success" callback.'); On 2015/08/20 09:49:18, philipj wrote: > Actually it should call the error callback :) > > Also, this could be written as this.unreached_func(description) to save some > characters. Done. https://codereview.chromium.org/1284193003/diff/60001/LayoutTests/http/tests/... LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html:104: this.done(); On 2015/08/20 09:49:17, philipj wrote: > Can be written as this.step_func_done(). Done.
The CQ bit was checked by jww@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org, philipj@opera.com Link to the patchset: https://codereview.chromium.org/1284193003/#ps100001 (title: "Dedupped secure scheme registry.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1284193003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1284193003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by jww@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1284193003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1284193003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://src.chromium.org/viewvc/blink?view=rev&revision=200986 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
