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

fix: avoid email delegation via GET request #398

Closed
wants to merge 4 commits into from

Conversation

natevw
Copy link
Contributor

@natevw natevw commented Jan 27, 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:

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.

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:

  • no changes needed to the email templates (* though still would be good to do)
  • 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

hugomrdias and others added 4 commits January 25, 2023 10:30
Fix: typos

Co-authored-by: Alan Shaw <[email protected]>
When explicitly passing `null` a default value is not set.
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 storacha#333 and presumably fixes all of storacha#348
@travis
Copy link
Member

travis commented Feb 9, 2023

closing, this has moved to #430

@travis travis closed this Feb 9, 2023
Peeja pushed a commit to storacha/upload-service that referenced this pull request Jan 17, 2025
Refactor a bit to make it easier to wrap terms and docs in the same
layout. Previously there was no link to get back to the main page from
terms, which was not great.

resolves storacha#398 

<img width="1223" alt="Screenshot 2023-03-29 at 6 20 18 PM"
src="https://user-images.githubusercontent.com/1113/228703406-ba85501f-e841-47fe-a394-d5200b7f7eaf.png">


<img width="1226" alt="Screenshot 2023-03-29 at 6 16 11 PM"
src="https://user-images.githubusercontent.com/1113/228702897-10c48408-c112-4ca8-a488-da3ac6e981df.png">
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants