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

Test payment request is showing boolean #14297

Merged
merged 5 commits into from
Jan 18, 2019
Merged

Test payment request is showing boolean #14297

merged 5 commits into from
Jan 18, 2019

Conversation

marcoscaceres
Copy link
Contributor

@marcoscaceres marcoscaceres commented Nov 29, 2018

Copy link
Member

@domenic domenic 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 overall (once you fix things like missing new, by running the tests to confirm that they work) but a popup/cross-global test would exercise the truly tricky things.

payment-request/payment-is-showing.https.html Outdated Show resolved Hide resolved
payment-request/payment-is-showing.https.html Outdated Show resolved Hide resolved
@marcoscaceres
Copy link
Contributor Author

@domenic, I refactored these quite a bit and added popup tests based on your feedback. Also added tests that mix frames, windows, and popups - sorry about the length of the file. I'm trying to be as thorough as possible, while also keeping each test as legible as possible as these tests could be tricky/annoying to debug.

Firefox passes all but the last two: we don't reject the showPromise if the browsing context that created the request is navigated - oops!

@rsolomakhin, I tried running these in Chrome using:

./wpt run chrome payment-request/payment-is-showing.https.html

But it keeps (again) complaining that "basic-card" is not supported by Chrome?

"NotSupportedError: The payment method "basic-card" is not supported".

I'm not sure what I'm doing wrong 😢 Would really appreciate your help. If you can run the locally, that would be greatly appreciated.

@rsolomakhin
Copy link
Contributor

"NotSupportedError: The payment method "basic-card" is not supported".

That happens if your HTTPS icon is not green. Are you using a self-signed HTTPS cert? That would do it. Use localhost, local file, or a valid HTTPS cert instead. For example, press Ctrl-O, navigate to payment-is-showing.https.html and open it. Is it doing anything special that requires the ./wpt tool to make it work?

@marcoscaceres
Copy link
Contributor Author

@rsolomakhin, thanks for the clarification! The problem is clearly around the certs - because you are perfectly describing my issue :) However, the Trusting Root CA section of the docs state:

To avoid this problem when running tests in Chrome or Firefox use wpt run, which disables certificate checks and therefore doesn't require the root CA to be trusted.

I guess the above doesn't hold for Payment Request API in Chrome (the API is there, but "basic-card" is not available)? @foolip just wanted to bring this problem to your attention too (probably not something you can help me with... I guess just FYI).

@foolip
Copy link
Member

foolip commented Jan 16, 2019

@marcoscaceres thanks for pointing this out. This is the first case I've been made aware of where there's a behavioral difference depending on the "level" of the secure connection. I think we want the HTTPS setup used to be trusted enough to get access to all APIs that a real site could. I'm not sure I understand the criteria used exactly, could you please file an issue? Perhaps it's unfixable without doing things that security folks would object to, but who knows.

@rsolomakhin
Copy link
Contributor

@foolip : Oh my, I may have violated the age-old maxim, "Don't reinvent your own security." The behavior you're seeing with basic-card being unsupported with certificate errors are due to my design decision early on in Payment Request development process. Although the spec says the API should be available in secure context, I nerfed the capabilities if the certificate has any kind of errors. This prevents the user from bypassing a security interstitial and then sending their payment info to a sketchy website. The result is the behavior that @marcoscaceres has observed. Although we typically don't do this, would the security team allow the special case tightening in the context of payments?

@foolip
Copy link
Member

foolip commented Jan 16, 2019

@rsolomakhin, I suspect that this isn't the only behavior that depends on something about the cert, but I'm no expert and don't know of any other cases. I'd ask @mikewest, and now I did :)

If you do think the wpt setup should change to make this testable, please file an issue. I know we do things slightly different for content_shell, passing the digest of a cert to trust, so it's possible that the same could be done here in upstream wpt. @Hexcles please correct me if I'm wrong.

@rsolomakhin
Copy link
Contributor

@foolip : No need to change wpt. I have a bug filed against me to fix PaymentRequest ignoring the command-line flag to relax certificate handling.

@foolip
Copy link
Member

foolip commented Jan 16, 2019

Excellent, thanks @rsolomakhin!

@marcoscaceres
Copy link
Contributor Author

@rsolomakhin, thanks for looking deeper into the security issue. In the interest of moving this forward, would you mind giving this PR one more look (and re-approval🤞)? It's changed quite a bit since you last approved it, so would feel more comfortable merging only once it's had another set of eyes on it.

Copy link
Contributor

@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.

You're a machine! So many test cases....

@marcoscaceres marcoscaceres merged commit c2eb40e into master Jan 18, 2019
@marcoscaceres marcoscaceres deleted the show_payment branch January 18, 2019 02:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants