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

Vercel returning incorrect CORS headers with GraphQL yoga #5933

Closed
dac09 opened this issue Jul 14, 2022 · 1 comment · Fixed by #5970
Closed

Vercel returning incorrect CORS headers with GraphQL yoga #5933

dac09 opened this issue Jul 14, 2022 · 1 comment · Fixed by #5970
Assignees
Labels
bug/needs-info More information is needed for reproduction

Comments

@dac09
Copy link
Contributor

dac09 commented Jul 14, 2022

Original issue from Redwood startup club

This may actually be an issue with Vercel, but has come out with the recent RW deploy. I can see return shape from RW graphql handler to be:

  statusCode: 400,
  multiValueHeaders: {
    'access-control-allow-credentials': [ 'true' ],
    'access-control-allow-origin': [ 'https://app.buildpass.com.au/' ],
    'content-length': [ '4784' ],
    'content-type': [ 'application/json' ],
    server: [ 'GraphQL Yoga' ]
  },
  headers: { 'Content-Type': 'application/json' }

A quick google I found this somewhat recent issue posted: vercel/vercel#7820

A bit more debugging
I did some quick digging into the change to yoga and noticed these lines https://github.com/redwoodjs/redwood/pull/4712/files#diff-46853308693ceb8d5244a4caed6f4301331faaf4f267e15401e60815a7962574R254-R259
Atleast within this PR I dont see the extra work to replace these removed lines to splice the multiValueHeaders back onto the response headers?


Currently we blocked from going above version 0.48 as a result. We could put a patch in to essentially await the response from RW graphQL handlers and then re-configure the headers to splice multiValueHeaders onto headers. I don’t know enough about the use of multiValueHeaders to understand if this would have unknown consequences. I’ve done this here on a test branch for the fresh deploy of v2 and it works, but it feels very hacky to do this, id rather wait till an offical patch (edited)

@dac09 dac09 self-assigned this Jul 14, 2022
@redwoodjs-bot redwoodjs-bot bot moved this to In progress in Main Jul 14, 2022
@dac09 dac09 added the bug/needs-info More information is needed for reproduction label Jul 14, 2022
@dac09
Copy link
Contributor Author

dac09 commented Jul 14, 2022

I've investigated it a little further, to me it looks like this is an issue on both Vercel and Netlify (testing with RW 2.1).

To me it seems that neither work correctly, my test using OPTIONS requests, where I configured the graphql handler with the following options:

  cors: {
    origin: ['https://bazinga.kittens', 'https://www.kittens.bazinga'],
  },

And the request I'm sending has the origin set to one of those origins.

Netlify

Partially correct results, with most of the headers except the origin being returned.

Response headers

HTTP/1.1 204 No Content
Access-Control-Allow-Credentials: true
Access-Control-Allow-Headers: content-type, content-type
Access-Control-Allow-Methods: POST, PUT, GET, POST, PUT, GET
Access-Control-Allow-Origin: null 👈 WHY?!

Which probably means, its not reading the origin correctly in the incoming request, but respects the multiValueHeaders returned in the graphql function (judging by the other headers)

Vercel

Completely incorrect results, where no CORS headers are returned

HTTP/1.1 204 No Content
Date: Thu, 14 Jul 2022 21:00:30 GMT
Connection: close
X-Robots-Tag: noindex
content-length: 0
x-vercel-cache: MISS
server: Vercel
x-vercel-id: cle1::iad1::k4mcx-1657832427595-c33f6e2c16b1
strict-transport-security: max-age=63072000; includeSubDomains; preload
cache-control: public, max-age=0, must-revalidate

Which means Vercel ignores the multiValueHeaders key in the Response, as described in the linked issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/needs-info More information is needed for reproduction
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant