electerious / Ackee

Self-hosted, Node.js based analytics tool for those who care about privacy.
https://ackee.electerious.com
MIT License
4.2k stars 350 forks source link

CORS headers are not attached for serverless function on Vercel #330

Closed birjj closed 2 years ago

birjj commented 2 years ago

🐞 Describe the bug

After deploying Ackee to Vercel and setting ACKEE_AUTO_ORIGIN=true, I am not seeing any CORS headers in response to the OPTIONS ackee.example.com/api request (where example.com is replaced with my domain). I have also tested with ACKEE_ALLOW_ORIGIN=site.example.com, with the same result. In both cases I'm seeing:

Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at https://ackee.example.com/api. (Reason: CORS header ‘Access-Control-Allow-Origin’ missing). Status code: 204.

📚 To Reproduce

  1. Set up the project on Vercel
  2. Set the env variable ACKEE_AUTO_ORIGIN=true
  3. Log in and create a site on Ackee for your domain
  4. Add the tracking script to your site
  5. Observe requests in the browser dev tools to see lack of CORS headers

💡 Expected behavior

The OPTIONS /api response includes CORS headers to allow the tracker script to connect.

⚙️ Environment

📋 Additional context

I have validated that the origin handler is called and returns ["site.example.com"]. It seems that the CORS headers are calculated correctly, but somehow not applied.

electerious commented 2 years ago

Just double checked how I'm passing the origin to Apollo server and it seems to be the correct way.

type StaticOrigin = boolean | string | RegExp | (boolean | string | RegExp)[];

type CustomOrigin = (requestOrigin: string | undefined, callback: (err: Error | null, origin?: StaticOrigin) => void) => void;

That's basically what I'm doing: https://github.com/electerious/Ackee/blob/ba976a89dcde893371b881d5fc4496cfb1c2625c/src/serverless.js#L51

I'm not sure what's happening with the headers.

birjj commented 2 years ago

Yes, as far as I can see you're passing it correctly. I've been debugging this for a few hours with various logs and workaround attempts, but so far it seems like Vercel might not be passing the headers, or something else is happening between the Apollo server and the client. I'll report back if I figure out what's going wrong.

electerious commented 2 years ago

Thanks for the help!

That's how the response of the handler looks when I log it. There is a access-control-allow-origin and a access-control-allow-credentials, but I'm not sure what happens to the allowedHeaders.

{
  statusCode: 200,
  body: '[{"data":{"facts":{"id":"e875350a-09ae-59bf-ae72-2ae687ba00db","activeVisitors":0,"__typename":"Facts"}}}]\n',
  multiValueHeaders: {
    'x-powered-by': [ 'Express' ],
    'access-control-allow-origin': [ 'https://example.com' ],
    vary: [ 'Origin' ],
    'access-control-allow-credentials': [ 'true' ],
    'content-type': [ 'application/json; charset=utf-8' ],
    'content-length': [ '107' ],
    etag: [ 'W/"6b-LOgac06NQapfhEsmrItvJVDfH1c"' ]
  },
  isBase64Encoded: false
}
birjj commented 2 years ago

Thanks, that's great for debugging. I think that might've helped me uncover a related bug:

Bug with auto detected domains (ACKEE_AUTO_ORIGIN=true) When using the current `master` branch and adding ```diff const handler = apolloServer.createHandler({ expressGetMiddlewareOptions: { cors: { origin, credentials: true, methods: 'GET,POST,PATCH,OPTIONS', allowedHeaders: 'Content-Type, Authorization, Time-Zone', }, }, }) + const response = handler(event, context) + response.then((value) => console.log('Response', value)) + return response ``` the `access-control-allow-origin` isn't set (I'm guessing express thinks that the user domain `https://site.example.com` doesn't match the string `site.example.com`): ```js Response { statusCode: 204, body: '', multiValueHeaders: { 'x-powered-by': [ 'Express' ], vary: [ 'Origin' ], 'access-control-allow-credentials': [ 'true' ], // note lack of access-control-allow-origin 'access-control-allow-methods': [ 'GET,POST,PATCH,OPTIONS' ], 'access-control-allow-headers': [ 'Content-Type, Authorization, Time-Zone' ], 'content-length': [ '0' ] }, isBase64Encoded: false } ``` but prepending protocols fixes that: ```diff const origin = (origin, callback) => { if (config.autoOrigin === true) { fullyQualifiedDomainNames() + .then((names) => callback(null, names.flatMap((name) => [ `http://${ name }`, `https://${ name }` ]))) .catch((error) => callback(error, false)) return } ``` ```js Response { statusCode: 204, body: '', multiValueHeaders: { 'x-powered-by': [ 'Express' ], 'access-control-allow-origin': [ 'https://site.example.com' ], // now it's here :) vary: [ 'Origin' ], 'access-control-allow-credentials': [ 'true' ], 'access-control-allow-methods': [ 'GET,POST,PATCH,OPTIONS' ], 'access-control-allow-headers': [ 'Content-Type, Authorization, Time-Zone' ], 'content-length': [ '0' ] }, isBase64Encoded: false } ``` Note that the protocol cannot be added to the site name directly in Ackee because it then wouldn't be recognized as a domain by `is-valid-domain`.

Regardless, the headers still aren't being returned to the browser for some reason. It looks like multiValueHeaders is just AWS specific syntax for setting headers, so it should work - I'll keep digging to see if I can force it to work.

Note: workaround if all you need is a single site or * The only success I've had with getting headers sent out to the browser has been by setting them in `vercel.json`. This doesn't work for dynamic headers, but in case anyone is desperate for a workaround here's the `vercel.json` that forces CORS headers: ```json { "build": { "env": { "NODEJS_AWS_HANDLER_NAME": "handler" } }, "headers": [ { "source": "/(.*)", "headers": [ { "key": "Access-Control-Allow-Origin", "value": "https://site.example.com" }, { "key": "Access-Control-Allow-Credentials", "value": "true" }, { "key": "Access-Control-Allow-Headers", "value": "X-Requested-With, Access-Control-Allow-Origin, X-HTTP-Method-Override, Content-Type, Cache-Control, Authorization, Accept" }, { "key": "Access-Control-Max-Age", "value": "86400" } ] } ] } ```
birjj commented 2 years ago

Finally! I think I've finally found the issue, thanks to your log @electerious: the AWS Lambda API is poorly documented, but seems to use both .headers and .multiValueHeaders to define headers. The Vercel shim that lets us run the serverless function without coding it specifically for Vercel seems to lack support for .multiValueHeaders.

The issue is fixed if we convert .multiValueHeaders to .headers:

// api/index.js
'use strict'

/**
 * A serverless function handler for the '/api' route, for use with Vercel.
 * This handler follows the AWS Lambda API; Vercel deployments are opted-in
 * using the "NODEJS_AWS_HANDLER_NAME" environment variable defined in vercel.json.
 *
 * See:
 *  - https://vercel.com/docs/serverless-functions/supported-languages#node.js
 *  - https://vercel.com/docs/runtimes#advanced-usage/advanced-node-js-usage/aws-lambda-api
 */
const handler = require('../src/serverless').handler

const convertMultiValueHeaders = (response) => {
    if (!(response instanceof Object) || !('multiValueHeaders' in response)) {
        console.log('No multiValueHeaders, not converting')
        return response
    }
    response.headers = response.headers || {}
    for (const [ key, value ] of Object.entries(response.multiValueHeaders)) {
        if (value.length === 1) {
            response.headers[key] = value[0]
        }
    }
    return response
}

exports.handler = async (...args) => {
    const response = await handler(...args)
    console.log('Response before conversion:', response)
    const converted = convertMultiValueHeaders(response)
    console.log('Response after conversion:', response)
    return converted
}

I'll create a PR shortly

birjj commented 2 years ago

I've opened a support case with Vercel, which they say they'll try to respond to within 3-5 business days.

Would you like me to create a PR to fix this in Ackee @electerious, or should we wait for Vercel to respond and (possibly) update their AWS Lambda API shim?

electerious commented 2 years ago

Thanks a lot for your work! This would have taken me forever to figure out. A PR is welcome :)

electerious commented 2 years ago

Pushed a new version of Ackee. Please let me know if everything works now! :)

xfxx2022 commented 2 years ago

新版本还是Unexpected token A in JSON at position 0

birjj commented 2 years ago

Looks like it works! Thanks for the merge @electerious

For reference I've opened a public issue on Vercel's side (vercel/vercel#7820), which anyone interested can track for when this gets fixed on Vercel's end.