[FIX] Disable redirect slashes globally and remove trailing / from …

…`/query` (#328)

* add tests for routes with and without trailing slashes

* disable redirect slashes and remove trailing slash from /query route

* update routes in tests

* fix - explicitly enable auth in tests of request headers using fixture

* trust X-Forwarded... proxy headers from all remote IPs

* update slashes in README examples

* refactor tests of trailing slash behaviour into separate module

Dockerfile

@@ -12,4 +12,4 @@ COPY ./vocab /usr/src/vocab
 # NB_API_PORT, representing the port on which the API will be exposed, 
 # is an environment variable that will always have a default value of 8000 when building the image
 # but can be overridden when running the container.
-ENTRYPOINT uvicorn app.main:app --proxy-headers --host 0.0.0.0 --port ${NB_API_PORT:-8000}
+ENTRYPOINT uvicorn app.main:app --proxy-headers --forwarded-allow-ips=* --host 0.0.0.0 --port ${NB_API_PORT:-8000}

README.md

@@ -36,7 +36,8 @@ Please refer to our [**official documentation**](https://neurobagel.org/api/) fo
 ## Quickstart
 The API is hosted at https://api.neurobagel.org/ and interfaces with Neurobagel's graph database. Queries of the graph can be run using the `/query` route.
 
-Example: **I want to query for only female participants in the graph.** The URL for such a query would be https://api.neurobagel.org/query/?sex=snomed:248152002, where `snomed:248152002` is a [controlled term from the SNOMED CT database](http://purl.bioontology.org/ontology/SNOMEDCT/248152002) corresponding to female sex.
+Example: **I want to query for only female participants in the graph.** 
+The URL for such a query would be https://api.neurobagel.org/query?sex=snomed:248152002, where `snomed:248152002` is a [controlled term from the SNOMED CT database](http://purl.bioontology.org/ontology/SNOMEDCT/248152002) corresponding to female sex.
 
 Interactive documentation for the API is available at https://api.neurobagel.org/docs.
 
@@ -118,7 +119,7 @@ docker run -d --name=api -p ${NB_API_PORT_HOST}:${NB_API_PORT} --env-file=.env n
 By default, after running the above steps, the API should be served at localhost, http://127.0.0.1:8000/query, on the machine where you launched the Dockerized app. To check that the API is running and can access the knowledge graph as expected, you can navigate to the interactive API docs in your local browser (http://127.0.0.1:8000/docs) and enter a sample query, or send an HTTP request in your terminal using `curl`:
 ``` bash
 # example: query for female subjects in graph
-curl -L http://127.0.0.1:8000/query/?sex=snomed:248152002 
+curl http://127.0.0.1:8000/query?sex=snomed:248152002 
 ```
 The response should be a list of dictionaries containing info about datasets with participants matching the query.
 

app/api/routers/query.py

@@ -23,7 +23,12 @@
 )
 
 
-@router.get("/", response_model=List[CohortQueryResponse])
+# We (unconventionally) use an "" path prefix here because we have globally disabled
+# redirection of trailing slashes in the main app file. We use an empty string here
+# to ensure that a request without a trailing slash (e.g., to /query instead of /query/)
+# is correctly routed to this endpoint.
+# For more context, see https://github.com/neurobagel/api/issues/327.
+@router.get("", response_model=List[CohortQueryResponse])
 async def get_query(
     query: QueryModel = Depends(QueryModel),
     token: str | None = Depends(oauth2_scheme),

app/main.py

@@ -16,7 +16,10 @@
 from .api.security import check_client_id
 
 app = FastAPI(
-    default_response_class=ORJSONResponse, docs_url=None, redoc_url=None
+    default_response_class=ORJSONResponse,
+    docs_url=None,
+    redoc_url=None,
+    redirect_slashes=False,
 )
 favicon_url = "https://raw.githubusercontent.com/neurobagel/documentation/main/docs/imgs/logo/neurobagel_favicon.png"
 

tests/conftest.py

@@ -11,6 +11,12 @@ def test_app():
     yield client
 
 
+@pytest.fixture
+def enable_auth(monkeypatch):
+    """Enable the authentication requirement for the API."""
+    monkeypatch.setattr("app.api.security.AUTH_ENABLED", True)
+
+
 @pytest.fixture
 def disable_auth(monkeypatch):
     """

tests/test_app_events.py

@@ -39,7 +39,7 @@ def mock_httpx_post(**kwargs):
         return httpx.Response(status_code=401)
 
     monkeypatch.setattr(httpx, "post", mock_httpx_post)
-    response = test_app.get("/query/", headers=mock_auth_header)
+    response = test_app.get("/query", headers=mock_auth_header)
     assert response.status_code == 401
 
 

tests/test_attributes.py

@@ -7,16 +7,6 @@
 from app.api import utility as util
 
 
-def test_root(test_app):
-    """Given a GET request to the root endpoint, Check for 200 status and expected content."""
-
-    response = test_app.get("/")
-
-    assert response.status_code == 200
-    assert "Welcome to the Neurobagel REST API!" in response.text
-    assert '<a href="/docs">documentation</a>' in response.text
-
-
 @pytest.mark.filterwarnings("ignore:.*NB_API_ALLOWED_ORIGINS")
 @pytest.mark.parametrize(
     "valid_data_element_URI",

tests/test_query.py

@@ -6,6 +6,8 @@
 import app.api.utility as util
 from app.api import crud
 
+ROUTE = "/query"
+
 
 def test_get_subjects_by_query(monkeypatch):
     """Test that graph results for dataset size queries are correctly parsed into a dictionary."""
@@ -68,7 +70,7 @@ def test_null_modalities(
         crud, "query_matching_dataset_sizes", mock_query_matching_dataset_sizes
     )
 
-    response = test_app.get("/query/", headers=mock_auth_header)
+    response = test_app.get(ROUTE, headers=mock_auth_header)
     assert response.json()[0]["image_modals"] == [
         "http://purl.org/nidash/nidm#T1Weighted"
     ]
@@ -84,7 +86,7 @@ def test_get_all(
     """Given no input for any query parameters, returns a 200 status code and a non-empty list of results (should correspond to all subjects in graph)."""
 
     monkeypatch.setattr(crud, "get", mock_successful_get)
-    response = test_app.get("/query/", headers=mock_auth_header)
+    response = test_app.get(ROUTE, headers=mock_auth_header)
     assert response.status_code == 200
     assert response.json() != []
 
@@ -106,7 +108,7 @@ def test_get_valid_age_range(
 
     monkeypatch.setattr(crud, "get", mock_successful_get)
     response = test_app.get(
-        f"/query/?min_age={valid_min_age}&max_age={valid_max_age}",
+        f"{ROUTE}?min_age={valid_min_age}&max_age={valid_max_age}",
         headers=mock_auth_header,
     )
     assert response.status_code == 200
@@ -128,7 +130,7 @@ def test_get_valid_age_single_bound(
     """Given only a valid lower/upper age bound, returns a 200 status code and a non-empty list of results."""
 
     monkeypatch.setattr(crud, "get", mock_successful_get)
-    response = test_app.get(f"/query/?{age_keyval}", headers=mock_auth_header)
+    response = test_app.get(f"{ROUTE}?{age_keyval}", headers=mock_auth_header)
     assert response.status_code == 200
     assert response.json() != []
 
@@ -155,7 +157,7 @@ def test_get_invalid_age(
 
     monkeypatch.setattr(crud, "get", mock_get)
     response = test_app.get(
-        f"/query/?min_age={invalid_min_age}&max_age={invalid_max_age}",
+        f"{ROUTE}?min_age={invalid_min_age}&max_age={invalid_max_age}",
         headers=mock_auth_header,
     )
     assert response.status_code == 422
@@ -177,7 +179,7 @@ def test_get_valid_sex(
 
     monkeypatch.setattr(crud, "get", mock_successful_get)
     response = test_app.get(
-        f"/query/?sex={valid_sex}", headers=mock_auth_header
+        f"{ROUTE}?sex={valid_sex}", headers=mock_auth_header
     )
     assert response.status_code == 200
     assert response.json() != []
@@ -190,7 +192,7 @@ def test_get_invalid_sex(
     """Given an invalid sex string, returns a 422 status code."""
 
     monkeypatch.setattr(crud, "get", mock_get)
-    response = test_app.get("/query/?sex=apple", headers=mock_auth_header)
+    response = test_app.get(f"{ROUTE}?sex=apple", headers=mock_auth_header)
     assert response.status_code == 422
 
 
@@ -209,7 +211,7 @@ def test_get_valid_diagnosis(
 
     monkeypatch.setattr(crud, "get", mock_successful_get)
     response = test_app.get(
-        f"/query/?diagnosis={valid_diagnosis}", headers=mock_auth_header
+        f"{ROUTE}?diagnosis={valid_diagnosis}", headers=mock_auth_header
     )
     assert response.status_code == 200
     assert response.json() != []
@@ -231,7 +233,7 @@ def test_get_invalid_diagnosis(
 
     monkeypatch.setattr(crud, "get", mock_get)
     response = test_app.get(
-        f"/query/?diagnosis={invalid_diagnosis}", headers=mock_auth_header
+        f"{ROUTE}?diagnosis={invalid_diagnosis}", headers=mock_auth_header
     )
     assert response.status_code == 422
 
@@ -249,7 +251,7 @@ def test_get_valid_iscontrol(
 
     monkeypatch.setattr(crud, "get", mock_successful_get)
     response = test_app.get(
-        f"/query/?is_control={valid_iscontrol}", headers=mock_auth_header
+        f"{ROUTE}?is_control={valid_iscontrol}", headers=mock_auth_header
     )
     assert response.status_code == 200
     assert response.json() != []
@@ -263,7 +265,7 @@ def test_get_invalid_iscontrol(
 
     monkeypatch.setattr(crud, "get", mock_get)
     response = test_app.get(
-        "/query/?is_control=apple", headers=mock_auth_header
+        f"{ROUTE}?is_control=apple", headers=mock_auth_header
     )
     assert response.status_code == 422
 
@@ -276,7 +278,7 @@ def test_get_invalid_control_diagnosis_pair(
 
     monkeypatch.setattr(crud, "get", mock_get)
     response = test_app.get(
-        "/query/?diagnosis=snomed:35489007&is_control=True",
+        f"{ROUTE}?diagnosis=snomed:35489007&is_control=True",
         headers=mock_auth_header,
     )
     assert response.status_code == 422
@@ -305,7 +307,7 @@ def test_get_valid_min_num_sessions(
 
     monkeypatch.setattr(crud, "get", mock_successful_get)
     response = test_app.get(
-        f"/query/?{session_param}={valid_min_num_sessions}",
+        f"{ROUTE}?{session_param}={valid_min_num_sessions}",
         headers=mock_auth_header,
     )
     assert response.status_code == 200
@@ -331,7 +333,7 @@ def test_get_invalid_min_num_sessions(
 
     monkeypatch.setattr(crud, "get", mock_get)
     response = test_app.get(
-        f"/query/?{session_param}={invalid_min_num_sessions}",
+        f"{ROUTE}?{session_param}={invalid_min_num_sessions}",
         headers=mock_auth_header,
     )
     response.status_code = 422
@@ -348,7 +350,7 @@ def test_get_valid_assessment(
 
     monkeypatch.setattr(crud, "get", mock_successful_get)
     response = test_app.get(
-        "/query/?assessment=nb:cogAtlas-1234", headers=mock_auth_header
+        f"{ROUTE}?assessment=nb:cogAtlas-1234", headers=mock_auth_header
     )
     assert response.status_code == 200
     assert response.json() != []
@@ -370,7 +372,7 @@ def test_get_invalid_assessment(
 
     monkeypatch.setattr(crud, "get", mock_get)
     response = test_app.get(
-        f"/query/?assessment={invalid_assessment}", headers=mock_auth_header
+        f"{ROUTE}?assessment={invalid_assessment}", headers=mock_auth_header
     )
     assert response.status_code == 422
 
@@ -397,7 +399,7 @@ def test_get_valid_available_image_modal(
 
     monkeypatch.setattr(crud, "get", mock_successful_get)
     response = test_app.get(
-        f"/query/?image_modal={valid_available_image_modal}",
+        f"{ROUTE}?image_modal={valid_available_image_modal}",
         headers=mock_auth_header,
     )
     assert response.status_code == 200
@@ -421,7 +423,7 @@ def test_get_valid_unavailable_image_modal(
 
     monkeypatch.setattr(crud, "get", mock_get)
     response = test_app.get(
-        f"/query/?image_modal={valid_unavailable_image_modal}",
+        f"{ROUTE}?image_modal={valid_unavailable_image_modal}",
         headers=mock_auth_header,
     )
 
@@ -445,7 +447,7 @@ def test_get_invalid_image_modal(
 
     monkeypatch.setattr(crud, "get", mock_get)
     response = test_app.get(
-        f"/query/?image_modal={invalid_image_modal}", headers=mock_auth_header
+        f"{ROUTE}?image_modal={invalid_image_modal}", headers=mock_auth_header
     )
     assert response.status_code == 422
 
@@ -469,7 +471,7 @@ def test_get_undefined_prefix_image_modal(
 
     monkeypatch.setattr(crud, "get", mock_get_with_exception)
     response = test_app.get(
-        f"/query/?image_modal={undefined_prefix_image_modal}",
+        f"{ROUTE}?image_modal={undefined_prefix_image_modal}",
         headers=mock_auth_header,
     )
     assert response.status_code == 500
@@ -491,7 +493,7 @@ def test_aggregate_query_response_structure(
         crud, "query_matching_dataset_sizes", mock_query_matching_dataset_sizes
     )
 
-    response = test_app.get("/query/", headers=mock_auth_header)
+    response = test_app.get(ROUTE, headers=mock_auth_header)
     assert all(
         dataset["subject_data"] == "protected" for dataset in response.json()
     )
@@ -507,5 +509,5 @@ def test_query_without_token_succeeds_when_auth_disabled(
     Test that when authentication is disabled, a request to the /query route without a token succeeds.
     """
     monkeypatch.setattr(crud, "get", mock_successful_get)
-    response = test_app.get("/query/")
+    response = test_app.get(ROUTE)
     assert response.status_code == 200

tests/test_routing.py

@@ -0,0 +1,45 @@
+import pytest
+
+from app.api import crud
+
+
+@pytest.mark.parametrize(
+    "root_path",
+    ["/", ""],
+)
+def test_root(test_app, root_path):
+    """Given a GET request to the root endpoint, Check for 200 status and expected content."""
+
+    response = test_app.get(root_path, follow_redirects=False)
+
+    assert response.status_code == 200
+    assert "Welcome to the Neurobagel REST API!" in response.text
+    assert '<a href="/docs">documentation</a>' in response.text
+
+
+@pytest.mark.parametrize(
+    "valid_route",
+    ["/query", "/query?min_age=20"],
+)
+def test_request_without_trailing_slash_not_redirected(
+    test_app, monkeypatch, mock_successful_get, disable_auth, valid_route
+):
+    """Test that a request to a route without a / is not redirected to have a trailing slash."""
+    monkeypatch.setattr(crud, "get", mock_successful_get)
+    response = test_app.get(valid_route, follow_redirects=False)
+    assert response.status_code == 200
+
+
+@pytest.mark.parametrize(
+    "invalid_route",
+    ["/query/", "/query/?min_age=20", "/attributes/nb:SomeClass/"],
+)
+def test_request_with_trailing_slash_not_redirected(
+    test_app, disable_auth, invalid_route
+):
+    """
+    Test that a request to routes including a trailing slash, where none is expected,
+    is *not* redirected to exclude the slash, and returns a 404.
+    """
+    response = test_app.get(invalid_route)
+    assert response.status_code == 404

tests/test_security.py

@@ -6,14 +6,13 @@
 
 @pytest.mark.filterwarnings("ignore:.*NB_API_ALLOWED_ORIGINS")
 def test_missing_client_id_raises_error_when_auth_enabled(
-    monkeypatch, test_app, set_test_credentials
+    monkeypatch, test_app, set_test_credentials, enable_auth
 ):
     """Test that a missing client ID raises an error on startup when authentication is enabled."""
     # We're using what should be default values of CLIENT_ID and AUTH_ENABLED here
     # (if the corresponding environment variables are unset),
     # but we set the values explicitly here for clarity
     monkeypatch.setattr("app.api.security.CLIENT_ID", None)
-    monkeypatch.setattr("app.api.security.AUTH_ENABLED", True)
 
     with pytest.raises(ValueError) as exc_info:
         with test_app:
@@ -52,17 +51,20 @@ def test_invalid_token_raises_error(invalid_token):
     [{}, {"Authorization": ""}, {"badheader": "badvalue"}],
 )
 def test_query_with_malformed_auth_header_fails(
-    test_app, set_mock_verify_token, invalid_auth_header, monkeypatch
+    test_app,
+    set_mock_verify_token,
+    enable_auth,
+    invalid_auth_header,
+    monkeypatch,
 ):
     """
-    Test that when authentication is enabled, a request to the /query route with
-    a missing or malformed authorization header fails.
+    Test that when authentication is enabled, a request to the /query route with a
+    missing or malformed authorization header fails.
     """
     monkeypatch.setattr("app.api.security.CLIENT_ID", "foo.id")
-    monkeypatch.setattr("app.api.security.AUTH_ENABLED", True)
 
     response = test_app.get(
-        "/query/",
+        "/query",
         headers=invalid_auth_header,
     )