fix: avoid email delegation via GET request #430
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
the email validation approval process is now split into two stages: a GET request with no side effects except to load a page that then auto-submits a POST request to actually continue the flow. this fixes the API to follow proper API semantics and thus starts addressing some of #333 and presumably fixes all of #348
vasco-santos
approved these changes
Feb 9, 2023
There was a problem hiding this comment.
Code looks good to me. Thanks @travis ! It would be quite nice to test it and see if before shipping.
Sadly, we don't have per PR deployments in CF, so we should make sure we test this in staging once this gets merged and are happy with it
natevw
reviewed
Feb 9, 2023
| @@ -88,7 +88,7 @@ describe('access/authorize', function () { | |||
| /** @type {import('@web3-storage/access/types').EncodedDelegation<[import('@web3-storage/capabilities/types').AccessSession]>} */ ( | |||
| url.searchParams.get('ucan') | |||
| ) | |||
| const rsp = await mf.dispatchFetch(url) | |||
| const rsp = await mf.dispatchFetch(url, { method: 'POST' }) | |||
There was a problem hiding this comment.
ah, thanks for catching these up! I thought I had successfully re-run the tests but apparently not.
(and this is the exact change intended/expected — for anything out there in the wild that was relying on the old approval URLs they simply need to POST instead of GET after these changes)
gobengo
reviewed
Feb 9, 2023
| @@ -119,7 +119,7 @@ describe('access/authorize', function () { | |||
|
|
|||
| const url = new URL(inv) | |||
| // click email url | |||
| await mf.dispatchFetch(url) | |||
| await mf.dispatchFetch(url, { method: 'POST' }) | |||
There was a problem hiding this comment.
Why does the comment say 'click email url', which I assume would usually involve an HTTP GET, but this changes it to 'POST'?
There was a problem hiding this comment.
right so this change splits the approval process into 3 pieces (from the 2 that it is now):
- an email with a link
- a page that gets served on GET that uses JavaScript to redirect users to
- a page that gets served on POST that does the actual verification and triggers the websocket data flow
here's the page that gets served by the GET request:
There was a problem hiding this comment.
and for clarity, in this framing our current process is:
- an email with a link
- a page that gets served on GET that does the verification and triggers the websocket data flow
sweet - agreed! fwiw I did test this locally and it looked basically indistinguishable from the current process because the redirect process happens so quickly. I haven't tested in our staging environment before, is there a persistent env for that or do I need to do something to get that deployed once this is merged? I'll merge now and we can get this tested in staging today! |
travis
pushed a commit
that referenced
this pull request
Feb 16, 2023
🤖 I have created a release *beep* *boop* --- ## [4.10.0](access-api-v4.9.0...access-api-v4.10.0) (2023-02-16) ### Features * add support for access/authorize and update ([#392](#392)) ([9c8ca0b](9c8ca0b)), closes [#386](#386) * optional override for Postmark email From: field ([#354](#354)) ([f6b2350](f6b2350)) * rm /reproduce-cloudflare-error route ([#426](#426)) ([99cbd2f](99cbd2f)) * rm upload-api-proxy ability to route to separate environment audiences ([#407](#407)) ([5cfe274](5cfe274)) ### Bug Fixes * align postmark/welcome.txt with .html version ([#431](#431)) ([a53d6e6](a53d6e6)) * avoid email delegation via GET request ([#430](#430)) ([d282d6a](d282d6a)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
gobengo
pushed a commit
that referenced
this pull request
Apr 11, 2023
The email validation approval process is now split into two stages: a GET request with no side effects except to load a page, that then auto-submits a POST request to actually continue the flow. ## Summary of problem This fixes the API so as to follow [proper HTTP semantics](#333 (comment)): > The purpose of distinguishing between safe [i.e. like GET] and unsafe [like PUT/POST] methods is to allow automated retrieval processes (spiders) and cache performance optimization (pre-fetching) to work without fear of causing harm. In addition, it allows a user agent to apply appropriate constraints on the automated use of unsafe methods when processing potentially untrusted content. That is, a `PUT` or `POST` (rather than a `GET`) **must** be the method used in order to do things like * cause a message to be sent (forwarding a UCAN delegation via a separate connection's websocket) * cause an untrusted keypair to be associated with a billable email address (which is the outcome of that forwarding, in practice!) Fixing the HTTP semantics should address all of #348, and is the first step to addressing the security concerns in #333. ## Summary of solution Clicking (or scanning/pre-fetching/previewing/etc.) the link in the email no longer finishes the validation process. Instead, it loads a (harmless to scan/pre-fetch/preview) landing page which simply says "Validating Email" while using JavaScript to auto-complete the process. This patch is able to fix the core HTTP semantics in a very self-contained way: * no changes needed to the email templates * will not break any existing unexpired links at the moment it is deployed * is essentially the exact same UX from a user's perspective (they might notice just a little extra blink) * does degrade gracefully if user has JS disabled, and any non-browser clients could still trigger the `POST` ± just as easy as before * no changes needed on the `w3ui` side for this part of the email validation improvements --------- Co-authored-by: Nathan Vander Wilt <nate@calftrail.com>
gobengo
pushed a commit
that referenced
this pull request
Apr 11, 2023
🤖 I have created a release *beep* *boop* --- ## [4.10.0](access-api-v4.9.0...access-api-v4.10.0) (2023-02-16) ### Features * add support for access/authorize and update ([#392](#392)) ([bf41071](bf41071)), closes [#386](#386) * optional override for Postmark email From: field ([#354](#354)) ([00db0ec](00db0ec)) * rm /reproduce-cloudflare-error route ([#426](#426)) ([158f309](158f309)) * rm upload-api-proxy ability to route to separate environment audiences ([#407](#407)) ([eefb6c6](eefb6c6)) ### Bug Fixes * align postmark/welcome.txt with .html version ([#431](#431)) ([0d72795](0d72795)) * avoid email delegation via GET request ([#430](#430)) ([e0f67e8](e0f67e8)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Originally proposed in #398, moved here to make rebasing simpler and to fix tests.
Original PR message from @natevw:
The email validation approval process is now split into two stages: a GET request with no side effects except to load a page, that then auto-submits a POST request to actually continue the flow.
Summary of problem
This fixes the API so as to follow proper HTTP semantics:
That is, a
PUTorPOST(rather than aGET) must be the method used in order to do things likeFixing the HTTP semantics should address all of #348, and is the first step to addressing the security concerns in #333.
Summary of solution
Clicking (or scanning/pre-fetching/previewing/etc.) the link in the email no longer finishes the validation process. Instead, it loads a (harmless to scan/pre-fetch/preview) landing page which simply says "Validating Email" while using JavaScript to auto-complete the process.
My preference would have been to move more of the approval process out of the email and into this landing page. (So rather than auto-approving, this landing page would contain details/context and force an informed clear "Yes, approve this new space" vs. "No, I didn't want this" decision.) However, the team preferred to keep* all that in the initial email and requested that this fix be based on an auto-approval.
Given that preference, this patch is able to fix the core HTTP semantics very self-contained:
POST± just as easy as beforew3uiside for this part of the email validation improvements