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

externalUrlBase behaves incorrectly with proxies + subscriptions + graphiql #1232

Closed
1 of 3 tasks
cmanou opened this issue Feb 19, 2020 · 4 comments · Fixed by #1361
Closed
1 of 3 tasks

externalUrlBase behaves incorrectly with proxies + subscriptions + graphiql #1232

cmanou opened this issue Feb 19, 2020 · 4 comments · Fixed by #1361

Comments

@cmanou
Copy link

cmanou commented Feb 19, 2020

I'm submitting a ...

  • bug report
  • feature request
  • question

PostGraphile version: 4.4.4 (but applicable to all versions since #1070)

Steps to reproduce:

  1. Have a running instance of postgraphile with graphiql enabled (eg: http://localhost:3000/graphql)
  2. Have a proxy server in front of the postgraphile server that routes on a path (eg: https://example.com/api/graphql -> http://localhost:3000/graphql)
  3. Alternate setting externalUrlBase to /api or undefined

Current behavior:

Expected behavior:
That it works in both cases, this will most likely mean that there will have to be 2 config options as opposed to just externalUrlBase. One option to support the mounting of postgraphile at an express route (ie the reasoning behind the change that was made in #1070 ) and the other which is purely for graphiql config

@benjie
Copy link
Member

benjie commented Feb 24, 2020

Yes, you're right - we need two settings here. If we take a slightly more complex example:

https://example.com/api/graphql -> http://localhost:3000/route/graphql

assuming you have something like app.use('/route', postgraphile(...))

To PostGraphile within Express over HTTP we have graphqlRoute = '/graphql' and we can detect originalUrl = '/route/graphql' so we can infer our "internalUrlBase" to be /route but we know nothing about our "actual" location which is /api/graphql. Setting externalUrlBase to /api here should work as expected since we just tell the user externalUrlBase + graphqlRoute is where they can find us.

To PostGraphile websockets we need to match incoming /route/graphql requests directly since we have no express middleware to do the originalUrl dance for us, and we know nothing of the external API. So we need an internalUrlBase = '/route'.

Often externalUrlBase and internalUrlBase are the same, which is how we've got away with having just one setting so far, but apparently no more!

I'd love a PR for this 👍

@danbiagini
Copy link

Alright, I'll bite. Just hit this, and seems like a reasonably straight forward place to jump in as 1st time contributor?

@benjie
Copy link
Member

benjie commented Jul 29, 2020

I think so! 🙌 Any questions; @ me on #dev-postgraphile in Discord: https://discord.gg/graphile

@benjie
Copy link
Member

benjie commented Sep 8, 2020

Hey @danbiagini did you get a chance to look at this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants