Conversation
- resources/list: ui://proofscan/trace-viewer (text/html;profile=mcp-app) - resources/read: trace-viewer 単一HTML (JS/CSS同梱) - tools/list: proofscan_getEvents (outputSchema + _meta.ui.resourceUri) - ui/initialize: sessionToken 発行 - UI: postMessage token検証 See: SEP-1865, /home/shin/vault/03_Projects/proofscan/3032 - proofscan Phase 6 - MCP Apps 設計書.md
Code Review - PR #101: Phase 6.1 MCP Apps 基盤実装SummaryThis PR implements the foundation for MCP Apps Extension (SEP-1865) with a minimal viable path to display a UI. The implementation adds three new MCP protocol handlers ( ✅ Positive Aspects1. Architecture & Design
2. Security Implementation
3. Protocol Compliance
4. Code Quality
🔴 Issues Found1. CRITICAL: Missing postMessage Origin Validation (Security)Location: window.parent.postMessage(request, '*');Issue: Using wildcard Risk: If the HTML is ever served in an unexpected context (browser extension, third-party iframe, etc.), sensitive data could leak to malicious origins. Recommendation: // Store the expected origin after first verified message
let trustedOrigin = null;
function sendRequest(method, params) {
const targetOrigin = trustedOrigin || '*'; // Use * only for initialization
window.parent.postMessage(request, targetOrigin);
}
function handleMessage(event) {
// Lock to first received origin
if (!trustedOrigin && event.origin) {
trustedOrigin = event.origin;
}
// ... rest of validation
}2. CRITICAL: Race Condition in RPC ID HandlingLocation: if (msg.id === 1 && msg.method === undefined) {
// Response to ui/initializeIssue: Hardcoded check for
Recommendation: // Track pending requests
const pendingRequests = new Map();
function sendRequest(method, params) {
const id = currentRpcId++;
pendingRequests.set(id, { method });
// ... send message
return id;
}
function handleMessage(event) {
if ('id' in msg && !msg.method) {
const pending = pendingRequests.get(msg.id);
if (pending && pending.method === 'ui/initialize') {
// Handle ui/initialize response
pendingRequests.delete(msg.id);
}
}
}3. HIGH: Missing Input Validation in resources/readLocation: if (!params || typeof params.uri !== 'string') {
this.sendError(id, MCP_ERROR.INVALID_PARAMS, 'Missing required parameter: uri');
return;
}Issue: No validation for:
Recommendation: const MAX_URI_LENGTH = 2048;
if (!params || typeof params.uri !== 'string') {
this.sendError(id, MCP_ERROR.INVALID_PARAMS, 'Missing required parameter: uri');
return;
}
const { uri } = params;
// Validate length
if (uri.length > MAX_URI_LENGTH) {
this.sendError(id, MCP_ERROR.INVALID_PARAMS, 'URI too long');
return;
}
// Validate format (whitelist approach)
if (!uri.startsWith('ui://proofscan/')) {
this.sendError(id, MCP_ERROR.INVALID_PARAMS, `Invalid URI scheme or host: ${uri}`);
return;
}4. MEDIUM: Incomplete Error Handling in getTraceViewerHtml()Location: function getTraceViewerHtml(): string {
const __filename = fileURLToPath(import.meta.url);
const __dirname = dirname(__filename);
const htmlPath = join(__dirname, '../html/trace-viewer.html');
return readFileSync(htmlPath, 'utf-8');
}Issue: No error handling if file doesn't exist or can't be read. This will crash the entire server. Recommendation: function getTraceViewerHtml(): string {
try {
const __filename = fileURLToPath(import.meta.url);
const __dirname = dirname(__filename);
const htmlPath = join(__dirname, '../html/trace-viewer.html');
return readFileSync(htmlPath, 'utf-8');
} catch (error) {
logger.error('Failed to load trace-viewer.html:', error);
throw new Error('UI resource unavailable');
}
}And in the handler: try {
const html = getTraceViewerHtml();
// ... send result
} catch (error) {
this.sendError(id, MCP_ERROR.INTERNAL_ERROR, 'Failed to load UI resource');
return;
}5. MEDIUM: Missing Test CoverageIssue: No automated tests for the new functionality:
Recommendation: Add test file describe('MCP Resources', () => {
describe('resources/list', () => {
it('should return trace-viewer resource', async () => {
const result = await server.handleResourcesList(null);
expect(result.resources).toHaveLength(1);
expect(result.resources[0].uri).toBe('ui://proofscan/trace-viewer');
expect(result.resources[0].mimeType).toBe('text/html;profile=mcp-app');
});
});
describe('resources/read', () => {
it('should return HTML for trace-viewer', async () => {
const result = await server.handleResourcesRead(null, {
uri: 'ui://proofscan/trace-viewer'
});
expect(result.contents[0].text).toContain('<!DOCTYPE html>');
expect(result.contents[0].mimeType).toBe('text/html;profile=mcp-app');
});
it('should reject unknown URIs', async () => {
// Test error handling
});
it('should reject invalid URI formats', async () => {
// Test path traversal attempts
});
});
describe('ui/initialize', () => {
it('should generate unique session tokens', async () => {
const result1 = await server.handleUiInitialize(null, {});
const result2 = await server.handleUiInitialize(null, {});
expect(result1.sessionToken).not.toBe(result2.sessionToken);
});
});
});6. LOW: Inconsistent Protocol Version ConstantsLocation: const PROTOCOL_VERSION = '2024-11-05';
const UI_PROTOCOL_VERSION = '2025-11-21';Issue: Two different version constants may cause confusion. Consider if these need to be synchronized or documented. Recommendation: Add comment explaining the difference: /** MCP Core protocol version */
const PROTOCOL_VERSION = '2024-11-05';
/** MCP Apps Extension protocol version (SEP-1865) */
const UI_PROTOCOL_VERSION = '2025-11-21';7. LOW: Potential Memory Leak in UILocation: window.addEventListener('message', handleMessage);Issue: Event listener is never removed. While not critical for a long-lived UI, it's best practice to clean up. Recommendation: // Add cleanup if UI supports reload/unmount
function cleanup() {
window.removeEventListener('message', handleMessage);
}
// Expose for testing or if reload is implemented
window.__cleanup = cleanup;🟡 Observations & Suggestions1. Code OrganizationThe const PROOFSCAN_GET_EVENTS_TOOL = {
name: 'proofscan_getEvents',
description: 'Get protocol events (paginated). Returns text summary + structured data.',
// ... rest of definition
};
// In handleToolsList:
const toolsList = [
...tools.map((t) => ({ ... })),
PROOFSCAN_GET_EVENTS_TOOL,
];2. Type Safety EnhancementThe export interface ToolsCallResult {
content?: unknown[];
isError?: boolean;
structuredContent?: unknown; // ← New
_meta?: { ... }; // ← New
}Consider if existing tools expect 3. DocumentationMissing JSDoc comments for new public functions:
4. Protocol ValidationThe UI validates 📊 Acceptance Criteria ReviewBased on the PR description:
All acceptance criteria met ✅ 🎯 Recommendations PriorityMust Fix (before merge):
Should Fix (this PR or next):
Nice to Have (can defer):
📝 Manual Testing RecommendationsSince tests weren't run in CI, please verify:
🎉 ConclusionThis is a well-structured foundation for MCP Apps support with good architectural decisions. The minimal viable approach is appropriate for Phase 6.1, and the code follows project conventions. However, there are critical security issues that must be addressed before merging:
Recommendation: Request changes to address the security issues, then approve. The stub implementation approach is correct for this phase. Great work on the CSP policy and token validation! 🔒 Reviewed by: Claude Code (Sonnet 4.5) |
- postMessage origin validation: lock to first origin received - RPC ID race condition: use pending requests Map instead of hardcoded id - URI validation: add length, format, and path traversal checks - getTraceViewerHtml error handling: wrap in try/catch with proper logging
Code Review: Phase 6.1 - MCP Apps 基盤実装SummaryThis PR implements the foundational infrastructure for MCP Apps Extension (SEP-1865), adding UI resource capabilities to the ProofScan MCP proxy server. The implementation follows a minimal success path approach, enabling basic UI display functionality. ✅ Strengths1. Clean Architecture
2. Security Considerations
3. Type Safety
4. Error Handling
🔍 Issues & ConcernsHIGH PRIORITY: Security VulnerabilityLocation: Issue: Using Node.js const sessionToken = crypto.randomUUID();Problem: The Fix: Add import at the top of the file: import { randomUUID } from 'crypto';
// Then use:
const sessionToken = randomUUID();MEDIUM PRIORITY: Code Quality1. Duplicate File Path Resolution LogicLocation: The
2. Magic NumbersLocation: const MAX_URI_LENGTH = 2048;Good to have this constant! However, consider documenting why 2048 was chosen (is it based on browser limits, security concerns, or arbitrary?). 3. Inconsistent Error MessagesLocation: throw new Error('UI resource unavailable');vs. this.sendError(id, MCP_ERROR.INTERNAL_ERROR, 'Failed to load UI resource');The caught error message differs from the thrown message. Consider using the original error message or being consistent. LOW PRIORITY: Improvements1. Missing Test CoverageThe PR description mentions all tests pass (1778 tests), but there don't appear to be new tests for:
Recommendation: Add integration tests for the new handlers, especially around:
2. HTML JavaScript Could Be More RobustLocation: The origin locking mechanism locks to the first origin that sends a message. This could be problematic if a malicious iframe sends a message before the legitimate host. Consider:
3. Stub Implementation ClarityLocation: The
4. Type Definition CompletenessLocation: The structuredContent?: unknown;
_meta?: {
ui?: { resourceUri?: string; };
outputSchemaVersion?: string;
[key: string]: unknown;
};The 🔒 Security Assessment✅ Good Practices
|
PR Review: Phase 6.1 - MCP Apps 基盤実装SummaryThis PR implements the foundation for MCP Apps Extension (SEP-1865), adding UI capabilities to the MCP proxy server. The implementation follows a minimal viable path to display UI and includes three new handlers: Code Quality ⭐⭐⭐⭐ (4/5)Strengths
Areas for Improvement1. CRITICAL: HTML File Not Included in Build OutputLocation: The TypeScript compiler only processes Impact: The Recommended Fix: // package.json - add copy script
"scripts": {
"build": "tsc && npm run copy-assets",
"copy-assets": "mkdir -p dist/html && cp src/html/*.html dist/html/",
}2. Missing Test CoverageLocation: No test files for new functionality The PR mentions "1778 tests passed" but no new tests were added. The new features lack:
3. Security: Weak Token GenerationLocation: const sessionToken = crypto.randomUUID();While UUIDs are random, they are not cryptographically strong for session tokens. Consider: import { randomBytes } from "crypto";
const sessionToken = randomBytes(32).toString("base64url");Severity: Medium - UUIDs are probably sufficient, but stronger tokens are better practice. 4. Type Safety: Unsafe Type AssertionsLocation: The 5. HTML: Inline Scripts Violate CSP Best PracticesLocation: Using Performance Considerations ✅Good
Suggestions
Security Concerns 🔒Implemented Protections ✅
Additional Recommendations1. Origin Validation Too PermissiveLocation: The code locks to the first origin that sends a message, which could be an attacker if they win the race. Consider using an origin whitelist instead. 2. Missing Rate LimitingThe 3. Token Not Stored or Validated on ServerThe server generates a token but never stores or validates it on subsequent requests. The client validates it, but the server should also validate that tokens match expected sessions. Test Coverage Assessment
|
PR Review: Phase 6.1 - MCP Apps 基盤実装SummaryThis PR implements the foundational infrastructure for MCP Apps Extension (SEP-1865), adding UI capabilities through resources/list, resources/read, and ui/initialize endpoints. The implementation looks solid overall with good security considerations. Here's my detailed feedback: ✅ Strengths1. Security ImprovementsThe security fixes from the second commit are excellent:
2. Clean Architecture
3. CSP ImplementationThe Content Security Policy is appropriately restrictive (src/html/trace-viewer.html:6-7): 🔍 Issues & RecommendationsCritical: Test CoverageIssue: No test files were added for the new functionality. The PR description mentions "1778 tests pass", but there are no specific tests for handleResourcesList(), handleResourcesRead(), handleUiInitialize(), URI validation logic, or HTML file loading. Recommendation: Add src/proxy/mcp-server-apps.test.ts with tests covering resources/list, resources/read, and ui/initialize endpoints. Test URI validation (length limits, path traversal, invalid schemes) and error handling. High: Session Token Storage and ValidationIssue: The session token is generated but never stored or validated server-side (src/proxy/mcp-server.ts:814). Tokens are "fire and forget" with no server-side tracking. Recommendation:
Note: If tokens aren't meant to be validated server-side in Phase 6.1, add a comment explaining this is intentional. Medium: Build Script Platform CompatibilityIssue: The build script uses Unix-specific cp command (package.json:20): This will fail on Windows systems. Recommendation: Use cross-platform solution with Node.js fs.cpSync() or create a separate build script. Medium: Missing Import StatementIssue: crypto is used but not explicitly imported (src/proxy/mcp-server.ts:814). Recommendation: Add explicit import: Low: HTML File Loading on Every RequestIssue: getTraceViewerHtml() reads the file from disk on every resources/read request. Performance Impact: Minimal for now since this is infrequent. Recommendation (future): Cache HTML content at startup to avoid repeated disk I/O. Low: Magic String in Tools ListIssue: proofscan_getEvents is hardcoded in the tools list (src/proxy/mcp-server.ts:607-641). Recommendation: Extract to a constant for maintainability. 🔐 Security Assessment✅ Well Handled
|
- Add mcp-server-resources.test.ts with 12 tests: - resources/list returns trace-viewer with correct mimeType - resources/read returns HTML and validates URIs (length, scheme, path traversal) - ui/initialize generates unique session tokens - Session tokens are stored and validated - Add session token management to McpProxyServer: - Add sessionTokens Set to store active tokens - Store tokens on ui/initialize - Add isValidSessionToken() for future use Addresses PR #101 review comments
Code Review - PR #101: MCP Apps FoundationOverviewThis PR implements the foundation for MCP Apps Extension (SEP-1865), providing a minimal viable UI capability. The implementation is well-structured and follows the project's patterns. ✅ Strengths1. Security-First Design
2. Test Coverage
3. Type Safety
4. Clean Architecture
🔍 Issues & RecommendationsCritical IssuesNone - the security validations are solid. High Priority1. Build Script Reliability (package.json:10) "build": "tsc && cp -r src/html dist/"Issue: Recommendation: Add validation or use a more robust build tool. 2. HTML Loading Error Handling (src/proxy/mcp-server.ts:656-665) function getTraceViewerHtml(): string {
try {
return readFileSync(htmlPath, 'utf-8');
} catch (error) {
throw new Error('UI resource unavailable');
}
}Issue: File read happens synchronously on every Recommendation:
Medium Priority3. Session Token Management (src/proxy/mcp-server.ts:104) private sessionTokens: Set<string> = new Set();Issue: Tokens are never cleaned up, leading to unbounded memory growth. Recommendation: Add token expiration with TTL-based cleanup. 4. Origin Lock Race Condition (src/html/trace-viewer.html:186-193) if (!trustedOrigin && event.origin) {
trustedOrigin = event.origin;
}Issue: In theory, a malicious frame could send a message before the legitimate parent, though this is low risk in practice. Recommendation: Accept origin as initialization parameter or use postMessage's 5. Resource URI Constant Duplication Recommendation: Export from a shared constants file to ensure consistency. Low Priority6. Console Logging in Production (src/html/trace-viewer.html) 7. Protocol Version Hardcoding (src/proxy/mcp-server.ts:42) const UI_PROTOCOL_VERSION = '2025-11-21';Recommendation: Document why this differs from 8. Unused isValidSessionToken Method (src/proxy/mcp-server.ts:917-919) 📊 Test Results ConfirmationPR description claims:
All acceptance criteria met for Phase 6.1. 🎯 Performance Considerations
🔒 Security AssessmentOverall: Strong ✅ The implementation demonstrates good security practices:
No critical vulnerabilities found. 📝 Code Quality
🚀 Suggestions for Follow-up PRs
✅ Approval RecommendationAPPROVED with suggestions for follow-up improvements. This is a solid foundation PR that implements the minimal success path correctly. The security validations are thorough, tests are comprehensive, and the code follows project conventions. The suggested improvements are enhancements rather than blockers. Reviewed by: Claude Code |
PR Review: Phase 6.1 - MCP Apps 基盤実装SummaryThis PR implements the foundation for MCP Apps Extension (SEP-1865), adding support for serving HTML-based UIs through the MCP protocol. The implementation follows a minimal viable path to display UI content. ✅ Strengths1. Strong Security Posture
2. Comprehensive Test Coverage
3. Clean Architecture
4. Good Documentation
🔍 Issues FoundCritical IssuesNone found. High Priority1. Missing Error Handling in Copy Script (scripts/copy-html.cjs:11) fs.cpSync(src, dest, { recursive: true });Issue: No error handling if source directory doesn't exist or copy fails. try {
fs.cpSync(src, dest, { recursive: true });
console.log('Copied src/html -> dist/html');
} catch (err) {
console.error('Failed to copy HTML assets:', err.message);
process.exit(1);
}2. HTML Not Self-Contained (trace-viewer.html:1-249) // Add after line 238
setTimeout(() => {
if (!sessionToken) {
setStatus('error', 'Connection timeout');
messageDiv.textContent = 'Failed to connect to MCP host. Please reload.';
}
}, 10000); // 10 second timeoutMedium Priority3. Session Token Never Expires (src/proxy/mcp-server.ts:106, 821)
4. Unused Private Method (src/proxy/mcp-server.ts:840-842) private isValidSessionToken(token: string): boolean {
return this.sessionTokens.has(token);
}Issue: Method is defined but never called (only in tests). 5. Tool outputSchema Mismatch (src/proxy/mcp-server.ts:622-641, 696-705)
Low Priority6. Magic Number in HTML (trace-viewer.html:158) const targetOrigin = trustedOrigin || '*';Issue: Using wildcard origin before locking is safe but could be documented. 7. Console.log in Production Code (trace-viewer.html:157, 169, 186, 191, 202, 217, 222) 💡 SuggestionsCode Quality
Performance
private cachedTraceViewerHtml: string | null = null;
function getTraceViewerHtml(): string {
if (!this.cachedTraceViewerHtml) {
this.cachedTraceViewerHtml = readFileSync(htmlPath, 'utf-8');
}
return this.cachedTraceViewerHtml;
}
🔒 Security AssessmentOverall security posture is excellent for a Phase 1 implementation:
🧪 Test Coverage AssessmentTest coverage is comprehensive:
📊 Performance Considerations
✅ Acceptance Criteria CheckPer the PR description, all ACs are met:
🎯 RecommendationApprove with minor fixes recommended. This is a solid foundation for MCP Apps. The implementation is clean, secure, and well-tested. The issues identified are mostly minor and can be addressed in follow-up PRs or before merge. Before Merge (Optional but Recommended):
Can Be Deferred:
🙌 Nice Work!The code quality, test coverage, and security considerations demonstrate careful engineering. The "minimum viable path" approach is exactly right for a foundational PR. Review completed by Claude Code |
2 participants
PR1 - Apps基盤
概要
MCP Apps Extension (SEP-1865) の基盤実装。UIが表示できる「最小成功ルート」を実装しました。
変更ファイル
src/html/trace-viewer.html(新規) - UI HTML (JS/CSS同梱)src/proxy/types.ts- Apps拡張の型定義追加src/proxy/mcp-server.ts- ハンドラー追加 (resources/list, resources/read, ui/initialize)実装内容
1. resources/list
ui://proofscan/trace-viewerを追加mimeType: "text/html;profile=mcp-app"を指定2. resources/read
3. tools/list
proofscan_getEventsを追加outputSchema宣言済み_meta.ui.resourceUri追加済み_meta.outputSchemaVersion = "1"追加済み4. ui/initialize
sessionToken発行5. UI側
ui/initialize送信で token 取得ACチェック結果
resources/listがtext/html;profile=mcp-appmimeType を返すresources/readが単一HTMLを返す（JS/CSS同梱、外部CDNなし）tools/listが outputSchema と_meta.ui.resourceUriを返す手動確認ログ
npm run build成功npm test全パス (1778 tests)default-src 'none'; script-src 'unsafe-inline'; style-src 'unsafe-inline'テスト結果
スコープ
関連ドキュメント
/home/shin/vault/03_Projects/proofscan/3032 - proofscan Phase 6 - MCP Apps 設計書.md