diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 999243e..9b09c4a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -14,8 +14,8 @@ jobs: test: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7 - - uses: actions/setup-node@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 with: node-version: 24 - run: npm ci diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 0910384..e3b7890 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -10,6 +10,9 @@ on: schedule: - cron: "17 3 * * 1" +# The analyze job widens its own permissions; everything else gets none. +permissions: read-all + jobs: analyze: name: Analyze @@ -20,12 +23,12 @@ jobs: security-events: write steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - - uses: github/codeql-action/init@v3 + - uses: github/codeql-action/init@42947a340483f03ba47bb1a039b2c519aab3df85 # v3.37.8 with: languages: javascript-typescript - - uses: github/codeql-action/analyze@v3 + - uses: github/codeql-action/analyze@42947a340483f03ba47bb1a039b2c519aab3df85 # v3.37.8 with: category: "/language:javascript-typescript" diff --git a/.github/workflows/dependency-review.yml b/.github/workflows/dependency-review.yml index 460031c..b040b64 100644 --- a/.github/workflows/dependency-review.yml +++ b/.github/workflows/dependency-review.yml @@ -13,5 +13,5 @@ jobs: dependency-review: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7 - - uses: actions/dependency-review-action@v4 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: actions/dependency-review-action@2031cfc080254a8a887f58cffee85186f0e49e48 # v4.9.0 diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index fe9b716..006e846 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -35,15 +35,15 @@ jobs: outputs: channel: ${{ steps.channel.outputs.channel }} steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 # No registry-url here: it writes an .npmrc auth-token line with a # placeholder value, and npm then authenticates with that instead of # falling through to OIDC trusted publishing. - - uses: actions/setup-node@v7 + - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 with: node-version: 24 - # Trusted publishing needs npm 11.5.1 or newer. - - run: npm install -g npm@latest + # Trusted publishing needs npm 11.5.1 or newer; node 24 has bundled a + # new-enough npm since 24.4, so nothing extra is installed here. - run: npm ci - run: npm test - name: pick channel and version @@ -103,7 +103,7 @@ jobs: if: needs.publish.outputs.channel == 'latest' runs-on: ubuntu-latest steps: - - uses: actions/setup-node@v7 + - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 with: node-version: 24 # Same invocation the install-loop experiment proved out, telemetry left diff --git a/.github/workflows/scorecard.yml b/.github/workflows/scorecard.yml index 8d5acfa..5e3500a 100644 --- a/.github/workflows/scorecard.yml +++ b/.github/workflows/scorecard.yml @@ -22,7 +22,7 @@ jobs: actions: read steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: persist-credentials: false @@ -32,12 +32,12 @@ jobs: results_format: sarif publish_results: true - - uses: actions/upload-artifact@v4 + - uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 with: name: SARIF file path: results.sarif retention-days: 5 - - uses: github/codeql-action/upload-sarif@v3 + - uses: github/codeql-action/upload-sarif@42947a340483f03ba47bb1a039b2c519aab3df85 # v3.37.8 with: sarif_file: results.sarif diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 0000000..65258f5 --- /dev/null +++ b/SECURITY.md @@ -0,0 +1,24 @@ +# Security + +## Reporting a vulnerability + +Report vulnerabilities privately through GitHub: [Security > Report a +vulnerability](https://github.com/only-cli/oc/security/advisories/new). +Please do not open a public issue for anything exploitable. + +Expect an acknowledgement within a week. Fixes ship as a patch release, +and the advisory is published once the fix is out. + +## Scope + +oc fetches untrusted web pages by design, so the interesting bugs are the +ones where page content escapes its role as data: rendered text that can +alter what an agent executes, URLs that reach private or internal hosts +despite the SSRF guard, or a crafted page that breaks the distiller. Bugs +in the experiments/ directory are out of scope; nothing there ships in +the package. + +## Supported versions + +Only the latest release on npm is supported. There is no backporting; a +security fix means a new release. diff --git a/experiments/skills-install-remove-loop/Dockerfile b/experiments/skills-install-remove-loop/Dockerfile index 4c6ce48..b559398 100644 --- a/experiments/skills-install-remove-loop/Dockerfile +++ b/experiments/skills-install-remove-loop/Dockerfile @@ -1,4 +1,4 @@ -FROM node:24-bookworm-slim +FROM node:24-bookworm-slim@sha256:3638d9a6fe4030bd716be989438248074489337ba3275657f93595428be4fc03 RUN apt-get update \ && apt-get install --yes --no-install-recommends ca-certificates git \ diff --git a/src/act.js b/src/act.js index b61a0a7..fc8e463 100644 --- a/src/act.js +++ b/src/act.js @@ -136,6 +136,15 @@ export function read(n, { session = DEFAULT_SESSION, budget = 2000 } = {}) { if (!line) continue; const cost = estimateTokens(line) + 1; if (spent + cost > budget && lines.length) break; + // The first line always prints so read never answers with nothing, but + // its text is the page's to write and so has no natural size. Alone over + // budget it still gets cut: 'up to N tokens' is a promise the page must + // not be able to break. + if (!lines.length && cost > budget) { + lines.push(`${line.slice(0, budget * 4)} ... cut at ~${budget} tokens, raise --budget for the rest`); + spent += budget; + continue; + } spent += cost; lines.push(line); } diff --git a/src/cli.js b/src/cli.js index 4e9eec8..62dfd31 100755 --- a/src/cli.js +++ b/src/cli.js @@ -193,8 +193,12 @@ async function main() { // Only the blank case here. `raw` is the fallback the compact view's // failure line names, so it must not fail on the same pages: a page // whose only text is its menu still has markup, and printing it is the - // whole point of `raw`. - if (outTokens < MIN_CONTENT) noContent(finalUrl, `~${outTokens} tokens of markdown`, ''); + // whole point of `raw`. And a short page that arrived short is not + // blank, so the verdict needs the same evidence the compact view asks + // for: near-nothing distilled out of markup that promised more. + if (outTokens < MIN_CONTENT && contentFailure(outTokens, htmlTokens)) { + noContent(finalUrl, `~${outTokens} tokens of markdown`, ''); + } return; } const page = distill(html, finalUrl); diff --git a/src/fetch.js b/src/fetch.js index 37ea8a4..1ebd76f 100644 --- a/src/fetch.js +++ b/src/fetch.js @@ -35,6 +35,29 @@ const PROXY_TIMEOUT_MS = 300_000; // pages of mojibake an agent then pays for, so it is refused by name instead. const READABLE_TYPE = /^\s*(?:text\/|application\/(?:json|xml|javascript|x-ndjson|[\w.+-]*\+(?:json|xml)))/i; +// The whole decoded body is buffered before the distiller sees it, so an +// unbounded response is an unbounded allocation, and a URL is often the +// page's to name, not the caller's. The cap is generous because oc fetches +// some large corpora on purpose (the Node.js docs reference is 8.5MB +// decoded); three times that and a response is not a page anyone reads. +// Content-Length rejects a known-large response before its bytes arrive, but +// the header is optional and untrusted, so every transport also counts what +// actually lands, after decoding, which is what stops a decompression bomb. +export const MAX_BODY = 25 * 1024 * 1024; + +/** + * Refuse a body larger than oc will buffer. Called on the Content-Length + * header first and again on the bytes as they arrive, since only the second + * count is trustworthy. + * @param {number} size - bytes seen so far, or claimed by the header + * @param {string} url + */ +export function assertBodySize(size, url) { + if (size > MAX_BODY) { + throw new Error(`response body over ${MAX_BODY / 1048576}MB for ${url}, more than oc will read`); + } +} + /** * Refuse a response oc cannot read as text. Both transports call this: the * gate has to live on whichever client got the page, or the same URL renders @@ -310,7 +333,18 @@ function wrapNodeResponse(res, url) { }; const text = () => new Promise((resolve, reject) => { const chunks = []; - res.on('data', (c) => chunks.push(c)); + let size = 0; + res.on('data', (c) => { + size += c.length; + try { + assertBodySize(size, url); + } catch (err) { + // destroy surfaces the refusal through 'error', and stops the read. + res.destroy(err); + return; + } + chunks.push(c); + }); res.on('end', () => resolve(Buffer.concat(chunks).toString('utf8'))); res.on('error', reject); }); @@ -507,7 +541,12 @@ async function viaImpers(impers, target) { } if (status >= 400) throw new Error(`fetch failed: ${status} for ${target}`); assertReadableType(res.headers.get('content-type')); + assertBodySize(Number(res.headers.get('content-length')) || 0, target); + // impers buffers inside its own binding, so the size of what it already + // holds is all there is to check; the bound still stops an oversized body + // from travelling any further. const html = typeof res.text === 'function' ? await res.text() : String(res.text ?? res.body ?? ''); + assertBodySize(html.length, target); return { url: res.url ?? target, html, status, via }; } @@ -522,5 +561,27 @@ async function viaFetch(target) { throw new Error(`fetch failed: ${res.status} ${res.statusText} for ${current}`); } assertReadableType(res.headers.get('content-type')); - return { url: res.url || current, html: await res.text(), status: res.status, via: 'fetch' }; + assertBodySize(Number(res.headers.get('content-length')) || 0, current); + return { url: res.url || current, html: await readBody(res, current), status: res.status, via: 'fetch' }; +} + +/** + * The decoded body as text, counted as it arrives so crossing the cap aborts + * the transfer instead of finishing it. Throwing mid-iteration cancels the + * stream. A proxy response has no web stream to iterate; its text() counts + * inside wrapNodeResponse instead. + * @param {any} res + * @param {string} url + * @returns {Promise} + */ +export async function readBody(res, url) { + if (!res.body?.getReader) return res.text(); + const chunks = []; + let size = 0; + for await (const chunk of res.body) { + size += chunk.byteLength; + assertBodySize(size, url); + chunks.push(chunk); + } + return Buffer.concat(chunks).toString('utf8'); } diff --git a/src/render.js b/src/render.js index 54f17d2..4d4d74d 100644 --- a/src/render.js +++ b/src/render.js @@ -35,8 +35,9 @@ const num = (v) => v.toLocaleString('en-US'); // which is what tells a link-list page (Hacker News, search results) from a // page whose only links are its own menu. const CONTENT_LABEL = 25; -// Below this there is nothing to read whatever the page is, so how much markup -// it arrived in does not matter. +// Below this a render is suspiciously thin, but thin is only a verdict when +// the page's own size says there should have been more. A terse page that +// arrived terse (a status endpoint, a one-line answer) distilled fine. export const MIN_CONTENT = 25; // Below this, with markup that large behind it, the fetch worked and the render // did not: a real page of that weight always distills to more. A genuinely @@ -65,7 +66,11 @@ export const contentTokens = (page) => * @returns {string|null} */ export function contentFailure(content, htmlTokens) { - if (content < MIN_CONTENT) return `~${content} tokens of text on the whole page`; + // Nothing extracted is empty whatever the page weighed. Anything more is + // only a failure with evidence: a small page that renders small is not + // gated, it is small, and exit 2 on it would send an agent to a browser + // for a page it was already holding. + if (content === 0) return 'no text on the whole page'; if (content < THIN_CONTENT && htmlTokens > THIN_HTML) { return `~${content} tokens of text out of ~${htmlTokens} of HTML`; } @@ -92,7 +97,7 @@ export const FINISH = 4; */ export function render(page, { budget = 500, from = 0 } = {}) { const blocks = collapseRuns(page.blocks); - const head = page.title ? [from > 0 ? `# ${page.title} (continued)` : `# ${page.title}`] : []; + const head = page.title ? [from > 0 ? `# ${truncate(page.title)} (continued)` : `# ${truncate(page.title)}`] : []; const lines = [...head]; let spent = estimateTokens(lines.join('\n')); let hasLinks = false; @@ -204,13 +209,13 @@ export function formatBlock(b, { full = false } = {}) { const tag = b.n == null ? '' : `[${b.n}] `; switch (b.type) { case 'heading': - return `${'#'.repeat(Math.min(b.level ?? 2, 3))} ${tag}${b.text}`; + return `${'#'.repeat(Math.min(b.level ?? 2, 3))} ${tag}${full ? b.text : truncate(b.text)}`; case 'link': return `${tag}${full ? b.text : truncate(b.text)}`; case 'button': return `${tag}button "${full ? b.text : truncate(b.text)}"`; case 'input': - return `${tag}input ${b.name} (${b.text})`; + return `${tag}input ${truncate(b.name ?? '')} (${truncate(b.text ?? '')})`; case 'divider': return b.text; default: diff --git a/tests/act.test.js b/tests/act.test.js index c755536..5242f69 100644 --- a/tests/act.test.js +++ b/tests/act.test.js @@ -21,6 +21,19 @@ const open = (name = 'default', budget = 500) => { saveSession(name, sessionFromPage(p, loadSession(name), { cursor: render(p, { budget }).stats.next })); }; +test("read cuts even a first block bigger than its whole budget", () => { + // The first line of a read always prints, but its text is the page's to + // write, so alone-over-budget still cuts: 'up to N tokens' is a promise the + // page must not be able to break. + const wall = 'sentence after sentence of the same thing. '.repeat(500); + const p = distill(`

${wall}

`, 'https://example.test/wall'); + saveSession('wall', sessionFromPage(p, null, { cursor: null })); + const n = p.blocks.find((b) => b.type === 'text').n; + const out = read(n, { session: 'wall', budget: 100 }); + assert.ok(out.length < 100 * 4 + 200, `read printed ${out.length} chars against a budget of 100 tokens`); + assert.match(out, /cut at ~100 tokens, raise --budget/); +}); + test('a rendered page is remembered with absolute URLs for every handle', () => { open(); const state = loadSession('default'); diff --git a/tests/distill.test.js b/tests/distill.test.js index d8f99df..c2f3957 100644 --- a/tests/distill.test.js +++ b/tests/distill.test.js @@ -490,13 +490,13 @@ test('a page that arrives with no readable text is reported as a failure', () => }; // Nothing at all, whatever the page weighed. - assert.match(verdict('
', 0), /~0 tokens of text on the whole page/); + assert.match(verdict('
', 0), /no text on the whole page/); // Menu links only: short labels are furniture, so this page has no content // either, however much markup came with it. const chrome = ['Help', 'Log in', 'Content Policy', 'About', 'Careers', 'Press'] .map((t) => `${t}`).join(''); - assert.match(verdict(chrome, 60_000), /~0 tokens of text on the whole page/); + assert.match(verdict(chrome, 60_000), /no text on the whole page/); // A consent wall or a login gate: a sentence or two of real text, out of // markup far too big to have carried only that. @@ -508,6 +508,36 @@ test('a page that arrives with no readable text is reported as a failure', () => assert.equal(verdict(gate, 0), null); }); +test('a terse page that arrived terse is content, not a failed render', () => { + // A status endpoint or a one-line answer distills fine and has to exit 0: + // calling it gated would send an agent to a browser for a page it was + // already holding. Only weight it never rendered is evidence of a gate. + const html = 'status

All systems operational.

'; + const page = distill(html, 'https://fixture.test/status'); + assert.equal(contentFailure(contentTokens(page), estimateTokens(html)), null); + const json = distill('{"status":"ok"}', 'https://fixture.test/health'); + assert.equal(contentFailure(contentTokens(json), 4), null); +}); + +test('page-written scalars are capped at the render boundary', () => { + // The title and every heading are the page's to write, so without a cap one + // hostile scalar prints unbounded output whatever the budget says. + const bigTitle = 'title word '.repeat(1000).trim(); + const bigHeading = 'heading word '.repeat(1000).trim(); + const page = distill( + `${bigTitle}

${bigHeading}

short

`, + 'https://fixture.test/big'); + const { text } = render(page, { budget: 100 }); + for (const line of text.split('\n')) { + assert.ok(line.length < 300, `a render line ran to ${line.length} chars`); + } + assert.match(text, /\.\.\. \+[\d,]+ chars/); + // The distilled page keeps the full values: --json is the machine-stable + // view, its size is bounded by the fetch cap, and machines cut for + // themselves. + assert.equal(page.title, bigTitle); +}); + test('a link-list page counts as content even with no prose on it', () => { // Hacker News and search results are links and nothing else, so a rule that // counted only prose would call the tool's best pages empty. diff --git a/tests/fetch.test.js b/tests/fetch.test.js index ae5584c..8963003 100644 --- a/tests/fetch.test.js +++ b/tests/fetch.test.js @@ -530,3 +530,41 @@ test('proxyGet refuses a non-HTTP proxy scheme', () => { /unsupported proxy protocol \(socks5\)/, ); }); + +test('a body over the cap is refused, from the header or from the bytes', async () => { + const { assertBodySize, readBody, MAX_BODY } = await import('../src/fetch.js'); + + // The header check catches a response honest about its size early. + assert.doesNotThrow(() => assertBodySize(MAX_BODY, 'https://example.test/big')); + assert.throws(() => assertBodySize(MAX_BODY + 1, 'https://example.test/big'), /more than oc will read/); + + // The header is optional and untrusted, so the stream is counted too: a + // chunked response crossing the cap fails deterministically, and one just + // below it arrives whole. + const mb = new Uint8Array(1024 * 1024).fill(120); + const stream = (chunks) => new Response(new ReadableStream({ + start(c) { + for (let i = 0; i < chunks; i++) c.enqueue(mb); + c.close(); + }, + })); + await assert.rejects(() => readBody(stream(26), 'https://example.test/bomb'), /more than oc will read/); + const small = await readBody(stream(2), 'https://example.test/fine'); + assert.equal(small.length, 2 * 1024 * 1024); +}); + +test('the proxy transport counts the body against the same cap', async () => { + const proxy = http.createServer((req, res) => { + res.writeHead(200, { 'content-type': 'text/html' }); + const mb = Buffer.alloc(1024 * 1024, 'x'); + for (let i = 0; i < 26; i++) res.write(mb); + res.end(); + }); + const port = await listen(proxy); + try { + const res = await proxyGet('http://example.test/bomb', `http://127.0.0.1:${port}`); + await assert.rejects(() => res.text(), /more than oc will read/); + } finally { + proxy.close(); + } +});