[Alphaxiaoteng]

Summary

Hardens backend/database/mcp_api_key.py against path traversal, empty input allocations, and unexpected type crashes:

• Input Sanitization & Path Traversal Guard:
• Adds _clean_user_id(user_id) rejecting empty strings, non-string types, and path traversal characters (/, \\, ..), protecting Firestore user document collections.
• Adds _clean_key_name(name) validating non-empty names, normalizing whitespace, capping length at 255 characters, and guarding against raw token leakage.
• Early Validation & Fast Fail-Safe:
• In get_mcp_keys_for_user_with_repair_info(), malformed or empty user IDs return fast ([], frozenset()) without issuing unnecessary remote Firestore stream queries.
• In delete_mcp_key(), invalid or malformed user IDs and key IDs no-op cleanly rather than provoking Firestore document path traversal errors.
• In get_api_key_auth_result(), validates that api_key is a string before calling .startswith(), preventing AttributeError crashes on non-string inputs.
• Hermetic Unit Tests:
• Added 43 comprehensive unit tests in backend/tests/unit/test_mcp_api_key_resilience.py covering valid paths, invalid types, traversal attacks, length boundaries, and mock database operations (100% pass in 0.78s).

Failure-Class: none

Proposed Bounty

• Suggested reward: $50
• Contributor payout: PayPal chenpaiteng@gmail.com

Verification

• pytest backend/tests/unit/test_mcp_api_key_resilience.py (43 passed in 0.78s)
• pytest backend/tests/unit/test_mcp_api_key_auth_context.py (6 passed in 1.15s)
• black --line-length 120 --skip-string-normalization backend/database/mcp_api_key.py backend/tests/unit/test_mcp_api_key_resilience.py (clean)

[Git-on-my-level]

Thanks @Alphaxiaoteng — solid, well-tested hardening of the MCP API-key module.

What I verified (isolated worktree at your head 527d3b1):

• backend/database/mcp_api_key.py — the new _clean_user_id/_clean_key_name validators are sound: traversal characters and length bounds are enforced before any Firestore write, and the contains_raw_api_key guard from main is preserved inside _clean_key_name. The guard is substantive: user_id does reach a document path via _seed_mcp_memory_grant -> collection('users').document(user_id), and create_mcp_key now persists the cleaned values consistently for both the key doc and the seeded grant.
• get_mcp_keys_for_user_with_repair_info returning ([], frozenset()) for malformed user_id avoids a pointless remote query, and get_mcp_keys_for_user inherits it cleanly via delegation.
• get_api_key_auth_result now fails closed on non-string input instead of raising AttributeError — good fail-safe direction for an auth path.
• backend/tests/unit/test_mcp_api_key_resilience.py — ran the suite: 43/43 passed, plus 6/6 in the pre-existing test_mcp_api_key_auth_context.py, and black (line-120, skip-string-norm) is clean on both files.

Two non-blocking notes:

1. delete_mcp_key now silently returns on malformed user_id/key_id, which the router turns into a 204 even though nothing was deleted. Real callers (routers/api_key_management.py via Depends(get_current_user_id), services/users/account_deletion.py with ids taken from listed keys) always pass well-formed values, so this doesn't mask anything in practice — but if you want, a shared predicate or surfacing 400 for malformed input would make the intent explicit.
2. The traversal/length check is duplicated inline in get_mcp_keys_for_user_with_repair_info and delete_mcp_key rather than reusing _clean_user_id (which raises instead of returning). A small shared helper returning a bool would keep the three call sites from drifting.

One heads-up: this module is the MCP API-key validation/revocation surface, so it needs a maintainer sign-off on the auth-path behavior before merge. The bounty proposal in the description is a maintainer decision, not something this review can rule on.

---

by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

[Alphaxiaoteng]

Hi @Git-on-my-level need human response

Thanks for the thorough review and validation (43/43 new tests + 6/6 existing auth-context tests passing, black-clean).

Re your non-blocking note on delete_mcp_key: the silent-return on malformed user_id/key_id was intentional to keep the auth path fail-closed and avoid raising from a 204 handler under current get_current_user_id/account-deletion callers. Happy to extract the duplicated traversal/length check into a shared bool helper if that unblocks review — pointing me at the preferred direction and I'll push it within the day.

Two asks for a human maintainer:

1. This module is the MCP API-key validation/revocation surface, so it needs your sign-off on the auth-path behavior before merge.
2. The linked issue is a [Bounty proposal] — could you confirm whether the bounty is funded/approved, or if it should be treated as a non-bounty contribution?

Settlement details are already on the linked bounty issue. Appreciate the quick turnaround so far.

[undivisible]

@Alphaxiaoteng this conflicts with main now. Please rebase and push; it still looks valid.

[github-actions]

Hey @Alphaxiaoteng 👋

Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request.

After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:

• Project standards — Ensuring consistency across the codebase
• User needs — Making sure changes align with what our users need
• Code best practices — Maintaining code quality and maintainability
• Project direction — Keeping aligned with our product principles and locked invariants

Before your next PR, please skim:

• PRODUCT.md — product north star
• Product invariants — locked rules (shared chat, memory tiers, agent control plane, integrations, brand)

If this was declined for direction or taste, maintainers should cite an invariant ID or open a proposed one — ask if that citation is missing.

Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out.

Thank you for being part of the Omi community!