fix: OAuth consent owner-auth + shared MCP memory across agents#45
Merged
Conversation
The /oauth/authorize consent step issued an authorization code to anyone who clicked "Authorize", with no owner authentication. Because the OAuth endpoints are public, any party who knew the server URL could add it as a custom connector, complete consent, and obtain a token with full access to the single user's memories. Require MEM0_API_KEY at the consent step: the form now has an API-key field and POST /oauth/authorize validates it (constant-time) before issuing a code, re-rendering the form with a 401 on failure. Add tests for the wrong/missing password cases and document the security model. https://claude.ai/code/session_01U3EtN3puoZRq2t7nedcnHY
There was a problem hiding this comment.
Pull request overview
This PR closes an OAuth Phase 2 authorization vulnerability by requiring resource-owner authentication at the consent step, reusing MEM0_API_KEY as the gate before issuing authorization codes.
Changes:
- Add an API-key password field to the OAuth consent page and require
MEM0_API_KEY(constant-time compare) onPOST /oauth/authorize. - Update OAuth tests to supply the key and add negative tests ensuring no code is issued on wrong/missing key.
- Update user/developer documentation to describe the new consent authentication invariant.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| app/oauth.py | Adds consent-page rendering helper and enforces MEM0_API_KEY check before issuing authorization codes. |
| tests/test_oauth.py | Updates authorize flow tests to include the API key; adds 401 rejection tests for wrong/missing key. |
| docs/USER_GUIDE.md | Updates Claude.ai/Cowork OAuth instructions to include entering MEM0_API_KEY at consent, with a security note. |
| docs/DEVELOPER_GUIDE.md | Documents that /oauth/authorize POST authenticates the resource owner via MEM0_API_KEY. |
| CLAUDE.md | Records the consent-authentication invariant as a “don’t remove” security property. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
235
to
237
| client = oauth_store.get_client(client_id) | ||
| if not client or redirect_uri not in client["redirect_uris"]: | ||
| raise HTTPException(status_code=400, detail="invalid client or redirect_uri") |
| This requires **Phase 2** (`OAUTH_SIGNING_KEY` set). In the client's connector settings: | ||
|
|
||
| 1. Add a **custom connector** pointing at `https://mem0.your-domain.com/mcp/`. | ||
| 1. Add a **custom connector** pointing at `https://mem0.your-domain.com/mcp`. |
The MCP search_memories/list_memories tools filtered by agent_id when the caller supplied it. Clients' models populate agent_id (observed: a memory tagged agent_id="claude-web"), so each agent effectively read only its own writes — Claude Code and Codex appeared to have separate memories even though the store is physically shared under one user_id. Remove agent_id from the read tools so search/list always span the whole user store. Keep agent_id on add_memory as an optional write-only provenance tag. The REST API still supports agent_id read filters for explicit scripted use. https://claude.ai/code/session_01U3EtN3puoZRq2t7nedcnHY
imonroe
changed the title
Comment on lines
+196
to
+201
| def _owner_authenticated(password: str) -> bool: | ||
| # Single-user: the resource owner proves ownership at the consent step with | ||
| # the same MEM0_API_KEY that protects the API. Constant-time compare; an empty | ||
| # configured key must never authenticate. | ||
| key = get_settings().mem0_api_key | ||
| return bool(key) and secrets.compare_digest(password, key) |
| This requires **Phase 2** (`OAUTH_SIGNING_KEY` set). In the client's connector settings: | ||
|
|
||
| 1. Add a **custom connector** pointing at `https://mem0.your-domain.com/mcp/`. | ||
| 1. Add a **custom connector** pointing at `https://mem0.your-domain.com/mcp`. |
| This requires **Phase 2** (`OAUTH_SIGNING_KEY` set). In the client's connector settings: | ||
|
|
||
| 1. Add a **custom connector** pointing at `https://mem0.your-domain.com/mcp/`. | ||
| 1. Add a **custom connector** pointing at `https://mem0.your-domain.com/mcp`. |
Comment on lines
123
to
+127
| The OAuth flow (`app/oauth.py`) is OAuth 2.1 with PKCE (S256 required) and public clients only — no | ||
| client secrets are issued. Endpoints: `/oauth/register` (DCR), `/oauth/authorize` (GET form + POST | ||
| client secrets are issued. The `/oauth/authorize` consent step **authenticates the resource owner**: | ||
| the POST handler requires the `MEM0_API_KEY` (constant-time compared) before issuing a code. Without | ||
| this gate, anyone who reached the public consent screen could mint a token for the single user's | ||
| memories just by clicking "Authorize". Endpoints: `/oauth/register` (DCR), `/oauth/authorize` (GET form + POST |
| - **`MEM0_EMBED_DIMS` must match the embedder's real output dimension** (3-small=1536, 3-large=3072). A mismatch causes *silent* search failures, not errors. Changing embedding models requires dropping and recreating the Qdrant collection. | ||
| - **FastMCP = the PrefectHQ `fastmcp` PyPI package**, imported `from fastmcp import FastMCP`. It is NOT the older `mcp.server.fastmcp` module. | ||
| - **Same `MEM0_API_KEY` protects both** the REST endpoints (`require_bearer` dependency) and the MCP endpoint (`StaticTokenVerifier` in Phase 1). | ||
| - **The Phase 2 OAuth `/oauth/authorize` consent step authenticates the resource owner** by requiring `MEM0_API_KEY` (constant-time compare) before issuing a code. Don't remove this — the OAuth endpoints are public, so without it anyone reaching the consent screen could mint a token for the single user's memories. |
…nsistency Address review feedback on PR #45: - POST /oauth/authorize now rejects an empty code_challenge (400) so a direct POST can't mint a code that could never be exchanged - _owner_authenticated hashes both the submitted password and the configured key to fixed-length digests before secrets.compare_digest, so the comparison is genuinely constant-time (no length leak) - standardize the Claude Code/Desktop example URLs on the canonical /mcp (no trailing slash) to match the OAuth section https://claude.ai/code/session_01U3EtN3puoZRq2t7nedcnHY
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
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.
Two fixes.
1. Authenticate the resource owner at OAuth consent (security)
POST /oauth/authorizeissued an authorization code to anyone who clicked "Authorize" — no resource-owner authentication. Since the OAuth endpoints are public, any party who knew the server URL could add it as a connector in their own Claude, complete consent, and get a token with full access to the single user's memories. URL obscurity was the only protection.Fix: require
MEM0_API_KEYat consent. The page has an API-key field;POST /oauth/authorizevalidates it (constant-time, rejects an empty configured key) before issuing a code, and re-renders with a401on failure. Reuses the existing master credential — no new env var.2. Make MCP memory reads span all agents (shared store)
Diagnosed live: with both Claude Code and Codex connected, memory wasn't shared. A
GET /api/v1/memoriesdump showed all memories under oneuser_id(so the store is shared) but one taggedagent_id="claude-web"— i.e. clients' models populateagent_id. The MCPsearch_memories/list_memoriestools filtered byagent_idwhen supplied, so each agent read only its own writes.Fix: remove
agent_idfrom the MCP read tools (search/list) so reads always span the whole user store.agent_idstays onadd_memoryas a write-only provenance tag. The REST API keepsagent_idread filters for explicit scripted use.Tests
test_authorize_rejects_wrong_password,test_authorize_rejects_missing_password(401, no code issued); existing flow tests supply the key.search_memoriesassertsfilters == {"user_id": "ian"}(no agent scoping);test_read_tools_do_not_expose_agent_idlocksagent_idout of the read tools' schemas.ruffclean.Docs
MEM0_API_KEYat consent (+ security rationale);agent_idis write-only over MCP.CLAUDE.md: consent-auth invariant and shared-read (no agent_id filtering) invariant.Not included
spaCy/fastembed deps intentionally skipped (heavy footprint for optional NLP/BM25; OpenAI semantic search is sufficient).
https://claude.ai/code/session_01U3EtN3puoZRq2t7nedcnHY