neurobagel/api

Routes without trailing slash cause `307` redirect which breaks reverse proxies via paths

Closed this issue · 2 comments

alyssadai commented

Context

Depending on whether a route in FastAPI is defined with or without a trailing slash, FastAPI by default tries to automatically redirect the alternative to the URL with the / added or removed (see examples below).

Right now:

  • if follow redirects are enabled (curl -L):
    • api.neurobagel.org/query -> api.neurobagel.org/query/ -> response
    • api.neurobagel.org/attributes/nb:Assessment/ -> api.neurobagel.org/attributes/nb:Assessment -> response
  • if follow redirects are not enabled:
    • api.neurobagel.org/query -> no response (307)
    • api.neurobagel.org/attributes/nb:Assessment/ -> no response (307)

Some consequences:

  • If a user hosts our API on a server behind a reverse proxy via a path (that is configured correctly to strip the base path during requests), e.g., my-neurobagel-node.org/api -> localhost:8000, when we send a request to my-neurobagel-node.org/api/query, the redirect is followed leading to my-neurobagel-node.org/query, which does not exist from the perspective of the proxy
  • Apparently, when the originating request uses https://, this can become lost and turn into http:// during the redirect (e.g., fastapi/fastapi#8514)
    • It looks like this also depends on how the proxy is set up. We actually don't encounter this with our current APIs behind nginx + acme / cloudflare (e.g., try curl -v -L https://qpn.neurobagel.org/query), possibly because (1) the Dockerized NGINX config which forwards headers by default, and (2) the NGINX container is on the same Docker network as the Uvicorn server, there might be an implicit trust mechanism between IPs on the same network

This hasn't been an issue for us in the past because:

  • we have always used subdomains, so any redirects haven't affected the base path
  • most users testing our deployment have deployed locally as opposed to behind a proxy
  • the browser follows redirects automatically
  • most of our docs examples include -L (or, we have been using it pretty liberally)

Rather than the current redirect behaviour (which is also less efficient due to multiple round trips), we probably want one of the following outcomes:

Option 1:

(can be achieved via two route decorators)

  • api.neurobagel.org/query -> response
  • api.neurobagel.org/query/ -> response
  • api.neurobagel.org/attributes/nb:Assessment -> response
  • api.neurobagel.org/attributes/nb:Assessment/ -> response

Option 2:

(can be achieved w/ the redirect_slashes parameter of the FastAPI and/or APIRouter class)

  • api.neurobagel.org/query -> response
  • api.neurobagel.org/query/ -> no response
  • api.neurobagel.org/attributes/nb:Assessment -> response
  • api.neurobagel.org/attributes/nb:Assessment/ -> no response

Decisions

  • Disable redirect_slashes globally (option 2) for more predictable behaviour
  • Update examples in README
  • Add --forwarded-allow-ips=* flag as recommended in fastapi/fastapi#9328 (comment) to ensure we don't lose HTTPS behind a proxy (this shouldn't happen anymore now that we're getting rid of redirects, but might be a good safeguard regardless)

Relevant issues:

alyssadai commented

PR #328 is ready to merge, but will wait for the PR for neurobagel/query-tool#234 to maintain compatibility with the query tool.

neurobagel-bot commented

🚀 Issue was released in v0.3.0 🚀