diff --git a/docs/plans/20260704-1946-fix-illegal-user-name-header/plan.md b/docs/plans/20260704-1946-fix-illegal-user-name-header/plan.md new file mode 100644 index 0000000..ca9600e --- /dev/null +++ b/docs/plans/20260704-1946-fix-illegal-user-name-header/plan.md @@ -0,0 +1,294 @@ +# Fix Illegal `X-User-Name` Header Value on New-User Registration + +**Date:** 2026-07-04 +**Status:** Proposed +**Slug:** fix-illegal-user-name-header + +## Summary + +When an OIDC identity provider returns a `name` claim containing leading or +trailing whitespace (e.g. `"Matt "`) at registration time, every authenticated +proxied API request begins to fail with `502 API proxy error` and the log line +`API proxy error: Illegal header value b'Matt '`. The dirty value flows +unmodified from the IdP token, into the Starlette session, and is injected +verbatim as the `X-User-Name` request header by the web proxy layer; `httpx` +then rejects it under RFC 7230 (which forbids leading/trailing OWS and embedded +control characters in header field values). + +This plan normalizes the display name at ingress and adds a defensive guard at +the header-construction boundary so the proxy can never emit an illegal header +value regardless of where the data originated. The fix is safe because +`X-User-Name` is purely informational: its sole consumer seeds the non-unique +`UserProfile.name` display column on first profile creation, and identity / +authorization are keyed on `X-User-Id` (the OIDC `sub`) and `X-User-Roles` +respectively. + +## Background & Motivation + +### Reported incident + +A new user signed up via the IdP, then attempted to update their profile name. +The profile update (and in fact **all** subsequent authenticated API calls) +returned `502`, and the collector log recorded: + +``` +meshcore_hub.web.app - ERROR - API proxy error: Illegal header value b'Matt ' +``` + +### Root cause (verified by code trace) + +1. The IdP returned a `name` claim with a trailing space at registration. +2. `strip_userinfo()` in `src/meshcore_hub/web/oidc.py:58` copies the claim + verbatim into the session dict — no sanitization. +3. On every authenticated API call the web proxy injects the session name as a + request header at `src/meshcore_hub/web/app.py:751`: + `headers["X-User-Name"] = user["name"]`. +4. `httpx.AsyncClient.request(...)` enforces RFC 7230 and rejects the value, + raising an exception that is swallowed by the generic handler at + `src/meshcore_hub/web/app.py:793`, returning `502 {"detail": "API proxy error"}`. +5. The same dirty value is injected at the auth-callback bootstrap + (`src/meshcore_hub/web/app.py:1128`), so the user's `UserProfile` is never + seeded on first login either. + +### Safety analysis — `X-User-Name` is informational only + +A full sweep of every `request.headers.get(...)` call in `src/` confirms the +**only** reader of `X-User-Name` is +`src/meshcore_hub/api/profile_utils.py:38` inside `get_or_create_profile()`: + +```python +query = select(UserProfile).where(UserProfile.user_id == user_id) # lookup by sub +profile = session.execute(query).scalar_one_or_none() +if not profile: + idp_name = request.headers.get(X_USER_NAME_HEADER) or None # ONLY read + profile = UserProfile(user_id=user_id, name=idp_name) # seeds display name +``` + +Concretely: + +- The header is read **only** on the `if not profile:` branch — first-time + profile creation. +- It is assigned to `UserProfile.name`, a non-unique display column + (`src/meshcore_hub/common/models/user_profile.py:40`, no `unique=True`). +- It is **never** a lookup key, identity check, or authorization input. + Identity = `X-User-Id` (`src/meshcore_hub/api/auth.py:183`); + authorization = `X-User-Roles` (`auth.py:210,237`, + `channel_visibility.py:18`). + +Therefore normalizing (stripping) the value cannot affect identity, auth, or +uniqueness, and two users whose display names differ only by whitespace +collapsing to the same string is correct display-normalization behavior, not a +clash. + +### Relevant history + +The `X-User-Name` plumbing was introduced by the OIDC support plan +(`20260428-1300-oidc-oauth-support`) and the consumer helper by the members +refactor (`20260430-0805-members-refactor`). Neither sanitized the IdP-supplied +name; this plan closes that gap. + +## Goals + +- Eliminate the `Illegal header value` 502 for any user whose IdP `name` claim + contains leading/trailing whitespace or embedded RFC-illegal control + characters (CR/LF/NUL). +- Keep session-stored display names and seeded `UserProfile.name` values clean + at the ingress point. +- Guarantee the web proxy can never construct an illegal header value, + regardless of future data sources (defense in depth). +- Add regression coverage so the bug cannot silently return. +- Trim user-supplied names at the profile-update endpoint to prevent whitespace + from being saved through the editor. + +## Non-Goals + +- **Backfill of existing dirty rows.** Profiles already seeded with whitespace + names will continue to display with whitespace until the user edits them. A + one-time data migration is not in scope (see Open Questions). +- **Changing the IdP.** The IdP may legitimately emit trailing spaces; the hub + must tolerate this. +- **Renaming or repurposing** the `X-User-Name` header contract. + +## Requirements + +### Functional Requirements + +- FR-1: A user whose IdP `name` contains leading/trailing whitespace (e.g. + `"Matt "`) must be able to complete login, profile bootstrap, and any + authenticated API call without receiving a `502`. +- FR-2: The session-stored `name` and any newly seeded `UserProfile.name` must + have leading/trailing whitespace removed. +- FR-3: The forwarded `X-User-Name` header must be a valid RFC 7230 field value + (no leading/trailing OWS, no embedded CR/LF/NUL). +- FR-4: Existing authentication, authorization, identity, and uniqueness + semantics must be unchanged (no new collisions, no altered access control). + +### Technical Requirements + +- TR-1: Normalize in `strip_userinfo()` (`src/meshcore_hub/web/oidc.py`) so the + session dict never carries leading/trailing whitespace on `name`. +- TR-2: Add a private helper in `src/meshcore_hub/web/app.py`, e.g. + `_sanitize_header_value(value: str) -> str`, that strips leading/trailing + whitespace and removes all RFC 7230-forbidden CTL characters + (`0x00-0x1F` excluding HTAB/SP, plus DEL `0x7F`). Apply it at both + header-injection sites (`app.py:751` and `app.py:1128`). +- TR-3: Preserve `None`/empty semantics — an empty/whitespace-only name must + result in the `X-User-Name` header being omitted (existing + `if user.get("name")` guard retained). +- TR-4: No new dependencies. No DB schema change. No migration. +- TR-5: Tests added under `tests/test_web/` (and an OIDC unit test) following + existing fixture/mock patterns; full suite plus `pre-commit run --all-files` + must pass. + +## Implementation Plan + +### Phase 1: Ingress normalization + +- Edit `strip_userinfo()` in `src/meshcore_hub/web/oidc.py`: after resolving + `name` from the `name` / `preferred_username` / `username` / `nickname` + fallback chain, apply `name = name.strip() if isinstance(name, str) else name`. +- Rationale: fixes the reported case at its source; session data and the + resulting seeded `UserProfile.name` are clean from the first login. + +### Phase 1b: Profile-update input trimming + +- Edit the `update_profile()` endpoint in `src/meshcore_hub/api/routes/user_profiles.py` + (the `PUT` handler around line 227): if the request body includes a `name` + field with a `str` value, apply `.strip()` before assigning it to the model. +- Rationale: prevents users from accidentally or deliberately saving + whitespace-padded display names via the profile editor. Completes the + defense-in-depth coverage with minimal effort. + +### Phase 2: Header-boundary guard (defense in depth) + +- Add a module-level helper in `src/meshcore_hub/web/app.py`: + ```python + _ILLEGAL_HEADER_CHARS = "".join( + chr(c) for c in range(0x00, 0x20) if chr(c) not in "\t " + ) + "\x7f" + + def _sanitize_header_value(value: str) -> str: + # RFC 7230 § 3.2.6: field-value must not contain CTL (0x00-0x1F + # excluding HTAB 0x09 and SP 0x20, plus DEL 0x7F), and must not + # have leading/trailing OWS. + stripped = value.strip() + if stripped != value: + logger.debug("Stripped whitespace from header value %r -> %r", value, stripped) + if any(c in _ILLEGAL_HEADER_CHARS for c in stripped): + clean = "".join(c for c in stripped if c not in _ILLEGAL_HEADER_CHARS) + logger.debug("Dropped control chars from header value %r -> %r", stripped, clean) + return clean + return stripped + ``` +- Apply at `app.py:751` (API proxy): replace + `headers["X-User-Name"] = user["name"]` with: + ```python + sanitized = _sanitize_header_value(user["name"]) + if sanitized: + headers["X-User-Name"] = sanitized + ``` +- Apply at `app.py:1128` (auth-callback bootstrap): replace + `profile_headers["X-User-Name"] = session_user["name"]` with: + ```python + sanitized = _sanitize_header_value(session_user["name"]) + if sanitized: + profile_headers["X-User-Name"] = sanitized + ``` +- Nested guard (`if sanitized:`) prevents emitting an empty header value when + the input is whitespace-only (edge case where the `user.get("name")` outer + guard is truthy for a whitespace-only string). +- Debug-level logs surface malformed IdP data when `_sanitize_header_value` + actually alters a value, without spamming production logs. +- Rationale: even if a future code path or an IdP quirk reintroduces dirty data + into the session or elsewhere, the proxy cannot emit an illegal header. + +### Phase 3: Tests + +- **Proxy regression** (`tests/test_web/`): with `session["user"]["name"]` set to + `"Matt "`, assert a proxied API request returns a non-502 status and that the + forwarded request was sent with `X-User-Name == "Matt"`. Mock + `request.app.state.http_client` consistent with `tests/test_web/test_app.py`. +- **Bootstrap regression**: with a session name of `"Matt "`, assert the + auth-callback bootstrap request (`GET /api/v1/user/profile/me` from + `app.py:1128`) is forwarded with `X-User-Name == "Matt"`. Mock + `request.app.state.http_client` and verify the `.get()` call headers. +- **Control char edge cases**: test names containing embedded `DEL` (`\x7f`), + internal `CR`/`LF`/`NUL`, and tab (which should be preserved as RFC-allowed). + Trailing `"\r\n"` is forwarded cleanly with control characters dropped. +- **Whitespace-only name guard**: with `session["user"]["name"] = " "`, + assert `X-User-Name` header is omitted from the forwarded request (inner + guard prevents empty-string header value). +- **`strip_userinfo` unit test**: `strip_userinfo({"name": "Matt ", "sub": "x"}, + roles_claim)` returns `{"name": "Matt", ...}`; also cover the + `preferred_username` and `username` fallbacks being trimmed, `None` + passthrough for missing names, and leading+trailing whitespace both stripped. +- **Helper unit test**: `_sanitize_header_value("Matt \r\n") == "Matt"`; + `_sanitize_header_value("Ma\x7ftt") == "Matt"`; tab preserved; + `_sanitize_header_value(" ") == ""`. +- **Profile-update trim** (`tests/test_api/test_user_profiles.py`): + `PUT /user/profile/{id}` with body `{"name": " Matt "}` results in + the profile's `name` field being stored as `"Matt"` (no leading/trailing + whitespace). + +### Phase 4: Verification + +- Run targeted suites, then the full suite, then pre-commit: + ```bash + source .venv/bin/activate + pytest --no-cov tests/test_web/ tests/test_api/test_user_profiles.py + pytest -nauto --no-cov + pre-commit run --all-files + ``` +- Manual smoke (optional, in the compose stack): register a test IdP user whose + `name` claim carries trailing whitespace and confirm login + profile update + succeed without a 502. + +## Open Questions + +1. **~~Profile-update trimming.~~** Resolved: included in scope (Phase 1b). + `PUT /user/profile/{id}` will `.strip()` the user-supplied `name`. +2. **Backfill existing dirty rows.** Should a one-time Alembic/maintenance step + trim whitespace from already-seeded `UserProfile.name` values, or leave them + for users to self-correct via the profile editor? Default: **out of scope**. + +## Review + +**Status**: Approved with Changes + +**Reviewed**: 2026-07-04 + +### Resolutions + +- **RFC 7230 completeness** — `_sanitize_header_value` character filter + expanded from `\r\n\x00` to the full set of RFC-forbidden CTL chars + (`0x00-0x1F` excluding HTAB/SP, plus DEL `0x7F`). Tab is preserved (RFC + allows it). +- **Whitespace-only name edge case** — Both injection sites now have a nested + `if sanitized:` guard after sanitization, so a whitespace-only name + resolving to `""` correctly omits the header rather than emitting an empty + string value. +- **Observability** — `_sanitize_header_value` emits debug-level logs when it + strips whitespace or drops control characters. Non-altering calls are silent. +- **Bootstrap path test coverage** — Phase 3 tests now explicitly cover both + injection sites (`app.py:751` API proxy and `app.py:1128` auth-callback + bootstrap), plus control char edge cases and whitespace-only guard behavior. + +### Remaining Action Items + +- Decide on Open Question 2 (backfill) — does not gate this fix. + +## References + +- `docs/plans/20260428-1300-oidc-oauth-support/plan.md` — introduced + `strip_userinfo()` and the `X-User-*` proxy header contract. +- `docs/plans/20260430-0805-members-refactor/plan.md` — introduced + `get_or_create_profile()`, the sole consumer of `X-User-Name`. +- `docs/plans/20260428-1251-remove-header-auth/plan.md` — broader header-auth + context. +- Key source sites: + - `src/meshcore_hub/web/oidc.py:58` (`strip_userinfo`) + - `src/meshcore_hub/web/app.py:751`, `:1128` (header injection) + - `src/meshcore_hub/web/app.py:793` (generic 502 handler) + - `src/meshcore_hub/api/profile_utils.py:38` (sole `X-User-Name` reader) + - `src/meshcore_hub/common/models/user_profile.py:40` (non-unique `name`) + - `src/meshcore_hub/api/auth.py:17` (`X_USER_NAME_HEADER` constant) diff --git a/docs/plans/20260704-1946-fix-illegal-user-name-header/tasks.md b/docs/plans/20260704-1946-fix-illegal-user-name-header/tasks.md new file mode 100644 index 0000000..ab4f129 --- /dev/null +++ b/docs/plans/20260704-1946-fix-illegal-user-name-header/tasks.md @@ -0,0 +1,127 @@ +# Tasks: Fix Illegal `X-User-Name` Header Value on New-User Registration + +> Generated from `plan.md` on 2026-07-04 + +## Phase 1: Ingress Normalization + +- [x] Strip whitespace from IdP name claim in `strip_userinfo()` + - [x] In `src/meshcore_hub/web/oidc.py`, after the name fallback chain + (`name` / `preferred_username` / `username` / `nickname`), add + `name = name.strip() if isinstance(name, str) else name` + - [x] Verify `None` passthrough when no name claim exists + +## Phase 1b: Profile-Update Input Trimming + +- [x] Strip whitespace from user-supplied name in `update_profile()` + - [x] In `src/meshcore_hub/api/routes/user_profiles.py`, locate the + `PUT` handler body around line 227 + - [x] Where the request body `name` is assigned to the profile model, + apply `.strip()` before assignment (guard with `isinstance(name, str)`) + +## Phase 2: Header-Boundary Guard + +- [x] Add `_ILLEGAL_HEADER_CHARS` constant and `_sanitize_header_value()` helper + - [x] Add to `src/meshcore_hub/web/app.py` (module level, near the top + after imports and before the first endpoint / middleware definition): + ```python + _ILLEGAL_HEADER_CHARS = "".join( + chr(c) for c in range(0x00, 0x20) if chr(c) not in "\t " + ) + "\x7f" + + def _sanitize_header_value(value: str) -> str: + stripped = value.strip() + if stripped != value: + logger.debug("Stripped whitespace from header value %r -> %r", value, stripped) + if any(c in _ILLEGAL_HEADER_CHARS for c in stripped): + clean = "".join(c for c in stripped if c not in _ILLEGAL_HEADER_CHARS) + logger.debug("Dropped control chars from header value %r -> %r", stripped, clean) + return clean + return stripped + ``` + - [x] Verify `logger` is already available in scope (used throughout `app.py`) + +- [x] Apply sanitizer at API proxy injection site (`app.py:751`) + - [x] Replace `headers["X-User-Name"] = user["name"]` with: + ```python + sanitized = _sanitize_header_value(user["name"]) + if sanitized: + headers["X-User-Name"] = sanitized + ``` + - [x] Keep the existing outer `if user.get("name"):` guard unchanged + +- [x] Apply sanitizer at auth-callback bootstrap injection site (`app.py:1128`) + - [x] Replace `profile_headers["X-User-Name"] = session_user["name"]` with: + ```python + sanitized = _sanitize_header_value(session_user["name"]) + if sanitized: + profile_headers["X-User-Name"] = sanitized + ``` + - [x] Keep the existing outer `if session_user.get("name"):` guard unchanged + +## Phase 3: Tests + +- [x] Add `strip_userinfo` unit tests + - [x] In `tests/test_web/test_oidc.py` (or create if absent): test that + `strip_userinfo({"name": "Matt ", "sub": "x"}, roles_claim)` returns + `"Matt"` for name + - [x] Test `preferred_username` fallback with leading/trailing whitespace + - [x] Test `username` fallback with whitespace + - [x] Test `None` passthrough when no name-like claim exists + - [x] Test leading+trailing whitespace both stripped + - [x] Follow existing test patterns (pytest fixtures, mocks) + +- [x] Add `_sanitize_header_value` unit tests + - [x] In `tests/test_web/test_app.py`: test helper directly (import from + `src.meshcore_hub.web.app`) + - [x] `_sanitize_header_value("Matt \r\n") == "Matt"` (trailing CR/LF stripped) + - [x] `_sanitize_header_value("Ma\x7ftt") == "Matt"` (DEL stripped) + - [x] Tab character `"\t"` preserved (RFC-allowed) + - [x] `_sanitize_header_value(" ") == ""` (whitespace-only yields empty string) + - [x] `_sanitize_header_value("\x00foo\x00") == "foo"` (NUL stripped) + - [x] `_sanitize_header_value("clean") == "clean"` (no-op passthrough) + +- [x] Add proxy regression test (trailing whitespace name) + - [x] In `tests/test_web/test_app.py`: set `session["user"]["name"] = "Matt "` + - [x] Mock `request.app.state.http_client` following existing patterns + - [x] Assert proxied request returns non-502 status + - [x] Assert forwarded request has `X-User-Name == "Matt"` + +- [x] Add bootstrap regression test + - [x] With session name `"Matt "`, assert the auth-callback bootstrap + `GET /api/v1/user/profile/me` (from `app.py:1128`) is forwarded with + `X-User-Name == "Matt"` + - [x] Mock `request.app.state.http_client` and verify `.get()` call headers + +- [x] Add whitespace-only name guard test + - [x] With `session["user"]["name"] = " "`, assert `X-User-Name` header is + **omitted** from the forwarded request (inner `if sanitized:` guard) + +- [x] Add control char edge case tests + - [x] Name containing embedded DEL (`\x7f`) → forwarded cleanly + - [x] Name with internal CR/LF/NUL → forwarded with chars dropped + - [x] Name with tab (`\t`) → tab preserved in forwarded header + +- [x] Add profile-update trim test + - [x] In `tests/test_api/test_user_profiles.py`: make a `PUT` request to + `/user/profile/{id}` with body `{"name": " Matt "}` + - [x] Assert the profile's `name` is stored as `"Matt"` (no leading/trailing + whitespace) + +## Verification + +- [x] Run targeted test suites + - [x] `pytest --no-cov tests/test_web/ tests/test_api/test_user_profiles.py` + - [x] All tests pass (0 failures) + +- [x] Run full test suite + - [x] `pytest -nauto --no-cov` + - [x] All tests pass (0 failures) + +- [x] Run pre-commit checks + - [x] `pre-commit run --all-files` + - [x] All hooks pass (0 failures) + +- [ ] (Optional) Manual smoke test in compose stack + - [ ] Register a test IdP user whose `name` claim carries trailing whitespace + - [ ] Confirm login completes without 502 + - [ ] Confirm profile update succeeds without 502 diff --git a/src/meshcore_hub/api/routes/user_profiles.py b/src/meshcore_hub/api/routes/user_profiles.py index 057c4ea..4464de4 100644 --- a/src/meshcore_hub/api/routes/user_profiles.py +++ b/src/meshcore_hub/api/routes/user_profiles.py @@ -225,7 +225,7 @@ def update_profile( ) if profile_update.name is not None: - profile.name = profile_update.name + profile.name = profile_update.name.strip() update_data = profile_update.model_dump(exclude_unset=True, exclude={"name"}) for field, value in update_data.items(): diff --git a/src/meshcore_hub/web/app.py b/src/meshcore_hub/web/app.py index 94de64f..5bc70fd 100644 --- a/src/meshcore_hub/web/app.py +++ b/src/meshcore_hub/web/app.py @@ -38,6 +38,31 @@ PACKAGE_DIR = Path(__file__).parent TEMPLATES_DIR = PACKAGE_DIR / "templates" STATIC_DIR = PACKAGE_DIR / "static" +# RFC 7230 §3.2.6 forbidden field-value characters: CTL (0x00-0x1F) excluding +# HTAB (0x09) and SP (0x20), plus DEL (0x7F). These must never appear in a +# forwarded header value or httpx rejects the request. +_ILLEGAL_HEADER_CHARS = ( + "".join(chr(c) for c in range(0x00, 0x20) if chr(c) not in "\t ") + "\x7f" +) + + +def _sanitize_header_value(value: str) -> str: + """Sanitize a header value for RFC 7230 compliance. + + Strips leading/trailing OWS and removes any CTL/DEL characters that would + cause httpx to reject the request. Emits debug logs when altering a value. + """ + stripped = value.strip() + if stripped != value: + logger.debug("Stripped whitespace from header value %r -> %r", value, stripped) + if any(c in _ILLEGAL_HEADER_CHARS for c in stripped): + clean = "".join(c for c in stripped if c not in _ILLEGAL_HEADER_CHARS) + logger.debug( + "Dropped control chars from header value %r -> %r", stripped, clean + ) + return clean + return stripped + def _load_asset_manifest() -> dict[str, Any]: """Load the esbuild asset manifest from dist/assets.json. @@ -748,7 +773,9 @@ def create_app( if user and user.get("sub"): headers["X-User-Id"] = user["sub"] if user.get("name"): - headers["X-User-Name"] = user["name"] + sanitized = _sanitize_header_value(user["name"]) + if sanitized: + headers["X-User-Name"] = sanitized roles = get_session_roles(request, roles_claim) if roles: headers["X-User-Roles"] = ",".join(roles) @@ -1125,7 +1152,9 @@ def create_app( "X-User-Roles": ",".join(session_user.get("roles", [])), } if session_user.get("name"): - profile_headers["X-User-Name"] = session_user["name"] + sanitized = _sanitize_header_value(session_user["name"]) + if sanitized: + profile_headers["X-User-Name"] = sanitized await request.app.state.http_client.get( "/api/v1/user/profile/me", headers=profile_headers ) diff --git a/src/meshcore_hub/web/oidc.py b/src/meshcore_hub/web/oidc.py index 652c986..1fca05c 100644 --- a/src/meshcore_hub/web/oidc.py +++ b/src/meshcore_hub/web/oidc.py @@ -63,6 +63,7 @@ def strip_userinfo(userinfo: dict[str, Any], roles_claim: str) -> dict[str, Any] or userinfo.get("username") or userinfo.get("nickname") ) + name = name.strip() if isinstance(name, str) else name return { "sub": userinfo.get("sub"), "name": name, diff --git a/tests/test_api/test_user_profiles.py b/tests/test_api/test_user_profiles.py index 93e4153..65cd10a 100644 --- a/tests/test_api/test_user_profiles.py +++ b/tests/test_api/test_user_profiles.py @@ -252,6 +252,19 @@ class TestUpdateProfile: assert data["name"] == "New Name" assert data["callsign"] == sample_user_profile.callsign + def test_update_profile_name_trims_whitespace( + self, client_no_auth, sample_user_profile + ): + """Leading/trailing whitespace on a user-supplied name is stripped.""" + response = client_no_auth.put( + f"/api/v1/user/profile/{sample_user_profile.id}", + json={"name": " Matt "}, + headers=USER_HEADERS, + ) + assert response.status_code == 200 + data = response.json() + assert data["name"] == "Matt" + def test_update_profile_callsign(self, client_no_auth, sample_user_profile): """Test updating profile callsign.""" response = client_no_auth.put( diff --git a/tests/test_web/conftest.py b/tests/test_web/conftest.py index a1bdc52..6e340cd 100644 --- a/tests/test_web/conftest.py +++ b/tests/test_web/conftest.py @@ -31,6 +31,10 @@ class MockHttpClient: # Records the params forwarded by the most recent request() call so # tests can assert how the proxy forwards query parameters. self.last_request_params: Any = None + # Records the headers forwarded by the most recent request() call. + self.last_request_headers: dict[str, Any] | None = None + # Records the headers forwarded by the most recent get() call. + self.last_get_headers: dict[str, Any] | None = None self._default_responses() def _default_responses(self) -> None: @@ -269,6 +273,7 @@ class MockHttpClient: ) -> Response: """Mock generic request (used by API proxy).""" self.last_request_params = params + self.last_request_headers = headers key = f"{method.upper()}:{url}" if key in self._responses: return self._create_response(key) @@ -277,8 +282,14 @@ class MockHttpClient: key = f"{method.upper()}:{base_path}" return self._create_response(key) - async def get(self, path: str, params: dict | None = None) -> Response: + async def get( + self, + path: str, + params: dict | None = None, + headers: dict | None = None, + ) -> Response: """Mock GET request.""" + self.last_get_headers = headers # Try exact match first key = f"GET:{path}" if key in self._responses: diff --git a/tests/test_web/test_app.py b/tests/test_web/test_app.py index fb84378..0fddb1d 100644 --- a/tests/test_web/test_app.py +++ b/tests/test_web/test_app.py @@ -2,6 +2,7 @@ import json from typing import Any +from unittest.mock import AsyncMock, patch import pytest from fastapi.testclient import TestClient @@ -11,6 +12,7 @@ from meshcore_hub.web.app import ( _OPEN, _build_config_json, _build_endpoint_access, + _sanitize_header_value, check_api_access, create_app, ) @@ -591,3 +593,139 @@ class TestRolelessUserProfileUpdate: json={"callsign": "NR1"}, ) assert response.status_code == 200 + + +class TestSanitizeHeaderValue: + """Unit tests for the _sanitize_header_value RFC 7230 guard.""" + + def test_strips_trailing_whitespace(self) -> None: + assert _sanitize_header_value("Matt ") == "Matt" + + def test_strips_leading_and_trailing_whitespace(self) -> None: + assert _sanitize_header_value(" Matt ") == "Matt" + + def test_strips_trailing_crlf(self) -> None: + assert _sanitize_header_value("Matt\r\n") == "Matt" + + def test_strips_embedded_del(self) -> None: + assert _sanitize_header_value("Ma\x7ftt") == "Matt" + + def test_strips_embedded_nul(self) -> None: + assert _sanitize_header_value("\x00foo\x00") == "foo" + + def test_strips_embedded_cr_and_lf(self) -> None: + assert _sanitize_header_value("Ma\r\ntt") == "Matt" + + def test_whitespace_only_yields_empty(self) -> None: + assert _sanitize_header_value(" ") == "" + + def test_tab_preserved(self) -> None: + # HTAB (0x09) is allowed by RFC 7230 and must survive sanitization. + assert _sanitize_header_value("Ma\ttt") == "Ma\ttt" + + def test_clean_value_passthrough(self) -> None: + assert _sanitize_header_value("clean") == "clean" + + def test_strips_all_ctl_chars(self) -> None: + # Every CTL char 0x00-0x1F except HTAB/SP, plus DEL, is removed. + removed = [chr(c) for c in range(0x00, 0x20) if chr(c) not in "\t "] + dirty = "x" + "".join(removed) + "\x7f" + "y" + assert _sanitize_header_value(dirty) == "xy" + + +class TestProxyHeaderSanitization: + """The API proxy must sanitize X-User-Name before forwarding (regression).""" + + def test_trailing_whitespace_name_forwarded_clean( + self, + client_with_oidc: TestClient, + mock_http_client: MockHttpClient, + ) -> None: + """Reported bug: name='Matt ' caused 502; must now forward 'Matt'.""" + dirty_user = {"sub": "user-1", "name": "Matt ", "roles": ["member"]} + with ( + patch("meshcore_hub.web.app.get_session_user", return_value=dirty_user), + patch("meshcore_hub.web.oidc.get_session_user", return_value=dirty_user), + ): + response = client_with_oidc.get("/api/v1/nodes") + + assert response.status_code != 502 + forwarded = mock_http_client.last_request_headers + assert forwarded is not None + assert forwarded["X-User-Name"] == "Matt" + + def test_whitespace_only_name_omits_header( + self, + client_with_oidc: TestClient, + mock_http_client: MockHttpClient, + ) -> None: + """A whitespace-only name must NOT emit an empty X-User-Name header.""" + ws_user = {"sub": "user-1", "name": " ", "roles": ["member"]} + with ( + patch("meshcore_hub.web.app.get_session_user", return_value=ws_user), + patch("meshcore_hub.web.oidc.get_session_user", return_value=ws_user), + ): + response = client_with_oidc.get("/api/v1/nodes") + + assert response.status_code != 502 + forwarded = mock_http_client.last_request_headers + assert forwarded is not None + assert "X-User-Name" not in forwarded + + def test_control_char_name_forwarded_clean( + self, + client_with_oidc: TestClient, + mock_http_client: MockHttpClient, + ) -> None: + """Embedded DEL/CR/LF in the name must be dropped before forwarding.""" + dirty_user = {"sub": "user-1", "name": "Ma\x7f\r\ntt", "roles": ["member"]} + with ( + patch("meshcore_hub.web.app.get_session_user", return_value=dirty_user), + patch("meshcore_hub.web.oidc.get_session_user", return_value=dirty_user), + ): + response = client_with_oidc.get("/api/v1/nodes") + + assert response.status_code != 502 + forwarded = mock_http_client.last_request_headers + assert forwarded is not None + assert forwarded["X-User-Name"] == "Matt" + + +class TestBootstrapHeaderSanitization: + """The auth-callback bootstrap must forward a sanitized X-User-Name.""" + + def test_trailing_whitespace_stripped_on_bootstrap( + self, + client_with_oidc: TestClient, + mock_http_client: MockHttpClient, + ) -> None: + """Bootstrap GET forwards clean name after strip_userinfo trims it.""" + token = {"userinfo": {"sub": "user-1", "name": "Matt "}} + with patch( + "meshcore_hub.web.app.oauth.oidc.authorize_access_token", + new_callable=AsyncMock, + return_value=token, + ): + client_with_oidc.get("/auth/callback", follow_redirects=False) + + forwarded = mock_http_client.last_get_headers + assert forwarded is not None + assert forwarded["X-User-Name"] == "Matt" + + def test_control_char_dropped_on_bootstrap( + self, + client_with_oidc: TestClient, + mock_http_client: MockHttpClient, + ) -> None: + """Defense-in-depth: DEL survives strip_userinfo but is removed at header.""" + token = {"userinfo": {"sub": "user-1", "name": "Ma\x7ftt"}} + with patch( + "meshcore_hub.web.app.oauth.oidc.authorize_access_token", + new_callable=AsyncMock, + return_value=token, + ): + client_with_oidc.get("/auth/callback", follow_redirects=False) + + forwarded = mock_http_client.last_get_headers + assert forwarded is not None + assert forwarded["X-User-Name"] == "Matt" diff --git a/tests/test_web/test_oidc.py b/tests/test_web/test_oidc.py index dbba928..9d90cd8 100644 --- a/tests/test_web/test_oidc.py +++ b/tests/test_web/test_oidc.py @@ -332,6 +332,36 @@ class TestStripUserinfo: result = strip_userinfo(userinfo, "roles") assert result["name"] is None + def test_name_trailing_whitespace_stripped(self) -> None: + """Test trailing whitespace is stripped from the name claim.""" + userinfo = {"sub": "user-1", "name": "Matt "} + result = strip_userinfo(userinfo, "roles") + assert result["name"] == "Matt" + + def test_name_leading_and_trailing_whitespace_stripped(self) -> None: + """Test both leading and trailing whitespace are stripped.""" + userinfo = {"sub": "user-1", "name": " Matt "} + result = strip_userinfo(userinfo, "roles") + assert result["name"] == "Matt" + + def test_preferred_username_whitespace_stripped(self) -> None: + """Test whitespace is stripped from the preferred_username fallback.""" + userinfo = {"sub": "user-1", "preferred_username": " johndoe "} + result = strip_userinfo(userinfo, "roles") + assert result["name"] == "johndoe" + + def test_username_whitespace_stripped(self) -> None: + """Test whitespace is stripped from the username fallback.""" + userinfo = {"sub": "user-1", "username": "\tjohndoe\t"} + result = strip_userinfo(userinfo, "roles") + assert result["name"] == "johndoe" + + def test_name_none_passthrough_not_stripped(self) -> None: + """Test that a missing name remains None (no AttributeError).""" + userinfo = {"sub": "user-1"} + result = strip_userinfo(userinfo, "roles") + assert result["name"] is None + def test_roles_extracted(self) -> None: """Test roles are extracted from configured claim.""" userinfo = {"sub": "user-1", "custom_roles": ["admin", "member"]}