Skip to content

fix(teams): deliver documents via file consent cards - #89491

Closed
HermeticOrmus wants to merge 1 commit into
NousResearch:mainfrom
HermeticOrmus:feat/teams-file-consent
Closed

fix(teams): deliver documents via file consent cards#89491
HermeticOrmus wants to merge 1 commit into
NousResearch:mainfrom
HermeticOrmus:feat/teams-file-consent

Conversation

@HermeticOrmus

Copy link
Copy Markdown

What does this PR do?

send_document on Teams silently failed for every non-image file. The adapter base64s local files into a data: URI attachment; Bot Framework accepts that for images and returns HTTP 400 for everything else (.xlsx, .pdf, .csv, …). The text that went with the file still claimed it was attached.

The supported Bot Framework path for a bot to deliver a document in a personal chat is the file consent card: the user accepts, Teams returns a pre-authorized upload URL into that user's own OneDrive, and the bot PUTs the bytes there. No Graph permissions are required.

This PR routes local send_document calls in personal chats through that flow, and keeps the existing attachment path for group chats / channels (Teams has no consent flow there) and for HTTP(S) URLs.

Related Issue

No existing issue or PR covers Teams send_document / file consent (searched open + closed). Group-chat document delivery remains a platform constraint, not addressed here.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Security fix
  • Documentation update
  • Tests (adding or improving test coverage)
  • Refactor (no behavior change)
  • New skill (bundled or hub)

Changes Made

  • plugins/platforms/teams/adapter.py
    • Disk-backed pending map ($HERMES_HOME/pending_file_consents.json, mode 0600, 24h TTL) keyed by an opaque token — never a path — so a short-lived hermes send and the long-lived gateway can share state, and a tampered invoke cannot upload an arbitrary local file.
    • _send_file_consent offers the card; _on_file_consent (registered via @app.on_file_consent) completes or discards it.
    • Accept is gated on the same TEAMS_ALLOWED_USERS / TEAMS_ALLOW_ALL_USERS allowlist as card actions.
    • send_document uses the consent path for local files in personal chat; group/channel ids (19:…) and remote URLs keep the attachment path.
  • tests/gateway/test_teams.py — personal-chat offer, empty/oversized rejection, accept upload + token drop, decline, unknown token, allowlist gate. Existing group-chat send_document test still expects the attachment path.

Scope note: consent is personal-chat only. The tenant Teams app manifest must set bots[].supportsFiles: true or the Allow button fails with "This card action is not supported by <bot>".

How to Test

  1. Targeted: pytest tests/gateway/test_teams.py -q (30 passed on this branch).
  2. Live: enable Teams, send a local .xlsx from a personal chat. A consent card should render; after Allow, the file lands in the recipient's OneDrive and a file-info card appears. Gateway log: [teams] Uploaded … after consent.
  3. Group/channel send should be unchanged (attachment path; documents still cannot be delivered there).

I did not run the full pytest tests/ -q suite locally. The install/update tests in this repo rebuild .venv and reset the git checkout; targeted tests + CI are the safer split.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(teams):)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've run pytest tests/ -q and all tests pass — targeted test_teams.py only; see above
  • I've added tests for my changes
  • I've tested on my platform: Ubuntu 24.04 (targeted tests) + a production Teams tenant (live consent flow)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — N/A (adapter docstring + comments)
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) — pending store uses HERMES_HOME + os.replace
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Bot Framework rejects non-image data: URI attachments with HTTP 400,
so send_document silently failed for .xlsx/.pdf/.csv. Personal chats
now offer a file consent card; accept uploads to the user's OneDrive
via the pre-authorized URL. Pending offers persist on disk as opaque
tokens so a short-lived hermes send and the gateway process can share
state without echoing a path through the client.

Group chats and channels keep the old attachment path — Teams has no
consent flow there. Tenants must set bots[].supportsFiles in the app
manifest or Allow fails with "card action is not supported".

Verified: pytest tests/gateway/test_teams.py — 30 passed
@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins area/auth Authentication, OAuth, credential pools P3 Low — cosmetic, nice to have sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Aug 18, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference; please use your judgment.

Solid design overall (opaque token, disk-backed pending map, allowlist gate, behavioral tests against a temp HERMES_HOME). A few concrete concerns:

  • profiles: _file_consent_store() falls back to hardcoded os.path.expanduser("~/.hermes") instead of get_hermes_home() from hermes_constants — breaks profile isolation if HERMES_HOME isn't set in the process env (AGENTS.md pitfall; source of PR fix: replace hardcoded ~/.hermes paths with get_hermes_home() for profile support #3575-class bugs).
  • multiplex authz (fail-open risk): _is_allowed_teams_user reads TEAMS_ALLOWED_USERS / TEAMS_ALLOW_ALL_USERS via bare os.getenv. This file already has _get_scoped_secret for exactly this reason ([Bug] Feishu multiplex: DM allow-list and allow-all flags ignore profile-scoped .env — role-bot DMs silently rejected (dm_policy_rejected) #86905): under gateway.multiplex_profiles, os.environ holds the default profile's values, so a secondary profile's consent accept can be authorized by the default profile's allowlist/allow-all. The existing card-action guard at ~line 1085 shares the flaw, but new authorization reads should use the scoped helper.
  • concurrency: the pending store is an unsynchronized read-modify-write of one JSON file shared between a short-lived hermes send and the gateway — two concurrent offers (or offer + accept) can silently drop a token. Per-token files or a lock file would close it.
  • uploads: accept path reads up to 250 MB into memory for a single PUT; OneDrive simple upload may reject large payloads (chunked sessions exist). Consider streaming or a lower cap.
  • fallback: when the consent offer fails, send_document falls back to the data:-URI attachment that 400s for non-images — the original silent-failure symptom persists (now logged); returning the error may be more honest than a guaranteed-to-fail fallback.

@HermeticOrmus HermeticOrmus closed this by deleting the head repository Aug 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants