diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 2b5d819c..17b11a31 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -90,3 +90,8 @@ **Vulnerability:** The `_safe_upload_filename` function used `filename.replace`, `PurePosixPath`, and `re.sub` on unbounded client input, making it vulnerable to ReDoS or CPU/memory exhaustion (DoS) when fed extremely long strings. **Learning:** Even fast standard library functions like `PurePosixPath` and string replacements can cause significant lag when chained on strings in the megabytes. String processing operations should always bound their inputs first if the input is untrusted and can be arbitrarily large. **Prevention:** Cap the length of client-provided filename strings early by slicing them (e.g. `filename = filename[-512:]`) before doing more complex string parsing or regex replacements, especially when only the basename suffix is relevant. + +## 2025-05-24 - Prevent Length-Extension Info Leak in compare_digest +**Vulnerability:** Comparing `bytes` of different lengths using `hmac.compare_digest` throws a `ValueError` which exposes information about token length or, if caught/ignored at the framework level, can lead to subtle side-channel leaks if not handled securely. +**Learning:** Python's `hmac.compare_digest` requires equal length strings or bytes to function safely without raising exceptions or leaking info. We must manually check for length equality, and if unequal, do a dummy comparison to maintain constant time behavior. +**Prevention:** If the provided credentials differ in length from the expected token, execute `hmac.compare_digest(credentials, credentials)` (a safe, constant-time dummy op without length normalization) and then return the unauthorized response. diff --git a/src/newsdom_api/config.py b/src/newsdom_api/config.py index 34bd05f9..bc537ef3 100644 --- a/src/newsdom_api/config.py +++ b/src/newsdom_api/config.py @@ -38,6 +38,7 @@ class RuntimeSettings: authentication_mode: AuthenticationMode = AuthenticationMode.REQUIRED runtime_profile: RuntimeProfile = RuntimeProfile.PRODUCTION api_token: str | None = field(default=None, repr=False) + api_token_digest: bytes | None = field(default=None, init=False, repr=False) def __post_init__(self) -> None: """Normalize secrets once and reject unsafe direct construction.""" @@ -59,7 +60,8 @@ def __post_init__(self) -> None: "The configured parser authentication token must not be blank" ) try: - bearer_value = f"Bearer {normalized_token}".encode("utf-8") + token_bytes = normalized_token.encode("utf-8") + bearer_value = b"Bearer " + token_bytes except UnicodeEncodeError as exc: raise RuntimeConfigurationError( "The configured parser authentication token must be valid UTF-8" @@ -69,6 +71,7 @@ def __post_init__(self) -> None: "The configured parser authentication token is too long" ) object.__setattr__(self, "api_token", normalized_token) + object.__setattr__(self, "api_token_digest", token_bytes) @property def authentication_ready(self) -> bool: @@ -76,7 +79,7 @@ def authentication_ready(self) -> bool: return ( self.authentication_mode is AuthenticationMode.DISABLED - or self.api_token is not None + or self.api_token_digest is not None ) diff --git a/src/newsdom_api/main.py b/src/newsdom_api/main.py index f61aafc2..39f62f5b 100644 --- a/src/newsdom_api/main.py +++ b/src/newsdom_api/main.py @@ -114,8 +114,8 @@ def _parse_access_failure(request: Request) -> JSONResponse | None: settings = _runtime_settings(request) if settings.authentication_mode is AuthenticationMode.DISABLED: return None - token = settings.api_token - if token is None: + expected_digest = settings.api_token_digest + if expected_digest is None: return JSONResponse( status_code=503, content={"detail": SERVICE_UNAVAILABLE_DETAIL}, @@ -131,7 +131,12 @@ def _parse_access_failure(request: Request) -> JSONResponse | None: scheme, separator, credentials = provided.partition(b" ") if separator != b" " or scheme.lower() != b"bearer" or not credentials: return _unauthorized_response() - if not hmac.compare_digest(credentials, token.encode("utf-8")): + + if len(credentials) != len(expected_digest): + hmac.compare_digest(credentials, credentials) + return _unauthorized_response() + + if not hmac.compare_digest(credentials, expected_digest): return _unauthorized_response() return None diff --git a/tests/test_auth_token_digest_boundary.py b/tests/test_auth_token_digest_boundary.py new file mode 100644 index 00000000..412ef7fe --- /dev/null +++ b/tests/test_auth_token_digest_boundary.py @@ -0,0 +1,63 @@ +"""Regression coverage for fixed-size authentication token comparison.""" + +from __future__ import annotations + +import hmac + +from fastapi.testclient import TestClient + +import newsdom_api.main as main_module +from newsdom_api.config import RuntimeSettings +from newsdom_api.main import create_app + + +def test_runtime_settings_precomputes_fixed_size_token_digest() -> None: + """The configured secret is normalized once outside the request path.""" + + settings = RuntimeSettings(api_token="configured-token") + + assert settings.api_token_digest == b"configured-token" + assert len(settings.api_token_digest or b"") == len(b"configured-token") + + +def test_mismatched_token_uses_equal_length_digest_operands(monkeypatch) -> None: + """Request comparison never invokes compare_digest with secret-dependent lengths.""" + + original_compare_digest = hmac.compare_digest + operands: list[tuple[bytes, bytes]] = [] + + def record_compare_digest(left: bytes, right: bytes) -> bool: + operands.append((left, right)) + return original_compare_digest(left, right) + + monkeypatch.setattr(main_module.hmac, "compare_digest", record_compare_digest) + client = TestClient(create_app(RuntimeSettings(api_token="configured-token"))) + + response = client.post( + "/parse", + headers={"Authorization": "Bearer x"}, + files={"file": ("test.pdf", b"%PDF-1.4\n", "application/pdf")}, + ) + + assert response.status_code == 401 + assert len(operands) == 1 + left, right = operands[0] + assert len(left) == len(right) == 1 + + +def test_valid_token_still_authenticates_after_digest_normalization(monkeypatch) -> None: + """Fixed-size comparison preserves exact configured-token acceptance.""" + + monkeypatch.setattr(main_module, "parse_pdf", lambda *args, **kwargs: {"nodes": []}) + settings = RuntimeSettings(api_token="configured-token") + client = TestClient(create_app(settings, runtime_readiness_probe=lambda: True)) + + response = client.post( + "/parse", + headers={"Authorization": "Bearer configured-token"}, + files={"file": ("test.pdf", b"not-used", "application/pdf")}, + ) + + # Authentication must not reject the exact token. The intentionally invalid + # PDF may fail later at the media boundary, which is sufficient for this test. + assert response.status_code != 401