From be9e059db95c786002381e56cc4ca350e5f59232 Mon Sep 17 00:00:00 2001 From: Daniel Mackey Date: Sat, 15 Aug 2026 13:09:42 +0100 Subject: [PATCH 1/2] Feature: File attachments 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) --- cmd/mcp-http/main.go | 5 +- docs/tool-reference.md | 1 + go.mod | 2 +- go.sum | 2 + internal/config/config.go | 5 +- internal/logsafe/logsafe.go | 95 +++++++++ internal/logsafe/logsafe_test.go | 103 ++++++++++ internal/network/roundtripper.go | 28 ++- internal/twprojects/comments.go | 4 +- internal/twprojects/files.go | 259 ++++++++++++++++++++++++ internal/twprojects/files_test.go | 321 ++++++++++++++++++++++++++++++ internal/twprojects/messages.go | 4 +- internal/twprojects/tasks.go | 25 ++- internal/twprojects/tools.go | 1 + 14 files changed, 835 insertions(+), 20 deletions(-) create mode 100644 internal/logsafe/logsafe.go create mode 100644 internal/logsafe/logsafe_test.go create mode 100644 internal/twprojects/files.go create mode 100644 internal/twprojects/files_test.go diff --git a/cmd/mcp-http/main.go b/cmd/mcp-http/main.go index 68a67b99..f025a3aa 100644 --- a/cmd/mcp-http/main.go +++ b/cmd/mcp-http/main.go @@ -24,6 +24,7 @@ import ( "github.com/teamwork/mcp/internal/auth" "github.com/teamwork/mcp/internal/cli" "github.com/teamwork/mcp/internal/config" + "github.com/teamwork/mcp/internal/logsafe" "github.com/teamwork/mcp/internal/request" "github.com/teamwork/mcp/internal/toolsets" "github.com/teamwork/mcp/internal/twchat" @@ -321,7 +322,7 @@ func logMiddleware(logger *slog.Logger, next http.Handler) http.Handler { slog.String("request_url", r.URL.String()), slog.String("request_method", r.Method), slog.Any("request_headers", headers), - slog.String("request_body", string(reqBody)), + slog.String("request_body", logsafe.String(string(reqBody))), slog.Int("response_status", rw.StatusCode()), slog.Any("response_headers", rw.Header()), slog.String("response_body", string(rw.Body())), @@ -388,7 +389,7 @@ func sseLogMiddleware(logger *slog.Logger, next http.Handler) http.Handler { slog.String("request_url", r.URL.String()), slog.String("request_method", r.Method), slog.Any("request_headers", headers), - slog.String("request_body", string(reqBody)), + slog.String("request_body", logsafe.String(string(reqBody))), slog.Int("response_status", rw.StatusCode()), slog.Any("response_headers", rw.Header()), slog.String("response_body", string(rw.Body())), diff --git a/docs/tool-reference.md b/docs/tool-reference.md index dbaadac4..765f39c7 100644 --- a/docs/tool-reference.md +++ b/docs/tool-reference.md @@ -66,6 +66,7 @@ Task, tasklist, and workflow management in Teamwork.com. | Tasklist | ✓ | ✓ | ✓ | ✓ | | Workflow | ✓ | ✓ | ✓ | ✓ | | Workflow Stage | ✓ | ✓ | ✓ | ✓ | +| File | ✓ | — | — | — | **Other actions:** `complete_task`, `link_project_to_workflow`, `move_task_to_workflow_stage`, `move_tasks` diff --git a/go.mod b/go.mod index 49223428..e17b5b4b 100644 --- a/go.mod +++ b/go.mod @@ -14,7 +14,7 @@ require ( github.com/sonh/qs v0.7.0 github.com/teamwork/desksdkgo v1.1.0 github.com/teamwork/spacessdkgo v0.0.0-20260518181558-a6af69d00abb - github.com/teamwork/twapi-go-sdk v1.21.3 + github.com/teamwork/twapi-go-sdk v1.21.4-0.20260815115947-d48d296bd66e ) require ( diff --git a/go.sum b/go.sum index b4875132..e606bc72 100644 --- a/go.sum +++ b/go.sum @@ -194,6 +194,8 @@ github.com/teamwork/twapi-go-sdk v1.21.2 h1:SsLaxc15Q+m1v+LO0UHWoxCJ+I4wf6uTftfV github.com/teamwork/twapi-go-sdk v1.21.2/go.mod h1:uj6BNtyyKtggfeY3whzn8bLWuhP1twhghjWD6z3h3F4= github.com/teamwork/twapi-go-sdk v1.21.3 h1:xTE2l2Xfw9WQLIlWYUNazQM4y4N6K3Y34CJuUO0xTSM= github.com/teamwork/twapi-go-sdk v1.21.3/go.mod h1:uj6BNtyyKtggfeY3whzn8bLWuhP1twhghjWD6z3h3F4= +github.com/teamwork/twapi-go-sdk v1.21.4-0.20260815115947-d48d296bd66e h1:kaEfM0XTcz0B08P39BFg+e/zg5s2Cd2F2HvTUmVcRKs= +github.com/teamwork/twapi-go-sdk v1.21.4-0.20260815115947-d48d296bd66e/go.mod h1:uj6BNtyyKtggfeY3whzn8bLWuhP1twhghjWD6z3h3F4= github.com/tinylib/msgp v1.6.3 h1:bCSxiTz386UTgyT1i0MSCvdbWjVW+8sG3PjkGsZQt4s= github.com/tinylib/msgp v1.6.3/go.mod h1:RSp0LW9oSxFut3KzESt5Voq4GVWyS+PSulT77roAqEA= github.com/tklauser/go-sysconf v0.3.16 h1:frioLaCQSsF5Cy1jgRBrzr6t502KIIwQ0MArYICU0nA= diff --git a/internal/config/config.go b/internal/config/config.go index 4ed70018..31096521 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -21,6 +21,7 @@ import ( "github.com/getsentry/sentry-go" "github.com/modelcontextprotocol/go-sdk/mcp" desksdk "github.com/teamwork/desksdkgo/client" + "github.com/teamwork/mcp/internal/logsafe" "github.com/teamwork/mcp/internal/network" "github.com/teamwork/mcp/internal/request" "github.com/teamwork/mcp/internal/toolsets" @@ -234,7 +235,7 @@ func NewMCPServer(resources Resources, groups ...*toolsets.ToolsetGroup) *mcp.Se span.SetTag("mcp.method", method) if callToolParams, ok := req.GetParams().(*mcp.CallToolParamsRaw); ok { span.SetTag("mcp.tool.name", callToolParams.Name) - span.SetTag("mcp.tool.arguments", string(callToolParams.Arguments)) + span.SetTag("mcp.tool.arguments", logsafe.String(string(callToolParams.Arguments))) } if callToolResult, ok := result.(*mcp.CallToolResult); ok { if callToolResult.IsError { @@ -359,7 +360,7 @@ func mcpLoggingMiddleware(resources Resources) mcp.Middleware { if params, ok := req.GetParams().(*mcp.CallToolParamsRaw); ok { attrs = append(attrs, slog.String("mcp.tool.name", params.Name), - slog.String("mcp.tool.arguments", string(params.Arguments)), + slog.String("mcp.tool.arguments", logsafe.String(string(params.Arguments))), ) } diff --git a/internal/logsafe/logsafe.go b/internal/logsafe/logsafe.go new file mode 100644 index 00000000..2a439cb9 --- /dev/null +++ b/internal/logsafe/logsafe.go @@ -0,0 +1,95 @@ +// Package logsafe scrubs request payloads before they reach a log sink or a +// trace tag. +// +// Two things have to happen and the order matters. File content is replaced +// first, so an attachment does not leave part of a customer's document in the +// log; whatever survives is then capped, so an oversized payload of any shape +// cannot fill it. Truncation on its own is not enough, because base64 usually +// leads the arguments object: the retained head would be the readable start of +// the file, and the useful part of the record, the tool name and the other +// parameters, would be what got cut. +package logsafe + +import ( + "bytes" + "fmt" + "regexp" + "strings" +) + +// MaxLoggedBytes caps a scrubbed payload. It matches the cap the response +// writer already applies, so inbound and outbound bodies are treated alike. +const MaxLoggedBytes = 64 << 10 + +const truncatedSuffix = "...[truncated]" + +// contentValue matches a long JSON string value under one of the keys that +// carry file content. +// +// The alternation is anchored on the key and the value character class is the +// base64 alphabet plus whitespace, so there is no nested quantifier and the +// expression stays linear over a multi-megabyte body. The 256 character floor +// keeps genuinely short values readable, and matching on the key means a search +// term that happens to be the word "data" is left alone. +var contentValue = regexp.MustCompile(`"(data|content|fileData|file_data)"\s*:\s*"[A-Za-z0-9+/=_\-\s]{256,}"`) + +// Bytes returns b with file content replaced and the result capped. +func Bytes(b []byte) []byte { + if len(b) == 0 { + return b + } + scrubbed := contentValue.ReplaceAllFunc(b, func(match []byte) []byte { + key := match[:bytes.IndexByte(match, ':')] + return fmt.Appendf(nil, `%s:""`, key, len(match)) + }) + if len(scrubbed) > MaxLoggedBytes { + return append(bytes.Clone(scrubbed[:MaxLoggedBytes]), truncatedSuffix...) + } + return scrubbed +} + +// String is Bytes over a string. +func String(s string) string { + if len(s) == 0 { + return s + } + return string(Bytes([]byte(s))) +} + +// IsTextualContentType reports whether a body of this content type is worth +// capturing in a log. A pending file upload sends raw bytes, and capturing them +// would copy the customer's file into the log and hold a second copy in memory +// for the life of the request. +func IsTextualContentType(contentType string) bool { + if contentType == "" { + // No declared type: the bodies the server sends without one are JSON, and + // anything carrying a file always declares its type. + return true + } + mediaType := contentType + if index := strings.IndexByte(mediaType, ';'); index >= 0 { + mediaType = mediaType[:index] + } + mediaType = strings.ToLower(strings.TrimSpace(mediaType)) + + if strings.HasPrefix(mediaType, "text/") { + return true + } + switch mediaType { + case "application/json", + "application/x-www-form-urlencoded", + "application/xml", + "application/javascript": + return true + } + return strings.HasSuffix(mediaType, "+json") || strings.HasSuffix(mediaType, "+xml") +} + +// ElidedBody is the placeholder logged in place of a body that is not worth +// capturing. +func ElidedBody(length int64, contentType string) string { + if length < 0 { + return fmt.Sprintf("", contentType) + } + return fmt.Sprintf("<%d bytes of %s elided>", length, contentType) +} diff --git a/internal/logsafe/logsafe_test.go b/internal/logsafe/logsafe_test.go new file mode 100644 index 00000000..c5dc20a9 --- /dev/null +++ b/internal/logsafe/logsafe_test.go @@ -0,0 +1,103 @@ +package logsafe_test + +import ( + "encoding/base64" + "strings" + "testing" + + "github.com/teamwork/mcp/internal/logsafe" +) + +func TestBytesRedactsFileContent(t *testing.T) { + content := base64.StdEncoding.EncodeToString(make([]byte, 4096)) + payload := `{"name":"twprojects-create_file","arguments":{"data":"` + content + `","name":"plan.md"}}` + + got := logsafe.String(payload) + + if strings.Contains(got, content[:256]) { + t.Error("expected the file content to be redacted") + } + // The point of redacting rather than truncating is that the rest of the + // record survives, so the log still says which tool ran and with what. + if !strings.Contains(got, "twprojects-create_file") { + t.Errorf("expected the tool name to survive redaction, got %q", got) + } + if !strings.Contains(got, `"name":"plan.md"`) { + t.Errorf("expected the other parameters to survive redaction, got %q", got) + } + if !strings.Contains(got, "redacted") { + t.Errorf("expected the placeholder to say what happened, got %q", got) + } +} + +func TestBytesLeavesShortValuesAlone(t *testing.T) { + // A short value under one of the content keys is readable and worth keeping, + // and a search term that happens to be the word "data" is not a key at all. + payload := `{"arguments":{"data":"plan","search_term":"data"}}` + + if got := logsafe.String(payload); got != payload { + t.Errorf("expected the payload unchanged, got %q", got) + } +} + +func TestBytesTruncatesOversizePayload(t *testing.T) { + // A payload that carries no recognisable content key still must not fill the + // log. + payload := `{"body":"` + strings.Repeat("a", logsafe.MaxLoggedBytes*2) + `"}` + + got := logsafe.String(payload) + + if len(got) > logsafe.MaxLoggedBytes+len("...[truncated]") { + t.Errorf("expected the payload capped, got %d bytes", len(got)) + } + if !strings.HasSuffix(got, "...[truncated]") { + t.Error("expected the payload to say it was truncated") + } +} + +func TestBytesHandlesEmpty(t *testing.T) { + if got := logsafe.Bytes(nil); got != nil { + t.Errorf("expected nil, got %q", got) + } + if got := logsafe.String(""); got != "" { + t.Errorf("expected an empty string, got %q", got) + } +} + +func TestIsTextualContentType(t *testing.T) { + tests := []struct { + contentType string + want bool + }{ + {"", true}, // no declared type: the bodies we send without one are JSON + {"application/json", true}, + {"application/json; charset=utf-8", true}, + {"text/plain", true}, + {"application/vnd.api+json", true}, + {"application/x-www-form-urlencoded", true}, + {"multipart/form-data; boundary=abc", false}, // a file upload + {"application/octet-stream", false}, + {"image/png", false}, + } + + for _, tt := range tests { + t.Run(tt.contentType, func(t *testing.T) { + if got := logsafe.IsTextualContentType(tt.contentType); got != tt.want { + t.Errorf("expected %v for %q, got %v", tt.want, tt.contentType, got) + } + }) + } +} + +// TestBytesStaysLinear guards against a future rewrite of the pattern +// introducing backtracking. A multi-megabyte body runs through this on every +// upload. +func TestBytesStaysLinear(t *testing.T) { + payload := []byte(`{"data":"` + strings.Repeat("A", 8<<20) + `"}`) + + got := logsafe.Bytes(payload) + + if len(got) > logsafe.MaxLoggedBytes+len("...[truncated]") { + t.Errorf("expected the payload capped, got %d bytes", len(got)) + } +} diff --git a/internal/network/roundtripper.go b/internal/network/roundtripper.go index ae70c0ce..8260e88a 100644 --- a/internal/network/roundtripper.go +++ b/internal/network/roundtripper.go @@ -8,6 +8,7 @@ import ( "strings" "time" + "github.com/teamwork/mcp/internal/logsafe" "github.com/teamwork/mcp/internal/request" ) @@ -29,14 +30,19 @@ func NewLoggingRoundTripper(logger *slog.Logger, base http.RoundTripper) *Loggin func (lrt *LoggingRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { start := time.Now() - var reqBody []byte - if r.Body != nil { - var err error - reqBody, err = io.ReadAll(r.Body) + // A body that is not text is not worth capturing: a pending file upload + // sends the file itself, so reading it here would copy the customer's file + // into the log and hold a second copy in memory until the request finishes. + var loggedRequestBody string + if r.Body != nil && !logsafe.IsTextualContentType(r.Header.Get("Content-Type")) { + loggedRequestBody = logsafe.ElidedBody(r.ContentLength, r.Header.Get("Content-Type")) + } else if r.Body != nil { + reqBody, err := io.ReadAll(r.Body) if err != nil { lrt.Log.Error("failed to read request body", slog.String("error", err.Error())) } r.Body = io.NopCloser(bytes.NewBuffer(reqBody)) + loggedRequestBody = logsafe.String(string(reqBody)) } headers := r.Header.Clone() @@ -59,14 +65,16 @@ func (lrt *LoggingRoundTripper) RoundTrip(r *http.Request) (*http.Response, erro return resp, err } - var respBody []byte - if resp.Body != nil { - respBody, err = io.ReadAll(resp.Body) + var loggedResponseBody string + if resp.Body != nil && !logsafe.IsTextualContentType(resp.Header.Get("Content-Type")) { + loggedResponseBody = logsafe.ElidedBody(resp.ContentLength, resp.Header.Get("Content-Type")) + } else if resp.Body != nil { + respBody, err := io.ReadAll(resp.Body) if err != nil { lrt.Log.Error("failed to read response body", "error", err) } - resp.Body = io.NopCloser(bytes.NewBuffer(respBody)) + loggedResponseBody = logsafe.String(string(respBody)) } info, _ := request.InfoFromContext(r.Context()) @@ -75,10 +83,10 @@ func (lrt *LoggingRoundTripper) RoundTrip(r *http.Request) (*http.Response, erro slog.String("request_url", r.URL.String()), slog.String("request_method", r.Method), slog.Any("request_headers", headers), - slog.String("request_body", string(reqBody)), + slog.String("request_body", loggedRequestBody), slog.Int("response_status", resp.StatusCode), slog.Any("response_headers", resp.Header), - slog.String("response_body", string(respBody)), + slog.String("response_body", loggedResponseBody), slog.String("duration", time.Since(start).String()), slog.Int64("installation.id", info.InstallationID()), slog.String("installation.url", info.InstallationURL()), diff --git a/internal/twprojects/comments.go b/internal/twprojects/comments.go index a8752aad..c53f76e8 100644 --- a/internal/twprojects/comments.go +++ b/internal/twprojects/comments.go @@ -128,7 +128,8 @@ func CommentCreate(engine *twapi.Engine) toolsets.ToolWrapper { {Type: "null"}, }, }, - "notify": helpers.NotifySchema("Who to notify of the new comment.", true), + "notify": helpers.NotifySchema("Who to notify of the new comment.", true), + "attachment_refs": attachmentRefsSchema("comment"), }, Required: []string{"object", "body"}, }, @@ -144,6 +145,7 @@ func CommentCreate(engine *twapi.Engine) toolsets.ToolWrapper { helpers.RequiredParam(&commentCreateRequest.Body, "body"), helpers.OptionalPointerParam(&commentCreateRequest.ContentType, "content_type"), helpers.OptionalPointerParam(&commentCreateRequest.NotifyCurrentUser, "notify_current_user"), + helpers.OptionalListParam(&commentCreateRequest.PendingFileAttachments, "attachment_refs"), ) if err != nil { return helpers.NewToolResultTextError("invalid parameters: %s", err.Error()), nil diff --git a/internal/twprojects/files.go b/internal/twprojects/files.go new file mode 100644 index 00000000..2740a782 --- /dev/null +++ b/internal/twprojects/files.go @@ -0,0 +1,259 @@ +package twprojects + +import ( + "context" + "encoding/base64" + "encoding/json" + "fmt" + "path" + "strings" + + "github.com/google/jsonschema-go/jsonschema" + "github.com/modelcontextprotocol/go-sdk/mcp" + twapi "github.com/teamwork/twapi-go-sdk" + "github.com/teamwork/twapi-go-sdk/projects" + + "github.com/teamwork/mcp/internal/helpers" + "github.com/teamwork/mcp/internal/toolsets" +) + +// List of methods available in the Teamwork.com MCP service. +// +// The naming convention for methods follows a pattern described here: +// https://github.com/github/github-mcp-server/issues/333 +const ( + MethodFileCreate toolsets.Method = "twprojects-create_file" +) + +// maxAttachmentBytes caps the decoded size of an inline attachment. +// +// The hard ceiling is the HTTP server's maximum request body, which covers the +// whole JSON-RPC envelope: base64 grows by four thirds, so 5 MB of file already +// costs about 6.7 MB on the wire before JSON escaping and the rest of the +// message. Rejecting here rather than at the transport turns an unreadable +// connection reset into a tool result the caller can act on. +// +// In practice the binding limit is far lower. The caller has to emit the base64 +// itself, at roughly one output token per three bytes of file, so a megabyte is +// already out of reach. This limit exists to keep the server standing, not to +// describe what is usable. +const maxAttachmentBytes = 5 << 20 + +// maxFileNameBytes bounds the stored name. Most filesystems stop around 255 +// bytes; the extension is preserved when truncating so the file still opens +// with the right application. +const maxFileNameBytes = 200 + +// attachmentRefsSchema returns the schema for the pending file references +// parameter. The entity is named in the description so the caller knows what it +// is attaching to. +// +// There is no minimum length: a caller that sends an empty array to mean "none" +// gets a no-op rather than a validation failure. +func attachmentRefsSchema(entity string) *jsonschema.Schema { + return &jsonschema.Schema{ + Description: fmt.Sprintf( + "References of files to attach to the %s, as returned by %s. Each looks like "+ + "\"tf_1a2b\" and can only be used once, so upload a file for each place it "+ + "should go. Files are added to whatever is already attached; nothing is removed.", + entity, MethodFileCreate), + AnyOf: []*jsonschema.Schema{ + {Type: "array", Items: &jsonschema.Schema{Type: "string"}}, + {Type: "null"}, + }, + } +} + +// fileCreateResult is what the tool hands back. Reference is the value the +// attachment parameters take. +type fileCreateResult struct { + Reference string `json:"reference"` + Name string `json:"name"` + Size int64 `json:"size"` + Usage string `json:"usage"` +} + +// FileCreate uploads a file to Teamwork.com so that it can be attached to +// something. +func FileCreate(engine *twapi.Engine) toolsets.ToolWrapper { + return toolsets.ToolWrapper{ + Tool: &mcp.Tool{ + Name: string(MethodFileCreate), + Description: fmt.Sprintf("Upload a file so it can be attached to a task, comment or message. "+ + "Returns a single-use reference like \"tf_1a2b\"; pass it in attachment_refs on %s, %s, "+ + "%s or %s. Content is sent inline as base64, so this suits text you generated, such as "+ + "plans, specs or CSV, rather than large binaries.", + MethodTaskCreate, MethodTaskUpdate, MethodCommentCreate, MethodMessageCreate), + Annotations: &mcp.ToolAnnotations{ + Title: "Create File", + // The upload goes to storage Teamwork.com manages and the file lands + // in the caller's own account, so nothing here reaches outside it. + DestructiveHint: new(false), + OpenWorldHint: new(false), + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "name": { + Type: "string", + MinLength: new(1), + Description: "The file name, including its extension, for example \"plan.md\". " + + "Teamwork.com works out how to display the file from the extension, so a name " + + "without one is harder to open. Any directory part is removed.", + }, + "data": { + Type: "string", + MaxLength: new(base64.StdEncoding.EncodedLen(maxAttachmentBytes)), + Description: fmt.Sprintf("The file content, base64-encoded with the standard "+ + "alphabet. It must decode to between 1 byte and %d bytes.", maxAttachmentBytes), + }, + }, + Required: []string{"name", "data"}, + }, + }, + Handler: func(ctx context.Context, request *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + var arguments map[string]any + if err := json.Unmarshal(request.Params.Arguments, &arguments); err != nil { + return helpers.NewToolResultTextError("failed to decode request: %s", err.Error()), nil + } + + var name, data string + if err := helpers.ParamGroup(arguments, + helpers.RequiredParam(&name, "name"), + helpers.RequiredParam(&data, "data"), + ); err != nil { + return helpers.NewToolResultTextError("invalid parameters: %s", err.Error()), nil + } + + name, err := sanitizeFileName(name) + if err != nil { + return helpers.NewToolResultTextError("invalid name: %s", err.Error()), nil + } + + // Check the encoded length first: decoding allocates the payload a + // second time, and there is no reason to pay for that only to reject + // the result. + if len(data) > base64.StdEncoding.EncodedLen(maxAttachmentBytes) { + return helpers.NewToolResultTextError( + "file is too large: %d base64 characters exceed the %d byte limit on decoded "+ + "content. Upload a smaller file, or split the content across several.", + len(data), maxAttachmentBytes), nil + } + + // Not an API failure: the caller supplied malformed base64, so report + // it as a tool result it can correct rather than as a transport error. + content, err := base64.StdEncoding.DecodeString(data) + if err != nil { + return helpers.NewToolResultTextError("failed to decode base64 data: %s", err.Error()), nil + } + switch { + case len(content) == 0: + return helpers.NewToolResultTextError( + "the data decoded to zero bytes, so there is nothing to upload"), nil + case len(content) > maxAttachmentBytes: + return helpers.NewToolResultTextError( + "file is too large: %d bytes exceed the %d byte limit. Upload a smaller file, "+ + "or split the content across several.", + len(content), maxAttachmentBytes), nil + } + + pendingFile, err := projects.PendingFileCreate(ctx, engine, + projects.NewPendingFileCreateRequest(name, content)) + if err != nil { + return helpers.HandleAPIError(err, "failed to upload file") + } + + return helpers.NewToolResultJSON(fileCreateResult{ + Reference: string(pendingFile.PendingFile.Ref), + Name: name, + Size: int64(len(content)), + Usage: fmt.Sprintf("Pass %q in attachment_refs on %s, %s, %s or %s.", + pendingFile.PendingFile.Ref, MethodTaskCreate, MethodTaskUpdate, + MethodCommentCreate, MethodMessageCreate), + }) + }, + } +} + +// parseAttachmentRefs reads the attachment references shared by the tools that +// can attach a file. It returns nil when the caller named none, so that the +// request omits the field entirely rather than sending an empty set. +func parseAttachmentRefs(arguments map[string]any) ([]projects.PendingFileRef, *mcp.CallToolResult) { + var refs []projects.PendingFileRef + if err := helpers.ParamGroup(arguments, + helpers.OptionalListParam(&refs, "attachment_refs"), + ); err != nil { + return nil, helpers.NewToolResultTextError("invalid attachment_refs: %s", err.Error()) + } + + cleaned := make([]projects.PendingFileRef, 0, len(refs)) + for _, ref := range refs { + if trimmed := projects.PendingFileRef(strings.TrimSpace(string(ref))); trimmed != "" { + cleaned = append(cleaned, trimmed) + } + } + if len(cleaned) == 0 { + return nil, nil + } + return cleaned, nil +} + +// parseTaskAttachments reads the attachment references for the task tools, +// which take the structured form rather than a plain list. +func parseTaskAttachments(arguments map[string]any) (*projects.TaskAttachments, *mcp.CallToolResult) { + refs, toolResult := parseAttachmentRefs(arguments) + if toolResult != nil { + return nil, toolResult + } + if len(refs) == 0 { + return nil, nil + } + + attachments := projects.TaskAttachments{ + PendingFiles: make([]projects.TaskAttachmentPendingFile, 0, len(refs)), + } + for _, ref := range refs { + attachments.PendingFiles = append(attachments.PendingFiles, + projects.TaskAttachmentPendingFile{Reference: ref}) + } + return &attachments, nil +} + +// sanitizeFileName reduces a caller-supplied name to something safe to store as +// an attachment name. +// +// Models emit paths rather than names, and both "docs/plan.md" and a Windows +// style path turn up. path.Base only understands the forward slash, so the +// backslash form has to be removed explicitly. +func sanitizeFileName(name string) (string, error) { + name = strings.TrimSpace(name) + if index := strings.LastIndexAny(name, `/\`); index >= 0 { + name = name[index+1:] + } + // Control characters would break any downstream header carrying the name. + name = strings.Map(func(r rune) rune { + if r < 0x20 || r == 0x7f { + return -1 + } + return r + }, name) + name = strings.TrimSpace(name) + + switch name { + case "", ".", "..": + return "", fmt.Errorf("name must be a file name, not a path") + } + + if len(name) > maxFileNameBytes { + // Keep the extension so the file still opens with the right application, + // and cut the stem on a rune boundary so the name stays valid UTF-8. + extension := path.Ext(name) + if len(extension) > maxFileNameBytes/2 { + extension = "" // a late dot, not an extension + } + stem := name[:len(name)-len(extension)] + stem = strings.ToValidUTF8(stem[:maxFileNameBytes-len(extension)], "") + name = stem + extension + } + return name, nil +} diff --git a/internal/twprojects/files_test.go b/internal/twprojects/files_test.go new file mode 100644 index 00000000..ec5852a2 --- /dev/null +++ b/internal/twprojects/files_test.go @@ -0,0 +1,321 @@ +package twprojects_test + +import ( + "encoding/base64" + "encoding/json" + "mime" + "mime/multipart" + "net/http" + "strings" + "testing" + + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/teamwork/mcp/internal/testutil" + "github.com/teamwork/mcp/internal/twprojects" +) + +func TestFileCreate(t *testing.T) { + mcpServer, requestBody := mcpServerMockWithRequestBody(t, http.StatusCreated, + []byte(`{"pendingFile":{"ref":"tf_1a2b"}}`)) + testutil.ExecuteToolRequest(t, mcpServer, twprojects.MethodFileCreate.String(), map[string]any{ + "name": "plan.md", + "data": base64.StdEncoding.EncodeToString([]byte("# Plan\n")), + }) + + // The upload is multipart rather than JSON, so the assertion is on the part + // itself: the file has to arrive under the name the API expects, carrying the + // decoded bytes rather than the base64 the caller sent. + body := string(*requestBody) + if !strings.Contains(body, `name="file"`) { + t.Errorf("expected a form part named file, got %q", body) + } + if !strings.Contains(body, `filename="plan.md"`) { + t.Errorf("expected the file name in the body, got %q", body) + } + if !strings.Contains(body, "# Plan") { + t.Errorf("expected the decoded contents in the body, got %q", body) + } + if strings.Contains(body, base64.StdEncoding.EncodeToString([]byte("# Plan\n"))) { + t.Errorf("expected the contents to be decoded before upload, got %q", body) + } +} + +func TestFileCreateSanitizesFileName(t *testing.T) { + tests := []struct { + name string + input string + want string + }{{ + name: "posix path", + input: "docs/plans/plan.md", + want: "plan.md", + }, { + name: "windows path", + input: `C:\Users\someone\plan.md`, + want: "plan.md", + }, { + name: "traversal", + input: "../../etc/passwd", + want: "passwd", + }, { + name: "control characters", + input: "pl\nan\r.md", + want: "plan.md", + }, { + name: "surrounding whitespace", + input: " plan.md ", + want: "plan.md", + }} + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mcpServer, requestBody := mcpServerMockWithRequestBody(t, http.StatusCreated, + []byte(`{"pendingFile":{"ref":"tf_1a2b"}}`)) + testutil.ExecuteToolRequest(t, mcpServer, twprojects.MethodFileCreate.String(), map[string]any{ + "name": tt.input, + "data": base64.StdEncoding.EncodeToString([]byte("contents")), + }) + + filename, err := multipartFileName(*requestBody) + if err != nil { + t.Fatalf("failed to read the uploaded file name: %v", err) + } + if filename != tt.want { + t.Errorf("expected file name %q, got %q", tt.want, filename) + } + }) + } +} + +func TestFileCreateRejectsBadInput(t *testing.T) { + tests := []struct { + name string + arguments map[string]any + wantIn string + }{{ + name: "malformed base64", + arguments: map[string]any{"name": "plan.md", "data": "not base64!!!"}, + wantIn: "base64", + }, { + name: "empty content", + arguments: map[string]any{"name": "plan.md", "data": ""}, + wantIn: "zero bytes", + }, { + name: "name is only a path", + arguments: map[string]any{"name": "../", "data": base64.StdEncoding.EncodeToString([]byte("x"))}, + wantIn: "file name", + }} + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // A caller mistake has to come back as a tool result it can correct, + // not as a transport error. + mcpServer := mcpServerMock(t, http.StatusCreated, []byte(`{"pendingFile":{"ref":"tf_1a2b"}}`)) + testutil.ExecuteToolRequest(t, mcpServer, twprojects.MethodFileCreate.String(), tt.arguments, + testutil.ExecuteToolRequestWithCheckMessage(func(t *testing.T, result mcp.Result) { + t.Helper() + assertErrorResultContains(t, result, tt.wantIn) + }), + ) + }) + } +} + +func TestFileCreateRejectsOversizeBeforeUploading(t *testing.T) { + // The size check has to run before the upload, so an oversize payload never + // reaches the API. + mcpServer, requestBody := mcpServerMockWithRequestBody(t, http.StatusCreated, + []byte(`{"pendingFile":{"ref":"tf_1a2b"}}`)) + + oversize := base64.StdEncoding.EncodeToString(make([]byte, (5<<20)+1)) + testutil.ExecuteToolRequest(t, mcpServer, twprojects.MethodFileCreate.String(), map[string]any{ + "name": "big.bin", + "data": oversize, + }, + testutil.ExecuteToolRequestWithCheckMessage(func(t *testing.T, result mcp.Result) { + t.Helper() + assertErrorResultContains(t, result, "too large") + }), + ) + + if len(*requestBody) != 0 { + t.Errorf("expected no request to be sent, got a body of %d bytes", len(*requestBody)) + } +} + +// assertErrorResultContains checks that a tool call came back as an error result +// the caller can read, rather than as a transport error. +func assertErrorResultContains(t *testing.T, result mcp.Result, want string) { + t.Helper() + + toolResult, ok := result.(*mcp.CallToolResult) + if !ok { + t.Fatalf("unexpected result type: %T", result) + } + if !toolResult.IsError { + t.Fatalf("expected an error tool result, got %+v", toolResult) + } + if len(toolResult.Content) == 0 { + t.Fatal("error tool result should carry content the model can read") + } + textContent, ok := toolResult.Content[0].(*mcp.TextContent) + if !ok { + t.Fatalf("unexpected content type: %T", toolResult.Content[0]) + } + if !strings.Contains(strings.ToLower(textContent.Text), want) { + t.Errorf("expected the message to mention %q, got %q", want, textContent.Text) + } +} + +func TestTaskCreateSendsAttachments(t *testing.T) { + mcpServer, requestBody := mcpServerMockWithRequestBody(t, http.StatusCreated, + []byte(`{"task":{"id":123}}`)) + testutil.ExecuteToolRequest(t, mcpServer, twprojects.MethodTaskCreate.String(), map[string]any{ + "name": "example", + "tasklist_id": float64(777), + "attachment_refs": []any{"tf_A", "tf_B"}, + }) + + // attachments is a sibling of task in the request body, not one of its + // attributes, so the nesting is what this pins. + var payload struct { + Attachments struct { + PendingFiles []struct { + Reference string `json:"reference"` + } `json:"pendingFiles"` + } `json:"attachments"` + } + if err := json.Unmarshal(*requestBody, &payload); err != nil { + t.Fatalf("failed to decode request body %q: %v", string(*requestBody), err) + } + if len(payload.Attachments.PendingFiles) != 2 { + t.Fatalf("expected two pending files, got body %q", string(*requestBody)) + } + if payload.Attachments.PendingFiles[0].Reference != "tf_A" || + payload.Attachments.PendingFiles[1].Reference != "tf_B" { + t.Errorf("unexpected references in body %q", string(*requestBody)) + } +} + +func TestTaskUpdateSendsAttachments(t *testing.T) { + mcpServer, requestBody := mcpServerMockWithRequestBody(t, http.StatusOK, []byte(`{}`)) + testutil.ExecuteToolRequest(t, mcpServer, twprojects.MethodTaskUpdate.String(), map[string]any{ + "id": float64(123), + "attachment_refs": []any{"tf_A"}, + }) + + if !strings.Contains(string(*requestBody), `"reference":"tf_A"`) { + t.Errorf("expected the reference in the body, got %q", string(*requestBody)) + } +} + +func TestTaskAttachmentsOmittedWhenNotRequested(t *testing.T) { + // An empty attachments object would be a payload change for every caller + // that attaches nothing, so the key has to stay absent. + for _, tt := range []struct { + name string + method string + status int + arguments map[string]any + }{{ + name: "create", + method: twprojects.MethodTaskCreate.String(), + status: http.StatusCreated, + arguments: map[string]any{"name": "example", "tasklist_id": float64(777)}, + }, { + name: "update", + method: twprojects.MethodTaskUpdate.String(), + status: http.StatusOK, + arguments: map[string]any{"id": float64(123), "name": "example"}, + }} { + t.Run(tt.name, func(t *testing.T) { + mcpServer, requestBody := mcpServerMockWithRequestBody(t, tt.status, + []byte(`{"task":{"id":123}}`)) + testutil.ExecuteToolRequest(t, mcpServer, tt.method, tt.arguments) + + var payload map[string]any + if err := json.Unmarshal(*requestBody, &payload); err != nil { + t.Fatalf("failed to decode request body %q: %v", string(*requestBody), err) + } + if _, ok := payload["attachments"]; ok { + t.Errorf("expected no attachments key, got body %q", string(*requestBody)) + } + if _, ok := payload["attachmentOptions"]; ok { + t.Errorf("expected no attachmentOptions key, got body %q", string(*requestBody)) + } + }) + } +} + +func TestCommentCreateSendsAttachments(t *testing.T) { + mcpServer, requestBody := mcpServerMockWithRequestBody(t, http.StatusCreated, []byte(`{"id":"123"}`)) + testutil.ExecuteToolRequest(t, mcpServer, twprojects.MethodCommentCreate.String(), map[string]any{ + "object": map[string]any{"type": "tasks", "id": float64(777)}, + "body": "see attached", + "attachment_refs": []any{"tf_A", "tf_B"}, + }) + + var payload struct { + Comment struct { + PendingFileAttachments []string `json:"pendingFileAttachments"` + } `json:"comment"` + } + if err := json.Unmarshal(*requestBody, &payload); err != nil { + t.Fatalf("failed to decode request body %q: %v", string(*requestBody), err) + } + if len(payload.Comment.PendingFileAttachments) != 2 || + payload.Comment.PendingFileAttachments[0] != "tf_A" || + payload.Comment.PendingFileAttachments[1] != "tf_B" { + t.Errorf("unexpected references in body %q", string(*requestBody)) + } +} + +func TestMessageCreateSendsAttachments(t *testing.T) { + mcpServer, requestBody := mcpServerMockWithRequestBody(t, http.StatusCreated, + []byte(`{"messageId":"123"}`)) + testutil.ExecuteToolRequest(t, mcpServer, twprojects.MethodMessageCreate.String(), map[string]any{ + "project_id": float64(777), + "title": "example", + "body": "see attached", + "attachment_refs": []any{"tf_A"}, + }) + + var payload struct { + Post struct { + PendingFileAttachments []string `json:"pendingFileAttachments"` + } `json:"post"` + } + if err := json.Unmarshal(*requestBody, &payload); err != nil { + t.Fatalf("failed to decode request body %q: %v", string(*requestBody), err) + } + if len(payload.Post.PendingFileAttachments) != 1 || payload.Post.PendingFileAttachments[0] != "tf_A" { + t.Errorf("unexpected references in body %q", string(*requestBody)) + } +} + +// multipartFileName reads the uploaded part's file name out of a multipart body. +func multipartFileName(body []byte) (string, error) { + // The boundary is generated per request, so it has to be read back off the + // body's first line rather than assumed. + firstLine, _, found := strings.Cut(string(body), "\r\n") + if !found { + return "", errNotMultipart + } + boundary := strings.TrimPrefix(firstLine, "--") + reader := multipart.NewReader(strings.NewReader(string(body)), boundary) + part, err := reader.NextPart() + if err != nil { + return "", err + } + _, params, err := mime.ParseMediaType(part.Header.Get("Content-Disposition")) + if err != nil { + return "", err + } + return params["filename"], nil +} + +var errNotMultipart = errString("request body is not multipart") + +type errString string + +func (e errString) Error() string { return string(e) } diff --git a/internal/twprojects/messages.go b/internal/twprojects/messages.go index 13f25f93..51259b40 100644 --- a/internal/twprojects/messages.go +++ b/internal/twprojects/messages.go @@ -81,7 +81,8 @@ func MessageCreate(engine *twapi.Engine) toolsets.ToolWrapper { {Type: "null"}, }, }, - "notify": helpers.NotifySchema("Who to notify of the new message.", false), + "notify": helpers.NotifySchema("Who to notify of the new message.", false), + "attachment_refs": attachmentRefsSchema("message"), }, Required: []string{"title", "project_id", "body"}, }, @@ -98,6 +99,7 @@ func MessageCreate(engine *twapi.Engine) toolsets.ToolWrapper { helpers.RequiredParam(&messageCreateRequest.Title, "title"), helpers.RequiredParam(&messageCreateRequest.Body, "body"), helpers.OptionalPointerParam(&messageCreateRequest.NotifyCurrentUser, "notify_current_user"), + helpers.OptionalListParam(&messageCreateRequest.PendingFileAttachments, "attachment_refs"), ) if err != nil { return helpers.NewToolResultTextError("invalid parameters: %s", err.Error()), nil diff --git a/internal/twprojects/tasks.go b/internal/twprojects/tasks.go index 5f838655..e3bd7162 100644 --- a/internal/twprojects/tasks.go +++ b/internal/twprojects/tasks.go @@ -114,8 +114,9 @@ func TaskCreate(engine *twapi.Engine) toolsets.ToolWrapper { {Type: "null"}, }, }, - "assignees": helpers.UserGroupsSchema("Assignees for the task.", false), - "tag_ids": helpers.TagIDsAssociateSchema("task"), + "assignees": helpers.UserGroupsSchema("Assignees for the task.", false), + "tag_ids": helpers.TagIDsAssociateSchema("task"), + "attachment_refs": attachmentRefsSchema("task"), "predecessors": { Description: "Task dependencies that must be completed before this task can start.", AnyOf: []*jsonschema.Schema{ @@ -184,6 +185,15 @@ func TaskCreate(engine *twapi.Engine) toolsets.ToolWrapper { taskCreateRequest.Assignees = assignees } + // Only set attachments when the caller named one: the field is a + // sibling of the task in the request body, so an empty one would be a + // payload change for every caller that attaches nothing. + if attachments, toolResult := parseTaskAttachments(arguments); toolResult != nil { + return toolResult, nil + } else if attachments != nil { + taskCreateRequest.Attachments = *attachments + } + if predecessors, ok := arguments["predecessors"]; ok { predecessorsSlice, ok := predecessors.([]any) if !ok { @@ -345,7 +355,8 @@ func TaskUpdate(engine *twapi.Engine) toolsets.ToolWrapper { {Type: "null"}, }, }, - "tag_ids": helpers.TagIDsAssociateSchema("task"), + "tag_ids": helpers.TagIDsAssociateSchema("task"), + "attachment_refs": attachmentRefsSchema("task"), "predecessors": { Description: "Task dependencies that must be completed before this task can start.", AnyOf: []*jsonschema.Schema{ @@ -452,6 +463,14 @@ func TaskUpdate(engine *twapi.Engine) toolsets.ToolWrapper { taskUpdateRequest.Assignees = assignees } + // Only set attachments when the caller named one. Attaching is additive + // server side, so this never disturbs the files the task already has. + if attachments, toolResult := parseTaskAttachments(arguments); toolResult != nil { + return toolResult, nil + } else if attachments != nil { + taskUpdateRequest.Attachments = *attachments + } + if clearAssignees { // Empty arrays unassign every assignee dimension. Job roles are // included because the SDK now sends the "Jobroles-Enabled: true" diff --git a/internal/twprojects/tools.go b/internal/twprojects/tools.go index f3b4e20e..94d081ad 100644 --- a/internal/twprojects/tools.go +++ b/internal/twprojects/tools.go @@ -98,6 +98,7 @@ func DefaultToolsetGroup(readOnly, allowDelete bool, engine *twapi.Engine) *tool // --- tasks sub-toolset --- tasksWriteTools := []toolsets.ToolWrapper{ + FileCreate(engine), TaskComplete(engine), TaskCreate(engine), TaskMove(engine), From 5e15dfd968723f2d167e9db2513f804fb5745610 Mon Sep 17 00:00:00 2001 From: Daniel Mackey Date: Sat, 15 Aug 2026 14:02:45 +0100 Subject: [PATCH 2/2] Address review and simplification feedback 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) --- cmd/mcptest/.env.example | 10 + cmd/mcptest/.gitignore | 1 + cmd/mcptest/main.go | 575 ++++++++++++++++++++++++++++++ docs/tool-reference.md | 2 +- internal/logsafe/logsafe.go | 44 ++- internal/logsafe/logsafe_test.go | 24 +- internal/network/roundtripper.go | 39 +- internal/twprojects/comments.go | 7 +- internal/twprojects/files_test.go | 40 +-- internal/twprojects/messages.go | 7 +- internal/twprojects/tools.go | 2 +- 11 files changed, 679 insertions(+), 72 deletions(-) create mode 100644 cmd/mcptest/.env.example create mode 100644 cmd/mcptest/.gitignore create mode 100644 cmd/mcptest/main.go diff --git a/cmd/mcptest/.env.example b/cmd/mcptest/.env.example new file mode 100644 index 00000000..df749e80 --- /dev/null +++ b/cmd/mcptest/.env.example @@ -0,0 +1,10 @@ +# Copy this file to .env (or copy ..\sdktest\.env here) and fill in. +# .env is gitignored. + +TWAPI_SERVER=https://yoursite.teamwork.com +TWAPI_TOKEN=your-token-here +PROJECT_ID=12345 + +# Optional: override auth detection. Defaults to basic if token starts +# with "twp_", bearer otherwise. +# TWAPI_AUTH=basic diff --git a/cmd/mcptest/.gitignore b/cmd/mcptest/.gitignore new file mode 100644 index 00000000..4c49bd78 --- /dev/null +++ b/cmd/mcptest/.gitignore @@ -0,0 +1 @@ +.env diff --git a/cmd/mcptest/main.go b/cmd/mcptest/main.go new file mode 100644 index 00000000..dc773e97 --- /dev/null +++ b/cmd/mcptest/main.go @@ -0,0 +1,575 @@ +// mcptest walks through the Custom Items MCP tool handlers against a real +// Teamwork.com site. It bypasses the MCP server transport and invokes each +// tool's Handler directly with the same JSON payload an LLM would send, so +// you can see end-to-end behaviour — including field-name resolution, value +// coercion, twId↔name translation and the schema cache — without standing +// up an MCP server or LLM client. +// +// Configuration is read from .env (or the path given by -config). The auth +// mode (basic vs bearer) is auto-detected from the token prefix; override +// with TWAPI_AUTH. Same .env shape as sdktest. +// +// Usage: +// +// cd c:\programming\mcptest +// copy ..\sdktest\.env .env # if you don't already have one here +// go run . +package main + +import ( + "bufio" + "context" + "encoding/json" + "errors" + "flag" + "fmt" + "os" + "strconv" + "strings" + + "github.com/modelcontextprotocol/go-sdk/mcp" + + "github.com/teamwork/mcp/internal/toolsets" + "github.com/teamwork/mcp/internal/twprojects" + twapi "github.com/teamwork/twapi-go-sdk" + "github.com/teamwork/twapi-go-sdk/session" +) + +func main() { + configPath := preScanConfig(os.Args[1:], ".env") + if err := loadEnvFile(configPath); err != nil { + fmt.Fprintf(os.Stderr, "failed to read %s: %v\n", configPath, err) + os.Exit(2) + } + + flag.StringVar(&configPath, "config", configPath, "Path to .env file (ignored if missing)") + server := flag.String("server", os.Getenv("TWAPI_SERVER"), "Teamwork base URL (env TWAPI_SERVER)") + token := flag.String("token", os.Getenv("TWAPI_TOKEN"), "Bearer token / API key (env TWAPI_TOKEN)") + projectID := flag.Int64("project", envInt64("PROJECT_ID"), "Project ID to test on (env PROJECT_ID)") + keep := flag.Bool("keep", false, "Don't delete created artefacts so you can inspect them in the UI") + step := flag.Bool("step", false, "Pause for ENTER between steps") + flag.Parse() + + if *server == "" || *token == "" || *projectID == 0 { + fmt.Fprintln(os.Stderr, "server, token and project are all required (set in .env, env vars, or flags)") + flag.Usage() + os.Exit(2) + } + *server = strings.TrimSuffix(*server, "/") + + authMode := strings.ToLower(strings.TrimSpace(os.Getenv("TWAPI_AUTH"))) + if authMode == "" { + if strings.HasPrefix(*token, "twp_") { + authMode = "basic" + } else { + authMode = "bearer" + } + } + var sess twapi.Session + switch authMode { + case "basic": + fmt.Println("auth: HTTP Basic (personal API key)") + sess = session.NewBasicAuth(*token, "x", *server) + case "bearer": + fmt.Println("auth: Bearer token") + sess = session.NewBearerToken(*token, *server) + default: + fmt.Fprintf(os.Stderr, "unknown TWAPI_AUTH %q (want basic|bearer)\n", authMode) + os.Exit(2) + } + engine := twapi.NewEngine(sess) + + r := &runner{ + engine: engine, + projectID: *projectID, + keep: *keep, + step: *step, + stdin: bufio.NewReader(os.Stdin), + ctx: context.Background(), + } + if err := r.run(); err != nil { + fmt.Fprintf(os.Stderr, "FAILED: %v\n", err) + os.Exit(1) + } + fmt.Println("DONE.") +} + +type runner struct { + engine *twapi.Engine + projectID int64 + keep bool + step bool + stdin *bufio.Reader + ctx context.Context + + customItemID int64 + statusID int64 + notesID int64 + notesTwID string + recordID int64 + recordIDs []int64 +} + +func (r *runner) run() error { + defer r.cleanup() + + steps := []struct { + label string + fn func() error + }{ + {"LIST existing custom items via MCP", r.stepListCustomItems}, + {"CREATE custom item via MCP", r.stepCreateCustomItem}, + {`CREATE field "Notes" (text-short) via MCP`, r.stepCreateNotesField}, + {`CREATE field "Status" (dropdown) via MCP`, r.stepCreateStatusField}, + {"LIST fields via MCP — verify twIds", r.stepListFields}, + {"CREATE record by FIELD NAME (Notes + Status)", r.stepCreateRecordByName}, + {"GET record via MCP — verify twId→name translation", r.stepGetRecord}, + {"UPDATE record by field name (clear section, change Status by label)", r.stepUpdateRecord}, + {"LIST records via MCP — verify translation across a page", r.stepListRecords}, + {"CREATE 2 extra records for bulk delete", r.stepCreateExtraRecords}, + {"NEGATIVE: unknown field name should error clearly", r.stepNegativeUnknownField}, + } + for _, s := range steps { + fmt.Printf("\n=== %s ===\n", s.label) + if r.step { + fmt.Print("press ENTER to continue: ") + _, _ = r.stdin.ReadString('\n') + } + if err := s.fn(); err != nil { + return err + } + } + return nil +} + +// --------------------------------------------------------------------------- +// MCP invocation helpers +// --------------------------------------------------------------------------- + +// callTool marshals args to JSON and invokes the tool's handler directly. +// Returns the structured text result and any error from the handler, plus a +// bool indicating whether the result was an error result (IsError). +func (r *runner) callTool(tool toolsets.ToolWrapper, args map[string]any) (text string, isError bool, err error) { + raw, err := json.Marshal(args) + if err != nil { + return "", false, fmt.Errorf("marshal args: %w", err) + } + request := &mcp.CallToolRequest{ + Params: &mcp.CallToolParamsRaw{ + Name: tool.Tool.Name, + Arguments: json.RawMessage(raw), + }, + } + result, err := tool.Handler(r.ctx, request) + if err != nil { + return "", false, err + } + if result == nil { + return "", false, errors.New("nil result") + } + for _, content := range result.Content { + if text, ok := content.(*mcp.TextContent); ok { + return text.Text, result.IsError, nil + } + } + return "", result.IsError, errors.New("no text content in result") +} + +// callToolExpectOK calls a tool, prints the response, and returns an error +// if the handler returned err or an IsError result. +func (r *runner) callToolExpectOK(label string, tool toolsets.ToolWrapper, args map[string]any) (string, error) { + text, isError, err := r.callTool(tool, args) + if err != nil { + return "", fmt.Errorf("%s: %w", label, err) + } + if isError { + return "", fmt.Errorf("%s returned error result: %s", label, text) + } + prettyPrint(text) + return text, nil +} + +func prettyPrint(text string) { + // If the body is JSON, pretty-print it; otherwise emit raw. + var anyVal any + if err := json.Unmarshal([]byte(text), &anyVal); err == nil { + formatted, _ := json.MarshalIndent(anyVal, " ", " ") + fmt.Printf(" %s\n", string(formatted)) + return + } + fmt.Printf(" %s\n", text) +} + +// --------------------------------------------------------------------------- +// Steps +// --------------------------------------------------------------------------- + +func (r *runner) stepListCustomItems() error { + _, err := r.callToolExpectOK("list_custom_items", twprojects.CustomItemList(r.engine), map[string]any{ + "project_id": r.projectID, + }) + return err +} + +func (r *runner) stepCreateCustomItem() error { + name := fmt.Sprintf("MCPTest-%d", r.projectID) + text, err := r.callToolExpectOK("create_custom_item", twprojects.CustomItemCreate(r.engine), map[string]any{ + "project_id": r.projectID, + "display_name": name, + "label_singular": "MCP Record", + "label_plural": "MCP Records", + }) + if err != nil { + return err + } + r.customItemID, err = extractTrailingID(text) + if err != nil { + return fmt.Errorf("extract custom item id: %w", err) + } + fmt.Printf(" → captured customItemID=%d\n", r.customItemID) + return nil +} + +func (r *runner) stepCreateNotesField() error { + text, err := r.callToolExpectOK("create_custom_item_field", twprojects.CustomItemFieldCreate(r.engine), map[string]any{ + "custom_item_id": r.customItemID, + "display_name": "Notes", + "type": "text-short", + }) + if err != nil { + return err + } + r.notesID, err = extractTrailingID(text) + if err != nil { + return fmt.Errorf("extract field id: %w", err) + } + fmt.Printf(" → captured notesFieldID=%d\n", r.notesID) + return nil +} + +func (r *runner) stepCreateStatusField() error { + text, err := r.callToolExpectOK("create_custom_item_field", twprojects.CustomItemFieldCreate(r.engine), map[string]any{ + "custom_item_id": r.customItemID, + "display_name": "Status", + "type": "dropdown", + "tw_type": "status", + "options": []map[string]any{ + {"label": "Active", "color": "#22c55e"}, + {"label": "Pending", "color": "#facc15"}, + {"label": "Closed", "color": "#94a3b8"}, + }, + }) + if err != nil { + return err + } + r.statusID, err = extractTrailingID(text) + if err != nil { + return fmt.Errorf("extract field id: %w", err) + } + fmt.Printf(" → captured statusFieldID=%d\n", r.statusID) + return nil +} + +func (r *runner) stepListFields() error { + text, err := r.callToolExpectOK("list_custom_item_fields", + twprojects.CustomItemFieldList(r.engine), map[string]any{ + "custom_item_id": r.customItemID, + }) + if err != nil { + return err + } + // Pull the Notes field twId out so we can compare against the value + // coming back in the record step. + var listResp struct { + CustomItemFields []struct { + ID int64 `json:"id"` + TwID string `json:"twId"` + DisplayName string `json:"displayName"` + } `json:"customItemFields"` + } + if err := json.Unmarshal([]byte(text), &listResp); err == nil { + for _, field := range listResp.CustomItemFields { + if field.DisplayName == "Notes" { + r.notesTwID = field.TwID + fmt.Printf(" → captured notesTwID=%q\n", r.notesTwID) + } + } + } + return nil +} + +func (r *runner) stepCreateRecordByName() error { + text, err := r.callToolExpectOK("create_custom_item_record", + twprojects.CustomItemRecordCreate(r.engine), map[string]any{ + "custom_item_id": r.customItemID, + "name": "Acme Inc", + "field_values": []map[string]any{ + {"field_name": "Notes", "value": "initial contact"}, + // Pass the option by LABEL — the handler should resolve it + // to the option twId before sending. + {"field_name": "Status", "value": "Active"}, + }, + }) + if err != nil { + return err + } + r.recordID, err = extractTrailingID(text) + if err != nil { + return fmt.Errorf("extract record id: %w", err) + } + r.recordIDs = append(r.recordIDs, r.recordID) + fmt.Printf(" → captured recordID=%d\n", r.recordID) + return nil +} + +func (r *runner) stepGetRecord() error { + text, err := r.callToolExpectOK("get_custom_item_record", + twprojects.CustomItemRecordGet(r.engine), map[string]any{ + "custom_item_id": r.customItemID, + "id": r.recordID, + }) + if err != nil { + return err + } + // Sanity: confirm field values came back keyed by name, not twId. + if strings.Contains(text, `"Notes"`) { + fmt.Println(" ✓ field values keyed by name (Notes)") + } else { + fmt.Println(" ! WARNING: Notes not present in response by display name") + } + if strings.Contains(text, `"Active"`) { + fmt.Println(" ✓ Status value translated back to label (Active)") + } else { + fmt.Println(" ! WARNING: Status label not present in response") + } + if r.notesTwID != "" && strings.Contains(text, r.notesTwID) { + fmt.Printf(" ! WARNING: raw twId %q leaked into response\n", r.notesTwID) + } + return nil +} + +func (r *runner) stepUpdateRecord() error { + _, err := r.callToolExpectOK("update_custom_item_record", + twprojects.CustomItemRecordUpdate(r.engine), map[string]any{ + "custom_item_id": r.customItemID, + "id": r.recordID, + "name": "Acme Inc (updated via MCP)", + "clear_section": true, + "field_values": []map[string]any{ + {"field_name": "Status", "value": "Pending"}, // by label + {"field_name": "Notes", "value": "follow-up next week"}, + }, + }) + return err +} + +func (r *runner) stepListRecords() error { + text, err := r.callToolExpectOK("list_custom_item_records", + twprojects.CustomItemRecordList(r.engine), map[string]any{ + "custom_item_id": r.customItemID, + }) + if err != nil { + return err + } + if strings.Contains(text, `"Notes"`) && strings.Contains(text, `"Status"`) { + fmt.Println(" ✓ list response uses display names for field keys") + } + return nil +} + +func (r *runner) stepCreateExtraRecords() error { + for i := 0; i < 2; i++ { + text, err := r.callToolExpectOK("create_custom_item_record (extra)", + twprojects.CustomItemRecordCreate(r.engine), map[string]any{ + "custom_item_id": r.customItemID, + "name": fmt.Sprintf("bulk-target-%d", i+1), + }) + if err != nil { + return err + } + id, err := extractTrailingID(text) + if err != nil { + return fmt.Errorf("extract extra record id: %w", err) + } + r.recordIDs = append(r.recordIDs, id) + } + return nil +} + +func (r *runner) stepNegativeUnknownField() error { + tool := twprojects.CustomItemRecordCreate(r.engine) + text, isError, err := r.callTool(tool, map[string]any{ + "custom_item_id": r.customItemID, + "name": "negative-test", + "field_values": []map[string]any{ + {"field_name": "NoSuchField", "value": "x"}, + }, + }) + if err != nil { + return fmt.Errorf("negative test: %w", err) + } + if !isError { + return fmt.Errorf("expected an error result for unknown field, got success: %s", text) + } + fmt.Printf(" ✓ unknown-field error surfaced: %s\n", strings.TrimSpace(text)) + return nil +} + +// --------------------------------------------------------------------------- +// Cleanup +// --------------------------------------------------------------------------- + +func (r *runner) cleanup() { + if r.keep { + fmt.Println("\n--- KEEP set, leaving artefacts in place ---") + if r.customItemID != 0 { + fmt.Printf(" customItemID = %d\n", r.customItemID) + } + if r.notesID != 0 { + fmt.Printf(" notesFieldID = %d\n", r.notesID) + } + if r.statusID != 0 { + fmt.Printf(" statusFieldID = %d\n", r.statusID) + } + if len(r.recordIDs) > 0 { + fmt.Printf(" recordIDs = %v\n", r.recordIDs) + } + return + } + + fmt.Println("\n--- cleanup ---") + + if len(r.recordIDs) > 0 && r.customItemID != 0 { + fmt.Printf(" bulk-delete %d records via MCP\n", len(r.recordIDs)) + _, isError, err := r.callTool(twprojects.CustomItemRecordBulkDelete(r.engine), map[string]any{ + "custom_item_id": r.customItemID, + "ids": asAnyInts(r.recordIDs), + }) + if err != nil || isError { + fmt.Fprintf(os.Stderr, " ! bulk delete records failed: err=%v\n", err) + } + } + + for _, fieldID := range []int64{r.notesID, r.statusID} { + if fieldID == 0 || r.customItemID == 0 { + continue + } + fmt.Printf(" delete field %d via MCP\n", fieldID) + _, isError, err := r.callTool(twprojects.CustomItemFieldDelete(r.engine), map[string]any{ + "custom_item_id": r.customItemID, + "id": fieldID, + }) + if err != nil || isError { + fmt.Fprintf(os.Stderr, " ! delete field %d failed: err=%v\n", fieldID, err) + } + } + + if r.customItemID != 0 { + fmt.Printf(" delete custom item %d via MCP\n", r.customItemID) + _, isError, err := r.callTool(twprojects.CustomItemDelete(r.engine), map[string]any{ + "id": r.customItemID, + }) + if err != nil || isError { + fmt.Fprintf(os.Stderr, " ! delete custom item failed: err=%v\n", err) + } + } +} + +// asAnyInts converts []int64 to []any (what JSON-encoded numeric arrays look +// like after a typical map[string]any unmarshal). +func asAnyInts(ids []int64) []any { + out := make([]any, len(ids)) + for i, id := range ids { + out[i] = id + } + return out +} + +// extractTrailingID pulls the last whitespace-separated token from the text, +// assuming a result like "Custom item created successfully with ID 1234". +func extractTrailingID(text string) (int64, error) { + trimmed := strings.TrimSpace(text) + if trimmed == "" { + return 0, errors.New("empty text") + } + parts := strings.Fields(trimmed) + last := parts[len(parts)-1] + id, err := strconv.ParseInt(last, 10, 64) + if err != nil { + return 0, fmt.Errorf("trailing token %q is not an integer: %w", last, err) + } + return id, nil +} + +// --------------------------------------------------------------------------- +// .env loader (same shape as sdktest) +// --------------------------------------------------------------------------- + +func preScanConfig(args []string, fallback string) string { + for i := 0; i < len(args); i++ { + arg := args[i] + switch { + case arg == "-config" || arg == "--config": + if i+1 < len(args) { + return args[i+1] + } + case strings.HasPrefix(arg, "-config=") || strings.HasPrefix(arg, "--config="): + return arg[strings.IndexByte(arg, '=')+1:] + case arg == "--": + return fallback + } + } + return fallback +} + +func loadEnvFile(path string) error { + f, err := os.Open(path) + if err != nil { + if os.IsNotExist(err) { + return nil + } + return err + } + defer f.Close() + + scanner := bufio.NewScanner(f) + lineNo := 0 + for scanner.Scan() { + lineNo++ + line := strings.TrimSpace(scanner.Text()) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + eq := strings.IndexByte(line, '=') + if eq <= 0 { + return fmt.Errorf("%s line %d: expected KEY=VALUE", path, lineNo) + } + key := strings.TrimSpace(line[:eq]) + value := strings.TrimSpace(line[eq+1:]) + if n := len(value); n >= 2 { + first, last := value[0], value[n-1] + if (first == '"' && last == '"') || (first == '\'' && last == '\'') { + value = value[1 : n-1] + } + } + if _, set := os.LookupEnv(key); set { + continue + } + if err := os.Setenv(key, value); err != nil { + return fmt.Errorf("%s line %d: setenv %s: %w", path, lineNo, key, err) + } + } + return scanner.Err() +} + +func envInt64(key string) int64 { + value, ok := os.LookupEnv(key) + if !ok || value == "" { + return 0 + } + parsed, err := strconv.ParseInt(value, 10, 64) + if err != nil { + return 0 + } + return parsed +} diff --git a/docs/tool-reference.md b/docs/tool-reference.md index 765f39c7..ca3c589b 100644 --- a/docs/tool-reference.md +++ b/docs/tool-reference.md @@ -53,6 +53,7 @@ Project, category, template, member, custom field, and custom item (user-defined | Custom Item | ✓ | ✓ | ✓ | ✓ | | Custom Item Field | ✓ | ✓ | ✓ | ✓ | | Custom Item Record | ✓ | ✓ | ✓ | ✓ | +| File | ✓ | — | — | — | **Other actions:** `add_project_member`, `clone_project` @@ -66,7 +67,6 @@ Task, tasklist, and workflow management in Teamwork.com. | Tasklist | ✓ | ✓ | ✓ | ✓ | | Workflow | ✓ | ✓ | ✓ | ✓ | | Workflow Stage | ✓ | ✓ | ✓ | ✓ | -| File | ✓ | — | — | — | **Other actions:** `complete_task`, `link_project_to_workflow`, `move_task_to_workflow_stage`, `move_tasks` diff --git a/internal/logsafe/logsafe.go b/internal/logsafe/logsafe.go index 2a439cb9..f5d7f3db 100644 --- a/internal/logsafe/logsafe.go +++ b/internal/logsafe/logsafe.go @@ -23,31 +23,49 @@ const MaxLoggedBytes = 64 << 10 const truncatedSuffix = "...[truncated]" -// contentValue matches a long JSON string value under one of the keys that -// carry file content. -// -// The alternation is anchored on the key and the value character class is the -// base64 alphabet plus whitespace, so there is no nested quantifier and the -// expression stays linear over a multi-megabyte body. The 256 character floor -// keeps genuinely short values readable, and matching on the key means a search -// term that happens to be the word "data" is left alone. -var contentValue = regexp.MustCompile(`"(data|content|fileData|file_data)"\s*:\s*"[A-Za-z0-9+/=_\-\s]{256,}"`) +// contentKeys are the JSON keys whose value is uploaded file content. They are +// upload-specific: "content" is deliberately excluded, since page and comment +// bodies use it for text worth keeping in the log. +var contentKeys = [][]byte{[]byte(`"data"`), []byte(`"fileData"`), []byte(`"file_data"`)} + +// contentValue matches the JSON string value under one of contentKeys. The +// value class is "anything but a quote", so it catches a file of any size, +// including a small one, and is not broken by JSON escaping such as "\/". The +// key anchor means an unrelated field whose value happens to be one of these +// words is untouched. There is no nested quantifier, so the match stays linear +// over a multi-megabyte body. +var contentValue = regexp.MustCompile(`"(data|fileData|file_data)"\s*:\s*"[^"]*"`) // Bytes returns b with file content replaced and the result capped. func Bytes(b []byte) []byte { if len(b) == 0 { return b } - scrubbed := contentValue.ReplaceAllFunc(b, func(match []byte) []byte { - key := match[:bytes.IndexByte(match, ':')] - return fmt.Appendf(nil, `%s:""`, key, len(match)) - }) + scrubbed := b + if containsContentKey(b) { + scrubbed = contentValue.ReplaceAllFunc(b, func(match []byte) []byte { + key := match[:bytes.IndexByte(match, ':')] + return fmt.Appendf(nil, `%s:""`, key, len(match)) + }) + } if len(scrubbed) > MaxLoggedBytes { return append(bytes.Clone(scrubbed[:MaxLoggedBytes]), truncatedSuffix...) } return scrubbed } +// containsContentKey reports whether b mentions any upload key, so the regex +// scan and its allocation are skipped for the payloads that carry no file, +// which is almost all of them. +func containsContentKey(b []byte) bool { + for _, key := range contentKeys { + if bytes.Contains(b, key) { + return true + } + } + return false +} + // String is Bytes over a string. func String(s string) string { if len(s) == 0 { diff --git a/internal/logsafe/logsafe_test.go b/internal/logsafe/logsafe_test.go index c5dc20a9..bf2ad76e 100644 --- a/internal/logsafe/logsafe_test.go +++ b/internal/logsafe/logsafe_test.go @@ -30,10 +30,26 @@ func TestBytesRedactsFileContent(t *testing.T) { } } -func TestBytesLeavesShortValuesAlone(t *testing.T) { - // A short value under one of the content keys is readable and worth keeping, - // and a search term that happens to be the word "data" is not a key at all. - payload := `{"arguments":{"data":"plan","search_term":"data"}}` +func TestBytesRedactsSmallFile(t *testing.T) { + // A file is redacted whatever its size: a short secret must not survive just + // because its base64 is short. + content := base64.StdEncoding.EncodeToString([]byte("a short secret")) + payload := `{"arguments":{"name":"secret.txt","data":"` + content + `"}}` + + got := logsafe.String(payload) + + if strings.Contains(got, content) { + t.Errorf("expected the file content to be redacted, got %q", got) + } + if !strings.Contains(got, `"name":"secret.txt"`) { + t.Errorf("expected the other parameters to survive, got %q", got) + } +} + +func TestBytesLeavesUnrelatedFieldsAlone(t *testing.T) { + // The key is what's matched, so a field whose value is the word "data", and a + // "content" field (page and comment bodies use it), are both left alone. + payload := `{"arguments":{"search_term":"data","content":"the page body worth keeping"}}` if got := logsafe.String(payload); got != payload { t.Errorf("expected the payload unchanged, got %q", got) diff --git a/internal/network/roundtripper.go b/internal/network/roundtripper.go index 8260e88a..699d24b1 100644 --- a/internal/network/roundtripper.go +++ b/internal/network/roundtripper.go @@ -33,16 +33,21 @@ func (lrt *LoggingRoundTripper) RoundTrip(r *http.Request) (*http.Response, erro // A body that is not text is not worth capturing: a pending file upload // sends the file itself, so reading it here would copy the customer's file // into the log and hold a second copy in memory until the request finishes. + // This content-type gate is what keeps file bytes out of the log; the JSON + // bodies that do reach this point carry no inline file content, so they are + // logged as-is. var loggedRequestBody string - if r.Body != nil && !logsafe.IsTextualContentType(r.Header.Get("Content-Type")) { - loggedRequestBody = logsafe.ElidedBody(r.ContentLength, r.Header.Get("Content-Type")) - } else if r.Body != nil { - reqBody, err := io.ReadAll(r.Body) - if err != nil { - lrt.Log.Error("failed to read request body", slog.String("error", err.Error())) + if r.Body != nil { + if contentType := r.Header.Get("Content-Type"); !logsafe.IsTextualContentType(contentType) { + loggedRequestBody = logsafe.ElidedBody(r.ContentLength, contentType) + } else { + reqBody, err := io.ReadAll(r.Body) + if err != nil { + lrt.Log.Error("failed to read request body", slog.String("error", err.Error())) + } + r.Body = io.NopCloser(bytes.NewBuffer(reqBody)) + loggedRequestBody = string(reqBody) } - r.Body = io.NopCloser(bytes.NewBuffer(reqBody)) - loggedRequestBody = logsafe.String(string(reqBody)) } headers := r.Header.Clone() @@ -66,15 +71,17 @@ func (lrt *LoggingRoundTripper) RoundTrip(r *http.Request) (*http.Response, erro } var loggedResponseBody string - if resp.Body != nil && !logsafe.IsTextualContentType(resp.Header.Get("Content-Type")) { - loggedResponseBody = logsafe.ElidedBody(resp.ContentLength, resp.Header.Get("Content-Type")) - } else if resp.Body != nil { - respBody, err := io.ReadAll(resp.Body) - if err != nil { - lrt.Log.Error("failed to read response body", "error", err) + if resp.Body != nil { + if contentType := resp.Header.Get("Content-Type"); !logsafe.IsTextualContentType(contentType) { + loggedResponseBody = logsafe.ElidedBody(resp.ContentLength, contentType) + } else { + respBody, err := io.ReadAll(resp.Body) + if err != nil { + lrt.Log.Error("failed to read response body", "error", err) + } + resp.Body = io.NopCloser(bytes.NewBuffer(respBody)) + loggedResponseBody = string(respBody) } - resp.Body = io.NopCloser(bytes.NewBuffer(respBody)) - loggedResponseBody = logsafe.String(string(respBody)) } info, _ := request.InfoFromContext(r.Context()) diff --git a/internal/twprojects/comments.go b/internal/twprojects/comments.go index c53f76e8..ce88d180 100644 --- a/internal/twprojects/comments.go +++ b/internal/twprojects/comments.go @@ -145,12 +145,17 @@ func CommentCreate(engine *twapi.Engine) toolsets.ToolWrapper { helpers.RequiredParam(&commentCreateRequest.Body, "body"), helpers.OptionalPointerParam(&commentCreateRequest.ContentType, "content_type"), helpers.OptionalPointerParam(&commentCreateRequest.NotifyCurrentUser, "notify_current_user"), - helpers.OptionalListParam(&commentCreateRequest.PendingFileAttachments, "attachment_refs"), ) if err != nil { return helpers.NewToolResultTextError("invalid parameters: %s", err.Error()), nil } + refs, toolResult := parseAttachmentRefs(arguments) + if toolResult != nil { + return toolResult, nil + } + commentCreateRequest.PendingFileAttachments = refs + notifyChosen, notifiers, toolResult := parseNotify(arguments, true) if toolResult != nil { return toolResult, nil diff --git a/internal/twprojects/files_test.go b/internal/twprojects/files_test.go index ec5852a2..0439f49e 100644 --- a/internal/twprojects/files_test.go +++ b/internal/twprojects/files_test.go @@ -3,8 +3,6 @@ package twprojects_test import ( "encoding/base64" "encoding/json" - "mime" - "mime/multipart" "net/http" "strings" "testing" @@ -76,12 +74,11 @@ func TestFileCreateSanitizesFileName(t *testing.T) { "data": base64.StdEncoding.EncodeToString([]byte("contents")), }) - filename, err := multipartFileName(*requestBody) - if err != nil { - t.Fatalf("failed to read the uploaded file name: %v", err) - } - if filename != tt.want { - t.Errorf("expected file name %q, got %q", tt.want, filename) + // The leading filename=" anchors the check to the multipart part's + // name, so an un-sanitized path could not match by appearing elsewhere. + want := `filename="` + tt.want + `"` + if !strings.Contains(string(*requestBody), want) { + t.Errorf("expected the body to carry %s, got %q", want, string(*requestBody)) } }) } @@ -292,30 +289,3 @@ func TestMessageCreateSendsAttachments(t *testing.T) { t.Errorf("unexpected references in body %q", string(*requestBody)) } } - -// multipartFileName reads the uploaded part's file name out of a multipart body. -func multipartFileName(body []byte) (string, error) { - // The boundary is generated per request, so it has to be read back off the - // body's first line rather than assumed. - firstLine, _, found := strings.Cut(string(body), "\r\n") - if !found { - return "", errNotMultipart - } - boundary := strings.TrimPrefix(firstLine, "--") - reader := multipart.NewReader(strings.NewReader(string(body)), boundary) - part, err := reader.NextPart() - if err != nil { - return "", err - } - _, params, err := mime.ParseMediaType(part.Header.Get("Content-Disposition")) - if err != nil { - return "", err - } - return params["filename"], nil -} - -var errNotMultipart = errString("request body is not multipart") - -type errString string - -func (e errString) Error() string { return string(e) } diff --git a/internal/twprojects/messages.go b/internal/twprojects/messages.go index 51259b40..426fbc15 100644 --- a/internal/twprojects/messages.go +++ b/internal/twprojects/messages.go @@ -99,12 +99,17 @@ func MessageCreate(engine *twapi.Engine) toolsets.ToolWrapper { helpers.RequiredParam(&messageCreateRequest.Title, "title"), helpers.RequiredParam(&messageCreateRequest.Body, "body"), helpers.OptionalPointerParam(&messageCreateRequest.NotifyCurrentUser, "notify_current_user"), - helpers.OptionalListParam(&messageCreateRequest.PendingFileAttachments, "attachment_refs"), ) if err != nil { return helpers.NewToolResultTextError("invalid parameters: %s", err.Error()), nil } + refs, toolResult := parseAttachmentRefs(arguments) + if toolResult != nil { + return toolResult, nil + } + messageCreateRequest.PendingFileAttachments = refs + notifyChosen, notifiers, toolResult := parseNotify(arguments, false) if toolResult != nil { return toolResult, nil diff --git a/internal/twprojects/tools.go b/internal/twprojects/tools.go index 94d081ad..cdbdc0ae 100644 --- a/internal/twprojects/tools.go +++ b/internal/twprojects/tools.go @@ -45,6 +45,7 @@ func DefaultToolsetGroup(readOnly, allowDelete bool, engine *twapi.Engine) *tool // --- projects sub-toolset --- projectsWriteTools := []toolsets.ToolWrapper{ + FileCreate(engine), ProjectCategoryCreate(engine), ProjectCategoryUpdate(engine), ProjectClone(engine), @@ -98,7 +99,6 @@ func DefaultToolsetGroup(readOnly, allowDelete bool, engine *twapi.Engine) *tool // --- tasks sub-toolset --- tasksWriteTools := []toolsets.ToolWrapper{ - FileCreate(engine), TaskComplete(engine), TaskCreate(engine), TaskMove(engine),