From 7b44049e6e6ea30348c8860d3c9f5f4a4e948460 Mon Sep 17 00:00:00 2001 From: Louis King Date: Sat, 4 Jul 2026 20:06:04 +0100 Subject: [PATCH] fix(web): sanitize X-User-Name header to prevent 502 on registration When an IdP name claim contains trailing whitespace (e.g. 'Matt '), every proxied API request failed with 502 'Illegal header value' because httpx enforces RFC 7230 which forbids leading/trailing OWS in header values. Defense in depth: - strip_userinfo() trims the IdP name at ingress (oidc.py) - _sanitize_header_value() removes CTL/DEL chars at both header-injection sites (API proxy + auth-callback bootstrap) in app.py - update_profile() trims user-supplied names in PUT handler X-User-Name is informational only (seeds non-unique UserProfile.name); identity/auth are keyed on X-User-Id and X-User-Roles, so sanitization is safe. --- .../plan.md | 294 ++++++++++++++++++ .../tasks.md | 127 ++++++++ src/meshcore_hub/api/routes/user_profiles.py | 2 +- src/meshcore_hub/web/app.py | 33 +- src/meshcore_hub/web/oidc.py | 1 + tests/test_api/test_user_profiles.py | 13 + tests/test_web/conftest.py | 13 +- tests/test_web/test_app.py | 138 ++++++++ tests/test_web/test_oidc.py | 30 ++ 9 files changed, 647 insertions(+), 4 deletions(-) create mode 100644 docs/plans/20260704-1946-fix-illegal-user-name-header/plan.md create mode 100644 docs/plans/20260704-1946-fix-illegal-user-name-header/tasks.md 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"]}