awslabs / cognito-at-edge

Serverless authentication solution to protect your website or Amplify application
Apache License 2.0
168 stars 54 forks source link

Consider using HTTP 307 instead of HTP 302 #82

Open swantzter opened 5 months ago

swantzter commented 5 months ago

What happened:

Our tokens are being refreshed because of a background POST request that is then invalidly retried as a GET request by the browser because of the semantic ambiguity of 301 and 302.

For example:

  1. accessToken expires
  2. user-agent makes a background fetch request to POST /api/whatever
  3. Lambda@Edge refreshes the tokens using the refresh token
  4. Lambda@Edge responds with a 302 Location /api/whatever and Set-Cookie headers
  5. user-agent retries the background fetch request to /api/whatever as a GET request which fails

What did you expect to have happen:

  1. Should retry the request as a POST request again - a 307 status code rather than 302 would enforce this

How to reproduce this (as precisely and succinctly as possible):

This server behind cognito at edge should be able to reproduce the error (visible in the console/network panel)

const http = require('http')

const server = http.createServer((req, res) => {
  const url = new URL(req.url, 'http://localhost:8000')

  if (url.pathname === '/' && req.method === 'GET') {
    res.writeHead(200, { 'Content-type': 'text/html' })
    res.end(siteHtml)
    return
  } else if (url.pathname === '/api' && req.method === 'POST') {
    res.writeHead(200, { 'content-type': 'text/plain', 'access-control-allow-origin': '*', 'access-control-allow-credentials': '*' })
    res.end('Success')
    return
  }

  res.writeHead(404, { 'content-type': 'text/plain' })
  res.end('Not found')
})

server.listen(8000, () => {
  console.log('listening on http://localhost:8000')
})

const siteHtml = `
<html>
  <head><title>Test Cognito At Edge Background Redirect</title></head>
  <body>
    <script>
      setInterval(() => {
        fetch('/api', { method: 'POST', mode: 'cors', credentials: 'include' })
          .then(async res => { console.log(res.status, await res.text()) })
          .catch(err => { console.error(err) })
      }, 10_000)
    </script>
  </body>
</html>
`

Anything else you think we should know?

Environment:

swantzter commented 5 months ago

We've tested using this module patched to use 307 in our setup - it works