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.

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

fix: avoid email delegation via GET request #430

Merged
merged 2 commits into from
Feb 10, 2023
Merged

fix: avoid email delegation via GET request #430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2cacaaf
fix: avoid email delegation via GET request
natevw Jan 27, 2023
6e2950f
fix: update tests to POST to email vaidation URL
travis Feb 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions packages/access-api/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { notFound } from '@web3-storage/worker-utils/response'
import { Router } from '@web3-storage/worker-utils/router'
import { postRaw } from './routes/raw.js'
import { postRoot } from './routes/root.js'
import { validateEmail } from './routes/validate-email.js'
import { preValidateEmail, validateEmail } from './routes/validate-email.js'
import { validateWS } from './routes/validate-ws.js'
import { version } from './routes/version.js'
import { getContext } from './utils/context.js'
Expand All @@ -14,7 +14,8 @@ const r = new Router({ onNotFound: notFound })

r.add('options', '*', preflight)
r.add('get', '/version', version)
r.add('get', '/validate-email', validateEmail)
r.add('get', '/validate-email', preValidateEmail)
r.add('post', '/validate-email', validateEmail)
r.add('get', '/validate-ws', validateWS)
r.add('post', '/', postRoot)
r.add('post', '/raw', postRaw)
Expand Down
15 changes: 15 additions & 0 deletions packages/access-api/src/routes/validate-email.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,23 @@ import {
HtmlResponse,
ValidateEmail,
ValidateEmailError,
PendingValidateEmail,
} from '../utils/html.js'

/**
* @param {import('@web3-storage/worker-utils/router').ParsedRequest} req
* @param {import('../bindings.js').RouteContext} env
*/
export async function preValidateEmail(req, env) {
if (!req.query?.ucan) {
return new HtmlResponse(
<ValidateEmailError msg={'Missing delegation in the URL.'} />
)
}

return new HtmlResponse(<PendingValidateEmail autoApprove={true} />)
}

/**
* @param {import('@web3-storage/worker-utils/router').ParsedRequest} req
* @param {import('../bindings.js').RouteContext} env
Expand Down
34 changes: 34 additions & 0 deletions packages/access-api/src/utils/html.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,40 @@ export class HtmlResponse extends Response {
}
}

/**
*
* @param {object} props
* @param {boolean} [props.autoApprove]
*/
export const PendingValidateEmail = ({ autoApprove }) => (
<div class="fcenter">
<img
src="https://web3.storage/android-chrome-512x512.png"
height="80"
width="80"
/>
<div>
<h1>Validating Email</h1>
<form id="approval" method="post" class="fcenter">
<button class="mcenter">Approve</button>
</form>
{autoApprove ? (
<script
dangerouslySetInnerHTML={{
// NOTE: this script sticks to ES3-era syntax for compat with more browsers
__html: `(function () {
// auto-submit the form for any user w/JS enabled
var form = document.getElementById('approval');
form.style.display = 'none';
form.submit();
})();`,
}}
/>
) : undefined}
</div>
</div>
)

/**
*
* @param {object} param0
Expand Down
4 changes: 2 additions & 2 deletions packages/access-api/test/access-authorize.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

const html = await rsp.text()

assert(html.includes(encoded))
Expand Down Expand Up @@ -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' })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the comment say 'click email url', which I assume would usually involve an HTTP GET, but this changes it to 'POST'?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right so this change splits the approval process into 3 pieces (from the 2 that it is now):

  1. an email with a link
  2. a page that gets served on GET that uses JavaScript to redirect users to
  3. 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:

https://github.com/web3-storage/w3protocol/pull/430/files#diff-1a9cb8eda7a1020a96a7a7c0db5406ee04e5f8953fb6c00ab35f3bb8e6fb2519R103

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and for clarity, in this framing our current process is:

  1. an email with a link
  2. a page that gets served on GET that does the verification and triggers the websocket data flow


// ws
const res = await mf.dispatchFetch('http://localhost:8787/validate-ws', {
Expand Down
2 changes: 1 addition & 1 deletion packages/access-api/test/space-recover.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ describe('space-recover', function () {
assert.deepEqual(del.audience.did(), issuer.did())
assert.deepEqual(del.issuer.did(), service.did())
assert.deepEqual(del.capabilities[0].can, 'space/recover')
const rsp = await mf.dispatchFetch(url)
const rsp = await mf.dispatchFetch(url, { method: 'POST' })
const html = await rsp.text()

assert(html.includes(encoded))
Expand Down