diff --git a/docs/tutorial/03-vscode-planning.md b/docs/tutorial/03-vscode-planning.md index 6937db00..5e47e187 100644 --- a/docs/tutorial/03-vscode-planning.md +++ b/docs/tutorial/03-vscode-planning.md @@ -50,6 +50,8 @@ Click it to run `lol impl `. The implementation output appears in a separate **Implementation Log** panel, and the button is disabled while the implementation run is active. +Both logs render canonical GitHub issue and PR URLs as clickable links. + ## Next Steps - [Tutorial 02: Issue to Implementation](./02-issue-to-impl.md) for the CLI flow diff --git a/docs/workflows/vscode-extension.md b/docs/workflows/vscode-extension.md index 79a6f077..7038a3ce 100644 --- a/docs/workflows/vscode-extension.md +++ b/docs/workflows/vscode-extension.md @@ -57,6 +57,10 @@ The extension scans stdout/stderr lines in real time and stores the first matchi number on the session. Capturing the issue number during execution ensures the UI can surface the Implement and View PR actions immediately after the plan completes. +Plan and implementation logs also linkify canonical GitHub issue and pull request URLs. +External opening is restricted to canonical `github.com///issues/` and +`github.com///pull/` routes. + When a plan emits a local markdown path (for example, `.tmp/issue-928.md`), the UI surfaces a View Plan button that opens the file inside the workspace. diff --git a/tests/vscode/test-link-detection.sh b/tests/vscode/test-link-detection.sh index d2403533..6f0cfd07 100755 --- a/tests/vscode/test-link-detection.sh +++ b/tests/vscode/test-link-detection.sh @@ -9,17 +9,17 @@ test_info "Testing link detection patterns" TEST_SCRIPT=$(make_temp_dir "link-detection-test")/test.js cat > "$TEST_SCRIPT" << 'NODEJS' -// Regex patterns extracted from vscode/webview/plan/index.ts +// Regex patterns extracted from vscode/webview/plan/utils.ts -// GitHub issue URLs: https://github.com/owner/repo/issues/N -const githubRegex = /https:\/\/github\.com\/([^\/\s]+)\/([^\/\s]+)\/issues\/(\d+)/g; +// Canonical GitHub issue/PR URLs inside log text. +const githubRegex = /https:\/\/github\.com\/[^/\s]+\/[^/\s]+\/(?:issues|pull)\/\d+(?=$|[^\w/?#])/g; // Local markdown paths: .tmp/issue-N.md or /path/to/file.md const mdPathRegex = /(?<=\s|^)(\.tmp\/[^\s\n]+\.md|[\w\-\/]+\.tmp\/[^\s\n]+\.md)(?=\s|$)/g; // isValidGitHubUrl from vscode/src/view/unifiedViewProvider.ts function isValidGitHubUrl(url) { - return /^https:\/\/github\.com\/[^/]+\/[^/]+\/issues\/\d+$/.test(url); + return /^https:\/\/github\.com\/[^/]+\/[^/]+\/(?:issues|pull)\/\d+$/.test(url); } // Test cases for GitHub URL detection @@ -28,42 +28,69 @@ const githubTestCases = [ name: 'Standard GitHub issue URL', input: 'Check https://github.com/owner/repo/issues/123 for details', expectedMatch: ['https://github.com/owner/repo/issues/123'], - expectedValid: true, }, { name: 'GitHub URL with numbers in owner name', input: 'See https://github.com/user123/repo-name/issues/456', expectedMatch: ['https://github.com/user123/repo-name/issues/456'], - expectedValid: true, }, { name: 'Multiple GitHub URLs', input: 'Issues: https://github.com/a/b/issues/1 and https://github.com/c/d/issues/2', expectedMatch: ['https://github.com/a/b/issues/1', 'https://github.com/c/d/issues/2'], - expectedValid: true, }, { - name: 'GitHub PR URL (should not match issue regex)', + name: 'GitHub PR URL', input: 'PR at https://github.com/owner/repo/pull/123', - expectedMatch: [], - expectedValid: false, + expectedMatch: ['https://github.com/owner/repo/pull/123'], }, { name: 'GitHub non-issue URL', input: 'Visit https://github.com/owner/repo', expectedMatch: [], - expectedValid: false, }, { name: 'Non-GitHub URL', input: 'Visit https://example.com/some/path', expectedMatch: [], - expectedValid: false, }, { name: 'Invalid GitHub URL format', input: 'Check https://github.com/owner/issues/123 (no repo)', expectedMatch: [], + }, + { + name: 'GitHub PR URL with extra route segment', + input: 'See https://github.com/owner/repo/pull/123/files', + expectedMatch: [], + }, +]; + +// Test cases for host URL allowlist validation +const githubValidationCases = [ + { + name: 'Canonical issue URL is valid', + url: 'https://github.com/owner/repo/issues/123', + expectedValid: true, + }, + { + name: 'Canonical PR URL is valid', + url: 'https://github.com/owner/repo/pull/456', + expectedValid: true, + }, + { + name: 'Unsupported GitHub route is invalid', + url: 'https://github.com/owner/repo/actions/runs/1', + expectedValid: false, + }, + { + name: 'PR URL with extra segment is invalid', + url: 'https://github.com/owner/repo/pull/123/files', + expectedValid: false, + }, + { + name: 'Non-GitHub URL is invalid', + url: 'https://example.com/owner/repo/issues/1', expectedValid: false, }, ]; @@ -114,25 +141,31 @@ let failed = 0; console.log('Testing GitHub URL detection:\n'); githubTestCases.forEach(tc => { const matches = [...tc.input.matchAll(githubRegex)].map(m => m[0]); - const isValid = tc.expectedMatch.length > 0 ? isValidGitHubUrl(tc.expectedMatch[0]) : false; - - let matchOk = JSON.stringify(matches) === JSON.stringify(tc.expectedMatch); - let validOk = isValid === tc.expectedValid; - - if (matchOk && validOk) { + + if (JSON.stringify(matches) === JSON.stringify(tc.expectedMatch)) { console.log(`✓ PASS: ${tc.name}`); passed++; } else { console.log(`✗ FAIL: ${tc.name}`); console.log(` Input: ${tc.input}`); - if (!matchOk) { - console.log(` Expected matches: ${JSON.stringify(tc.expectedMatch)}`); - console.log(` Got matches: ${JSON.stringify(matches)}`); - } - if (!validOk) { - console.log(` Expected valid: ${tc.expectedValid}`); - console.log(` Got valid: ${isValid}`); - } + console.log(` Expected matches: ${JSON.stringify(tc.expectedMatch)}`); + console.log(` Got matches: ${JSON.stringify(matches)}`); + failed++; + } +}); + +// Test URL allowlist validation +console.log('\nTesting GitHub URL allowlist validation:\n'); +githubValidationCases.forEach(tc => { + const actual = isValidGitHubUrl(tc.url); + if (actual === tc.expectedValid) { + console.log(`✓ PASS: ${tc.name}`); + passed++; + } else { + console.log(`✗ FAIL: ${tc.name}`); + console.log(` URL: ${tc.url}`); + console.log(` Expected valid: ${tc.expectedValid}`); + console.log(` Got valid: ${actual}`); failed++; } }); diff --git a/vscode/src/view/unifiedViewProvider.md b/vscode/src/view/unifiedViewProvider.md index f0c99b3d..898aef53 100644 --- a/vscode/src/view/unifiedViewProvider.md +++ b/vscode/src/view/unifiedViewProvider.md @@ -26,7 +26,7 @@ Consumes UI messages: - `plan/view-plan` - `plan/view-issue` - `plan/view-pr` -- `link/openExternal` (GitHub issue URLs) +- `link/openExternal` (canonical GitHub issue/PR URLs) - `link/openFile` (local markdown paths) Emits UI messages: @@ -111,7 +111,10 @@ Resolves the working directory for Plan/Implementation runs by preferring the `trees/main` layout and falling back to an Agentize worktree root when needed. ### Link Handling -- `isValidGitHubUrl(url: string)`: validates GitHub issue URLs. +- `isValidGitHubUrl(url: string)`: validates canonical GitHub issue/PR URLs + (`/issues/` or `/pull/`). +- `openExternalGitHubUrl(url: string)`: central allowlist gate for external URL open + paths (`plan/view-issue`, `plan/view-pr`, and `link/openExternal`). - `openLocalFile(filePath: string, options?)`: resolves relative paths from workspace roots, expands `~/...`, optionally creates missing files, and opens in the current editor group as a non-preview tab. diff --git a/vscode/src/view/unifiedViewProvider.ts b/vscode/src/view/unifiedViewProvider.ts index aef53832..2db31db3 100644 --- a/vscode/src/view/unifiedViewProvider.ts +++ b/vscode/src/view/unifiedViewProvider.ts @@ -462,8 +462,8 @@ export class UnifiedViewProvider implements vscode.WebviewViewProvider { return; } const issueUrl = await this.resolveIssueUrl(issueNumber); - if (issueUrl && this.isValidGitHubUrl(issueUrl)) { - void vscode.env.openExternal(vscode.Uri.parse(issueUrl)); + if (issueUrl) { + this.openExternalGitHubUrl(issueUrl); } return; } @@ -476,7 +476,7 @@ export class UnifiedViewProvider implements vscode.WebviewViewProvider { if (!session || !session.prUrl) { return; } - void vscode.env.openExternal(vscode.Uri.parse(session.prUrl)); + this.openExternalGitHubUrl(session.prUrl); return; } case 'settings/load': { @@ -488,10 +488,7 @@ export class UnifiedViewProvider implements vscode.WebviewViewProvider { return; } case 'link/openExternal': { - const url = message.url ?? ''; - if (this.isValidGitHubUrl(url)) { - void vscode.env.openExternal(vscode.Uri.parse(url)); - } + this.openExternalGitHubUrl(message.url ?? ''); return; } case 'link/openFile': { @@ -508,9 +505,16 @@ export class UnifiedViewProvider implements vscode.WebviewViewProvider { } } + private openExternalGitHubUrl(url: string): void { + if (!this.isValidGitHubUrl(url)) { + return; + } + void vscode.env.openExternal(vscode.Uri.parse(url)); + } + private isValidGitHubUrl(url: string): boolean { - // Validate GitHub issue URLs: https://github.com/owner/repo/issues/N - return /^https:\/\/github\.com\/[^/]+\/[^/]+\/issues\/\d+$/.test(url); + // Allow only canonical GitHub issue/PR URLs. + return /^https:\/\/github\.com\/[^/]+\/[^/]+\/(?:issues|pull)\/\d+$/.test(url); } private async openLocalFile( diff --git a/vscode/webview/plan/utils.md b/vscode/webview/plan/utils.md index 319044a0..b4fe7106 100644 --- a/vscode/webview/plan/utils.md +++ b/vscode/webview/plan/utils.md @@ -45,8 +45,10 @@ Builds and returns a DOM container with step indicators for the given session ke Uses `className` when provided; defaults to `step-indicators`. ### renderLinks(text) -Converts GitHub issue URLs and local `.tmp/*.md` paths in a log line into clickable links -for the webview message handlers. +Converts canonical GitHub issue/PR URLs and local `.tmp/*.md` paths in a log line into +clickable links for the webview message handlers. Only canonical routes are linkified: +`https://github.com///issues/` and +`https://github.com///pull/`. ### extractIssueNumber(line) Extracts an issue number from placeholder creation logs or GitHub issue URLs. diff --git a/vscode/webview/plan/utils.ts b/vscode/webview/plan/utils.ts index 4e24a4bd..79d74657 100644 --- a/vscode/webview/plan/utils.ts +++ b/vscode/webview/plan/utils.ts @@ -124,7 +124,7 @@ export const renderStepIndicatorsFrom = ( }; export const renderLinks = (text: string): string => { - const githubRegex = /https:\/\/github\.com\/([^\/\s]+)\/([^\/\s]+)\/issues\/(\d+)/g; + const githubRegex = /https:\/\/github\.com\/[^/\s]+\/[^/\s]+\/(?:issues|pull)\/\d+(?=$|[^\w/?#])/g; const mdPathRegex = /(?<=\s|^)(\.tmp\/[^\s\n]+\.md|[\w\-\/]+\.tmp\/[^\s\n]+\.md)(?=\s|$)/g; let result = escapeHtml(text);