diff --git a/CLAUDE.md b/CLAUDE.md index 18b93f4..ade2031 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -46,7 +46,7 @@ GitHub Actions runs the bundled `dist/index.js` directly (see `action.yml` → ` Key design seams to preserve when editing: - **`ReleaseApi` interface (`version.ts`)** is the network seam. `version.ts` holds only pure, testable resolution logic; `github.ts` (`createReleaseApi`) is the **only** module that performs HTTP. Keep network access out of `version.ts` so its tests stay HTTP-free. -- **Reliability core (`github.ts` `fetchJson`)**: rejects non-JSON bodies as errors. This is the project's reason for existing — unauthenticated GitHub returns HTML rate-limit pages that broke `arduino/setup-task`. Treat that case as *transient* (retryable) and surface a message pointing at `repo-token`. +- **Reliability core (`github.ts` `fetchJson`)**: rejects non-JSON bodies as errors. This is the project's reason for existing — unauthenticated GitHub returns HTML rate-limit pages that broke `arduino/setup-task`. Treat that case as *transient* (retryable) and surface a message pointing at `repo-token`. Bodies are read via `readCappedText` (byte cap, `MAX_RESPONSE_BYTES`) and every request carries an `AbortSignal.timeout` (`REQUEST_TIMEOUT_MS`) so a hijacked/hung trusted host can't exhaust memory or wedge the action (NFR-1) — a size overrun is permanent, a timeout transient. - **Retry vs. permanent (`download.ts` `withRetry` + `errors.ts` `PermanentError`)**: `withRetry` does exponential backoff, but `PermanentError` (and HTTP 404, including tool-cache's `httpStatusCode === 404`) bail out immediately with no retry. When adding a new failure mode, decide deliberately which side it falls on — checksum mismatches and 404s are permanent; everything network-ish is transient. - **Platform mapping (`platform.ts`)**: maps Node `process.platform`/`process.arch` to go-task's asset naming (`task__.`) and validates against a hardcoded `SUPPORTED` matrix mirroring go-task's published assets. Update `SUPPORTED` if upstream adds/drops a target. - **`constants.ts`** centralizes the upstream repo (`go-task/task`), the `task_checksums.txt` asset name, URL builders, and retry tuning. diff --git a/TODO.md b/TODO.md index be66cc5..17f6f03 100644 --- a/TODO.md +++ b/TODO.md @@ -53,7 +53,6 @@ | [#41](https://github.com/yk-lab/setup-task/issues/41) | `[test]` platform.test.ts を §9 全 os/arch 組合せに拡張 | `P3: low` | `test` | | [#44](https://github.com/yk-lab/setup-task/issues/44) | `[enhancement]` フォールバックソースをサポート(FR-11) | `P3: low` | `enhancement` | | [#45](https://github.com/yk-lab/setup-task/issues/45) | `[chore]` Biome 導入を評価 | `P3: low` | `chore` | -| [#56](https://github.com/yk-lab/setup-task/issues/56) | `[security]` 取得ボディにサイズ上限/タイムアウトを設ける | `P3: low` | `security` | | ### 未 Issue 化 @@ -81,3 +80,4 @@ v1.0.0 実装中に作成・解決済みの Issue(参考)。 | [#54](https://github.com/yk-lab/setup-task/issues/54) | `[enhancement]` HTTP(S) proxy 環境で全 fetch を proxy 経由にする | #57 | | [#55](https://github.com/yk-lab/setup-task/issues/55) | `[documentation]` テスト規約(CLAUDE.md)を stub-fetch unit test の実態に合わせる | #58 | | [#59](https://github.com/yk-lab/setup-task/issues/59) | `[ci]` paths-filter が root 直下の .md を docs 扱いせずフル CI が走る | #60 | +| [#56](https://github.com/yk-lab/setup-task/issues/56) | `[security]` 取得ボディにサイズ上限/タイムアウトを設ける | #61 | diff --git a/src/constants.ts b/src/constants.ts index 8e870ff..d4a1a22 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -18,6 +18,14 @@ export const DEFAULT_RETRY_BASE_MS = 1000; /** Cap on redirects followed while validating each hop's host (NFR-1). */ export const MAX_REDIRECTS = 5; +/** + * Per-request timeout and response-body size cap (NFR-1): bound how long a fetch + * may hang and how much a (possibly hijacked) trusted host may stream. Both are + * generous — the API pages and `task_checksums.txt` sit far below the cap. + */ +export const REQUEST_TIMEOUT_MS = 30_000; +export const MAX_RESPONSE_BYTES = 16 * 1024 * 1024; + /** * Build the download URL for a release asset. * Tags on go-task are `v`-prefixed (e.g. v3.51.1). diff --git a/src/download.ts b/src/download.ts index eed2391..4e4217e 100644 --- a/src/download.ts +++ b/src/download.ts @@ -59,7 +59,7 @@ export async function withRetry(fn: () => Promise, opts: RetryOptions = {} export async function downloadAsset(url: string, token?: string): Promise { // Vet the redirect chain's hosts before tool-cache (which follows redirects // opaquely) fetches the body (NFR-1). Only an untrusted host (PermanentError) - // is fatal; a transient network failure in the preflight falls through to the + // is fatal; a preflight that simply couldn't complete falls through to the // checksum-verified tool-cache download rather than blocking it. try { await assertRedirectTrusted(url, token); @@ -67,12 +67,20 @@ export async function downloadAsset(url: string, token?: string): Promise): Record { export async function secureFetch(url: string, headers: Record): Promise { let currentUrl = url; let currentHeaders = headers; + // One deadline for the whole operation — redirects plus the final body read + // (the returned response's body stays bound to this signal). A hang aborts + // and surfaces as a transient error withRetry can retry (NFR-1). + const signal = AbortSignal.timeout(REQUEST_TIMEOUT_MS); for (let hop = 0; hop <= MAX_REDIRECTS; hop++) { assertAllowedHost(currentUrl, 'request'); - const resp = await fetch(currentUrl, { headers: currentHeaders, redirect: 'manual' }); + const resp = await fetch(currentUrl, { headers: currentHeaders, redirect: 'manual', signal }); if (!REDIRECT_STATUSES.has(resp.status)) { return resp; @@ -121,18 +132,51 @@ function guardContentType(resp: Response, url: string, isExpected: (contentType: } } +/** + * Read a response body as UTF-8 text, refusing bodies over `maxBytes` (NFR-1): + * `resp.text()` buffers an unbounded stream, so a hijacked trusted host could + * exhaust memory. Count bytes as they arrive and bail (PermanentError — retrying + * just re-streams the same oversized body). + */ +export async function readCappedText( + resp: Response, + url: string, + maxBytes: number = MAX_RESPONSE_BYTES, +): Promise { + if (!resp.body) { + return ''; + } + const reader = resp.body.getReader(); + const chunks: Uint8Array[] = []; + let total = 0; + for (;;) { + const { done, value } = await reader.read(); + if (done) { + break; + } + total += value.byteLength; + if (total > maxBytes) { + await reader.cancel().catch(() => undefined); // don't let a cancel error mask the limit + throw new PermanentError(`Response from ${url} exceeded the ${maxBytes}-byte limit.`); + } + chunks.push(value); + } + return Buffer.concat(chunks).toString('utf-8'); +} + /** Fetch and parse JSON, requiring a JSON content-type (FR-3). */ export async function fetchJson(url: string, token?: string): Promise { const resp = await fetchOk(url, token); guardContentType(resp, url, (contentType) => contentType.includes('json')); - return (await resp.json()) as T; + // Strip a leading BOM the way resp.json() did — JSON.parse rejects it. + return JSON.parse((await readCappedText(resp, url)).replace(/^\uFEFF/, '')) as T; } /** Fetch a text body (the checksums file), rejecting HTML error pages (FR-3). */ export async function fetchText(url: string, token?: string): Promise { const resp = await fetchOk(url, token); guardContentType(resp, url, (contentType) => !contentType.includes('html')); - return resp.text(); + return readCappedText(resp, url); } /** GitHub-backed ReleaseApi implementation. */ diff --git a/tests/download.test.ts b/tests/download.test.ts index e7c1cdf..3f48674 100644 --- a/tests/download.test.ts +++ b/tests/download.test.ts @@ -104,6 +104,16 @@ describe('downloadAsset (redirect preflight wiring, NFR-1)', () => { expect(tc.downloadTool).toHaveBeenCalledOnce(); }); + it('falls through to tool-cache on a preflight timeout (DOMException)', async () => { + // AbortSignal.timeout rejects with a DOMException, not a TypeError — it must + // also be treated as "preflight couldn't run", not block the download. + vi.mocked(assertRedirectTrusted).mockRejectedValueOnce( + new DOMException('The operation timed out', 'TimeoutError'), + ); + await expect(downloadAsset(ASSET_URL, 'tok')).resolves.toBe('/tmp/task-archive'); + expect(tc.downloadTool).toHaveBeenCalledOnce(); + }); + it('propagates an unexpected (non-network) preflight error', async () => { vi.mocked(assertRedirectTrusted).mockRejectedValueOnce(new Error('unexpected')); await expect(downloadAsset(ASSET_URL, 'tok')).rejects.toThrow('unexpected'); diff --git a/tests/github.test.ts b/tests/github.test.ts index 3a300f3..9045f1d 100644 --- a/tests/github.test.ts +++ b/tests/github.test.ts @@ -6,6 +6,7 @@ import { createReleaseApi, fetchJson, fetchText, + readCappedText, secureFetch, } from '../src/github'; @@ -48,6 +49,11 @@ describe('fetchJson (content-type guard, FR-3)', () => { await expect(fetchJson(API_URL)).resolves.toEqual({ tag_name: 'v3.51.1' }); }); + it('strips a leading BOM before parsing (as resp.json() did)', async () => { + stubFetch(`\uFEFF${JSON.stringify({ tag_name: 'v3.51.1' })}`, { contentType: 'application/json' }); + await expect(fetchJson(API_URL)).resolves.toEqual({ tag_name: 'v3.51.1' }); + }); + it('rejects an HTML rate-limit page as a retryable Error pointing at repo-token', async () => { // 200 OK but non-JSON body = the "Unicorn" page. Must throw (transient), // not be parsed, so withRetry() can retry it. @@ -166,6 +172,28 @@ describe('secureFetch (redirect host validation, NFR-1)', () => { mockedFetch.mockImplementation(async () => redirectTo('https://evil.example.com/asset')); await expect(assertRedirectTrusted(ASSET_URL, 'tok')).rejects.toBeInstanceOf(PermanentError); }); + + it('passes an abort signal (per-request timeout) to fetch (NFR-1)', async () => { + mockedFetch.mockResolvedValueOnce(new Response('ok', { status: 200 })); + await secureFetch(CHECKSUMS_URL, {}); + expect(mockedFetch).toHaveBeenCalledWith( + CHECKSUMS_URL, + expect.objectContaining({ signal: expect.any(AbortSignal) }), + ); + }); +}); + +describe('readCappedText (response size cap, NFR-1)', () => { + it('returns the body when under the cap', async () => { + await expect(readCappedText(new Response('hello'), CHECKSUMS_URL, 100)).resolves.toBe('hello'); + }); + + it('throws PermanentError when the streamed body exceeds the cap', async () => { + // The streamed byte counter is the defence against an unbounded/hijacked body. + await expect( + readCappedText(new Response('abcdefghij'), CHECKSUMS_URL, 4), + ).rejects.toBeInstanceOf(PermanentError); + }); }); describe('createReleaseApi', () => {