Skip to content

Conversation

@qwang07
Copy link

@qwang07 qwang07 commented Oct 21, 2025

Closes #15

Summary

This PR implements file upload functionality for the MCP REST API Tester, allowing users to test API endpoints that require file uploads. The implementation uses multipart/form-data encoding and follows industry-standard practices.

Features

  • File Upload via Local Paths: Upload files from local filesystem
  • Multiple Files Support: Upload multiple files in a single request
  • Form Fields Mixing: Combine file uploads with form fields and JSON body
  • Configurable Size Limits: FILE_UPLOAD_SIZE_LIMIT environment variable (default: 10MB)
  • Security: Path traversal attack protection (rejects paths containing ..)
  • Modular Design: Extracted file utilities to src/file-utils.ts

Implementation Details

New Files

  • src/file-utils.ts: Modular file upload utilities
    • validateFilePath(): Path traversal protection
    • checkFileExists(): File existence validation
    • checkFileSize(): Size limit enforcement
    • validateFiles(): Batch validation
    • createFormData(): multipart/form-data builder

Modified Files

  • src/index.ts: Integrated file upload into MCP server
  • package.json: Added dependencies (form-data) and test scripts
  • README.md: Updated features and usage examples
  • src/resources/config.md: Added FILE_UPLOAD_SIZE_LIMIT documentation
  • src/resources/examples.md: Added comprehensive file upload examples
  • .gitignore: Added test-generated files and pnpm-lock.yaml

Test Coverage

  • 23 unit tests using Node.js native test runner
  • TDD approach (test-driven development)
  • Tests cover:
    • File path validation (path traversal protection)
    • File size checking
    • FormData creation
    • Error handling
  • All tests passing ✅

Usage Example

// Single file upload
use_mcp_tool('rest-api', 'test_request', {
  "method": "POST",
  "endpoint": "/upload",
  "files": [
    {
      "fieldName": "avatar",
      "filePath": "/path/to/profile.jpg"
    }
  ],
  "formFields": {
    "username": "john"
  }
});

Testing

Tested on macOS with:

  • Unit tests: npm test (23/23 passing)
  • Integration tests: Express test server
  • End-to-end tests: MCP Inspector and Claude Desktop
  • Security tests: Path traversal attack prevention verified

Breaking Changes

None. This is a backward-compatible feature addition.

Additional Notes

  • Following multipart/form-data industry standard (field-by-field approach)
  • Respects existing authentication and custom header configurations
  • Header priority maintained: FormData headers → Custom headers → Request headers → Auth headers

🤖 Generated with Claude Code

Quan Wang and others added 3 commits October 21, 2025 23:35
- Implement file upload via local file paths
- Support multiple files with form fields mixing
- Add file size limit (configurable via FILE_UPLOAD_SIZE_LIMIT)
- Modular file-utils.ts with validation and FormData creation
- Path traversal attack protection
- 23 unit tests with TDD approach
- Complete documentation and examples

Closes dkmaker#15

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
- Update package.json with @qwang007 scope
- Update package name to @qwang007/mcp-rest-api
- Update version to 0.5.0
- Update CHANGELOG.md for v0.5.0 release
- Update README with fork notice and new package name
- Remove Smithery installation (not applicable for fork)
- Update installation commands and configuration examples
- Add .gitignore rules for secrets and tokens

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@dkmaker
Copy link
Owner

dkmaker commented Oct 23, 2025

Great - i will check it in a few days - good work!!

@dkmaker
Copy link
Owner

dkmaker commented Jan 9, 2026

@qwang07

PR Review: Add file upload support with multipart/form-data

Thanks for this substantial contribution! The file upload feature itself is well-implemented with good test coverage and documentation. However, there are several issues that need to be addressed before this can be merged upstream.

Critical Issues

1. Package renaming and forking changes (Must Fix)

This PR changes the package identity to point to your fork, which is problematic for an upstream merge:

  • package.json: name changed to @qwang007/mcp-rest-api, repository/bugs/homepage URLs point to your fork
  • README.md: Title changed to "(Enhanced)", installation instructions point to your fork, badges removed
  • CHANGELOG.md: commit links point to your fork

Please revert these changes and keep only the file upload feature additions. The package should remain dkmaker-mcp-rest-api with original repository URLs.

2. Removed badges and installation methods

The PR removes:

  • MseeP.ai security badge
  • Glama badge
  • Smithery installation instructions

These should be preserved for the upstream repository.

Security Concerns

3. Path traversal protection is incomplete

The current check only looks for .. in the path:

if (filePath.includes('..')) {
  throw new Error('Path traversal attack detected');
}

This doesn't catch:

  • URL-encoded sequences like %2e%2e
  • Null bytes in paths
  • Symlink-based attacks

Consider strengthening this:

import * as path from 'path';

export const validateFilePath = (filePath: string): void => {
  // Check for null bytes
  if (filePath.includes('\0')) {
    throw new Error('Invalid null byte in path');
  }
  
  // Normalize and check for path traversal
  const normalized = path.normalize(filePath);
  if (normalized.includes('..')) {
    throw new Error('Path traversal attack detected');
  }
};

Other Issues

4. Chinese comments and documentation

Some code has Chinese comments (e.g., // FormData自动设置headers,需要合并 in src/index.ts), and test files (test/README.md, test/test-upload.md) are entirely in Chinese. For consistency with the English-language codebase, please translate these.

5. pnpm vs npm inconsistency

The prepare script was changed from npm run build to pnpm run build. This could break builds for npm users. Please keep npm for broader compatibility.


What's Good

  • Clean modular design with file-utils.ts separation
  • Comprehensive test coverage (23 unit tests)
  • Good documentation and examples
  • Configurable size limits via FILE_UPLOAD_SIZE_LIMIT
  • TDD approach using Node.js native test runner

Summary

The file upload implementation is solid and valuable. To merge this upstream, please:

  1. Must fix: Revert all package renaming/forking changes (keep dkmaker-mcp-rest-api)
  2. Must fix: Preserve existing badges and Smithery installation instructions
  3. Should fix: Translate Chinese comments and documentation to English
  4. Should fix: Keep npm run build in prepare script
  5. Nice to have: Strengthen path traversal protection

Happy to approve once the fork-specific changes are separated from the feature implementation!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature Request: support file upload

2 participants