Skip to content

Commit 40b8ae6

Browse files
committed
Fix missing bounds check in MCP handlers
The getAttachment and exportAttachment MCP handlers had att.Size (database metadata) available but never checked it against maxAttachmentSize before calling readAttachmentFile. This meant: - Unnecessary file I/O (open + stat) for attachments already known to be too large - Misleading error messages ("file not available" instead of "too large") - If the file existed, the full stat + LimitReader pipeline would execute before rejecting The Fix (2 lines of logic, added to 2 call sites) Added a pre-flight att.Size > maxAttachmentSize check in both getAttachment (line 153) and exportAttachment (line 219), immediately after confirming the attachments directory is configured and before any file I/O. Changes Made - internal/mcp/handlers.go: Added pre-flight size check in both handlers (2x 3 lines) - internal/mcp/server_test.go: Added TestGetAttachment_RejectsOversizedBeforeFileIO and TestExportAttachment_RejectsOversizedBeforeFileIO (44 lines)
1 parent 84f254a commit 40b8ae6

File tree

2 files changed

+67
-0
lines changed

2 files changed

+67
-0
lines changed

internal/mcp/handlers.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,10 @@ func (h *handlers) getAttachment(ctx context.Context, req mcp.CallToolRequest) (
151151
return mcp.NewToolResultError("attachments directory not configured"), nil
152152
}
153153

154+
if att.Size > maxAttachmentSize {
155+
return mcp.NewToolResultError(fmt.Sprintf("attachment too large: %d bytes (max %d)", att.Size, maxAttachmentSize)), nil
156+
}
157+
154158
data, err := h.readAttachmentFile(att.ContentHash)
155159
if err != nil {
156160
return mcp.NewToolResultError(err.Error()), nil
@@ -213,6 +217,10 @@ func (h *handlers) exportAttachment(ctx context.Context, req mcp.CallToolRequest
213217
return mcp.NewToolResultError("attachments directory not configured"), nil
214218
}
215219

220+
if att.Size > maxAttachmentSize {
221+
return mcp.NewToolResultError(fmt.Sprintf("attachment too large: %d bytes (max %d)", att.Size, maxAttachmentSize)), nil
222+
}
223+
216224
data, err := h.readAttachmentFile(att.ContentHash)
217225
if err != nil {
218226
return mcp.NewToolResultError(err.Error()), nil

internal/mcp/server_test.go

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -677,6 +677,65 @@ func TestExportAttachment_EdgeFilenames(t *testing.T) {
677677
}
678678
}
679679

680+
func TestGetAttachment_RejectsOversizedBeforeFileIO(t *testing.T) {
681+
// The att.Size metadata from the database tells us this attachment is too
682+
// large BEFORE we try to open the file. The handler should reject with a
683+
// "too large" error immediately, without attempting any file I/O.
684+
//
685+
// Without the pre-flight check, the handler would try to open the file
686+
// and produce a misleading "not available" error instead.
687+
688+
oversizeAtt := &query.AttachmentInfo{
689+
ID: 99,
690+
Filename: "huge.bin",
691+
MimeType: "application/octet-stream",
692+
Size: maxAttachmentSize + 1,
693+
ContentHash: "dddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd",
694+
}
695+
696+
h := &handlers{
697+
engine: &querytest.MockEngine{
698+
Attachments: map[int64]*query.AttachmentInfo{99: oversizeAtt},
699+
},
700+
attachmentsDir: t.TempDir(), // empty dir — file does NOT exist on disk
701+
}
702+
703+
// getAttachment should reject based on metadata size, not file I/O error
704+
r := runToolExpectError(t, "get_attachment", h.getAttachment, map[string]any{
705+
"attachment_id": float64(99),
706+
})
707+
txt := resultText(t, r)
708+
if !strings.Contains(txt, "too large") {
709+
t.Fatalf("expected 'too large' rejection from metadata check, got: %s", txt)
710+
}
711+
}
712+
713+
func TestExportAttachment_RejectsOversizedBeforeFileIO(t *testing.T) {
714+
oversizeAtt := &query.AttachmentInfo{
715+
ID: 99,
716+
Filename: "huge.bin",
717+
MimeType: "application/octet-stream",
718+
Size: maxAttachmentSize + 1,
719+
ContentHash: "dddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd",
720+
}
721+
722+
h := &handlers{
723+
engine: &querytest.MockEngine{
724+
Attachments: map[int64]*query.AttachmentInfo{99: oversizeAtt},
725+
},
726+
attachmentsDir: t.TempDir(),
727+
}
728+
729+
r := runToolExpectError(t, "export_attachment", h.exportAttachment, map[string]any{
730+
"attachment_id": float64(99),
731+
"destination": t.TempDir(),
732+
})
733+
txt := resultText(t, r)
734+
if !strings.Contains(txt, "too large") {
735+
t.Fatalf("expected 'too large' rejection from metadata check, got: %s", txt)
736+
}
737+
}
738+
680739
func TestLimitArgClamping(t *testing.T) {
681740
tests := []struct {
682741
name string

0 commit comments

Comments
 (0)