Skip to content

fix(ms365-app): require caller authentication on the chat-tool route - #16143

Closed
optimus-fulcria wants to merge 1 commit into
BasedHardware:mainfrom
optimus-fulcria:fix/ms365-tools-caller-auth
Closed

optimus-fulcria wants to merge 1 commit into
BasedHardware:mainfrom
optimus-fulcria:fix/ms365-tools-caller-auth

Conversation

@optimus-fulcria

@optimus-fulcria optimus-fulcria commented Sep 23, 2026 •

Copy link
Copy Markdown

Summary

POST /tools/{tool_name} in plugins/omi-ms365-app/main.py took uid from the request body and dispatched to a handler that acts on that user's stored Microsoft Graph token, with no caller authentication on the route:

@app.post("/tools/{tool_name}")
async def tool_dispatch(tool_name: str, request: Request) -> Any:
    ...
    uid: str | None = body.get("uid") or request.query_params.get("uid")
    ...
    await _auth_guard(uid)          # asserts the *victim* is connected, not that the caller may act
    return await handler(uid, **args)

_auth_guard only checks that a Microsoft token exists for the named uid, so naming a connected user is enough. The 17 registered tools include read_email, search_emails, send_email, list_recent_chats, send_chat_message, list_recent_files, read_file_text and upload_text_file, and config.GRAPH_SCOPES requests Mail.Read, Mail.Send, Mail.ReadWrite, Calendars.ReadWrite, Chat.ReadWrite and Files.ReadWrite.All. Any client that can reach the deployment and name a uid could read a user's mailbox and send mail as them.

Same defect class as the guards already merged for omi-github-app (github_tools_auth.py) and omi-shipbob-app (shipbob_tools_auth.py), and as the earlier zapier / paid-mentor / webhook fixes; ms365 was missed by all of them. /tools/* in this app is the most sensitive instance left: it is mail, not inventory.

Fix

New plugins/omi-ms365-app/ms365_tools_auth.py with require_ms365_tools_auth, mirroring the merged guards rather than inventing a second shape:

  • Shared secret from MS365_TOOLS_SECRET, presented as Authorization: Bearer <secret> or an ms365_tools_token query parameter.
  • hmac.compare_digest (timing-safe); a whitespace-only secret counts as unconfigured.
  • Fail-closed 503 when unconfigured, 401 on a missing, wrong or non-Bearer token.
  • Applied as dependencies=[Depends(require_ms365_tools_auth)] on the single chat-tool route. Caller authentication only — uid stays where it is, in the body.

Docs: MS365_TOOLS_SECRET added to the README's required-environment table, the endpoint table, the curl example, and .env.example, plus a short section stating that the same value has to reach the Omi backend or every tool call is refused.

plugins/omi-ms365-app/test_tool_dispatch.py and plugins/test_oauth_error_params_escaped.py import this main.py against fastapi stubs; both stubs gain a Depends attribute, which their decorator stubs then ignore.

Verification

  • python3 plugins/omi-ms365-app/test_ms365_tools_auth.py — 11 tests. Nine cover the guard's decision table (unconfigured→503, blank secret→503, no token→401, wrong bearer→401, wrong query token→401, Basic scheme→401, correct bearer→ok, bearer lowercase→ok, correct query token→ok); two load the real main.py through a recording stub and assert the chat-tool route — and only it — carries require_ms365_tools_auth, so the dependency cannot be dropped from the decorator unnoticed.
  • Also passes with fastapi blocked from import, matching this app's stdlib-only test lane.
  • Existing suites unchanged and passing: test_tool_dispatch.py (7), test_input_coercion.py (8), test_services_hardening.py, test_calendar_pagination.py, plugins/test_oauth_error_params_escaped.py (3), plugins/test_uid_reflection_remaining_apps.py (9).
  • make preflight — 21 checks pass, including the new ms365-tools-auth-tests entry registered in .github/checks-manifest.yaml (both lanes).

Failure-Class: none

🤖 Generated with Claude Code

Review in cubic

`POST /tools/{tool_name}` read `uid` from the request body and dispatched to
handlers that act on that user's stored Microsoft Graph token — `read_email`,
`send_email`, `send_chat_message`, `list_recent_files`, `read_file_text` and
`upload_text_file` among 17 tools — with no caller authentication at all.
Anyone who could reach the deployment and name a uid could read and send that
user's mail.

Adds `ms365_tools_auth.require_ms365_tools_auth`, mirroring the guards already
merged for `omi-github-app` and `omi-shipbob-app`: a shared secret presented as
`Authorization: Bearer <secret>` or an `ms365_tools_token` query parameter,
compared with `hmac.compare_digest` against `MS365_TOOLS_SECRET`. Fails closed —
503 while the secret is unset, 401 for a missing, wrong or non-Bearer one.

The hermetic suite covers the guard's decision table and asserts the chat-tool
route is still wired to it, so the dependency cannot be dropped from the
decorator unnoticed. The two shared stub harnesses that import this app's
`main.py` gain `fastapi.Depends`, which their decorator stubs then ignore.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@optimus-fulcria

Copy link
Copy Markdown
Author

Disclosure: this PR overlaps #14456 ("fix(plugins): require auth on /tools/* routes acting on uid-keyed credentials in 7 apps"), which was opened on 2026-09-18 — five days before this one — and already includes plugins/omi-ms365-app/ms365_tools_auth.py, the same Depends wiring on POST /tools/{tool_name}, and an ms365 auth test. I found #14456 only after opening this PR: my audit checked the guard state of the code at HEAD and I did not search open PRs for an existing claim on the class. That was my mistake, and #14456 has priority.

Two things here are not in #14456, and they're the only reason I'm not closing this unilaterally:

  1. CI registration. fix(plugins): require auth on /tools/* routes acting on uid-keyed credentials in 7 apps #14456 adds test_ms365_tools_auth.py but no .github/checks-manifest.yaml entry, so the new suite isn't executed by the gate. This PR registers it (make preflight selects it, 21/21).
  2. A wiring test that pins the dependency. test_ms365_tools_auth.py here loads the real main.py under hermetic stubs and records route registrations, so it fails if Depends(require_ms365_tools_auth) is ever dropped from the decorator — rather than only testing the guard module in isolation. That is the same gap @aryanorastar requested changes for on security(whoop-app): add caller authentication to /tools/* routes #15918.

Happy either way, maintainers' call: I can close this in favour of #14456 and open the two deltas above as a small follow-up against it once it lands, or rebase this down to just those deltas now. I'd suggest the former — #14456 is the better-scoped unit for the class, and its 503-until-provisioned rollout note applies to MS365_TOOLS_SECRET here identically.

@aryanorastar

Copy link
Copy Markdown
Collaborator

Keeping this open on 796b93c.

Microsoft 365 /tools/{tool_name} on main still has no auth dependency. This head adds require_ms365_tools_auth. Only cubic ran.

Not approving this SHA.

— Astar, on behalf of @aryanorastar

@Git-on-my-level Git-on-my-level added security-review Touches auth, provider routing, secrets, or security-sensitive surfaces needs-maintainer-review Needs a human maintainer to sign off before merge python labels Sep 23, 2026
@Git-on-my-level

Copy link
Copy Markdown
Member

Verified on head 796b93c in an isolated, network-disabled container — this is a solid fix for a real gap, and the guard mirrors the merged pattern rather than inventing a new one. File-by-file:

  • ms365_tools_auth.py (new): faithful mirror of omi-github-app/github_tools_auth.py / omi-shipbob-app/shipbob_tools_auth.py — same hmac.compare_digest comparison, same Bearer-header-or-ms365_tools_token-query-param presentation, same fail-closed semantics (503 unconfigured incl. whitespace-only secret, 401 missing/wrong/non-Bearer).
  • main.py: dependencies=[Depends(require_ms365_tools_auth)] is on the right route. Pre-fix, _auth_guard(uid) only asserted the named user was connected, so anyone who could reach the deployment and name a uid could call send_email / send_chat_message / upload_text_file on that user's Graph token. The browser/setup routes stay open, matching the github app's posture where only /tools/* is gated.
  • test_ms365_tools_auth.py (new): 11/11 pass hermetically — 503/401 matrix, both accept paths, plus a wiring test that loads the real main.py under stubs and asserts the dependency stays attached, so the guard can't be silently dropped from the decorator.
  • test_tool_dispatch.py + test_oauth_error_params_escaped.py: Depends added to the fastapi stubs so the stub decorators tolerate the new kwarg; 7/7 and 3/3 pass. The github sibling guard suite still passes 11/11.
  • .github/checks-manifest.yaml: registers the new suite in local+ci lanes — the delta fix(plugins): require auth on /tools/* routes acting on uid-keyed credentials in 7 apps #14456 lacks.
  • README.md / .env.example: accurate — 503/401 semantics documented, curl example gains the Bearer header, endpoint and env-var tables updated; docs match the code.

Two calls for a human maintainer before merge:

  1. Overlap with fix(plugins): require auth on /tools/* routes acting on uid-keyed credentials in 7 apps #14456 (opened 5 days earlier; author disclosed it above): same guard file and wiring there, while this PR uniquely adds the CI registration and the decorator-pinning wiring test. Whether to take this head as-is or fold the two deltas into fix(plugins): require auth on /tools/* routes acting on uid-keyed credentials in 7 apps #14456 is a scoping decision.
  2. Rollout: the backend's _call_tool_endpoint sends no auth header — the credential rides on the registered endpoint URL — so existing registered ms365 endpoints will fail closed (401/503) until MS365_TOOLS_SECRET is provisioned and endpoints are re-registered. Fail-closed is the right default; the re-registration step just needs an owner.

🤖 Automated review on behalf of the maintainers.


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

@kodjima33 kodjima33 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified: missing caller-auth on /tools/{tool_name} confirmed on current main (_auth_guard only checks the named uid has a token, not that the caller may act). Fix mirrors existing merged guards (omi-github-app, omi-shipbob-app) and adds 11 tests. Holding merge for manual sign-off since this touches auth/secret-handling code and the repo's own automation flagged it security-review + needs-maintainer-review.

@aryanorastar

Copy link
Copy Markdown
Collaborator

Keeping this open. Not approving 796b93c6ce27. origin/main ca828a8c617d plugins/omi-ms365-app/main.py:212 is still .post("/tools/{tool_name}") with no auth dependency. SHA adds require_ms365_tools_auth. CI rollup FAILURE. mergeable: UNKNOWN.

— Astar, on behalf of @aryanorastar

@aryanorastar aryanorastar left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved 796b93c6ce27f085eef9ce5f3c686b6115168970. plugins/omi-ms365-app/main.py on main posts /tools/{tool_name} with no caller dependency. This head requires require_ms365_tools_auth and Hygiene succeeded.

@Git-on-my-level

Copy link
Copy Markdown
Member

Verdict: correct, well-scoped security fix, consistent with the guards already merged for omi-github-app and omi-shipbob-app; nothing blocking from my side. Verified on 796b93c:

  • plugins/omi-ms365-app/ms365_tools_auth.py — genuinely fail-closed: _configured_secret() treats an unset or whitespace-only MS365_TOOLS_SECRET as unconfigured (503 before any token check), and require_ms365_tools_auth returns 401 on missing, wrong, or non-Bearer tokens, comparing with hmac.compare_digest. Effectively identical to the merged github_tools_auth.py.
  • plugins/omi-ms365-app/main.py — @app.post("/tools/{tool_name}", dependencies=[Depends(require_ms365_tools_auth)]). Given the 17 registered tools act on the named uid's Graph token (read_email, send_email, send_chat_message, read_file_text, upload_text_file among them), this closes the real hole where naming a connected uid was enough to read and send someone's mail.
  • plugins/omi-ms365-app/test_ms365_tools_auth.py — ran the suite locally: 11/11 pass, including a wiring test that loads the real main.py and asserts exactly /tools/{tool_name} carries the dependency, so the guard cannot be dropped from the decorator unnoticed.
  • plugins/omi-ms365-app/test_tool_dispatch.py and plugins/test_oauth_error_params_escaped.py — the Depends stub additions are mechanical and correct; the dispatch suite still passes 7/7 locally and CI is green on head.
  • plugins/omi-ms365-app/README.md and .env.example — accurate: the query-param delivery path is real, because the Omi backend POSTs to the registered endpoint URL verbatim (app_tools.py _call_tool_endpoint), so appending ?ms365_tools_token=... to the registered URL is how the backend presents the secret.
  • .github/checks-manifest.yaml — new ms365-tools-auth-tests entry wired with triggers, consistent with its neighbors.

Security/blast radius: this is the most sensitive of the plugin tool routes (mail, calendar, Teams, OneDrive), and the fix fails closed — the right default. Deployment coordination is the one operational item: until MS365_TOOLS_SECRET is set on the service and the registered endpoint carries the token, every MS365 chat-tool call returns 503. That is by design, but whoever rolls this out needs both sides at once.

USER IMPACT: MS365 chat tools (mail, calendar, Teams, files) stop responding on any deployment until the operator sets MS365_TOOLS_SECRET and re-registers the endpoint with the token — by design; it prevents anyone else from reading or sending a user's mail by naming their uid.

Both maintainer approvals on this head already stand; from a review standpoint this is ready for a merge decision, with the rollout note above.


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

@Git-on-my-level Git-on-my-level added positive-signal Automation verified a genuine fix/quality contribution and removed needs-maintainer-review Needs a human maintainer to sign off before merge labels Nov 3, 2026
@Git-on-my-level

Copy link
Copy Markdown
Member

Verified: correct, well-scoped security fix that mirrors the merged github/shipbob guard pattern; from automation's side nothing blocks beyond the human merge decision.

What I checked, file by file:

  • plugins/omi-ms365-app/ms365_tools_auth.py — line-for-line mirror of the merged plugins/omi-github-app/github_tools_auth.py: hmac.compare_digest comparison, fail-closed 503 when MS365_TOOLS_SECRET is unset or whitespace-only, 401 on missing/wrong/non-Bearer tokens; Bearer parse is case-insensitive on the scheme and requires non-empty credentials. No bypass path found.
  • plugins/omi-ms365-app/main.py — dependencies=[Depends(require_ms365_tools_auth)] sits on the /tools/{tool_name} decorator itself, so the guard runs before uid is read from the body; tool_args/_ENVELOPE_KEYS dispatch behavior is unchanged.
  • plugins/omi-ms365-app/test_ms365_tools_auth.py — substantive: 503 unconfigured/blank-secret, 401 missing/wrong-Bearer/wrong-query/non-Bearer, accepts Bearer (case-insensitive) and query token, plus wiring tests that import main.py and assert the route carries the dependency so it cannot be dropped unnoticed. I ran this suite and test_tool_dispatch.py hermetically (isolated container, no network, stdlib stubs only): 11 + 7 tests, all green.
  • plugins/omi-ms365-app/test_tool_dispatch.py — minimal, correct stub addition (Depends=lambda ...) so the stubbed import surface keeps matching main.py; existing dispatch assertions untouched.
  • plugins/test_oauth_error_params_escaped.py — same three-line Depends stub addition; no assertion changes.
  • .github/checks-manifest.yaml — new ms365-tools-auth-tests entry mirrors the neighboring ms365-tool-dispatch-tests triggers; nothing else touched.
  • plugins/omi-ms365-app/.env.example and README.md — accurate docs of the fail-closed semantics, both presentation forms, and the registered-endpoint-URL deployment path.

Security: closes a real gap — /tools/* acted on a named uid's stored Graph token (Mail.Read/Send, Calendars, Chat.ReadWrite, Files) with no caller authentication; unauthenticated callers now get 503/401.
Blast radius: the ms365 plugin tool route only; all other routes and plugins untouched.
Deployment note: the Omi backend's _call_tool_endpoint (backend/utils/retrieval/tools/app_tools.py) sends no auth headers today, so in practice the registered tool endpoint URL must carry ?ms365_tools_token=... (exactly as the README documents, same as the github/shipbob guards) or ms365 tool calls will fail closed after deploy — intended, but worth coordinating at registration time.
USER IMPACT: none once the secret is set at app registration; until then ms365 chat tools fail closed instead of answering, which is the intended safe default.

Non-blocking nit for the whole guard family (github/shipbob/composio too): hmac.compare_digest raises TypeError on a non-ASCII presented token, which surfaces as a 500 rather than 401 — the request is still denied, so cosmetic only.

Auth-surface sign-off stays with the human maintainers (two approvals already on this head); the merge call is theirs.


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

@github-actions

github-actions Bot commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

Hey @optimus-fulcria 👋

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

positive-signal Automation verified a genuine fix/quality contribution python security-review Touches auth, provider routing, secrets, or security-sensitive surfaces

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants