diff --git a/bin/install.js b/bin/install.js index c5878f4..cae2e6a 100644 --- a/bin/install.js +++ b/bin/install.js @@ -319,15 +319,20 @@ async function promptGlobalOrLocal(c) { // ── Local install helpers ───────────────────────────────────────────────── -// GET a raw text resource over HTTPS, following one level of redirects. -// Resolves to the body string, or null on any non-200/error/timeout. +// GET a raw text resource over HTTPS, following redirects up to MAX_REDIRECTS. +// Resolves to the body string, or null on any non-200/error/timeout, on a +// missing Location header, or when the redirect budget is exhausted (guards +// against redirect loops / unbounded recursion). +const MAX_REDIRECTS = 5; function fetchRawText(url) { return new Promise((resolve) => { - const get = (u) => { + const get = (u, redirectsLeft) => { const req = https.get(u, (res) => { if (res.statusCode === 301 || res.statusCode === 302) { - get(res.headers.location); res.resume(); + const next = res.headers.location; + if (!next || redirectsLeft <= 0) { resolve(null); return; } + get(next, redirectsLeft - 1); return; } if (res.statusCode !== 200) { resolve(null); return; } @@ -338,7 +343,7 @@ function fetchRawText(url) { req.on('error', () => resolve(null)); req.setTimeout(10000, () => { req.destroy(new Error('timeout')); }); }; - get(url); + get(url, MAX_REDIRECTS); }); } @@ -641,4 +646,4 @@ if (require.main === module) { } // Exported for tests (tests/install.test.js, tests/test_installer.py) — kept minimal. -module.exports = { loadTaskSkillSlugs, slugsFromTasksJson }; +module.exports = { loadTaskSkillSlugs, slugsFromTasksJson, fetchRawText }; diff --git a/tests/install.test.js b/tests/install.test.js index c103065..24af9e8 100644 --- a/tests/install.test.js +++ b/tests/install.test.js @@ -18,7 +18,36 @@ const { spawnSync } = require('node:child_process'); const REPO_ROOT = path.resolve(__dirname, '..'); const INSTALL_JS = path.join(REPO_ROOT, 'bin', 'install.js'); -const { slugsFromTasksJson, loadTaskSkillSlugs } = require(INSTALL_JS); +const https = require('node:https'); + +const { slugsFromTasksJson, loadTaskSkillSlugs, fetchRawText } = require(INSTALL_JS); + +// Swap https.get for a fake; returns a restore fn. install.js holds the same +// cached https module object, so mutating .get here is visible to fetchRawText. +function stubHttpsGet(handler) { + const real = https.get; + https.get = handler; + return () => { https.get = real; }; +} + +// Build a minimal response/request pair good enough for fetchRawText. +function fakeExchange({ statusCode, location, body }) { + return (_url, cb) => { + const res = { + statusCode, + headers: location === undefined ? {} : { location }, + resume() {}, + on(ev, fn) { + if (ev === 'data' && body != null) process.nextTick(() => fn(body)); + if (ev === 'end') process.nextTick(fn); + return res; + }, + }; + process.nextTick(() => cb(res)); + const req = { on() { return req; }, setTimeout() { return req; }, destroy() {} }; + return req; + }; +} test('slugsFromTasksJson lists the canonical skill slug first', () => { const slugs = slugsFromTasksJson(JSON.stringify({ @@ -75,6 +104,53 @@ test('slugsFromTasksJson throws when canonical_skill_slug is missing', () => { }))); }); +test('fetchRawText resolves null on a redirect loop instead of recursing forever', async () => { + // Every request 301s back to the same URL. Without a cap this overflows the + // stack / fans out requests unbounded; with a cap it must resolve null. + const restore = stubHttpsGet(fakeExchange({ statusCode: 301, location: 'https://x/loop' })); + try { + const out = await fetchRawText('https://x/loop'); + assert.equal(out, null); + } finally { + restore(); + } +}); + +test('fetchRawText follows a finite redirect chain within the cap and returns the body', async () => { + // 3 hops (< cap of 5) then a 200 with a body. + let hops = 0; + const restore = stubHttpsGet((_url, cb) => { + const step = hops++; + const res = { + statusCode: step < 3 ? 302 : 200, + headers: step < 3 ? { location: `https://x/hop${step}` } : {}, + resume() {}, + on(ev, fn) { + if (ev === 'data' && step >= 3) process.nextTick(() => fn('PAYLOAD')); + if (ev === 'end') process.nextTick(fn); + return res; + }, + }; + process.nextTick(() => cb(res)); + const req = { on() { return req; }, setTimeout() { return req; }, destroy() {} }; + return req; + }); + try { + assert.equal(await fetchRawText('https://x/start'), 'PAYLOAD'); + } finally { + restore(); + } +}); + +test('fetchRawText resolves null on a redirect with no Location header', async () => { + const restore = stubHttpsGet(fakeExchange({ statusCode: 302, location: undefined })); + try { + assert.equal(await fetchRawText('https://x/'), null); + } finally { + restore(); + } +}); + test('loadTaskSkillSlugs reads the real tasks.json from a local checkout', async () => { const slugs = await loadTaskSkillSlugs(REPO_ROOT); assert.ok(Array.isArray(slugs) && slugs.length > 0, 'expected a non-empty slug list');