Closed alyssadai closed 1 month ago
PR https://github.com/neurobagel/api/pull/328 is ready to merge, but will wait for the PR for https://github.com/neurobagel/query-tool/issues/234 to maintain compatibility with the query tool.
:rocket: Issue was released in v0.3.0
:rocket:
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:
curl -L
):307
)307
)Some consequences:
my-neurobagel-node.org/api
->localhost:8000
, when we send a request tomy-neurobagel-node.org/api/query
, the redirect is followed leading tomy-neurobagel-node.org/query
, which does not exist from the perspective of the proxyhttps://
, this can become lost and turn intohttp://
during the redirect (e.g., https://github.com/tiangolo/fastapi/discussions/8514)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 networkThis hasn't been an issue for us in the past because:
-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)
Option 2:
(can be achieved w/ the
redirect_slashes
parameter of the FastAPI and/or APIRouter class)Decisions
redirect_slashes
globally (option 2) for more predictable behaviour--forwarded-allow-ips=*
flag as recommended in https://github.com/tiangolo/fastapi/discussions/9328#discussioncomment-8242245 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: