Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2432723003:
[Presentation API] PresentationRequest should throw SecurityError for mixed contents (Closed)
Description[Presentation API] PresentationRequest should throw SecurityError for mixed contents
Make PresentationRequest consistent with latest spec. PresentationRequest::start(), reconnect() and getavailability() should throw SecurityError for mixed contents.
https://www.w3.org/TR/presentation-api/#dom-presentationrequest-start
BUG=657519
Committed: https://crrev.com/cd89f3ab381a7d9571e01a46a8a89d9318c31a46
Cr-Commit-Position: refs/heads/master@{#427433}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Resolve code review comments from Mark #
Total comments: 8
Patch Set 3 : resolve code review comments from mlamouri #Messages
Total messages: 19 (9 generated)
zhaobin@chromium.org changed reviewers: + imcheng@chromium.org, mfoltz@chromium.org, mlamouri@chromium.org
LGTM. Are there web platform tests that you can use to verify this change? https://codereview.chromium.org/2432723003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp (right): https://codereview.chromium.org/2432723003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:202: "url is an a priori unauthenticated URL. Url: " + I would say: "Presentation of an insecure document <m_url> is prohibited from a secure context."
Patchset #3 (id:40001) has been deleted
Hi Mark, Yes, there are several web platform tests for mixed contents. e.g. http://w3c-test.org/presentation-api/controlling-ua/getAvailability_mixedcont... Verified that tests passed for 1-UA in https context. https://codereview.chromium.org/2432723003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp (right): https://codereview.chromium.org/2432723003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:202: "url is an a priori unauthenticated URL. Url: " + On 2016/10/20 21:56:14, mark a. foltz wrote: > I would say: "Presentation of an insecure document <m_url> is prohibited from a > secure context." Done.
zhaobin@, can you write a test for this change? mfoltz@, it sounds a bit odd that we do the mixed content check on the method calls. Shouldn't we do this when constructing the PresentationRequest instead? It would avoid handing out an object that couldn't be used. https://codereview.chromium.org/2432723003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp (right): https://codereview.chromium.org/2432723003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:111: return rejectWithMixedContentException(scriptState); style: { } https://codereview.chromium.org/2432723003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:137: return rejectWithMixedContentException(scriptState); ditto for style https://codereview.chromium.org/2432723003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:162: return rejectWithMixedContentException(scriptState); ditto https://codereview.chromium.org/2432723003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/presentation/PresentationRequest.h (right): https://codereview.chromium.org/2432723003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/presentation/PresentationRequest.h:58: ScriptPromise rejectWithSandBoxException(ScriptState*); I don't think these methods need to be part of the PresentationRequest class. Can you add them in the anonymous namespace of the cpp file?
Mounir, we have some web platform tests for these code. Would that be sufficient? https://codereview.chromium.org/2432723003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp (right): https://codereview.chromium.org/2432723003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:111: return rejectWithMixedContentException(scriptState); On 2016/10/21 09:45:43, mlamouri wrote: > style: { } Done. https://codereview.chromium.org/2432723003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:137: return rejectWithMixedContentException(scriptState); On 2016/10/21 09:45:43, mlamouri wrote: > ditto for style Done. https://codereview.chromium.org/2432723003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:162: return rejectWithMixedContentException(scriptState); On 2016/10/21 09:45:43, mlamouri wrote: > ditto Done. https://codereview.chromium.org/2432723003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/presentation/PresentationRequest.h (right): https://codereview.chromium.org/2432723003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/presentation/PresentationRequest.h:58: ScriptPromise rejectWithSandBoxException(ScriptState*); On 2016/10/21 09:45:43, mlamouri wrote: > I don't think these methods need to be part of the PresentationRequest class. > Can you add them in the anonymous namespace of the cpp file? Done.
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/21 at 09:45:44, mlamouri wrote: > zhaobin@, can you write a test for this change? > > mfoltz@, it sounds a bit odd that we do the mixed content check on the method calls. Shouldn't we do this when constructing the PresentationRequest instead? It would avoid handing out an object that couldn't be used. It would simplify the implementation. It brings up a couple of questions though. Filed https://github.com/w3c/presentation-api/issues/362, feel free to post thoughts there. m.
lgtm
The CQ bit was checked by zhaobin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org Link to the patchset: https://codereview.chromium.org/2432723003/#ps60001 (title: "resolve code review comments from mlamouri")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Presentation API] PresentationRequest should throw SecurityError for mixed contents Make PresentationRequest consistent with latest spec. PresentationRequest::start(), reconnect() and getavailability() should throw SecurityError for mixed contents. https://www.w3.org/TR/presentation-api/#dom-presentationrequest-start BUG=657519 ========== to ========== [Presentation API] PresentationRequest should throw SecurityError for mixed contents Make PresentationRequest consistent with latest spec. PresentationRequest::start(), reconnect() and getavailability() should throw SecurityError for mixed contents. https://www.w3.org/TR/presentation-api/#dom-presentationrequest-start BUG=657519 Committed: https://crrev.com/cd89f3ab381a7d9571e01a46a8a89d9318c31a46 Cr-Commit-Position: refs/heads/master@{#427433} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/cd89f3ab381a7d9571e01a46a8a89d9318c31a46 Cr-Commit-Position: refs/heads/master@{#427433} |
