fix: user-info lookups leave out client headers that break them

.github/workflows/tests-deploy-docker.yml

@@ -549,6 +549,11 @@ jobs:
           test_req admin "-u usr:password" 403
           test_req admin "-u admin:admin" 200
 
+          # A chunked request with credentials, sent to HaRP directly because the nginx in front buffers bodies
+          code=$(printf 'body' | curl -sS -m 10 -o /dev/null -w "%{http_code}" -u usr:password -X GET \
+            -H "Transfer-Encoding: chunked" --data-binary @- http://127.0.0.1:8780/exapps/app-skeleton-python/user)
+          test "$code" = 200
+
           # clients get blacklisted for 5 bad requests for 5 minutes
           sleep 301
 

haproxy_agent.py

@@ -98,7 +98,24 @@ def _in_cluster_api_server() -> str:
 NC_REQ_URL = NC_INSTANCE_URL.removesuffix("/").removesuffix("/index.php")
 EX_APP_URL = f"{NC_REQ_URL}/index.php/apps/app_api/harp/exapp-meta"
 USER_INFO_URL = f"{NC_REQ_URL}/index.php/apps/app_api/harp/user-info"
-EXCLUDE_HEADERS_USER_INFO = {"host", "content-length"}
+# Client headers the user-info lookup leaves out, as it does every content-* header
+EXCLUDE_HEADERS_USER_INFO = {
+    "accept-encoding",
+    "connection",
+    "expect",
+    "host",
+    "keep-alive",
+    "proxy-authenticate",
+    "proxy-authorization",
+    "proxy-connection",
+    "range",
+    "te",
+    "trailer",
+    "transfer-encoding",
+    "upgrade",
+}
+# aiohttp refuses to send a header that contains one of these
+UNSENDABLE_HEADER_CHARS = re.compile(r"[\x00-\x08\x0a-\x1f\x7f]")
 
 # Keep total below the SPOE `timeout processing` (30s); aiohttp's default is 5 minutes.
 NC_HTTP_TIMEOUT = aiohttp.ClientTimeout(total=25.0, connect=5.0)
@@ -1033,7 +1050,13 @@ async def _get_or_fetch_exapp(exapp_id: str) -> ExApp | None:
 
 
 async def nc_get_user(app_id: str, all_headers: dict[str, str]) -> NcUser | None:
-    ext_headers = {k: v for k, v in all_headers.items() if k.lower() not in EXCLUDE_HEADERS_USER_INFO}
+    ext_headers = {
+        k: v
+        for k, v in all_headers.items()
+        if k.lower() not in EXCLUDE_HEADERS_USER_INFO
+        and not k.lower().startswith("content-")
+        and not UNSENDABLE_HEADER_CHARS.search(k + v)
+    }
     log_headers("Requesting user info for ExApp '%s' with headers %s", ext_headers, app_id)
     async with _get_nc_session().get(
         USER_INFO_URL,

tests/unit/test_requests.py

@@ -339,7 +339,9 @@ def test_k8s_create_refuses_an_invalid_role_before_contacting_the_cluster(no_doc
     assert text.startswith("Payload validation error: role_suffix: ")
 
 
-def test_user_info_requests_keep_the_credentials_but_the_log_does_not(monkeypatch, caplog):
+@pytest.fixture
+def user_info_sent(monkeypatch) -> dict:
+    """Answer user-info lookups with the admin user; the returned dict gets the headers the lookup sent."""
     sent = {}
 
     class Response:
@@ -360,17 +362,59 @@ def get(self, url, headers, params):
             return Response()
 
     monkeypatch.setattr(agent, "_get_nc_session", Session)
+    return sent
+
+
+def test_user_info_requests_keep_the_credentials_but_the_log_does_not(user_info_sent, caplog):
     headers = {"authorization": "Basic YWRtaW46YWRtaW4=", "cookie": "oc_sessionPassphrase=passphrase-value"}
     with caplog.at_level(logging.DEBUG, logger=agent.LOGGER.name):
         user = asyncio.run(agent.nc_get_user("app", headers))
     assert user.user_id == "admin"
-    assert sent["authorization"] == "Basic YWRtaW46YWRtaW4="  # Nextcloud still gets them
-    assert sent["cookie"] == "oc_sessionPassphrase=passphrase-value"
+    assert user_info_sent["authorization"] == "Basic YWRtaW46YWRtaW4="  # Nextcloud still gets them
+    assert user_info_sent["cookie"] == "oc_sessionPassphrase=passphrase-value"
     assert "Requesting user info for ExApp 'app'" in caplog.text
     assert "YWRtaW46YWRtaW4=" not in caplog.text
     assert "passphrase-value" not in caplog.text
 
 
+def test_user_info_requests_leave_out_the_client_headers_that_break_them(user_info_sent):
+    headers = agent.parse_headers(
+        "\n".join(
+            [
+                "Host: cloud.example.com",
+                "Authorization: Basic YWRtaW46YWRtaW4=",
+                "Cookie: oc_sessionPassphrase=passphrase-value",
+                "User-Agent: curl/8.22.0",
+                "X-Forwarded-For: 198.51.100.7",
+                "Transfer-Encoding: chunked",
+                "Expect: 100-continue",
+                "Content-Type: application/octet-stream",
+                "Connection: keep-alive, Upgrade",
+                "Keep-Alive: timeout=5",
+                "Upgrade: websocket",
+                "TE: trailers",
+                "Accept-Encoding: gzip, deflate, br, zstd",
+                "Proxy-Authorization: Basic cHJveHk6c2VjcmV0",
+                "Range: bytes=1000-",
+            ]
+        )
+    )
+    assert asyncio.run(agent.nc_get_user("app", headers)).user_id == "admin"
+    assert user_info_sent == {
+        "authorization": "Basic YWRtaW46YWRtaW4=",
+        "cookie": "oc_sessionPassphrase=passphrase-value",
+        "user-agent": "curl/8.22.0",
+        "x-forwarded-for": "198.51.100.7",
+        "harp-shared-key": KEY,
+    }
+
+
+def test_user_info_requests_leave_out_a_header_with_a_control_character(user_info_sent):
+    headers = {"authorization": "Basic YWRtaW46YWRtaW4=", "x-custom": "a\x01b"}
+    assert asyncio.run(agent.nc_get_user("app", headers)).user_id == "admin"
+    assert user_info_sent == {"authorization": "Basic YWRtaW46YWRtaW4=", "harp-shared-key": KEY}
+
+
 def test_create_refuses_a_read_write_system_mount_once_enforced(no_docker_engine, monkeypatch):
     # By default the read-write entries only warn (see test_bind_mounts.py); HP_EXAPP_BIND_MOUNTS_ENFORCE_ALL
     # empties the warn-only set.