[Spec] Move some input validation to construction time #195

stephenmcgruer · 2022-06-30T15:31:51Z

Fixes #194

Fixes #194 Depends on w3c/payment-request#977

stephenmcgruer · 2022-07-07T12:45:57Z

@rsolomakhin - PTAL, and in particular check that this matches what we check synchronously at construction time versus asynchronously for later canMakePayment()/show() calls.

rsolomakhin

HI @stephenmcgruer, the code and the spec seem to disagree a bit. Should we get the spec into a more sensible place (re-order some checks, add some more checks here and there) and then open a bug in Chrome side to match the spec better (mostly adding more checks)?

rsolomakhin · 2022-07-07T13:27:25Z

spec.bs

 </wpt>

-
 1. If |data|["{{SecurePaymentConfirmationRequest/credentialIds}}"] is empty,


Did you mean to sync up with this Chrome code? That code should be executing in the PaymentRequest() constructor, as far as I can see. Its sequence of checks is:

Credential ID list: non-empty.

Timeout: under 1 hour.

Instrument display name: non-empty.

Instrument icon: valid URL.

Either payee origin or name: non-empty.

RP ID: non-empty.

This spec here appears to have:

Credential ID list: non-empty. ✅

Timeout: no checks. (Spec may want to add this.) ⬅️

Instrument icon: no checks. (Spec should check this.) 🔥

Challenge: non-empty. (Chrome should start doing this.) 🔥

RP ID: valid domain. (Chrome should start doing this.) 🔥

Either payee name or origin: non-empty. ✅

Payee origin: valid https URL. (Chrome should start doing this.) 🔥

Instrument display name: non-empty. (Spec may want to move this up to position 3.) ⬅️

There are some differences here, including ordering of checks. Am I looking at the wrong chunk of Chrome code by accident?

Thank you for this excellent and detailed overview of the differences!

There are some differences here, including ordering of checks. Am I looking at the wrong chunk of Chrome code by accident?

Nope, you are just doing a great job of reviewing <3.

I've made the following changes in the latest commit:

Check each credentialId to be non-empty (I think Chrome should do this here too?)

Move displayName check up to after checking the challenge (so the order of the first few are credentialId checks, challenge check, instrument checks)

Added checking that icon is non-empty and valid.

Credential ID list: non-empty.

I think Chrome should also check that the individual IDs here are non-empty; WDYT?

Timeout: under 1 hour.

I think we should actually drop this check from Chrome currently, as the WebAuthn layer should handle it (https://w3c.github.io/webauthn/#:~:text=.publicKey.-,If%20the%20timeout%20member%20of%20options%20is%20present,-%2C%20check%20if%20its), and currently the timeout is only used for that layer. In the future if we make the timeout apply to the SPC dialog itself, we should do better.

WDYT?

Payee origin: valid https URL. (Chrome should start doing this.) 🔥

I think we do it here (part of the Chrome code you linked), so Chrome should be good here?

RP ID: valid domain. (Chrome should start doing this.) 🔥

Btw Chrome does partially do this here, but only checks for non-empty. It's also in the wrong check-order versus the spec, should be before payeeName/payeeOrigin.

rsolomakhin · 2022-07-07T13:41:16Z

spec.bs

+  authentication-rejected.https.html
+</wpt>
+
+1. If |data|["{{SecurePaymentConfirmationRequest/payeeOrigin}}"] is present:


Comparing to the Chrome code that determines whether canMakePayment() will return true:

Credential IDs list: non-empty. (Fails only on compromised renderer.) ✅

Each credential ID: non-empty. (The PaymentRequest() constructor should be checking this, too.) 🔥

Timeout: under 1 hour. (Fails only on compromised renderer.) ✅

Challenge: non-empty. (The PaymentRequest() constructor should be checking this, too.) 🔥

Instrument display name: non-empty. (Fails only on compromised renderer.) ✅

Instrument icon: valid URL. (Fails only on compromised renderer.) ✅

Download the icon.

Check for instrument IDs on device.

This spec here has:

Payee origin: valid https URL. (Chrome should check that either payeer origin is a valid https URL or payee name is non empty in here.) 🔥

Download the icon. ✅

Check for instrument IDs on device. ✅

Ack; I think there are no spec-changes needed from this comment? (As (2) and (4) are covered in the other comment, and since the spec shouldn't worry about compromised renderers ;)).

I will open a Chrome bug later to address all the mismatches we have identified too.

rsolomakhin

Thank you for making this spec more awesome 😆

This spec change LGTM and feel free to assign the Chrome bug report to me, if you need a helping hand. 👋

@stephenmcgruer

SHA: d002a0f Reason: push, by @stephenmcgruer Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

stephenmcgruer · 2022-07-07T18:29:50Z

Opened https://crbug.com/1342686 for Chrome changes, and will also send a WPT change to align tests with spec.

See w3c/secure-payment-confirmation#195

…alidation, a=testonly Automatic update from web-platform-tests [SPC] Update tests for input parameter validation (#34739) See w3c/secure-payment-confirmation#195 -- wpt-commits: 4d5244f69cd4b0a5edf2dc68371d597ed5873632 wpt-pr: 34739

w3c/secure-payment-confirmation#195 clarified some of the input parameter validation in the SPC spec, and revealed some places where Chrome needed to change. This CL implements most of those changes on the renderer side. The exception is domain-checking for rpId, which will require more work. Bug: 1342686 Change-Id: I87b4f67f0bbfaf397e8937573330e65ae53751bb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3749480 Reviewed-by: Mike West <mkwst@chromium.org> Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org> Auto-Submit: Stephen McGruer <smcgruer@chromium.org> Commit-Queue: Stephen McGruer <smcgruer@chromium.org> Cr-Commit-Position: refs/heads/main@{#1024194}

…alidation, a=testonly Automatic update from web-platform-tests [SPC] Update tests for input parameter validation (#34739) See w3c/secure-payment-confirmation#195 -- wpt-commits: 4d5244f69cd4b0a5edf2dc68371d597ed5873632 wpt-pr: 34739

stephenmcgruer added 2 commits June 30, 2022 11:31

[Spec] Move some input validation to construction time

79af939

Fixes #194 Depends on w3c/payment-request#977

link to PR spec

d4b557e

stephenmcgruer marked this pull request as ready for review July 7, 2022 12:45

stephenmcgruer requested a review from rsolomakhin July 7, 2022 12:45

rsolomakhin requested changes Jul 7, 2022

View reviewed changes

stephenmcgruer added 2 commits July 7, 2022 12:53

Address some comments

4b8f0c8

Fix one more case

a314305

rsolomakhin approved these changes Jul 7, 2022

View reviewed changes

stephenmcgruer merged commit d002a0f into main Jul 7, 2022

stephenmcgruer deleted the smcgruer/validation branch July 7, 2022 18:22

github-actions bot added a commit that referenced this pull request Jul 7, 2022

[Spec] Move some input validation to construction time (#195)

e9c7b74

SHA: d002a0f Reason: push, by @stephenmcgruer Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

stephenmcgruer added a commit to web-platform-tests/wpt that referenced this pull request Jul 7, 2022

[SPC] Update tests for input parameter validation

073dd99

See w3c/secure-payment-confirmation#195

stephenmcgruer mentioned this pull request Jul 7, 2022

[SPC] Update tests for input parameter validation web-platform-tests/wpt#34739

Merged

stephenmcgruer added a commit to web-platform-tests/wpt that referenced this pull request Jul 7, 2022

[SPC] Update tests for input parameter validation

9538cad

See w3c/secure-payment-confirmation#195

stephenmcgruer added a commit to web-platform-tests/wpt that referenced this pull request Jul 8, 2022

[SPC] Update tests for input parameter validation (#34739)

4d5244f

See w3c/secure-payment-confirmation#195

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Spec] Move some input validation to construction time #195

[Spec] Move some input validation to construction time #195

Uh oh!

stephenmcgruer commented Jun 30, 2022 •

edited by pr-preview bot

Loading

Uh oh!

stephenmcgruer commented Jul 7, 2022

Uh oh!

rsolomakhin left a comment

Uh oh!

rsolomakhin Jul 7, 2022

Uh oh!

stephenmcgruer Jul 7, 2022 •

edited

Loading

Uh oh!

stephenmcgruer Jul 7, 2022

Uh oh!

rsolomakhin Jul 7, 2022

Uh oh!

stephenmcgruer Jul 7, 2022

Uh oh!

rsolomakhin left a comment

Uh oh!

stephenmcgruer commented Jul 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		</wpt>


		1. If \|data\|["{{SecurePaymentConfirmationRequest/credentialIds}}"] is empty,

[Spec] Move some input validation to construction time #195

[Spec] Move some input validation to construction time #195

Uh oh!

Conversation

stephenmcgruer commented Jun 30, 2022 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephenmcgruer commented Jul 7, 2022

Uh oh!

rsolomakhin left a comment

Choose a reason for hiding this comment

Uh oh!

rsolomakhin Jul 7, 2022

Choose a reason for hiding this comment

Uh oh!

stephenmcgruer Jul 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephenmcgruer Jul 7, 2022

Choose a reason for hiding this comment

Uh oh!

rsolomakhin Jul 7, 2022

Choose a reason for hiding this comment

Uh oh!

stephenmcgruer Jul 7, 2022

Choose a reason for hiding this comment

Uh oh!

rsolomakhin left a comment

Choose a reason for hiding this comment

Uh oh!

stephenmcgruer commented Jul 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

stephenmcgruer commented Jun 30, 2022 •

edited by pr-preview bot

Loading

stephenmcgruer Jul 7, 2022 •

edited

Loading