w3c / secure-payment-confirmation

Secure Payment Confirmation (SPC)
https://w3c.github.io/secure-payment-confirmation/
Other
110 stars 39 forks source link

[Spec] Correctly validate payeeOrigin #160

Closed stephenmcgruer closed 2 years ago

stephenmcgruer commented 2 years ago

Fixes #149


Preview | Diff

stephenmcgruer commented 2 years ago

@annevk - PTAL (and apologies for the delay!). I wasn't sure if we should be doing the same 3rd step as postMessage does, which is:

Set targetOrigin to parsedURL's origin.

I assume this is for normalization, but I'm unclear if its desirable for SPC as it seems to be a tuple rather than a string then?

annevk commented 2 years ago

If you don't have an actual origin, how would you do a same origin check? Internally you also want origins to be "objects". If you need to output it at some point you can serialize it then.

stephenmcgruer commented 2 years ago

how would you do a same origin check

I may not be following you; nowhere in the spec is payeeOrigin subject to a same-origin check, I believe. The payeeOrigin is a claim by the SPC caller as to which origin is receiving payment, which is displayed to the user by the browser and signed into the cryptogram. It may or may not be the same as the origin that SPC was called from.

For example, in a redirect flow, merchant.com will redirect to psp.com, who may then call SPC with payeeOrigin: 'https://merchant.com'. In this flow, the browser has no way to enforce that payeeOrigin is anything in particular, but that's ok - we don't have the ability to enforce that (e.g.) the transaction amount is correct either. The browser's job is to clearly indicate to the user what transaction information they are agreeing to, and then sign that for later validation/verification by the Relying Party (e.g., the bank).

annevk commented 2 years ago

Okay, so you essentially use a serialized form of the origin for signing (which would still end up being compared I suspect at some other point by another party?).

In that case you do still want to extract the origin from URL. Then you probably want to check that it's not an opaque origin (and fail if it is one). And I guess you might even want to enforce that the origin's scheme is https? Then you want to serialize the origin and use the resulting value for signing.

And yeah, normalization is important here. E.g., if the supplied value is https://EXAMPLE.example/test, you want to sign with https://example.example, right?

stephenmcgruer commented 2 years ago

Ah yes, very good points. I'm embarrassed - I hadn't even considered that (I now assume) the URL parser would accept https://foo.com/this/is/a/path, nevermind things like opaque origins >_<.

Thanks, I'll update the PR and ping for re-review. I appreciate your help!

stephenmcgruer commented 2 years ago

@annevk - PTAAL. I now check the scheme is https (which I believe rules out opaque origins as well), and then overwrite the input with the normalization. It's a little dodgy to do the overwrite (since it makes the Steps to check if a payment can be made non-idempotent), but in practice it 'works' and long-term I expect us to move SPC away from PaymentRequest and to its own API shape (see https://github.com/w3c/secure-payment-confirmation/issues/65#issuecomment-902115127)

stephenmcgruer commented 2 years ago

(By the way, if you ever need to add something from a WHATWG specification to an anchors block, please file an issue. To a large extent they are code smell.)

Ack, thanks (and thanks for the review!)