Skip to content

Feature: File attachments - #460

Open
CFDan wants to merge 2 commits into
mainfrom
feature/file-attachments
Open

Feature: File attachments#460
CFDan wants to merge 2 commits into
mainfrom
feature/file-attachments

Conversation

@CFDan

@CFDan CFDan commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Important

Depends on Teamwork/twapi-go-sdk#117. Do not merge before it.

go.mod currently points at a commit on that PR's branch (v1.21.4-0.20260815115947-d48d296bd66e) because the SDK functions this uses are unreleased. Once #117 merges and is tagged, this needs go get github.com/teamwork/twapi-go-sdk@<new tag> and a fresh commit before merging.

Lets an LLM attach a file to a task, comment or message. This is the customer request behind #117: models generate plans and specs and currently have no way to put them on a task.

Shape

1. twprojects-create_file  {name: "plan.md", data: "<base64>"}
   -> {reference: "tf_1a2b", ...}

2. twprojects-create_task  {name: "Ship it", tasklist_id: 123,
                            attachment_refs: ["tf_1a2b"]}

attachment_refs is also on update_task, create_comment and create_message. Two calls rather than one keeps the file out of the message that creates the task, so a failed create does not mean re-sending the payload.

Notes for review

The log redaction in internal/logsafe is a prerequisite, not a nice-to-have. LoggingRoundTripper reads every outbound body in full, so twdesk-create_file is writing raw file bytes into the logs today — this fixes that as a side effect. Content is replaced before the payload is capped, because base64 usually leads the arguments object: truncating alone would keep the readable start of a customer's document and cut the useful part of the record. Non-textual outbound bodies are elided rather than read at all.

No url parameter, deliberately. On a hosted multi-tenant server that is an SSRF with a direct exfiltration path — a prompt-injected model asks for 169.254.169.254, the server fetches from inside the VPC, and the response is stored as an attachment the attacker can read. Doing it safely needs resolved-IP checks, a re-check at connect time to beat DNS rebinding, a per-hop redirect policy and OpenWorldHint: true. Base64 only.

5 MB decoded cap, and maxBodySize is unchanged. Raising it would not help: the binding constraint is the caller's output-token budget, not the transport, at roughly one output token per three bytes of file. The size is checked on the encoded length before decoding, so an oversized payload is rejected without being allocated twice — pinned by a test asserting no HTTP request is made.

attachmentOptions is not sent. Its only field defaults to false, and on task update the API seeds the "keep these" list from the task's current attachments, so it removes nothing. Attaching is additive. There is also no twprojects-list_files, so a caller could not obtain an identifier to detach with.

No mime_type parameter, unlike twdesk-create_file. The upload endpoint derives the type from the extension server side, and the Desk model genuinely stores one where this does not.

Testing

internal/twprojects/files_test.go asserts the parameters reach the wire: that the upload is multipart with the decoded bytes under a part named file, that attachments is a sibling of task rather than one of its attributes, that comment and message send pendingFileAttachments, and that no attachments key is sent when the caller asked for none. Filename sanitisation is table-driven over path, traversal and control-character inputs. Bad input comes back as a readable tool result rather than a transport error.

internal/logsafe has its own tests, including one that runs an 8 MB body through the pattern to guard a future rewrite from introducing backtracking.

Tool definitions grow by 553 tokens, +1.10%, measured with cmd/mcp-tokens.

The underlying API calls were verified live against a real installation as part of #117. I could not drive the MCP server itself end to end here: it authenticates with a bearer token and the API token I had is not valid as one, so the coverage above is the mock-level wire assertions rather than a live round trip.

🤖 Generated with Claude Code

Adds twprojects-create_file, which takes a file as base64 and returns a
single-use reference, and an attachment_refs parameter on create_task,
update_task, create_comment and create_message that consumes it.

Two calls rather than one keeps the file out of the message that creates the
task, so a failed create does not mean re-sending the payload. Uploading is
capped at 5 MB decoded, checked before decoding so an oversized payload is
rejected without allocating it twice. The real ceiling is far lower: the caller
has to emit the base64 itself, at roughly one output token per three bytes.

Adds internal/logsafe and routes the request body and tool argument log sites
through it. File content is replaced before the payload is capped, because
base64 usually leads the arguments object and truncation alone would keep the
readable start of a customer's document while cutting the tool name and the
other parameters. Outbound bodies that are not text are elided rather than
read, which also stops twdesk-create_file writing raw file bytes into the log
as it does today.

Attachments are additive. attachmentOptions is not sent: its only field
defaults to false, and on task update the API seeds the "keep these" list from
the task's current attachments, so it removes nothing.

Tool definitions grow by 553 tokens, 1.10%.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@CFDan
CFDan requested a review from rafaeljusto August 15, 2026 12:10
@CFDan
CFDan requested a review from a team as a code owner August 15, 2026 12:10
@CFDan CFDan added the enhancement New feature or request label Aug 15, 2026
Redaction now catches a file of any size: the value class is "anything but a
quote" with no length floor, so a small secret no longer survives because its
base64 is short, and it is not defeated by JSON escaping such as "\/". "content"
is dropped from the redacted keys, since page and comment bodies use it for text
worth keeping. A byte-scan fast path skips the regex for the payloads that carry
no file, which is almost all of them.

The logging round tripper no longer scans outbound API traffic for file
content. The only file it can see is the multipart upload, which the
content-type gate already elides, so the scan had no reachable file to catch and
could only corrupt an unrelated response body that happened to carry a long
value under one of those keys.

create_file moves to the projects toolset. attachment_refs is exposed by tools
in both the tasks and content toolsets, and a project-level file is the right
home for the tool that mints the references they consume.

create_comment and create_message route attachment_refs through the same
cleaner the task tools use, so a blank reference is dropped consistently rather
than forwarded to the API. The filename assertion in the tests drops the
bespoke multipart parser for the substring check the sibling test already uses.

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

CFDan commented Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Ran a full review (correctness + reuse/simplification/efficiency/altitude) and pushed 5e15dfd.

Fixed

  • Redaction missed small files. The old regex required a 256-char base64 run, so a file whose base64 was shorter (a short secret, a small .env) was logged in full — the one thing this feature exists to prevent. The value class is now [^"]* with no floor, so a file of any size is redacted, and it is no longer defeated by JSON \/ escaping. A bytes.Contains fast path skips the regex for the payloads that carry no file.
  • content dropped from the redacted keys. twspaces page/comment bodies use content for real text; redacting it would have scrubbed legitimate content from logs. Kept data/fileData/file_data, which are upload-only.
  • Removed the redaction scan from the logging round tripper. The only file it can see outbound is the multipart upload, which the content-type gate already elides, so the scan had no reachable file to catch and could only corrupt an unrelated API response that happened to carry a long value under one of those keys. The round tripper keeps the content-type elision (that is what actually protects the file bytes) and logs JSON bodies as-is.
  • create_file moved to the projects toolset. attachment_refs is exposed by tools in both the tasks (create/update_task) and content (create_comment/create_message) toolsets; a project-level file is the right home for the tool that mints the references, and it can't be registered in two toolsets (the doc-gen bijection guard forbids it).
  • create_comment/create_message now route attachment_refs through the same cleaner the task tools use, so a blank reference is dropped consistently instead of forwarded to the API.
  • Test simplification: the filename assertion drops a bespoke multipart parser (and the errString sentinel type) for the strings.Contains(filename="...") check the sibling test already uses.

Checked and left as-is

  • The [^"]* value class over a \/-escaped base64 is robust; Go/JS/Python encoders don't escape / anyway, but the class covers it if one does.
  • Round tripper body restoration (io.NopCloser) is correct on the textual path; the non-textual path never reads the body, so the transport still sends it intact.
  • Size-limit layering (schema MaxLength + encoded-length pre-check + decoded check) is correct and stays.

Reminder: still gated on twapi-go-sdk#117 — go.mod points at a branch pseudo-version until that tags.

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant