Skip to content
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

Resolve with null instead of reject with 'SecurityError' if PaymentRequestEvent.openWindow is opened in a different origin #202

Merged
merged 5 commits into from Aug 15, 2017

Conversation

gogerald
Copy link
Contributor

In the openWindow algorithm 14.1, the window is opened at that point, looks make no sense to reject with "SecurityError".

This is also how Clients.openWindow works for service work.

@gogerald
Copy link
Contributor Author

@rsolomakhin @ianbjacobs @romandev what you think?

@romandev
Copy link
Member

@gogerald, I left my opinion in #201.
This patch looks good to me but you should fix IDL as well I think. (Should be nullable type)

@romandev
Copy link
Member

Also, we can remove step 4.

@gogerald
Copy link
Contributor Author

Thanks. Why remove step 4 'If the url parsing throws an exception, return a Promise rejected with that exception.'? It looks make sense to keep it.

@romandev
Copy link
Member

Ah, sorry for confusing. I meant step 6:
"If url's origin is not the same as the service worker's origin associated with the payment handler, return a Promise rejected with a SecurityError."

IMHO, for consistency, we should resolve null instead of rejecting with SecurityError.

@gogerald
Copy link
Contributor Author

It makes sense to me, I removed that check,

Copy link
Collaborator

@rsolomakhin rsolomakhin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Please add a link to the relevant Service Worker algorithm in the commit description:

https://w3c.github.io/ServiceWorker/#clients-openwindow

index.html Outdated
<li>If <var>url</var>'s origin is not the same as the <a>service
worker</a>'s origin associated with the payment handler, return a <a>
Promise</a> rejected with a <a>SecurityError</a>.
</li>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep this check, as well, but we can return null for consistency. This check is to make sure that service worker fromhttps://bobpay.xyz does not try to show a page from https://alicepay.xyz. This can be checked synchronously, so there's no need to wait until the page opens.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://bobpay.xyz can do window.location.href = 'https://alicepay.xyz' immediately, so with this check we still can not prevent it happen.

My understanding is that we no need to do any origin related check (if it is user triggered) for openWindow.

The returned "The WindowClient interface of the ServiceWorker API represents the scope of a service worker client that is a document in a browser context, controlled by an active worker."

If the window is opened, redirected or navigated out of the origin, then the service worker has no control of it. We can not find that window through Clients.matchAll.

Make sense?

@rsolomakhin
Copy link
Collaborator

Great, thank you!

@gogerald gogerald changed the title Return null after the window is opened instead of reject with 'SecurityError' Resolve with null instead of reject with 'SecurityError' if PaymentRequestEvent.openWindow is opened in a different origin Aug 14, 2017
Copy link
Member

@romandev romandev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm2

@romandev romandev merged commit fd25f58 into w3c:gh-pages Aug 15, 2017
MXEBot pushed a commit to mirror/chromium that referenced this pull request Aug 16, 2017
… as the service worker

w3c/payment-handler#202

Bug: 755474
Change-Id: Iee7df4e8f6cf54404be4e74ec2ab5e2401723ac8
Reviewed-on: https://chromium-review.googlesource.com/615362
Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Commit-Queue: Ganggui Tang <gogerald@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494411}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants