feat: implement attachment download with OAuth authentication by brigleb · Pull Request #132 · needmore/bc4

brigleb · 2026-01-19T20:42:18Z

Summary

Implements functionality to download attachments (images, files) from Basecamp cards, todos, and messages using OAuth Bearer token authentication, as requested in #131.

Key Features

✅ Download attachments from cards: bc4 card download-attachments <card-id>
✅ Download attachments from todos: bc4 todo download-attachments <todo-id>
✅ Download attachments from messages: bc4 message download-attachments <message-id>
✅ OAuth Bearer token authentication (no browser cookies needed)
✅ Support for downloading all attachments or specific ones by index
✅ Configurable output directory
✅ File overwrite protection with --overwrite flag
✅ Progress indication and summary reports
✅ Comprehensive error handling for permissions, network issues, and file conflicts

Implementation Details

API Layer (internal/api/uploads.go):

Added Upload model with download URL and metadata
Implemented GetUpload() to fetch upload details by ID
Implemented DownloadAttachment() to download files with OAuth auth
Added helper function ExtractUploadID() to parse upload IDs from URLs

Attachment Parser (internal/attachments/parser.go):

Extended with ExtractBucketID() and ExtractUploadIDFromURL() helpers
Supports various URL formats from Basecamp

Commands:

cmd/card/download_attachments.go - Download card attachments
cmd/todo/download_attachments.go - Download todo attachments
cmd/message/download_attachments.go - Download message attachments
Flags: --output-dir, --attachment, --overwrite
Comprehensive error messages and progress indication
Human-readable file size formatting

Documentation:

Updated README.md with usage examples for all commands
Updated SPEC.md with technical details
Complete --help text with examples
Documented known limitation with comment attachments

Usage Examples

# Download all attachments from a card
bc4 card download-attachments 123456

# Download from a todo to specific directory
bc4 todo download-attachments 789012 --output-dir ~/Downloads

# Download from a message (only first attachment)
bc4 message download-attachments 345678 --attachment 1

# Overwrite existing files
bc4 card download-attachments 123456 --overwrite

Known Limitations

Comment Attachments Not Supported:

Comment attachments use blob storage URLs (storage.3.basecamp.com, preview.3.basecamp.com) that require browser session cookies and do not support OAuth Bearer token authentication.

What works:

✅ Card body attachments (created via uploads API)
✅ Todo attachments (created via uploads API)
✅ Message attachments (created via uploads API)

What doesn't work:

❌ Comment attachments (use blob storage, require browser cookies)

This is a Basecamp API architectural decision. Comment attachments are stored in Google Cloud Storage with session-signed URLs, while uploads created via the API (/attachments.json) generate download URLs that work with OAuth tokens.

Workaround: To download comment attachments, use a web browser while authenticated to Basecamp.

Error Handling

The implementation includes graceful error handling for:

Permission denied (403)
Not found (404)
Network errors
File already exists (with skip option)
Invalid upload IDs or URLs
Blob storage URLs (clear error message explaining limitation)

Testing

Build succeeds without errors
All existing tests pass
Commands appear in help output
Help text displays correctly with --help flag
Documented known limitation with comment attachments

What Changed from Original Plan

The original issue #131 requested download commands for cards, todos, messages, and comments. During implementation, we discovered that:

Comment attachments use a different storage system - They use Google Cloud blob storage with session-based authentication
OAuth tokens don't work with blob storage - These URLs return 302 redirects to login pages
Only uploads API attachments support OAuth - Attachments created via /attachments.json can be downloaded programmatically

Therefore, the comment download-attachments command was intentionally not included in this PR. The limitation is clearly documented for users.

Closes #131

🤖 Generated with Claude Code

This commit implements functionality to download attachments (images, files) from Basecamp cards using OAuth Bearer token authentication. Key changes: - Add Upload API model and operations (GetUpload, DownloadAttachment) - Implement `bc4 card download-attachments` command with flags: - --output-dir: specify download directory - --attachment: download specific attachment by index - --overwrite: overwrite existing files - Add helper functions to extract upload IDs and bucket IDs from URLs - Include progress indication and error handling for: - Permission denied (403) - Not found (404) - Network errors - File already exists - Update documentation (README.md, SPEC.md) with usage examples Users can now download attachments from any content they have permission to view in Basecamp, without requiring browser cookies. Resolves #131 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Copilot

Pull request overview

This PR implements attachment download functionality for Basecamp cards using OAuth Bearer token authentication. The feature allows users to download images and files from cards to their local filesystem with configurable options for output directory, overwrite behavior, and selective attachment download.

Changes:

Added new API layer for upload operations with GetUpload() and DownloadAttachment() methods
Extended attachment parser with helper functions for extracting bucket and upload IDs from URLs
Implemented bc4 card download-attachments command with comprehensive flags and user-friendly output
Updated documentation with usage examples and technical details

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
internal/api/uploads.go	New file implementing upload metadata retrieval and file download with OAuth authentication
internal/api/modular.go	Added UploadOperations interface and Uploads() accessor method
internal/attachments/parser.go	Added helper functions for extracting bucket and upload IDs from Basecamp URLs
cmd/card/download_attachments.go	New command implementing attachment download with flags for output directory, overwrite, and attachment selection
cmd/card/card.go	Registered new download-attachments subcommand
README.md	Added usage examples for the new download-attachments command
docs/SPEC.md	Documented attachment download flow and API integration details

Copilot · 2026-01-19T20:46:59Z

cmd/card/download_attachments.go

+			// Download each attachment
+			successful := 0
+			failed := 0
+			ctx := context.Background()


Using context.Background() creates a new context instead of using the context from the factory. This bypasses any timeout, cancellation, or tracing configured in the factory. Use f.Context() instead to ensure proper context propagation throughout the application.

Copilot · 2026-01-19T20:46:59Z

cmd/card/download_attachments.go

+// sanitizeFilename removes or replaces characters that are unsafe for filenames
+func sanitizeFilename(filename string) string {
+	// For now, just return the filename as-is
+	// In production, you might want to replace invalid characters
+	return filename


The sanitizeFilename function is not implemented and simply returns the filename as-is. This creates a security vulnerability where malicious filenames containing path traversal sequences (e.g., "../../../etc/passwd") or invalid filesystem characters could cause files to be written outside the intended directory or fail to write. Implement proper filename sanitization by removing or replacing directory separators, null bytes, and other filesystem-unsafe characters.

Copilot · 2026-01-19T20:47:00Z

internal/api/uploads.go

+}
+
+// GetUpload fetches upload details by ID
+func (c *Client) GetUpload(ctx context.Context, bucketID string, uploadID int64) (*Upload, error) {


The ctx parameter is declared but never used in the function. The GetUpload and DownloadAttachment calls should use this context parameter instead of ignoring it. Either remove the unused parameter or pass it to the API calls that support context (if the API methods are updated to accept context).

Suggested change

func (c *Client) GetUpload(ctx context.Context, bucketID string, uploadID int64) (*Upload, error) {

func (c *Client) GetUpload(ctx context.Context, bucketID string, uploadID int64) (*Upload, error) {

if err := ctx.Err(); err != nil {

return nil, err

}

Copilot · 2026-01-19T20:47:00Z

internal/api/uploads.go

+}
+
+// DownloadAttachment downloads a file from a download URL to the specified path
+func (c *Client) DownloadAttachment(ctx context.Context, downloadURL, destPath string) error {


The ctx parameter is declared but never used in this function. Context should be passed through to any network operations for proper timeout and cancellation support. Either use the context with the HTTP request via req.WithContext(ctx) or remove the unused parameter.

Copilot · 2026-01-19T20:47:00Z

internal/api/uploads.go

+package api
+
+import (
+	"context"
+	"encoding/json"
+	"fmt"
+	"io"
+	"net/http"
+	"os"
+	"path/filepath"
+	"regexp"
+	"time"
+
+	"github.com/needmore/bc4/internal/errors"
+	"github.com/needmore/bc4/internal/version"
+)
+
+// Upload represents a Basecamp upload (file attachment)
+type Upload struct {
+	ID             int64     `json:"id"`
+	Filename       string    `json:"filename"`
+	ContentType    string    `json:"content_type"`
+	ByteSize       int64     `json:"byte_size"`
+	DownloadURL    string    `json:"download_url"`
+	AppDownloadURL string    `json:"app_download_url"`
+	CreatedAt      time.Time `json:"created_at"`
+	UpdatedAt      time.Time `json:"updated_at"`
+}
+
+// GetUpload fetches upload details by ID
+func (c *Client) GetUpload(ctx context.Context, bucketID string, uploadID int64) (*Upload, error) {
+	path := fmt.Sprintf("/buckets/%s/uploads/%d.json", bucketID, uploadID)
+
+	resp, err := c.doRequest("GET", path, nil)
+	if err != nil {
+		return nil, err
+	}
+	defer func() { _ = resp.Body.Close() }()
+
+	var upload Upload
+	if err := json.NewDecoder(resp.Body).Decode(&upload); err != nil {
+		return nil, fmt.Errorf("failed to decode upload response: %w", err)
+	}
+
+	return &upload, nil
+}
+
+// DownloadAttachment downloads a file from a download URL to the specified path
+func (c *Client) DownloadAttachment(ctx context.Context, downloadURL, destPath string) error {
+	// Create the request with OAuth authentication
+	req, err := c.createAuthenticatedRequest("GET", downloadURL, nil)
+	if err != nil {
+		return fmt.Errorf("failed to create download request: %w", err)
+	}
+
+	// Execute the request
+	resp, err := c.httpClient.Do(req)
+	if err != nil {
+		return fmt.Errorf("failed to download attachment: %w", err)
+	}
+	defer func() { _ = resp.Body.Close() }()
+
+	// Check for HTTP errors
+	if resp.StatusCode >= 400 {
+		body, _ := io.ReadAll(resp.Body)
+		switch resp.StatusCode {
+		case 403:
+			return errors.NewAPIError(resp.StatusCode, fmt.Sprintf("permission denied: %s", string(body)), nil)
+		case 404:
+			return errors.NewNotFoundError("attachment", "", fmt.Errorf("attachment not found: %s", string(body)))
+		default:
+			return errors.NewAPIError(resp.StatusCode, string(body), nil)
+		}
+	}
+
+	// Create the destination directory if it doesn't exist
+	destDir := filepath.Dir(destPath)
+	if err := os.MkdirAll(destDir, 0755); err != nil {
+		return fmt.Errorf("failed to create destination directory: %w", err)
+	}
+
+	// Create the destination file
+	outFile, err := os.Create(destPath)
+	if err != nil {
+		return fmt.Errorf("failed to create destination file: %w", err)
+	}
+	defer func() { _ = outFile.Close() }()
+
+	// Copy the response body to the file
+	if _, err := io.Copy(outFile, resp.Body); err != nil {
+		return fmt.Errorf("failed to write attachment to file: %w", err)
+	}
+
+	// Set file permissions to 0644
+	if err := outFile.Chmod(0644); err != nil {
+		return fmt.Errorf("failed to set file permissions: %w", err)
+	}
+
+	return nil
+}
+
+// createAuthenticatedRequest creates an HTTP request with OAuth authentication
+func (c *Client) createAuthenticatedRequest(method, url string, body io.Reader) (*http.Request, error) {
+	req, err := http.NewRequest(method, url, body)
+	if err != nil {
+		return nil, err
+	}
+
+	req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", c.accessToken))
+	req.Header.Set("User-Agent", version.UserAgent())
+
+	return req, nil
+}
+
+// ExtractUploadID extracts the upload ID from various URL formats
+// Supports:
+// - https://3.basecampapi.com/{account}/buckets/{bucket}/uploads/{id}/download/{filename}
+// - https://3.basecamp.com/{account}/buckets/{bucket}/uploads/{id}
+func ExtractUploadID(url string) (int64, error) {
+	// Pattern for API download URL
+	apiPattern := regexp.MustCompile(`/uploads/(\d+)/download`)
+	if matches := apiPattern.FindStringSubmatch(url); len(matches) > 1 {
+		var id int64
+		if _, err := fmt.Sscanf(matches[1], "%d", &id); err != nil {
+			return 0, fmt.Errorf("failed to parse upload ID: %w", err)
+		}
+		return id, nil
+	}
+
+	// Pattern for app URL
+	appPattern := regexp.MustCompile(`/uploads/(\d+)`)
+	if matches := appPattern.FindStringSubmatch(url); len(matches) > 1 {
+		var id int64
+		if _, err := fmt.Sscanf(matches[1], "%d", &id); err != nil {
+			return 0, fmt.Errorf("failed to parse upload ID: %w", err)
+		}
+		return id, nil
+	}
+
+	return 0, fmt.Errorf("could not extract upload ID from URL: %s", url)
+}


Missing test coverage for the new upload operations. The repository has comprehensive test coverage for other API functions (e.g., attachments_test.go, messages_test.go, retry_test.go), but no tests exist for GetUpload, DownloadAttachment, and ExtractUploadID functions. Consider adding unit tests following the existing patterns in the codebase.

Copilot · 2026-01-19T20:47:00Z

internal/api/uploads.go

+// ExtractUploadID extracts the upload ID from various URL formats
+// Supports:
+// - https://3.basecampapi.com/{account}/buckets/{bucket}/uploads/{id}/download/{filename}
+// - https://3.basecamp.com/{account}/buckets/{bucket}/uploads/{id}
+func ExtractUploadID(url string) (int64, error) {
+	// Pattern for API download URL
+	apiPattern := regexp.MustCompile(`/uploads/(\d+)/download`)
+	if matches := apiPattern.FindStringSubmatch(url); len(matches) > 1 {
+		var id int64
+		if _, err := fmt.Sscanf(matches[1], "%d", &id); err != nil {
+			return 0, fmt.Errorf("failed to parse upload ID: %w", err)
+		}
+		return id, nil
+	}
+
+	// Pattern for app URL
+	appPattern := regexp.MustCompile(`/uploads/(\d+)`)
+	if matches := appPattern.FindStringSubmatch(url); len(matches) > 1 {
+		var id int64
+		if _, err := fmt.Sscanf(matches[1], "%d", &id); err != nil {
+			return 0, fmt.Errorf("failed to parse upload ID: %w", err)
+		}
+		return id, nil
+	}
+
+	return 0, fmt.Errorf("could not extract upload ID from URL: %s", url)
+}


There are two nearly identical functions for extracting upload IDs: ExtractUploadID in internal/api/uploads.go and ExtractUploadIDFromURL in internal/attachments/parser.go. Both use similar regex patterns but with slight differences. The API version tries to match /uploads/(\d+)/download first, while the attachments version uses /uploads/(\d+)(/|$). This duplication creates maintenance burden and potential inconsistency. Consider consolidating these into a single well-tested function in one location, or clearly document why both are needed if they serve different purposes.

Copilot · 2026-01-19T20:47:01Z

cmd/card/download_attachments.go

+					displayIndex = attachmentIndex
+				}
+
+				fmt.Printf("Downloading attachment %d/%d: %s\n", displayIndex, len(atts), att.GetDisplayName())


When filtering to a specific attachment with --attachment, the display shows incorrect values. For example, if card has 5 attachments and user requests --attachment 2, line 137 will show "Downloading attachment 2/1: filename" because len(atts) is 1 after filtering. The second parameter should remain the original count or be calculated differently to avoid confusion.

Suggested change

fmt.Printf("Downloading attachment %d/%d: %s\n", displayIndex, len(atts), att.GetDisplayName())

if attachmentIndex > 0 {

// When downloading a specific attachment, show only its index to avoid confusing "current/total" output

fmt.Printf("Downloading attachment %d: %s\n", displayIndex, att.GetDisplayName())

} else {

fmt.Printf("Downloading attachment %d/%d: %s\n", displayIndex, len(atts), att.GetDisplayName())

}

Copilot · 2026-01-19T20:47:01Z

internal/attachments/parser.go

+// ExtractBucketID extracts the bucket ID from a Basecamp URL
+func ExtractBucketID(url string) (string, error) {
+	re := regexp.MustCompile(`/buckets/(\d+)`)
+	matches := re.FindStringSubmatch(url)
+	if len(matches) > 1 {
+		return matches[1], nil
+	}
+	return "", fmt.Errorf("could not extract bucket ID from URL: %s", url)
+}
+
+// ExtractUploadIDFromURL extracts the upload ID from a download or app URL
+func ExtractUploadIDFromURL(url string) (int64, error) {
+	// Pattern for download URL with /uploads/{id}/
+	re := regexp.MustCompile(`/uploads/(\d+)(/|$)`)
+	matches := re.FindStringSubmatch(url)
+	if len(matches) > 1 {
+		var id int64
+		if _, err := fmt.Sscanf(matches[1], "%d", &id); err != nil {
+			return 0, fmt.Errorf("failed to parse upload ID: %w", err)
+		}
+		return id, nil
+	}
+	return 0, fmt.Errorf("could not extract upload ID from URL: %s", url)
+}


Missing test coverage for newly added helper functions. The repository has existing test coverage for the attachments package (parser_test.go), but the new functions ExtractBucketID and ExtractUploadIDFromURL lack tests. Consider adding tests following the existing patterns to validate various URL formats and error cases.

Copilot · 2026-01-19T20:47:01Z

internal/api/modular.go

+	return c.Client
+}
+
+


Extra blank line after the function definition. This is inconsistent with the formatting of other similar functions in the file. Remove the blank line at line 202 for consistency.

Suggested change

Copilot · 2026-01-19T20:47:01Z

internal/api/uploads.go

+	// Copy the response body to the file
+	if _, err := io.Copy(outFile, resp.Body); err != nil {
+		return fmt.Errorf("failed to write attachment to file: %w", err)
+	}


Potential issue with incomplete file writes. If io.Copy fails partway through, the partially written file will remain on disk. Consider using a temporary file pattern (write to .tmp suffix, then rename on success) or clean up the file on error to avoid leaving corrupted partial downloads.

Comment attachments use Google Cloud Storage blob URLs that require browser session cookies rather than OAuth Bearer tokens. This is a Basecamp API architectural decision that affects what can be downloaded programmatically. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Complete the attachment download feature by adding commands for todos and messages. Remove the comment download-attachments command since comment attachments use blob storage that doesn't support OAuth. Changes: - Add bc4 todo download-attachments command - Add bc4 message download-attachments command - Remove bc4 comment download-attachments (blob storage limitation) - Update README with examples for all working download commands - All tests pass, build succeeds The download-attachments feature now works for: ✅ Cards (via uploads API with OAuth) ✅ Todos (via uploads API with OAuth) ✅ Messages (via uploads API with OAuth) ❌ Comments (use blob storage, require browser cookies) Closes #131 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Add comprehensive documentation section for the download-attachments feature with examples for all three commands (card, todo, message), common options, and a clear note about the comment attachment limitation. This makes the feature more discoverable and provides users with a single reference point for attachment downloading capabilities. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Implement all critical and code quality fixes identified by Copilot review: Security & Critical Fixes: - Implement proper filename sanitization to prevent path traversal attacks (removes directory separators, null bytes, control characters, and filesystem-unsafe characters) - Use factory context (f.Context()) instead of context.Background() to enable proper timeout, cancellation, and tracing - Add atomic file writes with .tmp suffix and cleanup on error to prevent partial/corrupted downloads - Add context support to HTTP requests for proper cancellation Code Quality Fixes: - Fix display bug when using --attachment flag (now shows "Downloading attachment 2" instead of confusing "Downloading attachment 2/1") - Remove duplicate ExtractUploadID function from uploads.go (consolidated in attachments/parser.go) - Use context parameter in GetUpload and DownloadAttachment API methods - Remove extra blank line in modular.go for consistency - Remove leftover download-attachments reference from comment command All changes tested: - Build succeeds - All tests pass - Help output verified for all three commands Addresses Copilot review comments on PR #132 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings January 19, 2026 20:42

Copilot started reviewing on behalf of brigleb January 19, 2026 20:42 View session

Copilot AI reviewed Jan 19, 2026

View reviewed changes

brigleb and others added 4 commits January 19, 2026 14:28

brigleb merged commit 4cb9bad into main Jan 19, 2026
1 of 2 checks passed

brigleb deleted the feat/attachment-downloads branch January 19, 2026 23:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement attachment download with OAuth authentication#132

feat: implement attachment download with OAuth authentication#132
brigleb merged 5 commits intomainfrom
feat/attachment-downloads

brigleb commented Jan 19, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 19, 2026

Uh oh!

Copilot AI Jan 19, 2026

Uh oh!

Copilot AI Jan 19, 2026

Uh oh!

Copilot AI Jan 19, 2026

Uh oh!

Copilot AI Jan 19, 2026

Uh oh!

Copilot AI Jan 19, 2026

Uh oh!

Copilot AI Jan 19, 2026

Uh oh!

Copilot AI Jan 19, 2026

Uh oh!

Copilot AI Jan 19, 2026

Uh oh!

Copilot AI Jan 19, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-				fmt.Printf("Downloading attachment %d/%d: %s\n", displayIndex, len(atts), att.GetDisplayName())
+				if attachmentIndex > 0 {
+					// When downloading a specific attachment, show only its index to avoid confusing "current/total" output
+					fmt.Printf("Downloading attachment %d: %s\n", displayIndex, att.GetDisplayName())
+				} else {
+					fmt.Printf("Downloading attachment %d/%d: %s\n", displayIndex, len(atts), att.GetDisplayName())
+				}

Conversation

brigleb commented Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Key Features

Implementation Details

Usage Examples

Known Limitations

Error Handling

Testing

What Changed from Original Plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

brigleb commented Jan 19, 2026 •

edited

Loading