From 036364f844d872837e2a9c525a09a33b75177285 Mon Sep 17 00:00:00 2001 From: kimi-reviewer-andresmgsl Date: Sun, 26 Jul 2026 20:52:29 +0000 Subject: [PATCH] Address review: pager owns limit/page, reject GET+input, label delete exclusivity --- README.md | 9 ++++++--- src/api.js | 16 ++++++++++++--- src/cli.js | 8 ++++++++ test/api.test.js | 28 +++++++++++++++++++++----- test/cli.test.js | 51 ++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 101 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index bc1ac15..4da74e0 100644 --- a/README.md +++ b/README.md @@ -605,7 +605,7 @@ Calls `POST /api/v1/repos/{owner}/{repo}/labels`. ### `stoke label delete` -Delete a label from a repository, by `--id` or `--name` (one is required). +Delete a label from a repository, by `--id` or `--name` (exactly one is required; passing both is rejected). ```bash stoke label delete -o heavy-duty -r stoke --name needs-triage @@ -853,7 +853,8 @@ Options: -X, --method GET, POST, PUT, PATCH or DELETE (default: GET, or POST when --input is given) --input JSON request body, inline or @path to read from a file - --paginate fetch all pages (GET endpoints returning a JSON array) + --paginate fetch all pages (GET endpoints returning a JSON array); + overrides any limit/page in the endpoint ``` ```bash @@ -863,7 +864,9 @@ stoke api /repos/heavy-duty/stoke/issues/12/comments --input '{"body":"hi"}' stoke api /repos/heavy-duty/stoke/contents/CHANGELOG.md --input @payload.json ``` -Calls `{METHOD} /api/v1{endpoint}` with the stored token. The endpoint must start with `/`; the method, `--paginate` + non-GET, and malformed `--input` JSON are all rejected before any network call. +Calls `{METHOD} /api/v1{endpoint}` with the stored token. The endpoint must start with `/`; the method, `--paginate` + non-GET, `GET` + `--input` (a GET cannot carry a body), and malformed `--input` JSON are all rejected before any network call. + +**Security:** `stoke api` is a full authenticated passthrough — it does anything the stored token is allowed to do. Never interpolate untrusted strings (issue titles, PR bodies, user input) into the endpoint or `--input`; treat every call like the credential it carries. ## Architecture diff --git a/src/api.js b/src/api.js index 9a77c88..d0c5ba3 100644 --- a/src/api.js +++ b/src/api.js @@ -134,10 +134,20 @@ class ForgejoClient { async getAll(endpoint, params = {}) { const pageSize = 50; const all = []; - const separator = endpoint.includes('?') ? '&' : '?'; + // The pager owns limit/page: a caller-supplied pair must be overridden, + // not duplicated — a duplicated limit pins the page size the server + // honors first and can truncate or loop the walk. + const queryIndex = endpoint.indexOf('?'); + const path = queryIndex === -1 ? endpoint : endpoint.slice(0, queryIndex); + const baseQuery = new URLSearchParams(queryIndex === -1 ? '' : endpoint.slice(queryIndex + 1)); + baseQuery.delete('limit'); + baseQuery.delete('page'); for (let page = 1; page <= 1000; page += 1) { - const query = new URLSearchParams({ ...params, limit: String(pageSize), page: String(page) }).toString(); - const items = await this.get(`${endpoint}${separator}${query}`); + const query = new URLSearchParams(baseQuery); + for (const [key, value] of Object.entries(params)) query.set(key, value); + query.set('limit', String(pageSize)); + query.set('page', String(page)); + const items = await this.get(`${path}?${query.toString()}`); if (!Array.isArray(items) || items.length === 0) break; all.push(...items); if (items.length < pageSize) break; diff --git a/src/cli.js b/src/cli.js index 35d99b1..e7357fa 100755 --- a/src/cli.js +++ b/src/cli.js @@ -973,6 +973,10 @@ label console.error('One of --id or --name is required.'); process.exit(1); } + if (options.id && options.name) { + console.error('Use either --id or --name, not both.'); + process.exit(1); + } const config = loadConfig(); const client = ForgejoClient.fromConfig(config); const ids = options.id @@ -1365,6 +1369,10 @@ program console.error('--paginate only works with GET.'); process.exit(1); } + if (method === 'GET' && options.input !== undefined) { + console.error('GET requests cannot carry a body. Drop --input, or use -X POST/PUT/PATCH/DELETE.'); + process.exit(1); + } let body = null; if (options.input !== undefined) { diff --git a/test/api.test.js b/test/api.test.js index 24ee8eb..cd60fc8 100644 --- a/test/api.test.js +++ b/test/api.test.js @@ -187,6 +187,22 @@ test('getAll joins pagination with & when the endpoint already has a query', asy assert.equal(calls[0].url, 'https://forge.test/api/v1/repos/o/r/pulls?state=closed&limit=50&page=1'); }); +test('getAll overrides caller-supplied limit/page instead of duplicating them', async () => { + const calls = mockFetch((url) => { + const page = Number(new URL(url).searchParams.get('page')); + return jsonResponse(page === 1 ? Array.from({ length: 50 }, (_, i) => ({ id: i })) : []); + }); + const client = new ForgejoClient('https://forge.test', 'tok'); + const all = await client.getAll('/repos/o/r/pulls?state=closed&limit=1&page=9'); + assert.equal(all.length, 50); + const first = new URL(calls[0].url).searchParams; + const second = new URL(calls[1].url).searchParams; + assert.deepEqual(first.getAll('limit'), ['50']); + assert.deepEqual(first.getAll('page'), ['1']); + assert.deepEqual(second.getAll('page'), ['2']); + assert.equal(first.get('state'), 'closed'); +}); + test('release endpoints map to the expected URLs and payloads', async () => { const calls = mockFetch(() => jsonResponse({ tag_name: '1.0.0' })); const client = new ForgejoClient('https://forge.test', 'tok'); @@ -203,13 +219,15 @@ test('release endpoints map to the expected URLs and payloads', async () => { test('label endpoints map to the expected URLs and payloads', async () => { const calls = mockFetch(() => jsonResponse({ id: 3 })); const client = new ForgejoClient('https://forge.test', 'tok'); + await client.listLabels('owner', 'repo'); await client.createLabel('owner', 'repo', { name: 'release', color: '0e8a16', description: '' }); await client.deleteLabel('owner', 'repo', 3); - assert.equal(calls[0].url, 'https://forge.test/api/v1/repos/owner/repo/labels'); - assert.equal(calls[0].opts.method, 'POST'); - assert.deepEqual(JSON.parse(calls[0].opts.body), { name: 'release', color: '0e8a16', description: '' }); - assert.equal(calls[1].url, 'https://forge.test/api/v1/repos/owner/repo/labels/3'); - assert.equal(calls[1].opts.method, 'DELETE'); + assert.equal(calls[0].url, 'https://forge.test/api/v1/repos/owner/repo/labels?limit=50&page=1'); + assert.equal(calls[1].url, 'https://forge.test/api/v1/repos/owner/repo/labels'); + assert.equal(calls[1].opts.method, 'POST'); + assert.deepEqual(JSON.parse(calls[1].opts.body), { name: 'release', color: '0e8a16', description: '' }); + assert.equal(calls[2].url, 'https://forge.test/api/v1/repos/owner/repo/labels/3'); + assert.equal(calls[2].opts.method, 'DELETE'); }); test('issue label add/remove hit the issue labels endpoints', async () => { diff --git a/test/cli.test.js b/test/cli.test.js index 0db0743..02ce46b 100644 --- a/test/cli.test.js +++ b/test/cli.test.js @@ -206,6 +206,18 @@ test('api rejects --paginate with a non-GET method before any network call', () assert.match(res.stderr, /--paginate only works with GET/); }); +test('api rejects GET with --input before any network call', () => { + const res = run(['api', '/user', '-X', 'GET', '--input', '{}']); + assert.equal(res.status, 1); + assert.match(res.stderr, /GET requests cannot carry a body/); +}); + +test('label delete rejects --id and --name together before any network call', () => { + const res = run(['label', 'delete', '-o', 'o', '-r', 'r', '--id', '3', '--name', 'x']); + assert.equal(res.status, 1); + assert.match(res.stderr, /either --id or --name, not both/); +}); + test('api rejects invalid JSON input before any network call', () => { const res = run(['api', '/user', '--input', '{nope']); assert.equal(res.status, 1); @@ -251,6 +263,45 @@ test('api sends the token and prints the JSON response', async () => { } }); +test('label add fails closed on an unknown label name', async () => { + const cfg = path.join(os.tmpdir(), `stoke-cfg-${process.pid}-lbl.json`); + const TIMEOUT_MS = 5000; + let timer; + const result = await new Promise((resolve, reject) => { + const fail = (err) => { + clearTimeout(timer); + try { server.close(); } catch { /* already closed */ } + reject(err instanceof Error ? err : new Error(String(err))); + }; + const server = http.createServer((req, res) => { + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify([{ id: 1, name: 'bug', color: 'd73a4a' }])); + }); + timer = setTimeout(() => fail(new Error('timeout')), TIMEOUT_MS); + server.listen(0, '127.0.0.1', async () => { + const { port } = server.address(); + fs.writeFileSync(cfg, JSON.stringify({ url: `http://127.0.0.1:${port}`, token: 'tok' })); + try { + const res = await spawnAsync( + ['label', 'add', '-o', 'o', '-r', 'r', '-n', '7', '--name', 'ghost'], + { STOKE_CONFIG_FILE: cfg }, + ); + clearTimeout(timer); + server.close(() => resolve(res)); + } catch (err) { + fail(err); + } + }); + }).finally(() => clearTimeout(timer)); + + try { + assert.equal(result.status, 1); + assert.match(result.stderr, /Label not found in o\/r: ghost/); + } finally { + fs.unlinkSync(cfg); + } +}); + function spawnAsync(args, env = {}) { return new Promise((resolve, reject) => { const child = spawn(process.execPath, [CLI, ...args], {