KEMBAR78
Proposed addition: validate .data on construction by marcoscaceres · Pull Request #976 · w3c/payment-request · GitHub
Skip to content

Proposed addition: validate .data on construction #976

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 6, 2022
Merged

Proposed addition: validate .data on construction #976

merged 1 commit into from
Jul 6, 2022

Conversation

marcoscaceres
Copy link
Member

@marcoscaceres marcoscaceres commented Nov 14, 2021 •
edited by pr-preview bot
Loading

closes #975

The following tasks have been completed:

  • Modified Web platform tests - not testable, as it's for proprietary/arbitrary method data types.

Implementation commitment:

  • WebKit - patch
  • Chromium (commitment not required - it's for arbitrary method data types)
  • Gecko (commitment not required - it's for arbitrary method data types)

Optional, impact on Payment Handler spec?

Preview | Diff

Preview | Diff

@marcoscaceres marcoscaceres mentioned this pull request Nov 14, 2021
Closed
@dcrousso dcrousso mentioned this pull request Nov 15, 2021
Open
8 tasks
@rsolomakhin
Copy link
Collaborator

lgtm!

@stephenmcgruer
Copy link
Collaborator

Would it be useful to export a dfn here that can be pointed to by other specs, similarly to https://w3c.github.io/payment-request/#dfn-steps-to-check-if-a-payment-can-be-made (and respond, and change)? It's much easier to write other specs when you can say The [=steps to check if a payment can be made=] for [=my new payment method=] are: ....

(Also, I assume this change doesn't obsolete the steps to check if a payment can be made because technically handlers to methods are N:1, though in practice I think it is 1:1?)

(Also also, reminder - steps to check if a payment can be made have nothing to do with canMakePayment, they're actually a check used inside of show() :D)

@marcoscaceres
Copy link
Member Author

@stephenmcgruer, yes to all of the above... the spec actually does need quite an editorial overhaul. We have various issues like #889, #814, and #767 - so it would be great to actually add more hooks as needed.

However, if it's ok, I'd like to do that as a followup to this 🙏

@stephenmcgruer
Copy link
Collaborator

However, if it's ok, I'd like to do that as a followup to this 🙏

Ack, sgtm. I'm happy to send a follow-on PR myself for the bit I care about most (having a <dfn> to link to, just because selfishly it would be useful for SPC currently :D).

@marcoscaceres
Copy link
Member Author

@dcrousso, the working group is just wrapping up publishing the spec a W3C Recommendation (hopefully in the next 2 weeks). Right after we publish the Recommendation, we will immediately republish the specification with this amendment.

@stephenmcgruer stephenmcgruer mentioned this pull request Nov 19, 2021
Merged
webkit-commit-queue pushed a commit to WebKit/WebKit that referenced this pull request Dec 2, 2021
@dcrousso
[Payment Request] Validate payment method data on construction
6bc11f6 
https://bugs.webkit.org/show_bug.cgi?id=233292
<rdar://problem/85736007>

Reviewed by Andy Estes.

Source/WebCore:

This will allow developers to replace any `ApplePaySession.supportsVersion` check(s) by
attempting to create a `PaymentRequest` object with data specific to Apple Pay. This is
actually an improvement, because it'll also allow developers to catch most errors in that
data earlier, as previously any errors would only be thrown when `show()` is called.

Spec: <w3c/payment-request#976>

Test: http/tests/ssl/applepay/PaymentRequest.https.html

* Modules/paymentrequest/PaymentRequest.cpp:
(WebCore::PaymentRequest::show):
* Modules/paymentrequest/PaymentHandler.h:
Adjust `PaymentHandler::convertData` to take a `Document` so that additional validation
can be performed on the provided payment method(s) (e.g. check the Apple Pay version).

* Modules/applepay/PaymentRequestValidator.h:
* Modules/applepay/PaymentRequestValidator.mm:
(WebCore::PaymentRequestValidator::validate):
Allow callers to choose what fields to validate. This is needed because when the
`ApplePaySessionPaymentRequest` created/converted when validating the payment method data in
the `PaymentRequest` constructor isn't able to get fields from the `PaymentRequest` as it
hasn't been created yet.

* Modules/applepay/ApplePaySession.cpp:
(WebCore::convertAndValidate):
* Modules/applepay/paymentrequest/ApplePayPaymentHandler.h:
* Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:
(WebCore::convertAndValidateApplePayRequest):
(WebCore::ApplePayPaymentHandler::convertData):
(WebCore::ApplePayPaymentHandler::show):
* Modules/applepay-ams-ui/ApplePayAMSUIPaymentHandler.h:
* Modules/applepay-ams-ui/ApplePayAMSUIPaymentHandler.cpp:
(WebCore::convertAndValidateApplePayAMSUIRequest):
(WebCore::ApplePayAMSUIPaymentHandler::convertData):

LayoutTests:

* http/tests/ssl/applepay/PaymentRequest.https.html:
* http/tests/ssl/applepay/PaymentRequest.https-expected.txt:



Canonical link: https://commits.webkit.org/244794@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@286452 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@marcoscaceres
Copy link
Member Author

Landed in WebKit https://trac.webkit.org/changeset/286452/webkit .... still blocked on REC progress.

🔜

annulen pushed a commit to qtwebkit/qtwebkit that referenced this pull request Dec 3, 2021
[Payment Request] Validate payment method data on construction
6a1c69a 
https://bugs.webkit.org/show_bug.cgi?id=233292
<rdar://problem/85736007>

Reviewed by Andy Estes.

Source/WebCore:

This will allow developers to replace any `ApplePaySession.supportsVersion` check(s) by
attempting to create a `PaymentRequest` object with data specific to Apple Pay. This is
actually an improvement, because it'll also allow developers to catch most errors in that
data earlier, as previously any errors would only be thrown when `show()` is called.

Spec: <w3c/payment-request#976>

Test: http/tests/ssl/applepay/PaymentRequest.https.html

* Modules/paymentrequest/PaymentRequest.cpp:
(WebCore::PaymentRequest::show):
* Modules/paymentrequest/PaymentHandler.h:
Adjust `PaymentHandler::convertData` to take a `Document` so that additional validation
can be performed on the provided payment method(s) (e.g. check the Apple Pay version).

* Modules/applepay/PaymentRequestValidator.h:
* Modules/applepay/PaymentRequestValidator.mm:
(WebCore::PaymentRequestValidator::validate):
Allow callers to choose what fields to validate. This is needed because when the
`ApplePaySessionPaymentRequest` created/converted when validating the payment method data in
the `PaymentRequest` constructor isn't able to get fields from the `PaymentRequest` as it
hasn't been created yet.

* Modules/applepay/ApplePaySession.cpp:
(WebCore::convertAndValidate):
* Modules/applepay/paymentrequest/ApplePayPaymentHandler.h:
* Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:
(WebCore::convertAndValidateApplePayRequest):
(WebCore::ApplePayPaymentHandler::convertData):
(WebCore::ApplePayPaymentHandler::show):
* Modules/applepay-ams-ui/ApplePayAMSUIPaymentHandler.h:
* Modules/applepay-ams-ui/ApplePayAMSUIPaymentHandler.cpp:
(WebCore::convertAndValidateApplePayAMSUIRequest):
(WebCore::ApplePayAMSUIPaymentHandler::convertData):

LayoutTests:

* http/tests/ssl/applepay/PaymentRequest.https.html:
* http/tests/ssl/applepay/PaymentRequest.https-expected.txt:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@286452 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@marcoscaceres marcoscaceres mentioned this pull request Jan 19, 2022
Merged
4 tasks
@marcoscaceres marcoscaceres changed the title Validate .data on construction Proposed addition: validate .data on construction Feb 9, 2022
@stephenmcgruer stephenmcgruer mentioned this pull request Jun 22, 2022
Closed
@marcoscaceres marcoscaceres merged commit b6b0630 into gh-pages Jul 6, 2022
@marcoscaceres marcoscaceres deleted the early_validation branch July 6, 2022 01:24
marcoscaceres added a commit that referenced this pull request Aug 29, 2022
@marcoscaceres
Revert "Validate .data on construction (#976)"
676bfdd 
This reverts commit b6b0630.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rsolomakhin rsolomakhin rsolomakhin approved these changes

Assignees

No one assigned

Labels

Blocked

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Custom .data validation during construction of PaymentRequest

3 participants

@marcoscaceres @rsolomakhin @stephenmcgruer