diff --git a/src/memos/api/middleware/auth.py b/src/memos/api/middleware/auth.py index 15b217651..6d812337b 100644 --- a/src/memos/api/middleware/auth.py +++ b/src/memos/api/middleware/auth.py @@ -17,6 +17,13 @@ import memos.log +# Canonical format check; re-exported here so the middleware shares the +# same logic as ``src/memos/api/utils/api_keys.py``. Keeping one source of +# truth is what fixes issue #2427 (previously the middleware copy and the +# utils copy could drift independently). +from memos.api.utils.api_keys import validate_key_format as validate_key_format_canonical + + logger = memos.log.get_logger(__name__) # API key header configuration @@ -63,17 +70,16 @@ def hash_api_key(key: str) -> str: def validate_key_format(key: str) -> bool: - """Validate API key format: krlk_<64-hex>.""" - if not key or not key.startswith("krlk_"): - return False - hex_part = key[5:] # Remove 'krlk_' prefix - if len(hex_part) != 64: - return False - try: - int(hex_part, 16) - return True - except ValueError: - return False + """Validate API key format: krlk_<64-hex>. + + Re-exported from ``memos.api.utils.api_keys.validate_key_format`` for + backwards compatibility with callers that imported this symbol from + the middleware path. Issue #2427: the previous copy used ``int(...)`` + which accepted non-canonical hex (leading signs, ``0x`` prefix, ``_`` + separators, whitespace) — see the canonical implementation for the + fix. + """ + return validate_key_format_canonical(key) def get_key_prefix(key: str) -> str: diff --git a/src/memos/api/utils/api_keys.py b/src/memos/api/utils/api_keys.py index 29b493fd0..123420513 100644 --- a/src/memos/api/utils/api_keys.py +++ b/src/memos/api/utils/api_keys.py @@ -5,6 +5,7 @@ """ import hashlib +import re import secrets from dataclasses import dataclass @@ -45,11 +46,19 @@ def hash_key(key: str) -> str: return hashlib.sha256(key.encode()).hexdigest() +# Match the only form ``generate_api_key()`` produces: ``krlk_`` followed by +# exactly 64 lowercase hex characters. Using ``int(hex_part, 16)`` would also +# accept leading signs, ``0x`` prefixes, ``_`` separators, and surrounding +# whitespace, none of which a real key ever carries (issue #2427). +_KEY_FORMAT_HEX_RE = re.compile(r"[0-9a-f]{64}") + + def validate_key_format(key: str) -> bool: """ Validate API key format. - Valid format: krlk_<64-hex-chars> + Valid format: ``krlk_<64-hex-chars>`` where the hex part is exactly 64 + characters from ``[0-9a-f]`` — the only form ``generate_api_key()`` produces. """ if not key or not isinstance(key, str): return False @@ -58,14 +67,7 @@ def validate_key_format(key: str) -> bool: return False hex_part = key[5:] - if len(hex_part) != 64: - return False - - try: - int(hex_part, 16) - return True - except ValueError: - return False + return _KEY_FORMAT_HEX_RE.fullmatch(hex_part) is not None def generate_master_key() -> tuple[str, str]: diff --git a/tests/api/test_validate_key_format.py b/tests/api/test_validate_key_format.py new file mode 100644 index 000000000..757d2519a --- /dev/null +++ b/tests/api/test_validate_key_format.py @@ -0,0 +1,99 @@ +"""Regression tests for issue #2427. + +``validate_key_format`` exists in two copies — ``src/memos/api/utils/api_keys.py`` +(the canonical home) and ``src/memos/api/middleware/auth.py`` (the +middleware gating every authenticated request). Both check the hex part +with ``int(hex_part, 16)``, which accepts non-canonical forms a generated +key can never have: + +- leading ``+`` / ``-`` (sign) +- ``0x`` / ``0X`` prefix +- ``_`` separators (PEP 515) +- surrounding whitespace + +So the documented format ``krlk_<64-hex-chars>`` is not actually enforced: +keys with embedded underscores, signs, or ``0x`` prefixes pass the format +gate and are hashed/looked up as-is, bypassing the canonical form that +``generate_api_key()`` produces. + +These tests pin the new contract on both validator copies: + +- the hex part must match ``[0-9a-f]{64}`` exactly +- the only valid form is the canonical lowercase hex string +""" + +from __future__ import annotations + +import pytest + +from memos.api.middleware.auth import validate_key_format as validate_in_auth +from memos.api.utils.api_keys import generate_api_key, validate_key_format + + +CANONICAL = "krlk_" + "0123456789abcdef" * 4 # 64 hex chars, lowercase + + +class TestValidateKeyFormatUtils: + def test_canonical_key_passes(self): + assert validate_key_format(CANONICAL) is True + + def test_generated_key_always_passes(self): + # The validator must accept every key that generate_api_key() + # produces, since that is the documented format. + for _ in range(20): + assert validate_key_format(generate_api_key().key) is True + + @pytest.mark.parametrize( + "bad_key", + [ + # Leading sign + "krlk_" + "+" + "a" * 63, + "krlk_" + "-" + "a" * 63, + # 0x prefix + "krlk_" + "0x" + "a" * 62, + "krlk_" + "0X" + "a" * 62, + # Underscore separator (PEP 515) + "krlk_" + "a" * 32 + "_" + "b" * 31, + # Uppercase hex (the generator emits lowercase only) + "krlk_" + "A" * 64, + # Surrounding whitespace + " krlk_" + "a" * 64, + "krlk_" + "a" * 64 + " ", + # Wrong total length + "krlk_" + "a" * 63, + "krlk_" + "a" * 65, + # Empty hex part + "krlk_", + # Missing prefix + "0123456789abcdef" * 4, + "Bearer " + CANONICAL, + # Non-string + "", + ], + ) + def test_non_canonical_keys_rejected(self, bad_key): + assert validate_key_format(bad_key) is False, ( + f"validate_key_format({bad_key!r}) should return False" + ) + + +class TestAuthMiddlewareValidator: + """The middleware copy must enforce the same contract as the canonical + one — issue #2427 documents both copies accepting non-canonical hex.""" + + def test_canonical_key_passes(self): + assert validate_in_auth(CANONICAL) is True + + def test_signed_hex_rejected(self): + # The most surprising bit: ``int("+a..", 16)`` succeeds, so a signed + # hex passes the middleware's validator today. + assert validate_in_auth("krlk_" + "+" + "a" * 63) is False + + def test_underscore_rejected(self): + assert validate_in_auth("krlk_" + "a" * 32 + "_" + "b" * 31) is False + + def test_uppercase_rejected(self): + assert validate_in_auth("krlk_" + "A" * 64) is False + + def test_0x_prefix_rejected(self): + assert validate_in_auth("krlk_" + "0x" + "a" * 62) is False