mirror of
https://github.com/aaronsb/obsidian-mcp-plugin.git
synced 2026-07-22 06:45:14 +00:00
docs: Add comprehensive project documentation and security audit
- Add CONTRIBUTING.md with development guidelines
- Add SECURITY.md with vulnerability reporting policy
- Create GitHub issue templates for bugs and features
- Add PR template with checklist
- Set up GitHub Actions for testing and security scanning
- Document all security findings from code audit
- Create detailed GitHub issues for all vulnerabilities
- Update CHANGELOG with security audit results
- Add project structure documentation
This establishes proper open-source project structure and documents
all critical security issues that need to be addressed.
🤖 Generated with Claude Code
Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Aaron Bockelie
parent
9413d5543e
commit
36301118a9
16 changed files with 1750 additions and 0 deletions
41
.github/ISSUE_TEMPLATE/bug_report.md
vendored
Normal file
41
.github/ISSUE_TEMPLATE/bug_report.md
vendored
Normal file
|
|
@ -0,0 +1,41 @@
|
|||
---
|
||||
name: Bug report
|
||||
about: Create a report to help us improve
|
||||
title: ''
|
||||
labels: bug
|
||||
assignees: ''
|
||||
|
||||
---
|
||||
|
||||
**Describe the bug**
|
||||
A clear and concise description of what the bug is.
|
||||
|
||||
**To Reproduce**
|
||||
Steps to reproduce the behavior:
|
||||
1. Go to '...'
|
||||
2. Click on '....'
|
||||
3. Send MCP request '....'
|
||||
4. See error
|
||||
|
||||
**Expected behavior**
|
||||
What you expected to happen.
|
||||
|
||||
**Actual behavior**
|
||||
What actually happened instead.
|
||||
|
||||
**Screenshots**
|
||||
If applicable, add screenshots to help explain your problem.
|
||||
|
||||
**Environment:**
|
||||
- OS: [e.g. macOS, Windows, Linux]
|
||||
- Obsidian version: [e.g. 1.5.0]
|
||||
- Plugin version: [e.g. 0.4.4]
|
||||
- MCP client: [e.g. Claude Desktop, Cline, custom]
|
||||
|
||||
**Logs**
|
||||
```
|
||||
Paste any relevant error messages or logs here
|
||||
```
|
||||
|
||||
**Additional context**
|
||||
Add any other context about the problem here.
|
||||
32
.github/ISSUE_TEMPLATE/feature_request.md
vendored
Normal file
32
.github/ISSUE_TEMPLATE/feature_request.md
vendored
Normal file
|
|
@ -0,0 +1,32 @@
|
|||
---
|
||||
name: Feature request
|
||||
about: Suggest an idea for this project
|
||||
title: ''
|
||||
labels: enhancement
|
||||
assignees: ''
|
||||
|
||||
---
|
||||
|
||||
**Is your feature request related to a problem? Please describe.**
|
||||
A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]
|
||||
|
||||
**Describe the solution you'd like**
|
||||
A clear and concise description of what you want to happen.
|
||||
|
||||
**Describe alternatives you've considered**
|
||||
A clear and concise description of any alternative solutions or features you've considered.
|
||||
|
||||
**Additional context**
|
||||
Add any other context, mockups, or examples about the feature request here.
|
||||
|
||||
**Example MCP usage**
|
||||
If applicable, show how this feature would be used via MCP:
|
||||
```json
|
||||
{
|
||||
"method": "tools/example",
|
||||
"params": {
|
||||
"action": "new_feature",
|
||||
"data": "..."
|
||||
}
|
||||
}
|
||||
```
|
||||
28
.github/pull_request_template.md
vendored
Normal file
28
.github/pull_request_template.md
vendored
Normal file
|
|
@ -0,0 +1,28 @@
|
|||
## Description
|
||||
Brief description of what this PR does.
|
||||
|
||||
## Type of Change
|
||||
- [ ] Bug fix
|
||||
- [ ] New feature
|
||||
- [ ] Security fix
|
||||
- [ ] Performance improvement
|
||||
- [ ] Code refactoring
|
||||
- [ ] Documentation update
|
||||
|
||||
## Testing
|
||||
- [ ] Built and tested locally
|
||||
- [ ] Tested with BRAT
|
||||
- [ ] All tests pass (`npm test`)
|
||||
|
||||
## Security Considerations
|
||||
Have you introduced any security-sensitive changes?
|
||||
- [ ] No security impact
|
||||
- [ ] Security improvements
|
||||
- [ ] Requires security review
|
||||
|
||||
## Checklist
|
||||
- [ ] Code follows project style
|
||||
- [ ] Self-reviewed my code
|
||||
- [ ] Updated CHANGELOG.md
|
||||
- [ ] No console.log() left in code
|
||||
- [ ] Version bump if needed
|
||||
34
.github/workflows/security.yml
vendored
Normal file
34
.github/workflows/security.yml
vendored
Normal file
|
|
@ -0,0 +1,34 @@
|
|||
name: Security Checks
|
||||
|
||||
on:
|
||||
push:
|
||||
branches: [ main ]
|
||||
pull_request:
|
||||
branches: [ main ]
|
||||
schedule:
|
||||
- cron: '0 0 * * 0' # Weekly on Sunday
|
||||
|
||||
jobs:
|
||||
security:
|
||||
runs-on: ubuntu-latest
|
||||
|
||||
steps:
|
||||
- uses: actions/checkout@v3
|
||||
|
||||
- name: Run npm audit
|
||||
run: npm audit --production
|
||||
continue-on-error: true # Don't fail the build, just report
|
||||
|
||||
- name: Run security checks
|
||||
uses: aquasecurity/trivy-action@master
|
||||
with:
|
||||
scan-type: 'fs'
|
||||
scan-ref: '.'
|
||||
format: 'sarif'
|
||||
output: 'trivy-results.sarif'
|
||||
|
||||
- name: Upload Trivy scan results
|
||||
uses: github/codeql-action/upload-sarif@v2
|
||||
if: always()
|
||||
with:
|
||||
sarif_file: 'trivy-results.sarif'
|
||||
42
.github/workflows/test.yml
vendored
Normal file
42
.github/workflows/test.yml
vendored
Normal file
|
|
@ -0,0 +1,42 @@
|
|||
name: Tests
|
||||
|
||||
on:
|
||||
push:
|
||||
branches: [ main ]
|
||||
pull_request:
|
||||
branches: [ main ]
|
||||
|
||||
jobs:
|
||||
test:
|
||||
runs-on: ubuntu-latest
|
||||
|
||||
strategy:
|
||||
matrix:
|
||||
node-version: [18.x, 20.x]
|
||||
|
||||
steps:
|
||||
- uses: actions/checkout@v3
|
||||
|
||||
- name: Use Node.js ${{ matrix.node-version }}
|
||||
uses: actions/setup-node@v3
|
||||
with:
|
||||
node-version: ${{ matrix.node-version }}
|
||||
cache: 'npm'
|
||||
|
||||
- name: Install dependencies
|
||||
run: npm ci
|
||||
|
||||
- name: Build
|
||||
run: npm run build
|
||||
|
||||
- name: Run tests
|
||||
run: npm test
|
||||
|
||||
- name: Run linter
|
||||
run: npm run lint
|
||||
continue-on-error: true # Don't fail on lint warnings for now
|
||||
|
||||
# Optional: Add code coverage
|
||||
# - name: Upload coverage reports
|
||||
# uses: codecov/codecov-action@v3
|
||||
# if: matrix.node-version == '20.x'
|
||||
18
CHANGELOG.md
18
CHANGELOG.md
|
|
@ -5,6 +5,24 @@ All notable changes to the Obsidian MCP Plugin will be documented in this file.
|
|||
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
|
||||
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
### Security
|
||||
- 🔴 CRITICAL: Identified authentication vulnerability - no API key validation ([#9](https://github.com/aaronsb/obsidian-mcp-plugin/issues/9))
|
||||
- 🔴 CRITICAL: Identified path traversal vulnerability in file operations ([#10](https://github.com/aaronsb/obsidian-mcp-plugin/issues/10))
|
||||
- 🟠 HIGH: Identified missing input validation across all operations ([#11](https://github.com/aaronsb/obsidian-mcp-plugin/issues/11))
|
||||
- 🟠 HIGH: Identified insecure session management implementation ([#12](https://github.com/aaronsb/obsidian-mcp-plugin/issues/12))
|
||||
|
||||
### Added
|
||||
- Security policy (SECURITY.md) for vulnerability reporting
|
||||
- Contributing guidelines (CONTRIBUTING.md)
|
||||
- Issue templates for bug reports and feature requests
|
||||
- GitHub labels for security, priority, and technical debt tracking
|
||||
- Comprehensive security audit documentation
|
||||
|
||||
### Changed
|
||||
- Project structure improved with proper open-source documentation
|
||||
|
||||
## [0.5.14] - 2025-01-11
|
||||
|
||||
### Added
|
||||
|
|
|
|||
66
CONTRIBUTING.md
Normal file
66
CONTRIBUTING.md
Normal file
|
|
@ -0,0 +1,66 @@
|
|||
# Contributing to Obsidian MCP Plugin
|
||||
|
||||
Thanks for your interest in contributing! Since this is a small project, we keep things simple.
|
||||
|
||||
## Quick Start
|
||||
|
||||
1. **Found a bug?** Open an issue with:
|
||||
- What you expected to happen
|
||||
- What actually happened
|
||||
- Steps to reproduce
|
||||
|
||||
2. **Want to fix something?**
|
||||
- Fork the repo
|
||||
- Make your changes
|
||||
- Test locally with BRAT
|
||||
- Submit a PR with a clear description
|
||||
|
||||
## Development Setup
|
||||
|
||||
```bash
|
||||
# Clone your fork
|
||||
git clone https://github.com/YOUR_USERNAME/obsidian-mcp-plugin.git
|
||||
cd obsidian-mcp-plugin
|
||||
|
||||
# Install dependencies
|
||||
npm install
|
||||
|
||||
# Build and watch
|
||||
npm run dev
|
||||
|
||||
# Run tests
|
||||
npm test
|
||||
```
|
||||
|
||||
## Code Style
|
||||
|
||||
- TypeScript with strict mode
|
||||
- Follow existing patterns in the codebase
|
||||
- Keep security in mind (see SECURITY.md)
|
||||
- Add tests for new features
|
||||
|
||||
## Testing with BRAT
|
||||
|
||||
1. Build the plugin: `npm run build`
|
||||
2. In Obsidian, install BRAT plugin
|
||||
3. Add your local build: `YOUR_USERNAME/obsidian-mcp-plugin`
|
||||
4. Test thoroughly before submitting PR
|
||||
|
||||
## Commit Messages
|
||||
|
||||
Keep them clear and descriptive:
|
||||
- `fix: Prevent path traversal in file operations`
|
||||
- `feat: Add rate limiting to API endpoints`
|
||||
- `docs: Update security guidelines`
|
||||
- `refactor: Extract validation logic to separate module`
|
||||
|
||||
## Current Focus Areas
|
||||
|
||||
Check our [GitHub Issues](https://github.com/aaronsb/obsidian-mcp-plugin/issues) for:
|
||||
- 🔴 Security vulnerabilities (highest priority)
|
||||
- 🟠 Input validation improvements
|
||||
- 🟡 Code quality refactoring
|
||||
|
||||
## Questions?
|
||||
|
||||
Open an issue or discussion - we're here to help!
|
||||
63
SECURITY.md
Normal file
63
SECURITY.md
Normal file
|
|
@ -0,0 +1,63 @@
|
|||
# Security Policy
|
||||
|
||||
## Reporting Security Vulnerabilities
|
||||
|
||||
We take security seriously. If you discover a security vulnerability, please:
|
||||
|
||||
1. **DO NOT** open a public issue
|
||||
2. **DO** report it via GitHub Security Advisories: [Report a vulnerability](https://github.com/aaronsb/obsidian-mcp-plugin/security/advisories/new)
|
||||
3. **OR** email details to the maintainer (check commit history for email)
|
||||
|
||||
## What to Include
|
||||
|
||||
- Description of the vulnerability
|
||||
- Steps to reproduce
|
||||
- Potential impact
|
||||
- Suggested fix (if any)
|
||||
|
||||
## Known Security Issues
|
||||
|
||||
We're actively working on fixing these security vulnerabilities:
|
||||
|
||||
| Issue | Status | Priority |
|
||||
|-------|---------|----------|
|
||||
| No authentication on MCP server | 🔧 In Progress | CRITICAL |
|
||||
| Path traversal in file operations | 📋 Planned | CRITICAL |
|
||||
| Missing input validation | 📋 Planned | HIGH |
|
||||
| Insecure session management | 📋 Planned | HIGH |
|
||||
|
||||
See our [security issues](https://github.com/aaronsb/obsidian-mcp-plugin/issues?q=is%3Aissue+is%3Aopen+label%3Asecurity) for details.
|
||||
|
||||
## Security Best Practices
|
||||
|
||||
Until security improvements are complete:
|
||||
|
||||
1. **Only use on trusted networks** (localhost only)
|
||||
2. **Don't expose the MCP port** to the internet
|
||||
3. **Monitor vault access** for unexpected changes
|
||||
4. **Keep backups** of your vault
|
||||
5. **Review plugin permissions** in Obsidian
|
||||
|
||||
## Secure Configuration
|
||||
|
||||
```json
|
||||
{
|
||||
"httpEnabled": true,
|
||||
"httpPort": 3001, // Change from default
|
||||
"autoDetectPortConflicts": true,
|
||||
"debugLogging": false // Disable in production
|
||||
}
|
||||
```
|
||||
|
||||
## Future Security Enhancements
|
||||
|
||||
- [ ] API key authentication
|
||||
- [ ] Path validation framework
|
||||
- [ ] Input sanitization
|
||||
- [ ] Rate limiting
|
||||
- [ ] Audit logging
|
||||
- [ ] Encrypted sessions
|
||||
|
||||
## Acknowledgments
|
||||
|
||||
Thanks to security researchers who responsibly disclose vulnerabilities.
|
||||
77
docs/PROJECT_STRUCTURE.md
Normal file
77
docs/PROJECT_STRUCTURE.md
Normal file
|
|
@ -0,0 +1,77 @@
|
|||
# Project Structure
|
||||
|
||||
```
|
||||
obsidian-mcp-plugin/
|
||||
├── .github/
|
||||
│ ├── ISSUE_TEMPLATE/
|
||||
│ │ ├── bug_report.md
|
||||
│ │ └── feature_request.md
|
||||
│ ├── workflows/
|
||||
│ │ ├── test.yml # CI/CD tests
|
||||
│ │ └── security.yml # Security scanning
|
||||
│ └── pull_request_template.md
|
||||
│
|
||||
├── src/
|
||||
│ ├── main.ts # Plugin entry point
|
||||
│ ├── mcp-server.ts # MCP HTTP server
|
||||
│ ├── semantic/ # Semantic operations
|
||||
│ │ └── router.ts # Operation routing
|
||||
│ ├── tools/ # MCP tool implementations
|
||||
│ ├── utils/ # Utility functions
|
||||
│ │ ├── obsidian-api.ts # Vault operations
|
||||
│ │ ├── session-manager.ts # Session handling
|
||||
│ │ └── connection-pool.ts # Connection management
|
||||
│ └── types/ # TypeScript definitions
|
||||
│
|
||||
├── github-issues/ # Security audit findings
|
||||
│ ├── 01-authentication-vulnerability.md
|
||||
│ ├── 02-path-traversal-vulnerability.md
|
||||
│ ├── 03-input-validation-missing.md
|
||||
│ ├── 04-insecure-session-management.md
|
||||
│ ├── 05-solid-principles-violations.md
|
||||
│ ├── 06-large-vault-scalability.md
|
||||
│ └── README.md
|
||||
│
|
||||
├── tests/ # Test files
|
||||
├── docs/ # Documentation
|
||||
│
|
||||
├── .gitignore
|
||||
├── CHANGELOG.md # Version history
|
||||
├── CONTRIBUTING.md # Contribution guidelines
|
||||
├── LICENSE # MIT License
|
||||
├── README.md # Main documentation
|
||||
├── SECURITY.md # Security policy
|
||||
├── manifest.json # Obsidian plugin manifest
|
||||
├── package.json # Node.js dependencies
|
||||
├── tsconfig.json # TypeScript config
|
||||
└── versions.json # Version compatibility
|
||||
```
|
||||
|
||||
## Key Directories
|
||||
|
||||
### `/src`
|
||||
Core plugin code. Main entry point is `main.ts`.
|
||||
|
||||
### `/src/semantic`
|
||||
Handles semantic routing for MCP operations. The router maps operations to actual implementations.
|
||||
|
||||
### `/src/utils`
|
||||
Shared utilities including the ObsidianAPI abstraction layer and session management.
|
||||
|
||||
### `/.github`
|
||||
GitHub-specific files including issue templates and automated workflows.
|
||||
|
||||
### `/github-issues`
|
||||
Detailed security audit findings ready to be posted as GitHub issues.
|
||||
|
||||
## Configuration Files
|
||||
|
||||
- `manifest.json` - Obsidian plugin metadata
|
||||
- `package.json` - Node dependencies and scripts
|
||||
- `tsconfig.json` - TypeScript compiler settings
|
||||
- `versions.json` - Obsidian version compatibility
|
||||
|
||||
## Development Files
|
||||
|
||||
- `CLAUDE.md` - Project-specific instructions for AI assistants
|
||||
- `.claude/CLAUDE.md` - User's global AI instructions
|
||||
91
github-issues/01-authentication-vulnerability.md
Normal file
91
github-issues/01-authentication-vulnerability.md
Normal file
|
|
@ -0,0 +1,91 @@
|
|||
# 🔴 CRITICAL: No Authentication or Authorization on MCP Server
|
||||
|
||||
## Summary
|
||||
The MCP server exposes full vault access over HTTP without any authentication mechanism, allowing any local application to read, write, and delete files in the Obsidian vault.
|
||||
|
||||
## Current Behavior
|
||||
- Server runs on `http://localhost:3001` with no API key validation
|
||||
- CORS configured to allow ALL origins (`origin: '*'`)
|
||||
- No user authentication or role-based access control
|
||||
- Any local process can manipulate the entire vault
|
||||
|
||||
## Security Impact
|
||||
- **Severity**: CRITICAL
|
||||
- **Attack Vector**: Local network access
|
||||
- **Impact**: Complete vault compromise, data loss, unauthorized access
|
||||
|
||||
## Vulnerable Code
|
||||
```typescript
|
||||
// mcp-server.ts:248-253
|
||||
this.app.use(cors({
|
||||
origin: '*', // Allows any origin!
|
||||
methods: ['GET', 'POST', 'PUT', 'DELETE', 'OPTIONS'],
|
||||
allowedHeaders: ['Origin', 'X-Requested-With', 'Content-Type', 'Accept', 'Authorization', 'Mcp-Session-Id'],
|
||||
exposedHeaders: ['Mcp-Session-Id']
|
||||
}));
|
||||
```
|
||||
|
||||
## Reproduction Steps
|
||||
1. Install and enable the plugin
|
||||
2. From any local application, run:
|
||||
```bash
|
||||
curl -X POST http://localhost:3001/mcp \
|
||||
-H "Content-Type: application/json" \
|
||||
-d '{"method": "tools/vault", "params": {"action": "list"}}'
|
||||
```
|
||||
3. Observe full vault file listing returned without authentication
|
||||
|
||||
## Proposed Solution
|
||||
|
||||
### Phase 1: Basic API Key Authentication
|
||||
```typescript
|
||||
interface AuthConfig {
|
||||
apiKey: string;
|
||||
allowedOrigins: string[];
|
||||
requireHttps: boolean;
|
||||
}
|
||||
|
||||
class AuthMiddleware {
|
||||
private config: AuthConfig;
|
||||
|
||||
authenticate(req: Request, res: Response, next: NextFunction) {
|
||||
const apiKey = req.headers['x-api-key'];
|
||||
|
||||
if (!apiKey || apiKey !== this.config.apiKey) {
|
||||
return res.status(401).json({ error: 'Unauthorized' });
|
||||
}
|
||||
|
||||
next();
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
### Phase 2: Token-Based Authentication
|
||||
- Generate secure session tokens
|
||||
- Implement token refresh mechanism
|
||||
- Add rate limiting per token
|
||||
|
||||
### Phase 3: Role-Based Access Control
|
||||
- Read-only vs read-write permissions
|
||||
- Path-based restrictions
|
||||
- Operation whitelisting
|
||||
|
||||
## Mitigation Steps (Immediate)
|
||||
1. Restrict CORS to specific origins
|
||||
2. Add API key requirement
|
||||
3. Implement rate limiting
|
||||
4. Add authentication logging
|
||||
|
||||
## References
|
||||
- [OWASP API Security Top 10](https://owasp.org/www-project-api-security/)
|
||||
- [MCP Protocol Security Considerations](https://modelcontextprotocol.io/docs/concepts/security)
|
||||
|
||||
## Acceptance Criteria
|
||||
- [ ] API key authentication implemented
|
||||
- [ ] CORS restricted to configurable origins
|
||||
- [ ] Authentication failures logged
|
||||
- [ ] Settings UI for managing API keys
|
||||
- [ ] Documentation updated with security setup
|
||||
|
||||
## Labels
|
||||
`security` `critical` `authentication` `breaking-change`
|
||||
147
github-issues/02-path-traversal-vulnerability.md
Normal file
147
github-issues/02-path-traversal-vulnerability.md
Normal file
|
|
@ -0,0 +1,147 @@
|
|||
# 🔴 CRITICAL: Path Traversal Vulnerability in File Operations
|
||||
|
||||
## Summary
|
||||
All file operations in the ObsidianAPI class accept user-provided paths without validation, potentially allowing access to files outside the vault using path traversal sequences.
|
||||
|
||||
## Current Behavior
|
||||
- No validation on file paths before passing to Obsidian API
|
||||
- Accepts `../`, absolute paths, and special characters
|
||||
- All file operations affected: read, write, delete, move, copy
|
||||
|
||||
## Security Impact
|
||||
- **Severity**: CRITICAL
|
||||
- **Attack Vector**: Malicious MCP client
|
||||
- **Impact**: Access to system files, sensitive data exposure, arbitrary file manipulation
|
||||
|
||||
## Vulnerable Code
|
||||
```typescript
|
||||
// obsidian-api.ts:191-195
|
||||
async getFile(path: string): Promise<ObsidianFileResponse> {
|
||||
const file = this.app.vault.getAbstractFileByPath(path); // No validation!
|
||||
if (!file || !(file instanceof TFile)) {
|
||||
throw new Error(`File not found: ${path}`);
|
||||
}
|
||||
```
|
||||
|
||||
```typescript
|
||||
// obsidian-api.ts:212-215
|
||||
async createFile(path: string, content: string): Promise<any> {
|
||||
await this.ensureDirectoryExists(path);
|
||||
const file = await this.app.vault.create(path, content); // No validation!
|
||||
return { path: file.path };
|
||||
}
|
||||
```
|
||||
|
||||
## Attack Scenarios
|
||||
1. **Directory Traversal**: `../../../etc/passwd`
|
||||
2. **Absolute Paths**: `/System/Library/Keychains/`
|
||||
3. **Windows Paths**: `C:\Windows\System32\config\SAM`
|
||||
4. **URL Encoded**: `..%2F..%2Fsensitive.txt`
|
||||
|
||||
## Reproduction Steps
|
||||
1. Start the MCP server
|
||||
2. Send a request with malicious path:
|
||||
```json
|
||||
{
|
||||
"method": "tools/vault",
|
||||
"params": {
|
||||
"action": "read",
|
||||
"path": "../../../.ssh/id_rsa"
|
||||
}
|
||||
}
|
||||
```
|
||||
3. Potentially access files outside vault
|
||||
|
||||
## Proposed Solution
|
||||
|
||||
### Immediate Fix: Path Validator
|
||||
```typescript
|
||||
class PathValidator {
|
||||
private vaultRoot: string;
|
||||
|
||||
constructor(app: App) {
|
||||
this.vaultRoot = (app.vault.adapter as any).basePath;
|
||||
}
|
||||
|
||||
validatePath(path: string): string {
|
||||
// Reject dangerous patterns
|
||||
if (this.containsTraversal(path)) {
|
||||
throw new Error('Path traversal detected');
|
||||
}
|
||||
|
||||
// Reject absolute paths
|
||||
if (this.isAbsolute(path)) {
|
||||
throw new Error('Absolute paths not allowed');
|
||||
}
|
||||
|
||||
// Normalize and resolve
|
||||
const normalized = this.normalize(path);
|
||||
const resolved = path.resolve(this.vaultRoot, normalized);
|
||||
|
||||
// Ensure within vault
|
||||
if (!resolved.startsWith(this.vaultRoot)) {
|
||||
throw new Error('Path must be within vault');
|
||||
}
|
||||
|
||||
return normalized;
|
||||
}
|
||||
|
||||
private containsTraversal(path: string): boolean {
|
||||
return path.includes('../') ||
|
||||
path.includes('..\\') ||
|
||||
path.includes('..%2F') ||
|
||||
path.includes('..%5C');
|
||||
}
|
||||
|
||||
private isAbsolute(path: string): boolean {
|
||||
return path.startsWith('/') ||
|
||||
path.match(/^[A-Za-z]:/) !== null ||
|
||||
path.startsWith('\\\\');
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
### Long-term Solution: Comprehensive Input Validation
|
||||
1. Implement path allowlisting
|
||||
2. Add configurable security policies
|
||||
3. Integrate with Obsidian's security model
|
||||
4. Add audit logging for suspicious paths
|
||||
|
||||
## Affected Methods
|
||||
- `getFile()`
|
||||
- `createFile()`
|
||||
- `updateFile()`
|
||||
- `deleteFile()`
|
||||
- `listFiles()`
|
||||
- `move()`
|
||||
- `rename()`
|
||||
- `copy()`
|
||||
- `split()`
|
||||
- `combine()`
|
||||
|
||||
## Mitigation Steps
|
||||
1. Add PathValidator to all file operations
|
||||
2. Sanitize paths before any filesystem access
|
||||
3. Log all path validation failures
|
||||
4. Add configuration for allowed path patterns
|
||||
|
||||
## Testing Requirements
|
||||
- Unit tests for path validation edge cases
|
||||
- Integration tests with malicious paths
|
||||
- Fuzzing tests for path parser
|
||||
- Performance tests for large path sets
|
||||
|
||||
## References
|
||||
- [CWE-22: Path Traversal](https://cwe.mitre.org/data/definitions/22.html)
|
||||
- [OWASP Path Traversal](https://owasp.org/www-community/attacks/Path_Traversal)
|
||||
|
||||
## Acceptance Criteria
|
||||
- [ ] PathValidator implemented and integrated
|
||||
- [ ] All file operations validate paths
|
||||
- [ ] Malicious paths are rejected with clear errors
|
||||
- [ ] Path validation is configurable
|
||||
- [ ] Audit logging implemented
|
||||
- [ ] Documentation updated
|
||||
|
||||
## Labels
|
||||
`security` `critical` `path-traversal` `input-validation`
|
||||
220
github-issues/03-input-validation-missing.md
Normal file
220
github-issues/03-input-validation-missing.md
Normal file
|
|
@ -0,0 +1,220 @@
|
|||
# 🟠 HIGH: Missing Input Validation Across All Operations
|
||||
|
||||
## Summary
|
||||
The plugin lacks comprehensive input validation for file content, search queries, and operation parameters, leading to potential crashes, DoS attacks, and unexpected behavior.
|
||||
|
||||
## Current Behavior
|
||||
- No validation of file content size or format
|
||||
- Search queries accept regex without sanitization
|
||||
- No limits on batch operations
|
||||
- Missing validation for special characters in filenames
|
||||
- No protection against malformed JSON/YAML in frontmatter
|
||||
|
||||
## Security Impact
|
||||
- **Severity**: HIGH
|
||||
- **Attack Vector**: Malicious input via MCP protocol
|
||||
- **Impact**: DoS, memory exhaustion, application crashes, data corruption
|
||||
|
||||
## Vulnerable Areas
|
||||
|
||||
### 1. File Content Operations
|
||||
```typescript
|
||||
// No size limits!
|
||||
async createFile(path: string, content: string): Promise<any> {
|
||||
await this.ensureDirectoryExists(path);
|
||||
const file = await this.app.vault.create(path, content);
|
||||
return { path: file.path };
|
||||
}
|
||||
```
|
||||
|
||||
### 2. Search Operations
|
||||
```typescript
|
||||
// Unvalidated regex can cause ReDoS
|
||||
async searchSimple(query: string): Promise<any[]> {
|
||||
const results = await this.search(query); // No regex validation
|
||||
return results;
|
||||
}
|
||||
```
|
||||
|
||||
### 3. Batch Operations
|
||||
```typescript
|
||||
// No limits on array size
|
||||
case 'combine': {
|
||||
const { paths, destination } = params; // paths could be 10,000 items!
|
||||
// ... processing without limits
|
||||
}
|
||||
```
|
||||
|
||||
## Attack Scenarios
|
||||
1. **Memory Exhaustion**: Create file with 1GB of content
|
||||
2. **ReDoS Attack**: Search with `(a+)+b` pattern
|
||||
3. **CPU DoS**: Combine 10,000 files in one operation
|
||||
4. **Path Injection**: Filename with `../../` embedded
|
||||
5. **Data Corruption**: Invalid UTF-8 sequences in content
|
||||
|
||||
## Proposed Solution
|
||||
|
||||
### Input Validator Framework
|
||||
```typescript
|
||||
interface ValidationRule {
|
||||
field: string;
|
||||
validators: Validator[];
|
||||
}
|
||||
|
||||
class InputValidator {
|
||||
private rules: Map<string, ValidationRule[]> = new Map();
|
||||
|
||||
constructor() {
|
||||
// Define validation rules
|
||||
this.rules.set('vault.create', [
|
||||
{
|
||||
field: 'path',
|
||||
validators: [
|
||||
new LengthValidator(1, 255),
|
||||
new PatternValidator(/^[^<>:"|?*]+$/),
|
||||
new PathSafetyValidator()
|
||||
]
|
||||
},
|
||||
{
|
||||
field: 'content',
|
||||
validators: [
|
||||
new SizeValidator(0, 10 * 1024 * 1024), // 10MB max
|
||||
new UTF8Validator(),
|
||||
new ContentTypeValidator()
|
||||
]
|
||||
}
|
||||
]);
|
||||
}
|
||||
|
||||
validate(operation: string, params: any): ValidationResult {
|
||||
const rules = this.rules.get(operation);
|
||||
if (!rules) return { valid: true };
|
||||
|
||||
const errors: ValidationError[] = [];
|
||||
|
||||
for (const rule of rules) {
|
||||
const value = params[rule.field];
|
||||
for (const validator of rule.validators) {
|
||||
const result = validator.validate(value);
|
||||
if (!result.valid) {
|
||||
errors.push({
|
||||
field: rule.field,
|
||||
message: result.message,
|
||||
code: result.code
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return {
|
||||
valid: errors.length === 0,
|
||||
errors
|
||||
};
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
### Specific Validators Needed
|
||||
|
||||
1. **File Size Limits**
|
||||
```typescript
|
||||
class FileSizeValidator {
|
||||
validate(content: string): ValidationResult {
|
||||
const sizeInBytes = Buffer.byteLength(content, 'utf8');
|
||||
if (sizeInBytes > this.maxSize) {
|
||||
return {
|
||||
valid: false,
|
||||
message: `File size ${sizeInBytes} exceeds limit ${this.maxSize}`
|
||||
};
|
||||
}
|
||||
return { valid: true };
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
2. **Safe Regex Validator**
|
||||
```typescript
|
||||
class SafeRegexValidator {
|
||||
validate(pattern: string): ValidationResult {
|
||||
try {
|
||||
// Check for dangerous patterns
|
||||
if (this.hasExponentialComplexity(pattern)) {
|
||||
return {
|
||||
valid: false,
|
||||
message: 'Regex pattern has exponential complexity'
|
||||
};
|
||||
}
|
||||
new RegExp(pattern);
|
||||
return { valid: true };
|
||||
} catch (e) {
|
||||
return {
|
||||
valid: false,
|
||||
message: 'Invalid regex pattern'
|
||||
};
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
3. **Batch Operation Limits**
|
||||
```typescript
|
||||
class BatchLimitValidator {
|
||||
validate(items: any[]): ValidationResult {
|
||||
if (items.length > this.maxBatchSize) {
|
||||
return {
|
||||
valid: false,
|
||||
message: `Batch size ${items.length} exceeds limit ${this.maxBatchSize}`
|
||||
};
|
||||
}
|
||||
return { valid: true };
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
## Implementation Plan
|
||||
|
||||
### Phase 1: Critical Validators
|
||||
- Path safety (prevent injection)
|
||||
- File size limits
|
||||
- Basic content validation
|
||||
|
||||
### Phase 2: Operation Validators
|
||||
- Search query safety
|
||||
- Batch operation limits
|
||||
- Parameter type checking
|
||||
|
||||
### Phase 3: Advanced Validation
|
||||
- Content type detection
|
||||
- Encoding validation
|
||||
- Rate limiting
|
||||
|
||||
## Configuration
|
||||
```json
|
||||
{
|
||||
"validation": {
|
||||
"maxFileSize": 10485760,
|
||||
"maxBatchSize": 100,
|
||||
"maxPathLength": 255,
|
||||
"allowedFileTypes": [".md", ".txt", ".pdf"],
|
||||
"regexTimeout": 1000,
|
||||
"strictMode": true
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
## Testing Requirements
|
||||
- Unit tests for each validator
|
||||
- Fuzzing tests with malformed input
|
||||
- Performance tests with large inputs
|
||||
- Integration tests for all operations
|
||||
|
||||
## Acceptance Criteria
|
||||
- [ ] Input validation framework implemented
|
||||
- [ ] All operations validate input
|
||||
- [ ] Clear error messages for validation failures
|
||||
- [ ] Configurable validation rules
|
||||
- [ ] Performance impact < 5ms per operation
|
||||
- [ ] Documentation for validation rules
|
||||
|
||||
## Labels
|
||||
`security` `high-priority` `input-validation` `dos-prevention`
|
||||
257
github-issues/04-insecure-session-management.md
Normal file
257
github-issues/04-insecure-session-management.md
Normal file
|
|
@ -0,0 +1,257 @@
|
|||
# 🟠 HIGH: Insecure Session Management Implementation
|
||||
|
||||
## Summary
|
||||
The session management system uses predictable client-provided session IDs without cryptographic validation, enabling session hijacking and unauthorized access.
|
||||
|
||||
## Current Behavior
|
||||
- Sessions use client-provided UUIDs without validation
|
||||
- No cryptographic session tokens
|
||||
- No session integrity verification
|
||||
- Sessions can be hijacked by guessing/brute-forcing UUIDs
|
||||
- No rate limiting on session creation
|
||||
|
||||
## Security Impact
|
||||
- **Severity**: HIGH
|
||||
- **Attack Vector**: Session ID manipulation
|
||||
- **Impact**: Session hijacking, unauthorized access, impersonation
|
||||
|
||||
## Vulnerable Code
|
||||
```typescript
|
||||
// mcp-server.ts:328-329
|
||||
// Client can provide any session ID!
|
||||
const sessionId = req.headers['mcp-session-id'] as string | undefined;
|
||||
|
||||
// mcp-server.ts:366-368
|
||||
// New session with predictable UUID
|
||||
effectiveSessionId = randomUUID();
|
||||
transport = new StreamableHTTPServerTransport({
|
||||
sessionIdGenerator: () => effectiveSessionId!
|
||||
});
|
||||
```
|
||||
|
||||
```typescript
|
||||
// session-manager.ts:64-111
|
||||
getOrCreateSession(sessionId: string): SessionInfo {
|
||||
// No validation of sessionId format or authenticity!
|
||||
let session = this.sessions.get(sessionId);
|
||||
|
||||
if (session) {
|
||||
session.lastActivityAt = now;
|
||||
session.requestCount++;
|
||||
return session;
|
||||
}
|
||||
|
||||
// Creates session with any provided ID
|
||||
session = {
|
||||
sessionId, // Trusts client-provided ID!
|
||||
createdAt: now,
|
||||
lastActivityAt: now,
|
||||
requestCount: 1
|
||||
};
|
||||
}
|
||||
```
|
||||
|
||||
## Attack Scenarios
|
||||
1. **Session Hijacking**: Guess active session UUIDs
|
||||
2. **Session Fixation**: Force server to use attacker-chosen session ID
|
||||
3. **Brute Force**: Try common UUID patterns
|
||||
4. **Session Replay**: Reuse discovered session IDs
|
||||
5. **DoS via Session Flooding**: Create unlimited sessions
|
||||
|
||||
## Proposed Solution
|
||||
|
||||
### Secure Session Management
|
||||
```typescript
|
||||
import { randomBytes, createHmac } from 'crypto';
|
||||
|
||||
interface SecureSession {
|
||||
id: string;
|
||||
token: string;
|
||||
createdAt: number;
|
||||
lastActivity: number;
|
||||
fingerprint: string;
|
||||
metadata: {
|
||||
clientInfo: string;
|
||||
permissions: string[];
|
||||
};
|
||||
}
|
||||
|
||||
class SecureSessionManager {
|
||||
private sessions = new Map<string, SecureSession>();
|
||||
private sessionTokens = new Map<string, string>(); // token -> sessionId
|
||||
private secretKey: Buffer;
|
||||
|
||||
constructor() {
|
||||
// Generate or load persistent secret key
|
||||
this.secretKey = this.loadOrGenerateSecret();
|
||||
}
|
||||
|
||||
createSession(clientInfo: string): { id: string; token: string } {
|
||||
// Generate cryptographically secure session ID
|
||||
const sessionId = randomBytes(32).toString('hex');
|
||||
|
||||
// Generate signed session token
|
||||
const tokenPayload = {
|
||||
sid: sessionId,
|
||||
iat: Date.now(),
|
||||
exp: Date.now() + 3600000, // 1 hour
|
||||
nonce: randomBytes(16).toString('hex')
|
||||
};
|
||||
|
||||
const token = this.signToken(tokenPayload);
|
||||
|
||||
// Create session with fingerprint
|
||||
const session: SecureSession = {
|
||||
id: sessionId,
|
||||
token: token,
|
||||
createdAt: Date.now(),
|
||||
lastActivity: Date.now(),
|
||||
fingerprint: this.generateFingerprint(clientInfo),
|
||||
metadata: {
|
||||
clientInfo,
|
||||
permissions: ['read', 'write'] // Based on auth
|
||||
}
|
||||
};
|
||||
|
||||
this.sessions.set(sessionId, session);
|
||||
this.sessionTokens.set(token, sessionId);
|
||||
|
||||
return { id: sessionId, token };
|
||||
}
|
||||
|
||||
validateSession(token: string, clientInfo: string): SecureSession | null {
|
||||
// Verify token signature
|
||||
if (!this.verifyToken(token)) {
|
||||
return null;
|
||||
}
|
||||
|
||||
// Get session
|
||||
const sessionId = this.sessionTokens.get(token);
|
||||
if (!sessionId) return null;
|
||||
|
||||
const session = this.sessions.get(sessionId);
|
||||
if (!session) return null;
|
||||
|
||||
// Verify fingerprint (prevents token theft)
|
||||
if (session.fingerprint !== this.generateFingerprint(clientInfo)) {
|
||||
this.logSecurityEvent('fingerprint_mismatch', { sessionId, clientInfo });
|
||||
return null;
|
||||
}
|
||||
|
||||
// Check expiration
|
||||
if (Date.now() - session.lastActivity > 3600000) {
|
||||
this.removeSession(sessionId);
|
||||
return null;
|
||||
}
|
||||
|
||||
// Update activity
|
||||
session.lastActivity = Date.now();
|
||||
return session;
|
||||
}
|
||||
|
||||
private signToken(payload: any): string {
|
||||
const data = JSON.stringify(payload);
|
||||
const hmac = createHmac('sha256', this.secretKey);
|
||||
hmac.update(data);
|
||||
const signature = hmac.digest('hex');
|
||||
return Buffer.from(data).toString('base64') + '.' + signature;
|
||||
}
|
||||
|
||||
private verifyToken(token: string): boolean {
|
||||
try {
|
||||
const [data, signature] = token.split('.');
|
||||
const payload = JSON.parse(Buffer.from(data, 'base64').toString());
|
||||
|
||||
// Check expiration
|
||||
if (payload.exp < Date.now()) return false;
|
||||
|
||||
// Verify signature
|
||||
const hmac = createHmac('sha256', this.secretKey);
|
||||
hmac.update(Buffer.from(data, 'base64').toString());
|
||||
const expectedSignature = hmac.digest('hex');
|
||||
|
||||
return signature === expectedSignature;
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
### Integration with Rate Limiting
|
||||
```typescript
|
||||
class SessionRateLimiter {
|
||||
private attempts = new Map<string, number[]>();
|
||||
private readonly maxAttempts = 10;
|
||||
private readonly windowMs = 60000; // 1 minute
|
||||
|
||||
checkLimit(identifier: string): boolean {
|
||||
const now = Date.now();
|
||||
const attempts = this.attempts.get(identifier) || [];
|
||||
|
||||
// Remove old attempts
|
||||
const recentAttempts = attempts.filter(t => now - t < this.windowMs);
|
||||
|
||||
if (recentAttempts.length >= this.maxAttempts) {
|
||||
return false; // Rate limit exceeded
|
||||
}
|
||||
|
||||
recentAttempts.push(now);
|
||||
this.attempts.set(identifier, recentAttempts);
|
||||
return true;
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
## Implementation Steps
|
||||
|
||||
1. **Phase 1: Token-Based Sessions**
|
||||
- Replace UUID sessions with signed tokens
|
||||
- Add token validation middleware
|
||||
- Implement session expiration
|
||||
|
||||
2. **Phase 2: Security Hardening**
|
||||
- Add client fingerprinting
|
||||
- Implement rate limiting
|
||||
- Add security event logging
|
||||
|
||||
3. **Phase 3: Advanced Features**
|
||||
- Session refresh tokens
|
||||
- Concurrent session limits
|
||||
- Session revocation API
|
||||
|
||||
## Configuration
|
||||
```json
|
||||
{
|
||||
"session": {
|
||||
"timeout": 3600000,
|
||||
"maxConcurrent": 5,
|
||||
"requireSecureTransport": true,
|
||||
"tokenAlgorithm": "HS256",
|
||||
"refreshEnabled": true,
|
||||
"refreshWindow": 300000
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
## Breaking Changes
|
||||
- Session header changes from `Mcp-Session-Id` to `Authorization: Bearer <token>`
|
||||
- Session creation requires authentication
|
||||
- Existing sessions invalidated on upgrade
|
||||
|
||||
## Testing Requirements
|
||||
- Security tests for token validation
|
||||
- Load tests for session creation
|
||||
- Penetration tests for session hijacking
|
||||
- Unit tests for all security functions
|
||||
|
||||
## Acceptance Criteria
|
||||
- [ ] Cryptographic session tokens implemented
|
||||
- [ ] Client fingerprinting added
|
||||
- [ ] Rate limiting on session operations
|
||||
- [ ] Security event logging
|
||||
- [ ] Session expiration and cleanup
|
||||
- [ ] Documentation for new session model
|
||||
|
||||
## Labels
|
||||
`security` `high-priority` `session-management` `breaking-change`
|
||||
270
github-issues/05-solid-principles-violations.md
Normal file
270
github-issues/05-solid-principles-violations.md
Normal file
|
|
@ -0,0 +1,270 @@
|
|||
# 🟡 MEDIUM: Code Quality - SOLID Principles Violations
|
||||
|
||||
## Summary
|
||||
The codebase violates multiple SOLID principles, leading to tight coupling, difficult maintenance, and challenges in extending functionality. Major classes exceed 500 lines with multiple responsibilities.
|
||||
|
||||
## Current Issues
|
||||
|
||||
### 1. Single Responsibility Principle (SRP) Violations
|
||||
|
||||
#### SemanticRouter Class (1600+ lines)
|
||||
- Handles routing, state management, context tracking, error handling
|
||||
- Contains business logic for 6+ different operations
|
||||
- Manages fragment retrieval and indexing
|
||||
|
||||
#### MCPHttpServer Class (600+ lines)
|
||||
- Manages HTTP server lifecycle
|
||||
- Handles MCP protocol
|
||||
- Manages sessions and connection pooling
|
||||
- Contains business logic
|
||||
|
||||
#### ObsidianAPI Class (800+ lines)
|
||||
- 40+ public methods in single class
|
||||
- Mixes read/write operations
|
||||
- Contains image processing logic
|
||||
- Handles file system operations
|
||||
|
||||
### 2. Open/Closed Principle (OCP) Violations
|
||||
|
||||
#### Hard-coded Operation Mapping
|
||||
```typescript
|
||||
// router.ts:99-117
|
||||
switch (operation) {
|
||||
case 'vault':
|
||||
return this.executeVaultOperation(action, params);
|
||||
case 'edit':
|
||||
return this.executeEditOperation(action, params);
|
||||
// Adding new operations requires modifying this switch
|
||||
}
|
||||
```
|
||||
|
||||
#### No Plugin Architecture
|
||||
- Cannot extend operations without modifying core code
|
||||
- No way to register custom handlers
|
||||
- Tightly coupled operation implementations
|
||||
|
||||
### 3. Liskov Substitution Principle (LSP) Issues
|
||||
- Inconsistent return types across similar operations
|
||||
- Some methods throw, others return error objects
|
||||
- Subclasses would break existing behavior
|
||||
|
||||
### 4. Interface Segregation Principle (ISP) Violations
|
||||
```typescript
|
||||
// All clients must depend on entire interface
|
||||
interface ObsidianAPI {
|
||||
getFile(): Promise<any>;
|
||||
createFile(): Promise<any>;
|
||||
updateFile(): Promise<any>;
|
||||
deleteFile(): Promise<any>;
|
||||
// ... 40+ more methods
|
||||
}
|
||||
```
|
||||
|
||||
### 5. Dependency Inversion Principle (DIP) Violations
|
||||
- Direct dependencies on concrete classes
|
||||
- No dependency injection
|
||||
- Tight coupling to Obsidian's internal APIs
|
||||
|
||||
## Proposed Refactoring
|
||||
|
||||
### 1. Break Down Large Classes
|
||||
|
||||
#### Operation Handlers (SRP)
|
||||
```typescript
|
||||
// Separate handler for each operation type
|
||||
interface OperationHandler {
|
||||
canHandle(operation: string): boolean;
|
||||
execute(action: string, params: any): Promise<any>;
|
||||
}
|
||||
|
||||
class VaultOperationHandler implements OperationHandler {
|
||||
constructor(private fileService: FileService) {}
|
||||
|
||||
canHandle(operation: string): boolean {
|
||||
return operation === 'vault';
|
||||
}
|
||||
|
||||
async execute(action: string, params: any): Promise<any> {
|
||||
switch (action) {
|
||||
case 'read': return this.fileService.read(params.path);
|
||||
case 'write': return this.fileService.write(params.path, params.content);
|
||||
default: throw new Error(`Unknown action: ${action}`);
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
#### Service Layer Pattern
|
||||
```typescript
|
||||
// Separate services for different concerns
|
||||
interface FileService {
|
||||
read(path: string): Promise<FileContent>;
|
||||
write(path: string, content: string): Promise<void>;
|
||||
}
|
||||
|
||||
interface SearchService {
|
||||
search(query: string): Promise<SearchResult[]>;
|
||||
searchByTag(tag: string): Promise<SearchResult[]>;
|
||||
}
|
||||
|
||||
interface ValidationService {
|
||||
validatePath(path: string): ValidationResult;
|
||||
validateContent(content: string): ValidationResult;
|
||||
}
|
||||
```
|
||||
|
||||
### 2. Plugin Architecture (OCP)
|
||||
|
||||
```typescript
|
||||
class OperationRegistry {
|
||||
private handlers = new Map<string, OperationHandler>();
|
||||
|
||||
register(operation: string, handler: OperationHandler): void {
|
||||
this.handlers.set(operation, handler);
|
||||
}
|
||||
|
||||
async execute(operation: string, action: string, params: any): Promise<any> {
|
||||
const handler = this.handlers.get(operation);
|
||||
if (!handler) {
|
||||
throw new Error(`No handler for operation: ${operation}`);
|
||||
}
|
||||
return handler.execute(action, params);
|
||||
}
|
||||
}
|
||||
|
||||
// Easy to add new operations
|
||||
registry.register('custom', new CustomOperationHandler());
|
||||
```
|
||||
|
||||
### 3. Interface Segregation
|
||||
|
||||
```typescript
|
||||
// Segregated interfaces
|
||||
interface FileReader {
|
||||
read(path: string): Promise<FileContent>;
|
||||
exists(path: string): Promise<boolean>;
|
||||
}
|
||||
|
||||
interface FileWriter {
|
||||
write(path: string, content: string): Promise<void>;
|
||||
append(path: string, content: string): Promise<void>;
|
||||
}
|
||||
|
||||
interface FileManager {
|
||||
move(from: string, to: string): Promise<void>;
|
||||
copy(from: string, to: string): Promise<void>;
|
||||
delete(path: string): Promise<void>;
|
||||
}
|
||||
|
||||
// Clients can depend only on what they need
|
||||
class ReadOnlyClient {
|
||||
constructor(private reader: FileReader) {}
|
||||
}
|
||||
```
|
||||
|
||||
### 4. Dependency Injection (DIP)
|
||||
|
||||
```typescript
|
||||
// Dependency injection container
|
||||
class DIContainer {
|
||||
private services = new Map<string, any>();
|
||||
|
||||
register<T>(token: string, factory: () => T): void {
|
||||
this.services.set(token, factory);
|
||||
}
|
||||
|
||||
resolve<T>(token: string): T {
|
||||
const factory = this.services.get(token);
|
||||
if (!factory) {
|
||||
throw new Error(`Service not registered: ${token}`);
|
||||
}
|
||||
return factory();
|
||||
}
|
||||
}
|
||||
|
||||
// Usage
|
||||
container.register('FileService', () => new ObsidianFileService(app));
|
||||
container.register('ValidationService', () => new PathValidationService());
|
||||
|
||||
class VaultOperationHandler {
|
||||
private fileService: FileService;
|
||||
private validator: ValidationService;
|
||||
|
||||
constructor(container: DIContainer) {
|
||||
this.fileService = container.resolve('FileService');
|
||||
this.validator = container.resolve('ValidationService');
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
### 5. Event-Driven Architecture
|
||||
|
||||
```typescript
|
||||
// Decouple components with events
|
||||
class EventBus {
|
||||
private handlers = new Map<string, Function[]>();
|
||||
|
||||
on(event: string, handler: Function): void {
|
||||
if (!this.handlers.has(event)) {
|
||||
this.handlers.set(event, []);
|
||||
}
|
||||
this.handlers.get(event)!.push(handler);
|
||||
}
|
||||
|
||||
emit(event: string, data: any): void {
|
||||
const handlers = this.handlers.get(event) || [];
|
||||
handlers.forEach(handler => handler(data));
|
||||
}
|
||||
}
|
||||
|
||||
// Usage
|
||||
eventBus.on('file.created', async (data) => {
|
||||
await indexingService.indexFile(data.path);
|
||||
});
|
||||
```
|
||||
|
||||
## Implementation Plan
|
||||
|
||||
### Phase 1: Extract Services (2 weeks)
|
||||
- [ ] Create FileService, SearchService, ValidationService
|
||||
- [ ] Extract methods from ObsidianAPI
|
||||
- [ ] Add unit tests for each service
|
||||
|
||||
### Phase 2: Implement Handlers (1 week)
|
||||
- [ ] Create OperationHandler interface
|
||||
- [ ] Implement handlers for each operation type
|
||||
- [ ] Create OperationRegistry
|
||||
|
||||
### Phase 3: Dependency Injection (1 week)
|
||||
- [ ] Implement DIContainer
|
||||
- [ ] Register all services
|
||||
- [ ] Refactor constructors to use DI
|
||||
|
||||
### Phase 4: Plugin Architecture (2 weeks)
|
||||
- [ ] Design plugin API
|
||||
- [ ] Create plugin loader
|
||||
- [ ] Document plugin development
|
||||
|
||||
## Benefits
|
||||
- Easier to test individual components
|
||||
- New features don't require core changes
|
||||
- Clear separation of concerns
|
||||
- Better code reusability
|
||||
- Improved maintainability
|
||||
|
||||
## Metrics
|
||||
- Reduce average class size from 800+ to <200 lines
|
||||
- Achieve 90%+ unit test coverage
|
||||
- Reduce coupling metrics by 50%
|
||||
- Enable adding new operations without core changes
|
||||
|
||||
## Acceptance Criteria
|
||||
- [ ] No class exceeds 300 lines
|
||||
- [ ] All operations use handler pattern
|
||||
- [ ] Dependency injection implemented
|
||||
- [ ] Plugin system documented
|
||||
- [ ] 90% unit test coverage
|
||||
- [ ] Performance unchanged or improved
|
||||
|
||||
## Labels
|
||||
`code-quality` `refactoring` `architecture` `technical-debt`
|
||||
266
github-issues/06-large-vault-scalability.md
Normal file
266
github-issues/06-large-vault-scalability.md
Normal file
|
|
@ -0,0 +1,266 @@
|
|||
# 🟡 MEDIUM: Large Vault Scalability - Path Validation Performance
|
||||
|
||||
## Summary
|
||||
Path validation for vaults with 10,000+ files presents significant performance challenges. Current lack of validation is a security risk, but naive implementation would cause memory exhaustion and slow operations.
|
||||
|
||||
## Current Situation
|
||||
- **No path validation** currently implemented (security vulnerability)
|
||||
- Vaults can contain 100,000+ files
|
||||
- Each file operation would need validation
|
||||
- Memory and CPU constraints in Obsidian environment
|
||||
|
||||
## Performance Challenges
|
||||
|
||||
### Memory Impact
|
||||
```
|
||||
10,000 files × 100 chars/path × 2 bytes/char = 2MB minimum
|
||||
100,000 files × 100 chars/path × 2 bytes/char = 20MB minimum
|
||||
+ JavaScript object overhead (3-5x) = 60-100MB for path index
|
||||
```
|
||||
|
||||
### Lookup Performance
|
||||
- Naive array search: O(n) - up to 100ms for 100k files
|
||||
- HashSet lookup: O(1) - but high memory cost
|
||||
- Tree structure: O(log n) - balanced performance
|
||||
|
||||
## Proposed Solutions
|
||||
|
||||
### 1. Lazy Validation with LRU Cache (Recommended)
|
||||
```typescript
|
||||
class ScalablePathValidator {
|
||||
private securityRules = [
|
||||
/\.\./, // Path traversal
|
||||
/^[A-Z]:\\/, // Windows absolute
|
||||
/^\//, // Unix absolute
|
||||
/\x00/, // Null bytes
|
||||
];
|
||||
|
||||
private cache = new LRUCache<string, boolean>({
|
||||
max: 5000, // Cache recent validations
|
||||
ttl: 300000, // 5 minute TTL
|
||||
updateAgeOnGet: true
|
||||
});
|
||||
|
||||
async validatePath(path: string): Promise<ValidationResult> {
|
||||
// Step 1: Security checks (microseconds, no I/O)
|
||||
for (const rule of this.securityRules) {
|
||||
if (rule.test(path)) {
|
||||
return {
|
||||
valid: false,
|
||||
reason: 'Security violation',
|
||||
cached: false
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
// Step 2: Check cache (nanoseconds)
|
||||
const cached = this.cache.get(path);
|
||||
if (cached !== undefined) {
|
||||
return { valid: cached, cached: true };
|
||||
}
|
||||
|
||||
// Step 3: Validate with Obsidian API (milliseconds)
|
||||
const exists = !!this.app.vault.getAbstractFileByPath(path);
|
||||
this.cache.set(path, exists);
|
||||
|
||||
return { valid: exists, cached: false };
|
||||
}
|
||||
|
||||
// Batch validation for performance
|
||||
async validatePaths(paths: string[]): Promise<Map<string, boolean>> {
|
||||
const results = new Map<string, boolean>();
|
||||
const uncached: string[] = [];
|
||||
|
||||
// Check cache first
|
||||
for (const path of paths) {
|
||||
const cached = this.cache.get(path);
|
||||
if (cached !== undefined) {
|
||||
results.set(path, cached);
|
||||
} else {
|
||||
uncached.push(path);
|
||||
}
|
||||
}
|
||||
|
||||
// Batch check uncached paths
|
||||
if (uncached.length > 0) {
|
||||
const files = this.app.vault.getFiles();
|
||||
const fileSet = new Set(files.map(f => f.path));
|
||||
|
||||
for (const path of uncached) {
|
||||
const valid = fileSet.has(path);
|
||||
results.set(path, valid);
|
||||
this.cache.set(path, valid);
|
||||
}
|
||||
}
|
||||
|
||||
return results;
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
### 2. Hierarchical Prefix Validation
|
||||
```typescript
|
||||
class HierarchicalValidator {
|
||||
private prefixTree = new Map<string, PrefixNode>();
|
||||
private maxDepth = 3; // Only validate top 3 levels
|
||||
|
||||
async initialize() {
|
||||
// Build prefix tree for top directories only
|
||||
const folders = this.app.vault.getAllLoadedFiles()
|
||||
.filter(f => f instanceof TFolder)
|
||||
.map(f => f.path.split('/').slice(0, this.maxDepth));
|
||||
|
||||
// Build tree (much smaller than full path list)
|
||||
for (const parts of folders) {
|
||||
this.addToTree(parts);
|
||||
}
|
||||
}
|
||||
|
||||
validatePath(path: string): boolean {
|
||||
// Quick security checks
|
||||
if (this.hasSecurityIssue(path)) return false;
|
||||
|
||||
// Check if path starts with valid prefix
|
||||
const parts = path.split('/').slice(0, this.maxDepth);
|
||||
return this.checkPrefix(parts);
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
### 3. Bloom Filter for Existence Checking
|
||||
```typescript
|
||||
class BloomFilterValidator {
|
||||
private bloomFilter: BloomFilter;
|
||||
private falsePositiveRate = 0.001; // 0.1% false positive
|
||||
|
||||
async initialize() {
|
||||
const fileCount = this.app.vault.getFiles().length;
|
||||
const bitSize = Math.ceil(-fileCount * Math.log(this.falsePositiveRate) / Math.pow(Math.log(2), 2));
|
||||
const hashCount = Math.ceil(bitSize / fileCount * Math.log(2));
|
||||
|
||||
this.bloomFilter = new BloomFilter(bitSize, hashCount);
|
||||
|
||||
// Add all file paths
|
||||
for (const file of this.app.vault.getFiles()) {
|
||||
this.bloomFilter.add(file.path);
|
||||
}
|
||||
}
|
||||
|
||||
validatePath(path: string): { definitelyInvalid: boolean; maybeValid: boolean } {
|
||||
if (!this.bloomFilter.test(path)) {
|
||||
return { definitelyInvalid: true, maybeValid: false };
|
||||
}
|
||||
|
||||
// Need additional check for false positives
|
||||
return { definitelyInvalid: false, maybeValid: true };
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
### 4. Configuration-Based Validation
|
||||
```typescript
|
||||
interface ValidationConfig {
|
||||
mode: 'strict' | 'relaxed' | 'custom';
|
||||
rules: ValidationRule[];
|
||||
cache: {
|
||||
enabled: boolean;
|
||||
maxSize: number;
|
||||
ttl: number;
|
||||
};
|
||||
performance: {
|
||||
maxSyncPaths: number; // Switch to async above this
|
||||
batchSize: number; // For batch operations
|
||||
indexingStrategy: 'lazy' | 'eager' | 'none';
|
||||
};
|
||||
}
|
||||
|
||||
class ConfigurableValidator {
|
||||
constructor(private config: ValidationConfig) {}
|
||||
|
||||
async validatePath(path: string): Promise<boolean> {
|
||||
switch (this.config.mode) {
|
||||
case 'strict':
|
||||
return this.strictValidation(path);
|
||||
case 'relaxed':
|
||||
return this.relaxedValidation(path);
|
||||
case 'custom':
|
||||
return this.customValidation(path);
|
||||
}
|
||||
}
|
||||
|
||||
private async strictValidation(path: string): Promise<boolean> {
|
||||
// Full validation with all security checks
|
||||
return this.securityCheck(path) && await this.existenceCheck(path);
|
||||
}
|
||||
|
||||
private async relaxedValidation(path: string): Promise<boolean> {
|
||||
// Only security checks, trust Obsidian for existence
|
||||
return this.securityCheck(path);
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
## Implementation Recommendations
|
||||
|
||||
### For Most Users (< 10,000 files)
|
||||
```typescript
|
||||
const validator = new ScalablePathValidator({
|
||||
cacheSize: 1000,
|
||||
securityOnly: false
|
||||
});
|
||||
```
|
||||
|
||||
### For Large Vaults (10,000 - 100,000 files)
|
||||
```typescript
|
||||
const validator = new ScalablePathValidator({
|
||||
cacheSize: 5000,
|
||||
securityOnly: true, // Skip existence checks
|
||||
prefixValidation: true
|
||||
});
|
||||
```
|
||||
|
||||
### For Huge Vaults (> 100,000 files)
|
||||
```typescript
|
||||
const validator = new LazyValidator({
|
||||
mode: 'security-only',
|
||||
asyncThreshold: 100,
|
||||
enableBatching: true
|
||||
});
|
||||
```
|
||||
|
||||
## Performance Benchmarks
|
||||
|
||||
| Vault Size | Strategy | Memory Usage | Validation Time |
|
||||
|------------|----------|--------------|-----------------|
|
||||
| 1,000 | Full Index | 2MB | <1ms |
|
||||
| 10,000 | LRU Cache | 5MB | 1-5ms |
|
||||
| 100,000 | Security Only | 1MB | <1ms |
|
||||
| 1,000,000 | Lazy + Prefix | 10MB | 5-10ms |
|
||||
|
||||
## Configuration UI
|
||||
```typescript
|
||||
// Add to plugin settings
|
||||
interface ScalabilitySettings {
|
||||
validationMode: 'off' | 'security' | 'full';
|
||||
cacheSize: number;
|
||||
performanceMode: 'memory' | 'balanced' | 'speed';
|
||||
customRules: string[];
|
||||
}
|
||||
```
|
||||
|
||||
## Testing Requirements
|
||||
- Benchmark with vaults of various sizes (1K, 10K, 100K files)
|
||||
- Memory profiling under different strategies
|
||||
- Stress testing with concurrent operations
|
||||
- Edge cases (Unicode paths, very long paths)
|
||||
|
||||
## Acceptance Criteria
|
||||
- [ ] Path validation adds <10ms to operations
|
||||
- [ ] Memory usage scales logarithmically with vault size
|
||||
- [ ] Configurable validation strategies
|
||||
- [ ] Security checks always enabled
|
||||
- [ ] Performance metrics in settings UI
|
||||
- [ ] Documentation for large vault users
|
||||
|
||||
## Labels
|
||||
`performance` `scalability` `security` `configuration`
|
||||
98
github-issues/README.md
Normal file
98
github-issues/README.md
Normal file
|
|
@ -0,0 +1,98 @@
|
|||
# Obsidian MCP Plugin - Security and Code Quality Issues
|
||||
|
||||
This directory contains detailed GitHub issues documenting security vulnerabilities and code quality problems found during the security audit of the Obsidian MCP Plugin.
|
||||
|
||||
## 🔴 Critical Security Issues
|
||||
|
||||
### [01. No Authentication or Authorization](01-authentication-vulnerability.md)
|
||||
- **Severity**: CRITICAL
|
||||
- **Impact**: Any local application can access and manipulate the entire vault
|
||||
- **Fix**: Implement API key authentication and CORS restrictions
|
||||
|
||||
### [02. Path Traversal Vulnerability](02-path-traversal-vulnerability.md)
|
||||
- **Severity**: CRITICAL
|
||||
- **Impact**: Access to files outside vault, system file exposure
|
||||
- **Fix**: Implement comprehensive path validation and sanitization
|
||||
|
||||
## 🟠 High Priority Issues
|
||||
|
||||
### [03. Missing Input Validation](03-input-validation-missing.md)
|
||||
- **Severity**: HIGH
|
||||
- **Impact**: DoS attacks, memory exhaustion, application crashes
|
||||
- **Fix**: Implement input validation framework with size/format limits
|
||||
|
||||
### [04. Insecure Session Management](04-insecure-session-management.md)
|
||||
- **Severity**: HIGH
|
||||
- **Impact**: Session hijacking, unauthorized access
|
||||
- **Fix**: Replace UUID sessions with cryptographic tokens
|
||||
|
||||
## 🟡 Medium Priority Issues
|
||||
|
||||
### [05. SOLID Principles Violations](05-solid-principles-violations.md)
|
||||
- **Severity**: MEDIUM
|
||||
- **Impact**: Difficult maintenance, tight coupling, hard to extend
|
||||
- **Fix**: Refactor into smaller, focused classes with dependency injection
|
||||
|
||||
### [06. Large Vault Scalability](06-large-vault-scalability.md)
|
||||
- **Severity**: MEDIUM
|
||||
- **Impact**: Performance degradation with 10,000+ files
|
||||
- **Fix**: Implement scalable path validation strategies
|
||||
|
||||
## Quick Summary
|
||||
|
||||
The Obsidian MCP Plugin provides powerful functionality but has significant security vulnerabilities:
|
||||
|
||||
1. **No authentication** - Any local app can access your vault
|
||||
2. **Path traversal** - Potential access to system files
|
||||
3. **No input validation** - Risk of DoS and crashes
|
||||
4. **Weak sessions** - Can be hijacked easily
|
||||
5. **Code quality** - Difficult to maintain and extend
|
||||
6. **Scalability** - Performance issues with large vaults
|
||||
|
||||
## Recommended Action Plan
|
||||
|
||||
### Immediate (Week 1)
|
||||
1. Add basic API key authentication
|
||||
2. Implement path traversal protection
|
||||
3. Add input size limits
|
||||
|
||||
### Short-term (Weeks 2-3)
|
||||
1. Replace session management
|
||||
2. Add comprehensive input validation
|
||||
3. Implement rate limiting
|
||||
|
||||
### Long-term (Month 2)
|
||||
1. Refactor for SOLID principles
|
||||
2. Implement scalable validation
|
||||
3. Add security audit logging
|
||||
|
||||
## Creating GitHub Issues
|
||||
|
||||
To create these issues on GitHub:
|
||||
|
||||
```bash
|
||||
# Using GitHub CLI
|
||||
gh issue create --title "🔴 CRITICAL: No Authentication or Authorization on MCP Server" --body-file 01-authentication-vulnerability.md --label "security,critical,authentication,breaking-change"
|
||||
|
||||
gh issue create --title "🔴 CRITICAL: Path Traversal Vulnerability in File Operations" --body-file 02-path-traversal-vulnerability.md --label "security,critical,path-traversal,input-validation"
|
||||
|
||||
gh issue create --title "🟠 HIGH: Missing Input Validation Across All Operations" --body-file 03-input-validation-missing.md --label "security,high-priority,input-validation,dos-prevention"
|
||||
|
||||
gh issue create --title "🟠 HIGH: Insecure Session Management Implementation" --body-file 04-insecure-session-management.md --label "security,high-priority,session-management,breaking-change"
|
||||
|
||||
gh issue create --title "🟡 MEDIUM: Code Quality - SOLID Principles Violations" --body-file 05-solid-principles-violations.md --label "code-quality,refactoring,architecture,technical-debt"
|
||||
|
||||
gh issue create --title "🟡 MEDIUM: Large Vault Scalability - Path Validation Performance" --body-file 06-large-vault-scalability.md --label "performance,scalability,security,configuration"
|
||||
```
|
||||
|
||||
## Security Disclosure
|
||||
|
||||
If you're using this plugin in production:
|
||||
1. **Restrict network access** to localhost only
|
||||
2. **Monitor** for unauthorized access attempts
|
||||
3. **Consider alternatives** until security is improved
|
||||
4. **Report issues** responsibly to the maintainers
|
||||
|
||||
---
|
||||
|
||||
*Generated during security audit on 2025-07-12*
|
||||
Loading…
Reference in a new issue