From 61ea05bff7df0f64b7175cc059312c4435c635d7 Mon Sep 17 00:00:00 2001 From: jackwener Date: Mon, 16 Mar 2026 18:14:27 +0800 Subject: [PATCH] fix: URL injection, strictNullChecks, cross-platform build, +34 tests Security: - Fix URL injection in fetch.ts and bilibili.ts (JSON.stringify instead of string interpolation) - Fix unused scroll() amount parameter TypeScript: - Enable strictNullChecks in tsconfig - Change CliCommand.func signature to IPage (non-null) for browser adapters - Fix 93 compile errors across all adapters Build: - Remove || true from build-manifest (report failures instead of silencing) - Replace Unix shell commands with Node.js scripts for cross-platform builds Code quality: - Remove error-object special detection from pipeline executor - Unify error handling to throw pattern Tests: - New interceptor.test.ts (11 tests) - New executor.test.ts (13 tests) - Rewrite output.test.ts with comprehensive coverage (10 tests) --- package.json | 6 +- src/bilibili.ts | 4 +- src/browser.ts | 2 +- src/clis/twitter/notifications.ts | 2 +- src/engine.ts | 4 +- src/interceptor.test.ts | 94 +++++++++++++++++ src/output.test.ts | 73 +++++++++++++- src/pipeline/executor.test.ts | 161 ++++++++++++++++++++++++++++++ src/pipeline/executor.ts | 5 - src/pipeline/steps/fetch.ts | 7 +- src/registry.ts | 4 +- tsconfig.json | 1 + 12 files changed, 340 insertions(+), 23 deletions(-) create mode 100644 src/interceptor.test.ts create mode 100644 src/pipeline/executor.test.ts diff --git a/package.json b/package.json index f29a76b..f168c59 100644 --- a/package.json +++ b/package.json @@ -16,9 +16,9 @@ "scripts": { "dev": "tsx src/main.ts", "build": "tsc && npm run clean-yaml && npm run copy-yaml && npm run build-manifest", - "build-manifest": "node dist/build-manifest.js || true", - "clean-yaml": "find dist/clis -name '*.yaml' -o -name '*.yml' 2>/dev/null | xargs rm -f", - "copy-yaml": "find src/clis -name '*.yaml' -o -name '*.yml' | while read f; do d=\"dist/${f#src/}\"; mkdir -p \"$(dirname \"$d\")\"; cp \"$f\" \"$d\"; done", + "build-manifest": "node dist/build-manifest.js", + "clean-yaml": "node -e \"const{readdirSync:r,rmSync:d,existsSync:e,statSync:s}=require('fs'),p=require('path');function w(dir){if(!e(dir))return;for(const f of r(dir)){const fp=p.join(dir,f);s(fp).isDirectory()?w(fp):/\\.ya?ml$/.test(f)&&d(fp)}}w('dist/clis')\"", + "copy-yaml": "node -e \"const{readdirSync:r,copyFileSync:c,mkdirSync:m,existsSync:e,statSync:s}=require('fs'),p=require('path');function w(src,dst){if(!e(src))return;for(const f of r(src)){const sp=p.join(src,f),dp=p.join(dst,f);s(sp).isDirectory()?w(sp,dp):/\\.ya?ml$/.test(f)&&(m(p.dirname(dp),{recursive:!0}),c(sp,dp))}}w('src/clis','dist/clis')\"", "start": "node dist/main.js", "typecheck": "tsc --noEmit", "lint": "tsc --noEmit", diff --git a/src/bilibili.ts b/src/bilibili.ts index 3cf65bb..d92c7c8 100644 --- a/src/bilibili.ts +++ b/src/bilibili.ts @@ -84,10 +84,10 @@ export async function apiGet( } export async function fetchJson(page: IPage, url: string): Promise { - const escapedUrl = url.replace(/"/g, '\\"'); + const urlJs = JSON.stringify(url); return page.evaluate(` async () => { - const res = await fetch("${escapedUrl}", { credentials: "include" }); + const res = await fetch(${urlJs}, { credentials: "include" }); return await res.json(); } `); diff --git a/src/browser.ts b/src/browser.ts index 89e883b..d55426a 100644 --- a/src/browser.ts +++ b/src/browser.ts @@ -214,7 +214,7 @@ export class Page implements IPage { return this.call('tools/call', { name: 'browser_console_messages', arguments: { level } }); } - async scroll(direction: string = 'down', amount: number = 500): Promise { + async scroll(direction: string = 'down', _amount: number = 500): Promise { await this.call('tools/call', { name: 'browser_press_key', arguments: { key: direction === 'down' ? 'PageDown' : 'PageUp' } }); } diff --git a/src/clis/twitter/notifications.ts b/src/clis/twitter/notifications.ts index 2a454b8..d364808 100644 --- a/src/clis/twitter/notifications.ts +++ b/src/clis/twitter/notifications.ts @@ -30,7 +30,7 @@ cli({ let results: any[] = []; for (const req of requests) { try { - let instructions = []; + let instructions: any[] = []; if (req.data?.data?.viewer?.timeline_response?.timeline?.instructions) { instructions = req.data.data.viewer.timeline_response.timeline.instructions; } else if (req.data?.data?.viewer_v2?.user_results?.result?.notification_timeline?.timeline?.instructions) { diff --git a/src/engine.ts b/src/engine.ts index d2ded42..82602af 100644 --- a/src/engine.ts +++ b/src/engine.ts @@ -185,7 +185,7 @@ export async function executeCommand( const { getRegistry, fullName } = await import('./registry.js'); const updated = getRegistry().get(fullName(cmd)); if (updated && updated.func) { - return updated.func(page, kwargs, debug); + return updated.func(page!, kwargs, debug); } if (updated && updated.pipeline) { return executePipeline(page, updated.pipeline, { args: kwargs, debug }); @@ -193,7 +193,7 @@ export async function executeCommand( } if (cmd.func) { - return cmd.func(page, kwargs, debug); + return cmd.func(page!, kwargs, debug); } if (cmd.pipeline) { return executePipeline(page, cmd.pipeline, { args: kwargs, debug }); diff --git a/src/interceptor.test.ts b/src/interceptor.test.ts new file mode 100644 index 0000000..fc843b9 --- /dev/null +++ b/src/interceptor.test.ts @@ -0,0 +1,94 @@ +/** + * Tests for interceptor.ts: JavaScript code generators for XHR/Fetch interception. + */ + +import { describe, it, expect } from 'vitest'; +import { generateInterceptorJs, generateReadInterceptedJs, generateTapInterceptorJs } from './interceptor.js'; + +describe('generateInterceptorJs', () => { + it('generates valid JavaScript function source', () => { + const js = generateInterceptorJs('"api/search"'); + expect(js).toContain('window.fetch'); + expect(js).toContain('XMLHttpRequest'); + expect(js).toContain('"api/search"'); + // Should be a function expression wrapping + expect(js.trim()).toMatch(/^\(\)\s*=>/); + }); + + it('uses default array name and patch guard', () => { + const js = generateInterceptorJs('"test"'); + expect(js).toContain('__opencli_intercepted'); + expect(js).toContain('__opencli_interceptor_patched'); + }); + + it('uses custom array name and patch guard', () => { + const js = generateInterceptorJs('"test"', { + arrayName: '__my_data', + patchGuard: '__my_guard', + }); + expect(js).toContain('__my_data'); + expect(js).toContain('__my_guard'); + expect(js).not.toContain('__opencli_intercepted'); + }); + + it('includes fetch clone and json parsing', () => { + const js = generateInterceptorJs('"api"'); + expect(js).toContain('response.clone()'); + expect(js).toContain('clone.json()'); + }); + + it('includes XHR open and send patching', () => { + const js = generateInterceptorJs('"api"'); + expect(js).toContain('XMLHttpRequest.prototype'); + expect(js).toContain('__origOpen'); + expect(js).toContain('__origSend'); + }); +}); + +describe('generateReadInterceptedJs', () => { + it('generates valid JavaScript to read and clear data', () => { + const js = generateReadInterceptedJs(); + expect(js).toContain('__opencli_intercepted'); + // Should clear the array after reading + expect(js).toContain('= []'); + }); + + it('uses custom array name', () => { + const js = generateReadInterceptedJs('__custom_arr'); + expect(js).toContain('__custom_arr'); + expect(js).not.toContain('__opencli_intercepted'); + }); +}); + +describe('generateTapInterceptorJs', () => { + it('returns all required fields', () => { + const tap = generateTapInterceptorJs('"api/data"'); + + expect(tap.setupVar).toBeDefined(); + expect(tap.capturedVar).toBe('captured'); + expect(tap.promiseVar).toBe('capturePromise'); + expect(tap.resolveVar).toBe('captureResolve'); + expect(tap.fetchPatch).toBeDefined(); + expect(tap.xhrPatch).toBeDefined(); + expect(tap.restorePatch).toBeDefined(); + }); + + it('contains the capture pattern in setup', () => { + const tap = generateTapInterceptorJs('"my-pattern"'); + expect(tap.setupVar).toContain('"my-pattern"'); + }); + + it('restores original fetch and XHR in restorePatch', () => { + const tap = generateTapInterceptorJs('"test"'); + expect(tap.restorePatch).toContain('origFetch'); + expect(tap.restorePatch).toContain('origXhrOpen'); + expect(tap.restorePatch).toContain('origXhrSend'); + }); + + it('uses first-match capture (only first response)', () => { + const tap = generateTapInterceptorJs('"test"'); + // Both fetch and xhr patches should check !captured before storing + expect(tap.fetchPatch).toContain('!captured'); + expect(tap.xhrPatch).toContain('!captured'); + }); +}); diff --git a/src/output.test.ts b/src/output.test.ts index 53d7d76..b35cbf2 100644 --- a/src/output.test.ts +++ b/src/output.test.ts @@ -1,3 +1,7 @@ +/** + * Tests for output.ts: render function format coverage. + */ + import { describe, it, expect, vi, afterEach } from 'vitest'; import { render } from './output.js'; @@ -6,11 +10,67 @@ afterEach(() => { }); describe('render', () => { + it('renders JSON output', () => { + const log = vi.spyOn(console, 'log').mockImplementation(() => {}); + render([{ title: 'Hello', rank: 1 }], { fmt: 'json' }); + expect(log).toHaveBeenCalledOnce(); + const output = log.mock.calls[0]?.[0]; + const parsed = JSON.parse(output); + expect(parsed).toEqual([{ title: 'Hello', rank: 1 }]); + }); + + it('renders Markdown table output', () => { + const log = vi.spyOn(console, 'log').mockImplementation(() => {}); + render([{ name: 'Alice', score: 100 }], { fmt: 'md', columns: ['name', 'score'] }); + const calls = log.mock.calls.map(c => c[0]); + expect(calls[0]).toContain('| name | score |'); + expect(calls[1]).toContain('| --- | --- |'); + expect(calls[2]).toContain('| Alice | 100 |'); + }); + + it('renders CSV output with proper quoting', () => { + const log = vi.spyOn(console, 'log').mockImplementation(() => {}); + render([{ name: 'Alice, Bob', value: 'say "hi"' }], { fmt: 'csv' }); + const calls = log.mock.calls.map(c => c[0]); + // Header + expect(calls[0]).toBe('name,value'); + // Values with commas/quotes are quoted + expect(calls[1]).toContain('"Alice, Bob"'); + expect(calls[1]).toContain('"say ""hi"""'); + }); + + it('handles null and undefined data', () => { + const log = vi.spyOn(console, 'log').mockImplementation(() => {}); + render(null, { fmt: 'json' }); + expect(log).toHaveBeenCalledWith(null); + }); + + it('renders single object as single-row table', () => { + const log = vi.spyOn(console, 'log').mockImplementation(() => {}); + render({ title: 'Test' }, { fmt: 'json' }); + const output = log.mock.calls[0]?.[0]; + const parsed = JSON.parse(output); + expect(parsed).toEqual({ title: 'Test' }); + }); + + it('handles empty array gracefully', () => { + const log = vi.spyOn(console, 'log').mockImplementation(() => {}); + render([], { fmt: 'table' }); + // Should show "(no data)" for empty arrays + expect(log).toHaveBeenCalled(); + }); + + it('uses custom columns for CSV', () => { + const log = vi.spyOn(console, 'log').mockImplementation(() => {}); + render([{ a: 1, b: 2, c: 3 }], { fmt: 'csv', columns: ['a', 'c'] }); + const calls = log.mock.calls.map(c => c[0]); + expect(calls[0]).toBe('a,c'); + expect(calls[1]).toBe('1,3'); + }); + it('renders YAML output', () => { const log = vi.spyOn(console, 'log').mockImplementation(() => {}); - render([{ title: 'Hello', rank: 1 }], { fmt: 'yaml' }); - expect(log).toHaveBeenCalledOnce(); expect(log.mock.calls[0]?.[0]).toContain('- title: Hello'); expect(log.mock.calls[0]?.[0]).toContain('rank: 1'); @@ -18,10 +78,15 @@ describe('render', () => { it('renders yml alias as YAML output', () => { const log = vi.spyOn(console, 'log').mockImplementation(() => {}); - render({ title: 'Hello' }, { fmt: 'yml' }); - expect(log).toHaveBeenCalledOnce(); expect(log.mock.calls[0]?.[0]).toContain('title: Hello'); }); + + it('handles null values in CSV cells', () => { + const log = vi.spyOn(console, 'log').mockImplementation(() => {}); + render([{ name: 'test', value: null }], { fmt: 'csv' }); + const calls = log.mock.calls.map(c => c[0]); + expect(calls[1]).toBe('test,'); + }); }); diff --git a/src/pipeline/executor.test.ts b/src/pipeline/executor.test.ts new file mode 100644 index 0000000..1b6217e --- /dev/null +++ b/src/pipeline/executor.test.ts @@ -0,0 +1,161 @@ +/** + * Tests for pipeline/executor.ts: pipeline execution with mock page. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { executePipeline } from './index.js'; +import type { IPage } from '../types.js'; + +/** Create a minimal mock page for testing */ +function createMockPage(overrides: Partial = {}): IPage { + return { + goto: vi.fn(), + evaluate: vi.fn().mockResolvedValue(null), + snapshot: vi.fn().mockResolvedValue(''), + click: vi.fn(), + typeText: vi.fn(), + pressKey: vi.fn(), + wait: vi.fn(), + tabs: vi.fn().mockResolvedValue([]), + closeTab: vi.fn(), + newTab: vi.fn(), + selectTab: vi.fn(), + networkRequests: vi.fn().mockResolvedValue([]), + consoleMessages: vi.fn().mockResolvedValue(''), + scroll: vi.fn(), + autoScroll: vi.fn(), + installInterceptor: vi.fn(), + getInterceptedRequests: vi.fn().mockResolvedValue([]), + ...overrides, + }; +} + +describe('executePipeline', () => { + it('returns null for empty pipeline', async () => { + const result = await executePipeline(null, []); + expect(result).toBeNull(); + }); + + it('skips null/invalid steps', async () => { + const result = await executePipeline(null, [null, undefined, 42] as any); + expect(result).toBeNull(); + }); + + it('executes navigate step', async () => { + const page = createMockPage(); + await executePipeline(page, [ + { navigate: 'https://example.com' }, + ]); + expect(page.goto).toHaveBeenCalledWith('https://example.com'); + }); + + it('executes evaluate + select pipeline', async () => { + const page = createMockPage({ + evaluate: vi.fn().mockResolvedValue({ data: { list: [{ name: 'a' }, { name: 'b' }] } }), + }); + const result = await executePipeline(page, [ + { evaluate: '() => ({ data: { list: [{name: "a"}, {name: "b"}] } })' }, + { select: 'data.list' }, + ]); + expect(result).toEqual([{ name: 'a' }, { name: 'b' }]); + }); + + it('executes map step to transform items', async () => { + const page = createMockPage({ + evaluate: vi.fn().mockResolvedValue([ + { title: 'Hello', count: 10 }, + { title: 'World', count: 20 }, + ]), + }); + const result = await executePipeline(page, [ + { evaluate: 'test' }, + { map: { name: '${{ item.title }}', score: '${{ item.count }}' } }, + ]); + expect(result).toEqual([ + { name: 'Hello', score: 10 }, + { name: 'World', score: 20 }, + ]); + }); + + it('executes limit step', async () => { + const page = createMockPage({ + evaluate: vi.fn().mockResolvedValue([1, 2, 3, 4, 5]), + }); + const result = await executePipeline(page, [ + { evaluate: 'test' }, + { limit: '3' }, + ]); + expect(result).toEqual([1, 2, 3]); + }); + + it('executes sort step', async () => { + const page = createMockPage({ + evaluate: vi.fn().mockResolvedValue([{ n: 3 }, { n: 1 }, { n: 2 }]), + }); + const result = await executePipeline(page, [ + { evaluate: 'test' }, + { sort: { by: 'n', order: 'asc' } }, + ]); + expect(result).toEqual([{ n: 1 }, { n: 2 }, { n: 3 }]); + }); + + it('executes sort step with desc order', async () => { + const page = createMockPage({ + evaluate: vi.fn().mockResolvedValue([{ n: 1 }, { n: 3 }, { n: 2 }]), + }); + const result = await executePipeline(page, [ + { evaluate: 'test' }, + { sort: { by: 'n', order: 'desc' } }, + ]); + expect(result).toEqual([{ n: 3 }, { n: 2 }, { n: 1 }]); + }); + + it('executes wait step with number', async () => { + const page = createMockPage(); + await executePipeline(page, [ + { wait: 2 }, + ]); + expect(page.wait).toHaveBeenCalledWith(2); + }); + + it('handles unknown steps gracefully in debug mode', async () => { + const stderr = vi.spyOn(process.stderr, 'write').mockImplementation(() => true); + await executePipeline(null, [ + { unknownStep: 'test' }, + ], { debug: true }); + expect(stderr).toHaveBeenCalledWith(expect.stringContaining('Unknown step')); + stderr.mockRestore(); + }); + + it('passes args through template rendering', async () => { + const page = createMockPage({ + evaluate: vi.fn().mockResolvedValue([1, 2, 3, 4, 5]), + }); + const result = await executePipeline(page, [ + { evaluate: 'test' }, + { limit: '${{ args.count }}' }, + ], { args: { count: 2 } }); + expect(result).toEqual([1, 2]); + }); + + it('click step calls page.click', async () => { + const page = createMockPage(); + await executePipeline(page, [ + { click: '@5' }, + ]); + expect(page.click).toHaveBeenCalledWith('5'); + }); + + it('navigate preserves existing data through pipeline', async () => { + const page = createMockPage({ + evaluate: vi.fn().mockResolvedValue([{ a: 1 }]), + }); + const result = await executePipeline(page, [ + { evaluate: 'test' }, + { navigate: 'https://example.com' }, + ]); + // navigate should preserve existing data + expect(result).toEqual([{ a: 1 }]); + expect(page.goto).toHaveBeenCalledWith('https://example.com'); + }); +}); diff --git a/src/pipeline/executor.ts b/src/pipeline/executor.ts index 9e2b079..359f5a0 100644 --- a/src/pipeline/executor.ts +++ b/src/pipeline/executor.ts @@ -60,11 +60,6 @@ export async function executePipeline( if (debug) process.stderr.write(` ${chalk.yellow('⚠')} Unknown step: ${op}\n`); } - // Detect error objects returned by steps (e.g. tap store not found) - if (data && typeof data === 'object' && !Array.isArray(data) && data.error) { - process.stderr.write(` ${chalk.yellow('⚠')} ${chalk.yellow(op)}: ${data.error}\n`); - if (data.hint) process.stderr.write(` ${chalk.dim('💡')} ${chalk.dim(data.hint)}\n`); - } if (debug) debugStepResult(op, data); } } diff --git a/src/pipeline/steps/fetch.ts b/src/pipeline/steps/fetch.ts index 07dcc3f..80d88aa 100644 --- a/src/pipeline/steps/fetch.ts +++ b/src/pipeline/steps/fetch.ts @@ -45,11 +45,12 @@ async function fetchSingle( } const headersJs = JSON.stringify(renderedHeaders); - const escapedUrl = finalUrl.replace(/"/g, '\\"'); + const urlJs = JSON.stringify(finalUrl); + const methodJs = JSON.stringify(method.toUpperCase()); return page.evaluate(` async () => { - const resp = await fetch("${escapedUrl}", { - method: "${method}", headers: ${headersJs}, credentials: "include" + const resp = await fetch(${urlJs}, { + method: ${methodJs}, headers: ${headersJs}, credentials: "include" }); return await resp.json(); } diff --git a/src/registry.ts b/src/registry.ts index 22b9ce6..7af05a3 100644 --- a/src/registry.ts +++ b/src/registry.ts @@ -30,7 +30,7 @@ export interface CliCommand { browser?: boolean; args: Arg[]; columns?: string[]; - func?: (page: IPage | null, kwargs: Record, debug?: boolean) => Promise; + func?: (page: IPage, kwargs: Record, debug?: boolean) => Promise; pipeline?: any[]; timeoutSeconds?: number; source?: string; @@ -50,7 +50,7 @@ export interface CliOptions { browser?: boolean; args?: Arg[]; columns?: string[]; - func?: (page: IPage | null, kwargs: Record, debug?: boolean) => Promise; + func?: (page: IPage, kwargs: Record, debug?: boolean) => Promise; pipeline?: any[]; timeoutSeconds?: number; } diff --git a/tsconfig.json b/tsconfig.json index 3300a07..ce7a3a7 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -6,6 +6,7 @@ "outDir": "dist", "rootDir": "src", "strict": false, + "strictNullChecks": true, "esModuleInterop": true, "skipLibCheck": true, "forceConsistentCasingInFileNames": true,