From 20dd858b70400e69380dba9a47c4d637937f3b0e Mon Sep 17 00:00:00 2001 From: MemOS AutoDev Date: Mon, 17 Aug 2026 06:49:49 +0800 Subject: [PATCH 1/2] fix(api): close is_internal_request fail-open (#2259) `is_internal_request()` previously compared the incoming `X-Internal-Service` header value directly to `os.getenv("INTERNAL_SERVICE_SECRET")`. In every shipped deployment the env var is unset, so both operands were `None` for a normal external request and `None == None` evaluated to `True`. Any anonymous external caller was authorised as the `internal` principal with `scopes: ["all"]`, satisfying `require_scope("admin")` on every `/admin/*` route (create keys, list keys, revoke keys, generate master key). Fix per GHSA-9pw6-vmgx-qgwx: - Read the secret into a local variable, return False when either the secret or the header is missing / empty (fail closed). - Compare with `hmac.compare_digest` for constant-time equality. - `INTERNAL_SERVICE_IPS` allowlist branch is unchanged. Add `tests/api/test_auth_internal_request.py` (12 cases) covering the regression, the empty-string defence, the timing-safe compare, and an end-to-end reproduction of the advisory PoC against `verify_api_key`. --- src/memos/api/middleware/auth.py | 26 +++- tests/api/test_auth_internal_request.py | 176 ++++++++++++++++++++++++ 2 files changed, 199 insertions(+), 3 deletions(-) create mode 100644 tests/api/test_auth_internal_request.py diff --git a/src/memos/api/middleware/auth.py b/src/memos/api/middleware/auth.py index 15b217651..79b497334 100644 --- a/src/memos/api/middleware/auth.py +++ b/src/memos/api/middleware/auth.py @@ -6,6 +6,7 @@ """ import hashlib +import hmac import os import time @@ -142,16 +143,35 @@ async def lookup_api_key(key_hash: str) -> dict[str, Any] | None: def is_internal_request(request: Request) -> bool: - """Check if request is from internal service.""" + """Check if request is from internal service. + + Two authorised paths: + + 1. The client host address is a well-known trusted host in + ``INTERNAL_SERVICE_IPS``. + 2. The request carries an ``X-Internal-Service`` header whose value matches + the operator-configured ``INTERNAL_SERVICE_SECRET`` env var. Comparison + is constant-time (``hmac.compare_digest``) to avoid timing side channels. + + The header branch is **disabled** whenever either the environment secret or + the request header is missing or empty. This closes GHSA-9pw6-vmgx-qgwx / + issue #2259, where an unset secret combined with an absent header caused + ``None == None`` to be treated as a match and grant the ``internal`` + principal to unauthenticated remote callers. + """ client_host = request.client.host if request.client else None # Check internal IPs if client_host in INTERNAL_SERVICE_IPS: return True - # Check internal header (for container-to-container) + # Check internal header (for container-to-container). Treat an unset / + # empty secret or missing / empty header as "disabled" and fail closed. + secret = os.getenv("INTERNAL_SERVICE_SECRET") internal_header = request.headers.get("X-Internal-Service") - return internal_header == os.getenv("INTERNAL_SERVICE_SECRET") + if not secret or not internal_header: + return False + return hmac.compare_digest(internal_header, secret) async def verify_api_key( diff --git a/tests/api/test_auth_internal_request.py b/tests/api/test_auth_internal_request.py new file mode 100644 index 000000000..676c57fdf --- /dev/null +++ b/tests/api/test_auth_internal_request.py @@ -0,0 +1,176 @@ +"""Tests for `memos.api.middleware.auth.is_internal_request` fail-open regression. + +Regression test suite for GHSA-9pw6-vmgx-qgwx / issue #2259. + +The bug: when `INTERNAL_SERVICE_SECRET` is unset (its default across every shipped +Dockerfile / compose / Helm config) and an external client does not send the +`X-Internal-Service` header, `is_internal_request()` compared +``request.headers.get("X-Internal-Service")`` to ``os.getenv("INTERNAL_SERVICE_SECRET")`` +directly, so ``None == None`` returned True and the request was granted the +`internal` principal with ``scopes: ["all"]``. + +These tests lock in the fixed behaviour: unset secret OR missing header → not internal; +matching non-empty secret + header → internal (constant-time compare). +""" + +from __future__ import annotations + +import asyncio + +from typing import Any + +import pytest + +from starlette.requests import Request + +from memos.api.middleware import auth as auth_module +from memos.api.middleware.auth import is_internal_request, verify_api_key + + +def _make_request( + *, + client: tuple[str, int] | None = ("203.0.113.9", 53124), + headers: dict[str, str] | None = None, +) -> Request: + """Build a bare-bones Starlette Request for the dependency under test.""" + scope: dict[str, Any] = { + "type": "http", + "method": "GET", + "path": "/admin/keys", + "headers": [(k.lower().encode(), v.encode()) for k, v in (headers or {}).items()], + } + if client is not None: + scope["client"] = client + return Request(scope) + + +class TestIsInternalRequest: + def test_returns_false_when_secret_unset_and_header_missing( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Regression: None == None must not authenticate.""" + monkeypatch.delenv("INTERNAL_SERVICE_SECRET", raising=False) + request = _make_request(headers={}) + + assert is_internal_request(request) is False + + def test_returns_false_when_secret_unset_and_header_present( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Header path is disabled when secret is not configured.""" + monkeypatch.delenv("INTERNAL_SERVICE_SECRET", raising=False) + request = _make_request(headers={"X-Internal-Service": "guessed-value"}) + + assert is_internal_request(request) is False + + def test_returns_false_when_secret_empty_string(self, monkeypatch: pytest.MonkeyPatch) -> None: + """Empty string secret is treated as unset (defence in depth).""" + monkeypatch.setenv("INTERNAL_SERVICE_SECRET", "") + request = _make_request(headers={"X-Internal-Service": ""}) + + assert is_internal_request(request) is False + + def test_returns_false_when_secret_set_but_header_missing( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setenv("INTERNAL_SERVICE_SECRET", "super-secret-value") + request = _make_request(headers={}) + + assert is_internal_request(request) is False + + def test_returns_false_when_secret_set_but_header_wrong( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setenv("INTERNAL_SERVICE_SECRET", "super-secret-value") + request = _make_request(headers={"X-Internal-Service": "wrong-value"}) + + assert is_internal_request(request) is False + + def test_returns_true_when_secret_matches_header(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("INTERNAL_SERVICE_SECRET", "super-secret-value") + request = _make_request(headers={"X-Internal-Service": "super-secret-value"}) + + assert is_internal_request(request) is True + + def test_returns_true_when_client_ip_is_internal(self, monkeypatch: pytest.MonkeyPatch) -> None: + """The internal-IP branch remains untouched by the fix.""" + monkeypatch.delenv("INTERNAL_SERVICE_SECRET", raising=False) + request = _make_request(client=("127.0.0.1", 12345), headers={}) + + assert is_internal_request(request) is True + + def test_uses_constant_time_compare(self, monkeypatch: pytest.MonkeyPatch) -> None: + """The fix must call `hmac.compare_digest` (not plain ==) to avoid timing leaks.""" + monkeypatch.setenv("INTERNAL_SERVICE_SECRET", "super-secret-value") + calls: list[tuple[str, str]] = [] + + real_compare = auth_module.hmac.compare_digest + + def spy(a: str, b: str) -> bool: + calls.append((a, b)) + return real_compare(a, b) + + monkeypatch.setattr(auth_module.hmac, "compare_digest", spy) + request = _make_request(headers={"X-Internal-Service": "super-secret-value"}) + + assert is_internal_request(request) is True + assert calls, "expected hmac.compare_digest to be invoked for header comparison" + + def test_no_client_and_no_header_and_no_secret(self, monkeypatch: pytest.MonkeyPatch) -> None: + """Missing request.client + no header + no secret must NOT authenticate.""" + monkeypatch.delenv("INTERNAL_SERVICE_SECRET", raising=False) + request = _make_request(client=None, headers={}) + + assert is_internal_request(request) is False + + +class TestVerifyApiKeyEndToEnd: + """End-to-end reproduction of the advisory PoC against `verify_api_key`.""" + + def test_external_request_no_key_is_rejected_when_secret_unset( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """The exact PoC from GHSA-9pw6-vmgx-qgwx must now be rejected with 401.""" + monkeypatch.setattr(auth_module, "AUTH_ENABLED", True) + monkeypatch.delenv("INTERNAL_SERVICE_SECRET", raising=False) + request = _make_request(headers={}) + + from fastapi import HTTPException + + with pytest.raises(HTTPException) as exc_info: + asyncio.run(verify_api_key(request, api_key=None)) + + assert exc_info.value.status_code == 401 + assert "Missing API key" in exc_info.value.detail + + def test_external_request_with_wrong_header_still_rejected( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setattr(auth_module, "AUTH_ENABLED", True) + monkeypatch.delenv("INTERNAL_SERVICE_SECRET", raising=False) + request = _make_request(headers={"X-Internal-Service": "guess"}) + + from fastapi import HTTPException + + with pytest.raises(HTTPException) as exc_info: + asyncio.run(verify_api_key(request, api_key=None)) + + assert exc_info.value.status_code == 401 + + def test_internal_request_with_matching_secret_is_authorised( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setattr(auth_module, "AUTH_ENABLED", True) + monkeypatch.setenv("INTERNAL_SERVICE_SECRET", "shared-secret") + request = _make_request( + headers={"X-Internal-Service": "shared-secret"}, + client=("10.0.0.42", 40000), + ) + + result = asyncio.run(verify_api_key(request, api_key=None)) + assert result == { + "user_name": "internal", + "scopes": ["all"], + "is_master_key": False, + "is_internal": True, + } From 40ea871dd65872551687ac6be78630a31329e44e Mon Sep 17 00:00:00 2001 From: MemOS AutoDev Bot Date: Mon, 17 Aug 2026 07:04:57 +0800 Subject: [PATCH 2/2] fix(api): address OCR review feedback on auth fail-open patch - Guard `request.client` on the internal-header path: fallback to `` for the debug log so header-authenticated requests without a client tuple don't crash with AttributeError (OCR finding #1). - Hoist `INTERNAL_SERVICE_SECRET` to module level next to `AUTH_ENABLED` and `MASTER_KEY_HASH`, avoiding a per-request `os.getenv` on the hot auth path (OCR finding #2). - Rewrite the auth regression tests: move `HTTPException` to the top-level imports (OCR finding #3), convert `asyncio.run(...)` callers to `@pytest.mark.asyncio async def` so tests stay safe when a running event loop is already installed by pytest plugins (OCR finding #4), and patch `INTERNAL_SERVICE_SECRET` via `monkeypatch.setattr` on the module now that it is module-level. - Add a regression test locking in the header-authenticated / request-without-client path. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/memos/api/middleware/auth.py | 6 ++- tests/api/test_auth_internal_request.py | 67 ++++++++++++++++--------- 2 files changed, 47 insertions(+), 26 deletions(-) diff --git a/src/memos/api/middleware/auth.py b/src/memos/api/middleware/auth.py index 79b497334..125d17278 100644 --- a/src/memos/api/middleware/auth.py +++ b/src/memos/api/middleware/auth.py @@ -26,6 +26,7 @@ # Environment configuration AUTH_ENABLED = os.getenv("AUTH_ENABLED", "false").lower() == "true" MASTER_KEY_HASH = os.getenv("MASTER_KEY_HASH") # SHA-256 hash of master key +INTERNAL_SERVICE_SECRET = os.getenv("INTERNAL_SERVICE_SECRET") # shared secret for X-Internal-Service header INTERNAL_SERVICE_IPS = {"127.0.0.1", "::1", "memos-mcp", "moltbot", "clawdbot"} # Connection pool for auth queries (lazy init) @@ -167,7 +168,7 @@ def is_internal_request(request: Request) -> bool: # Check internal header (for container-to-container). Treat an unset / # empty secret or missing / empty header as "disabled" and fail closed. - secret = os.getenv("INTERNAL_SERVICE_SECRET") + secret = INTERNAL_SERVICE_SECRET internal_header = request.headers.get("X-Internal-Service") if not secret or not internal_header: return False @@ -200,7 +201,8 @@ async def verify_api_key( # Allow internal services if is_internal_request(request): - logger.debug(f"Internal request from {request.client.host}") + client_host = request.client.host if request.client else "" + logger.debug(f"Internal request from {client_host}") return { "user_name": "internal", "scopes": ["all"], diff --git a/tests/api/test_auth_internal_request.py b/tests/api/test_auth_internal_request.py index 676c57fdf..9a38b3820 100644 --- a/tests/api/test_auth_internal_request.py +++ b/tests/api/test_auth_internal_request.py @@ -15,12 +15,11 @@ from __future__ import annotations -import asyncio - from typing import Any import pytest +from fastapi import HTTPException from starlette.requests import Request from memos.api.middleware import auth as auth_module @@ -49,7 +48,7 @@ def test_returns_false_when_secret_unset_and_header_missing( self, monkeypatch: pytest.MonkeyPatch ) -> None: """Regression: None == None must not authenticate.""" - monkeypatch.delenv("INTERNAL_SERVICE_SECRET", raising=False) + monkeypatch.setattr(auth_module, "INTERNAL_SERVICE_SECRET", None) request = _make_request(headers={}) assert is_internal_request(request) is False @@ -58,14 +57,14 @@ def test_returns_false_when_secret_unset_and_header_present( self, monkeypatch: pytest.MonkeyPatch ) -> None: """Header path is disabled when secret is not configured.""" - monkeypatch.delenv("INTERNAL_SERVICE_SECRET", raising=False) + monkeypatch.setattr(auth_module, "INTERNAL_SERVICE_SECRET", None) request = _make_request(headers={"X-Internal-Service": "guessed-value"}) assert is_internal_request(request) is False def test_returns_false_when_secret_empty_string(self, monkeypatch: pytest.MonkeyPatch) -> None: """Empty string secret is treated as unset (defence in depth).""" - monkeypatch.setenv("INTERNAL_SERVICE_SECRET", "") + monkeypatch.setattr(auth_module, "INTERNAL_SERVICE_SECRET", "") request = _make_request(headers={"X-Internal-Service": ""}) assert is_internal_request(request) is False @@ -73,7 +72,7 @@ def test_returns_false_when_secret_empty_string(self, monkeypatch: pytest.Monkey def test_returns_false_when_secret_set_but_header_missing( self, monkeypatch: pytest.MonkeyPatch ) -> None: - monkeypatch.setenv("INTERNAL_SERVICE_SECRET", "super-secret-value") + monkeypatch.setattr(auth_module, "INTERNAL_SERVICE_SECRET", "super-secret-value") request = _make_request(headers={}) assert is_internal_request(request) is False @@ -81,27 +80,27 @@ def test_returns_false_when_secret_set_but_header_missing( def test_returns_false_when_secret_set_but_header_wrong( self, monkeypatch: pytest.MonkeyPatch ) -> None: - monkeypatch.setenv("INTERNAL_SERVICE_SECRET", "super-secret-value") + monkeypatch.setattr(auth_module, "INTERNAL_SERVICE_SECRET", "super-secret-value") request = _make_request(headers={"X-Internal-Service": "wrong-value"}) assert is_internal_request(request) is False def test_returns_true_when_secret_matches_header(self, monkeypatch: pytest.MonkeyPatch) -> None: - monkeypatch.setenv("INTERNAL_SERVICE_SECRET", "super-secret-value") + monkeypatch.setattr(auth_module, "INTERNAL_SERVICE_SECRET", "super-secret-value") request = _make_request(headers={"X-Internal-Service": "super-secret-value"}) assert is_internal_request(request) is True def test_returns_true_when_client_ip_is_internal(self, monkeypatch: pytest.MonkeyPatch) -> None: """The internal-IP branch remains untouched by the fix.""" - monkeypatch.delenv("INTERNAL_SERVICE_SECRET", raising=False) + monkeypatch.setattr(auth_module, "INTERNAL_SERVICE_SECRET", None) request = _make_request(client=("127.0.0.1", 12345), headers={}) assert is_internal_request(request) is True def test_uses_constant_time_compare(self, monkeypatch: pytest.MonkeyPatch) -> None: """The fix must call `hmac.compare_digest` (not plain ==) to avoid timing leaks.""" - monkeypatch.setenv("INTERNAL_SERVICE_SECRET", "super-secret-value") + monkeypatch.setattr(auth_module, "INTERNAL_SERVICE_SECRET", "super-secret-value") calls: list[tuple[str, str]] = [] real_compare = auth_module.hmac.compare_digest @@ -118,7 +117,7 @@ def spy(a: str, b: str) -> bool: def test_no_client_and_no_header_and_no_secret(self, monkeypatch: pytest.MonkeyPatch) -> None: """Missing request.client + no header + no secret must NOT authenticate.""" - monkeypatch.delenv("INTERNAL_SERVICE_SECRET", raising=False) + monkeypatch.setattr(auth_module, "INTERNAL_SERVICE_SECRET", None) request = _make_request(client=None, headers={}) assert is_internal_request(request) is False @@ -127,50 +126,70 @@ def test_no_client_and_no_header_and_no_secret(self, monkeypatch: pytest.MonkeyP class TestVerifyApiKeyEndToEnd: """End-to-end reproduction of the advisory PoC against `verify_api_key`.""" - def test_external_request_no_key_is_rejected_when_secret_unset( + @pytest.mark.asyncio + async def test_external_request_no_key_is_rejected_when_secret_unset( self, monkeypatch: pytest.MonkeyPatch ) -> None: """The exact PoC from GHSA-9pw6-vmgx-qgwx must now be rejected with 401.""" monkeypatch.setattr(auth_module, "AUTH_ENABLED", True) - monkeypatch.delenv("INTERNAL_SERVICE_SECRET", raising=False) + monkeypatch.setattr(auth_module, "INTERNAL_SERVICE_SECRET", None) request = _make_request(headers={}) - from fastapi import HTTPException - with pytest.raises(HTTPException) as exc_info: - asyncio.run(verify_api_key(request, api_key=None)) + await verify_api_key(request, api_key=None) assert exc_info.value.status_code == 401 assert "Missing API key" in exc_info.value.detail - def test_external_request_with_wrong_header_still_rejected( + @pytest.mark.asyncio + async def test_external_request_with_wrong_header_still_rejected( self, monkeypatch: pytest.MonkeyPatch ) -> None: monkeypatch.setattr(auth_module, "AUTH_ENABLED", True) - monkeypatch.delenv("INTERNAL_SERVICE_SECRET", raising=False) + monkeypatch.setattr(auth_module, "INTERNAL_SERVICE_SECRET", None) request = _make_request(headers={"X-Internal-Service": "guess"}) - from fastapi import HTTPException - with pytest.raises(HTTPException) as exc_info: - asyncio.run(verify_api_key(request, api_key=None)) + await verify_api_key(request, api_key=None) assert exc_info.value.status_code == 401 - def test_internal_request_with_matching_secret_is_authorised( + @pytest.mark.asyncio + async def test_internal_request_with_matching_secret_is_authorised( self, monkeypatch: pytest.MonkeyPatch ) -> None: monkeypatch.setattr(auth_module, "AUTH_ENABLED", True) - monkeypatch.setenv("INTERNAL_SERVICE_SECRET", "shared-secret") + monkeypatch.setattr(auth_module, "INTERNAL_SERVICE_SECRET", "shared-secret") request = _make_request( headers={"X-Internal-Service": "shared-secret"}, client=("10.0.0.42", 40000), ) - result = asyncio.run(verify_api_key(request, api_key=None)) + result = await verify_api_key(request, api_key=None) assert result == { "user_name": "internal", "scopes": ["all"], "is_master_key": False, "is_internal": True, } + + @pytest.mark.asyncio + async def test_internal_request_via_header_with_no_client_does_not_crash( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Regression for finding #1: ``request.client`` may be None on the header path. + + `verify_api_key` must not raise ``AttributeError`` when a request without a + ``client`` tuple is granted the internal principal via a matching + ``X-Internal-Service`` header. + """ + monkeypatch.setattr(auth_module, "AUTH_ENABLED", True) + monkeypatch.setattr(auth_module, "INTERNAL_SERVICE_SECRET", "shared-secret") + request = _make_request( + client=None, + headers={"X-Internal-Service": "shared-secret"}, + ) + + result = await verify_api_key(request, api_key=None) + assert result["is_internal"] is True + assert result["user_name"] == "internal"