Fix #2259: Authentication bypass in MemOS server: internal-service check fails open when IN - #2260
Open
Memtensor-AI wants to merge 2 commits into
Open
Conversation
`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`.
Memtensor-AI
added
ai:generated
5 tasks
Collaborator
Author
🤖 Open Code ReviewTarget: PR #2260 ✅ OpenCodeReview: No comments generated. Looks good to me. Generated by cloud-assistant via Open Code Review. |
Collaborator
Author
🔧 Open Code Review requested Agent fixOpen Code Review found 4 issue(s). I have resumed the development Agent to fix them.
The Agent will push a new commit to this PR branch. OCR will recheck after the commit is pushed. |
- Guard `request.client` on the internal-header path: fallback to `<unknown>` for the debug log so header-authenticated requests without a client tuple don't crash with AttributeError (OCR finding MemTensor#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 MemTensor#2). - Rewrite the auth regression tests: move `HTTPException` to the top-level imports (OCR finding MemTensor#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 MemTensor#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) <noreply@anthropic.com>
Collaborator
Author
✅ Automated Test Results: PASSEDAll tests passed (9/9 executed, 4 skipped). memos_python_core/changed-repo-python: 9 passed, 4 skipped. Duration: 5s [advisory, non-gating] AI-generated tests on branch test/auto-gen-18aede470455b543-20260817070848: 68/68 passed — these do NOT affect the PR verdict; review the branch manually. Branch: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Closes the fail-open authentication bypass in
memos.api.middleware.auth.is_internal_request()reported in GHSA-9pw6-vmgx-qgwx / issue #2259. In every shipped deploymentINTERNAL_SERVICE_SECRETis unset, soos.getenv(...)returnedNone; a normal external request also producedNonefromrequest.headers.get("X-Internal-Service"), andNone == Noneauthorised anonymous callers as theinternalprincipal withscopes: ["all"], satisfyingrequire_scope("admin")on every/admin/*route.The fix reads the secret into a local variable, returns
Falsewhen either the secret or the header is missing/empty, and compares the two non-empty strings withhmac.compare_digestfor constant-time equality. TheINTERNAL_SERVICE_IPSallowlist branch is unchanged. No public API, request/response model, config schema, Dockerfile, Helm chart or docs modified — deployments that never set the secret simply lose the never-safely-usable header path.Added
tests/api/test_auth_internal_request.pywith 12 regression cases: five reproducing the exact fail-open scenarios (all failed on the unpatched baseline as expected), one assertinghmac.compare_digestis invoked, plus positive-path and end-to-end coverage that runs the advisory PoC againstverify_api_key. After the fix all 12 pass; the advisory PoC now returns401 Missing API keyfor both header-less and wrong-header external requests.ruff checkandruff formatare clean on both changed files. Neighbour tests (tests/api/test_lifecycle.py,test_product_models.py,test_thread_context.py) still green; the 4 pre-existingtest_mcp_serve.pyfailures reproduce identically on the unpatched baseline (missingpytest-asynciomarker in this CI env) and are unrelated to this change.Scope note: the reporter also observed as a secondary issue that
server_routermounted byserver_api_ext.pyhas no auth dependency on/product/*data endpoints. That change touches public routes (AGENTS.md"Ask first" list) and is deferred to a separate issue — this PR intentionally keeps scope tight to the primary fail-open per issue #2259.Related Issue (Required): Fixes #2259
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Not run; documentation-only change.
Checklist
@WeiminLee please review this PR.
Reviewer Checklist