Skip to content

feat: stream attachment_get_data downloads to a directory - #364

Open
haizaar wants to merge 1 commit into
korotovsky:masterfrom
zarmory:feat/attachment-download-dir
Open

haizaar wants to merge 1 commit into
korotovsky:masterfrom
zarmory:feat/attachment-download-dir

Conversation

@haizaar

@haizaar haizaar commented Sep 30, 2026

Copy link
Copy Markdown

Summary

attachment_get_data returns the whole file inside the MCP response. That caps
it at 5 MB, and every file that is neither text nor an image arrives as base64
in a text block, which clients that cap tool output (common for agent hosts)
truncate into something undecodable well below 5 MB.

This adds an opt-in mode for clients that share the server's filesystem, such
as an agent running the server over stdio on the same host:

  • SLACK_MCP_ATTACHMENT_DIR: when set, the file is streamed into this
    directory as <file ID>-<file name> and the tool returns metadata plus the
    local path instead of the content. Bytes go to a hidden .partial file and
    are renamed into place when complete, so a failed download leaves nothing
    behind. Unset, behaviour is unchanged.
  • SLACK_MCP_ATTACHMENT_MAX_BYTES: the size limit, default 5 MiB as today.
    Checked against the size files.info reports and again while streaming. An
    invalid value fails at startup.
  • No 30 s cutoff on downloads. The shared HTTP client's Timeout: 30s
    covers reading the body, so any download slower than that failed, in either
    mode. Downloads now use a client without it and are bounded at 10 minutes
    through the request context.
  • The tool description follows the configured mode and limit.

Security

Following the concerns raised on #121 and #285:

  • Off by default. Without SLACK_MCP_ATTACHMENT_DIR nothing is written to disk.
  • The tool call supplies only file_id. The download URL is still the one
    files.info returns, so there is no caller-controlled URL to scope.
  • The directory is fixed by the operator. The file name is Slack's own, reduced
    to one path component (separators, characters Windows rejects and control
    characters replaced, length capped), so it cannot escape the directory.
  • Files are created 0644 subject to the process umask. The server never
    deletes them; retention is left to the operator.

Testing

  • Unit tests in pkg/handler/attachments_test.go (make test passes): atomic
    write and replace, no leftovers on an oversized body or a failed download,
    hostile file names, limit parsing, tool description.
  • Manually, against a local fake Slack reached through SLACK_MCP_PROXY:
    • 600 MiB file saved byte-identical in about 1 s, peak RSS 22 MiB.
    • 40 MiB file throttled to about 40 s completes; the same kind of throttled
      download fails at 30 s on master with Client.Timeout ... while reading body.
    • A file whose reported size exceeds the limit is refused before download; a
      body larger than its reported size is cut off at the limit, and no file is
      left.
  • Integration tests not run; they need live Slack tokens.

Relates to #247.

attachment_get_data returns the file inside the MCP response, which caps
it at 5 MB and sends everything that is neither text nor an image through
the client as base64 text.

- SLACK_MCP_ATTACHMENT_DIR: when set, the file is streamed into this
  directory and the tool returns its local path and metadata instead of
  its content. Bytes go to a hidden .partial file and are renamed into
  place once complete, so a failed download leaves nothing behind and a
  reader never sees a truncated file.
- SLACK_MCP_ATTACHMENT_MAX_BYTES: configurable size limit, default 5 MiB
  as before. Enforced against the size files.info reports and again while
  streaming. An invalid value fails at startup.
- File downloads no longer inherit the HTTP client's 30s request timeout,
  which cut off any download that took longer, in either mode. They are
  bounded at 10 minutes through the request context instead.
- The tool description follows the configured mode and limit.

Nothing new comes from the tool call: the download URL is still the one
files.info returns, and the file name is Slack's own, reduced to a single
path component inside the configured directory.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant