mirror of
https://github.com/tiennm99/ccs.git
synced 2026-07-17 20:17:20 +00:00
fix(cli): harden ccs bar install + align launch path
Resolve the app path via os.homedir() in launch to match install/uninstall; validate the host on every redirect hop (manual follow, not blind maxRedirections); reject zip-slip entries before extracting into ~/Applications.
This commit is contained in:
@@ -145,13 +145,17 @@ async function defaultFetchReleaseAsset(tag: string, asset: string): Promise<Rel
|
|||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Download a zip from `url` (following up to 5 GitHub 302 redirects) and
|
* Download a zip from `url` (following up to 5 GitHub 302 redirects, re-validating
|
||||||
* extract its contents into `dest`.
|
* each hop's Location hostname) and extract its contents into `dest`.
|
||||||
*
|
*
|
||||||
* Fixes applied:
|
* Fixes applied:
|
||||||
* - Finding #8: pass `maxRedirections: 5` so GitHub's 302 → objects.githubusercontent.com is followed.
|
* - Fix #6: maxRedirections:0 + manual redirect following so every hop's Location
|
||||||
|
* header is passed through validateDownloadUrl before following it.
|
||||||
|
* The previous maxRedirections:5 let undici follow redirects to ANY host unchecked.
|
||||||
|
* - Fix #14: list zip entries with `unzip -l` before extracting; reject the archive
|
||||||
|
* if any entry path contains ".." or starts with "/" (zip-slip guard).
|
||||||
* - Finding #11: check `statusCode` and throw a descriptive error before streaming.
|
* - Finding #11: check `statusCode` and throw a descriptive error before streaming.
|
||||||
* - Finding #9: validate host+HTTPS before making the request.
|
* - Finding #9: validate host+HTTPS before making the first request.
|
||||||
*/
|
*/
|
||||||
async function defaultDownloadAndExtract(url: string, dest: string): Promise<void> {
|
async function defaultDownloadAndExtract(url: string, dest: string): Promise<void> {
|
||||||
const { request } = await import('undici');
|
const { request } = await import('undici');
|
||||||
@@ -162,34 +166,107 @@ async function defaultDownloadAndExtract(url: string, dest: string): Promise<voi
|
|||||||
const { execFile } = await import('child_process');
|
const { execFile } = await import('child_process');
|
||||||
const execFileAsync = promisify(execFile);
|
const execFileAsync = promisify(execFile);
|
||||||
|
|
||||||
// Finding #9: validate host + HTTPS before any network request.
|
// Validate initial URL (Finding #9)
|
||||||
validateDownloadUrl(url);
|
validateDownloadUrl(url);
|
||||||
|
|
||||||
mkdirSync(dest, { recursive: true });
|
mkdirSync(dest, { recursive: true });
|
||||||
const tmpZip = path.join(os.tmpdir(), `ccs-bar-${Date.now()}.zip`);
|
|
||||||
|
|
||||||
// Finding #8: maxRedirections:5 ensures GitHub's 302 to objects.githubusercontent.com is followed.
|
// Fix #6: follow redirects manually so each hop is re-validated.
|
||||||
// Finding #11: destructure statusCode and reject non-200 before streaming.
|
const MAX_REDIRECTS = 5;
|
||||||
const { statusCode, body } = await request(url, {
|
let currentUrl = url;
|
||||||
maxRedirections: 5,
|
let redirectsFollowed = 0;
|
||||||
});
|
|
||||||
|
|
||||||
if (statusCode !== 200) {
|
while (true) {
|
||||||
throw new Error(`Download failed: HTTP ${statusCode} for ${url}`);
|
const { statusCode, headers, body } = await request(currentUrl, {
|
||||||
}
|
maxRedirections: 0, // disable undici's auto-follow; we follow manually
|
||||||
|
});
|
||||||
|
|
||||||
// Stream body to tmpZip — undici body is a Node ReadableStream
|
if (statusCode >= 300 && statusCode < 400) {
|
||||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
const location = Array.isArray(headers['location'])
|
||||||
await streamPipeline(body as any, createWriteStream(tmpZip));
|
? headers['location'][0]
|
||||||
|
: headers['location'];
|
||||||
|
|
||||||
// Extract the zip into dest
|
if (!location) {
|
||||||
await execFileAsync('unzip', ['-o', tmpZip, '-d', dest]);
|
throw new Error(`Redirect (HTTP ${statusCode}) from ${currentUrl} has no Location header`);
|
||||||
|
}
|
||||||
|
|
||||||
// Clean up the temp archive
|
// Resolve relative redirects against the current URL
|
||||||
try {
|
const resolved = new URL(location, currentUrl).toString();
|
||||||
fs.unlinkSync(tmpZip);
|
|
||||||
} catch {
|
// Re-validate the redirect target — this is the key fix for #6
|
||||||
/* ignore */
|
validateDownloadUrl(resolved);
|
||||||
|
|
||||||
|
if (redirectsFollowed >= MAX_REDIRECTS) {
|
||||||
|
throw new Error(`Too many redirects (>${MAX_REDIRECTS}) while downloading ${url}`);
|
||||||
|
}
|
||||||
|
|
||||||
|
// Drain the body to free the socket before following the redirect
|
||||||
|
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||||
|
await (body as any).dump?.();
|
||||||
|
currentUrl = resolved;
|
||||||
|
redirectsFollowed++;
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
|
||||||
|
if (statusCode !== 200) {
|
||||||
|
throw new Error(`Download failed: HTTP ${statusCode} for ${currentUrl}`);
|
||||||
|
}
|
||||||
|
|
||||||
|
const tmpZip = path.join(os.tmpdir(), `ccs-bar-${Date.now()}.zip`);
|
||||||
|
|
||||||
|
// Stream body to tmpZip
|
||||||
|
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||||
|
await streamPipeline(body as any, createWriteStream(tmpZip));
|
||||||
|
|
||||||
|
// Fix #14: zip-slip guard — inspect entries before extraction.
|
||||||
|
// `unzip -l` lists entries in a machine-readable format; we scan for ".." or
|
||||||
|
// absolute paths that would escape the destination directory.
|
||||||
|
try {
|
||||||
|
const { stdout: listing } = await execFileAsync('unzip', ['-l', tmpZip]);
|
||||||
|
const lines = listing.split('\n');
|
||||||
|
for (const line of lines) {
|
||||||
|
// Entry lines look like: " <size> <date> <time> <path>"
|
||||||
|
// We extract the path from the last whitespace-delimited field.
|
||||||
|
const match = /^\s+\d+\s+[\d-]+\s+[\d:]+\s+(.+)$/.exec(line);
|
||||||
|
if (!match || !match[1]) continue;
|
||||||
|
const entryPath = match[1].trim();
|
||||||
|
if (!entryPath || entryPath.endsWith('/')) continue; // skip directory entries
|
||||||
|
|
||||||
|
// Reject absolute paths and paths with traversal components
|
||||||
|
if (path.isAbsolute(entryPath) || entryPath.includes('..')) {
|
||||||
|
try {
|
||||||
|
fs.unlinkSync(tmpZip);
|
||||||
|
} catch {
|
||||||
|
/* ignore */
|
||||||
|
}
|
||||||
|
throw new Error(
|
||||||
|
`Zip-slip detected: archive entry "${entryPath}" contains a path traversal ` +
|
||||||
|
`component. Refusing to extract.`
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
} catch (err) {
|
||||||
|
// If the guard itself throws (e.g. zip-slip detected above), propagate it.
|
||||||
|
// If it's a system error (unzip not available), let extraction proceed and
|
||||||
|
// surface the issue then.
|
||||||
|
if ((err as Error).message?.includes('Zip-slip')) throw err;
|
||||||
|
// Warn but continue — extraction will likely also fail if unzip is missing
|
||||||
|
console.error(
|
||||||
|
`[!] Zip entry scan failed (will attempt extraction): ${(err as Error).message}`
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
// Extract the zip into dest
|
||||||
|
await execFileAsync('unzip', ['-o', tmpZip, '-d', dest]);
|
||||||
|
|
||||||
|
// Clean up the temp archive
|
||||||
|
try {
|
||||||
|
fs.unlinkSync(tmpZip);
|
||||||
|
} catch {
|
||||||
|
/* ignore */
|
||||||
|
}
|
||||||
|
|
||||||
|
break;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -8,6 +8,7 @@
|
|||||||
*/
|
*/
|
||||||
|
|
||||||
import * as fs from 'fs';
|
import * as fs from 'fs';
|
||||||
|
import * as os from 'os';
|
||||||
import * as path from 'path';
|
import * as path from 'path';
|
||||||
import { getCcsDir } from '../../config/config-loader-facade';
|
import { getCcsDir } from '../../config/config-loader-facade';
|
||||||
|
|
||||||
@@ -90,11 +91,10 @@ function defaultGetCcsDir(): string {
|
|||||||
return getCcsDir();
|
return getCcsDir();
|
||||||
}
|
}
|
||||||
|
|
||||||
const DEFAULT_APP_INSTALL_PATH = path.join(
|
// Fix #5: use os.homedir() to match install-subcommand.ts and uninstall-subcommand.ts.
|
||||||
process.env.HOME ?? process.env.CCS_HOME ?? '~',
|
// process.env.HOME may be unset in restricted environments, and CCS_HOME is the CCS
|
||||||
'Applications',
|
// data directory (~/.ccs), not the user's home — neither is a safe fallback here.
|
||||||
'CCS Bar.app'
|
const DEFAULT_APP_INSTALL_PATH = path.join(os.homedir(), 'Applications', 'CCS Bar.app');
|
||||||
);
|
|
||||||
|
|
||||||
// ---------------------------------------------------------------------------
|
// ---------------------------------------------------------------------------
|
||||||
// Implementation
|
// Implementation
|
||||||
|
|||||||
@@ -1074,3 +1074,121 @@ describe('version subcommand', () => {
|
|||||||
expect(exitCode).toBe(0);
|
expect(exitCode).toBe(0);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// ---------------------------------------------------------------------------
|
||||||
|
// 4f. Fix #6 — redirect host bypass: every hop is re-validated
|
||||||
|
// ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
describe('bar install: redirect host re-validation (fix #6)', () => {
|
||||||
|
const INITIAL_URL =
|
||||||
|
'https://github.com/kaitranntt/ccs/releases/download/ccs-bar-latest/CCS-Bar.app.zip';
|
||||||
|
const EVIL_REDIRECT_URL = 'https://evil.example.com/CCS-Bar.app.zip';
|
||||||
|
const FAKE_VERSION = '1.0.0';
|
||||||
|
|
||||||
|
it('rejects a download when a redirect leads to an untrusted host', async () => {
|
||||||
|
// Simulate a downloadAndExtract that follows a redirect to an untrusted host
|
||||||
|
// and validates each hop (the production fix). The mock replicates the fix logic.
|
||||||
|
const { handleBarInstall, validateDownloadUrl } = await loadInstallSubcommand();
|
||||||
|
|
||||||
|
const redirectFollowingExtract = async (url: string, _dest: string) => {
|
||||||
|
// Validate initial URL
|
||||||
|
validateDownloadUrl(url);
|
||||||
|
// Simulate a 302 to an evil host — production code re-validates Location
|
||||||
|
validateDownloadUrl(EVIL_REDIRECT_URL); // should throw
|
||||||
|
};
|
||||||
|
|
||||||
|
await handleBarInstall([], {
|
||||||
|
fetchReleaseAsset: async () => ({
|
||||||
|
downloadUrl: INITIAL_URL,
|
||||||
|
version: FAKE_VERSION,
|
||||||
|
}),
|
||||||
|
downloadAndExtract: redirectFollowingExtract,
|
||||||
|
verifyCompat: async () => ({ version: FAKE_VERSION, compatible: true }),
|
||||||
|
getCcsDir: () => path.join(tempHome, '.ccs'),
|
||||||
|
getAppsDir: () => path.join(tempHome, 'Applications'),
|
||||||
|
});
|
||||||
|
|
||||||
|
const allOutput = consoleOutput.join('\n');
|
||||||
|
// The validation error should surface as a download/extraction failure
|
||||||
|
expect(allOutput).toMatch(/\[X\]/);
|
||||||
|
expect(allOutput.toLowerCase()).toMatch(/download|extraction|failed/i);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('validateDownloadUrl blocks redirect targets with untrusted hostnames', async () => {
|
||||||
|
const { validateDownloadUrl } = await loadInstallSubcommand();
|
||||||
|
|
||||||
|
// The redirect target must be rejected just like any initial URL
|
||||||
|
expect(() => validateDownloadUrl(EVIL_REDIRECT_URL)).toThrow(/allowlist|trusted/i);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('validateDownloadUrl allows the expected redirect target (githubusercontent.com)', async () => {
|
||||||
|
const { validateDownloadUrl } = await loadInstallSubcommand();
|
||||||
|
const legitimateRedirect = 'https://objects.githubusercontent.com/file/CCS-Bar.app.zip';
|
||||||
|
expect(() => validateDownloadUrl(legitimateRedirect)).not.toThrow();
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
// ---------------------------------------------------------------------------
|
||||||
|
// 4g. Fix #14 — zip-slip guard: reject archives with traversal entries
|
||||||
|
// ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
describe('bar install: zip-slip guard (fix #14)', () => {
|
||||||
|
const FAKE_DOWNLOAD_URL =
|
||||||
|
'https://github.com/kaitranntt/ccs/releases/download/ccs-bar-latest/CCS-Bar.app.zip';
|
||||||
|
const FAKE_VERSION = '1.0.0';
|
||||||
|
|
||||||
|
it('rejects download when downloadAndExtract detects a zip-slip entry', async () => {
|
||||||
|
const { handleBarInstall } = await loadInstallSubcommand();
|
||||||
|
|
||||||
|
// Simulate an extractor that detects a zip-slip entry and throws
|
||||||
|
const zipSlipExtract = async (_url: string, _dest: string) => {
|
||||||
|
throw new Error(
|
||||||
|
'Zip-slip detected: archive entry "../../../evil" contains a path traversal component. Refusing to extract.'
|
||||||
|
);
|
||||||
|
};
|
||||||
|
|
||||||
|
await handleBarInstall([], {
|
||||||
|
fetchReleaseAsset: async () => ({
|
||||||
|
downloadUrl: FAKE_DOWNLOAD_URL,
|
||||||
|
version: FAKE_VERSION,
|
||||||
|
}),
|
||||||
|
downloadAndExtract: zipSlipExtract,
|
||||||
|
verifyCompat: async () => ({ version: FAKE_VERSION, compatible: true }),
|
||||||
|
getCcsDir: () => path.join(tempHome, '.ccs'),
|
||||||
|
getAppsDir: () => path.join(tempHome, 'Applications'),
|
||||||
|
});
|
||||||
|
|
||||||
|
const allOutput = consoleOutput.join('\n');
|
||||||
|
// Should report failure and not proceed to install
|
||||||
|
expect(allOutput).toMatch(/\[X\]/);
|
||||||
|
expect(allOutput.toLowerCase()).toMatch(/download|extraction|failed/i);
|
||||||
|
// App should not be installed
|
||||||
|
expect(fs.existsSync(path.join(tempHome, 'Applications', 'CCS Bar.app'))).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('allows a clean archive with safe paths to proceed normally', async () => {
|
||||||
|
const appsDir = path.join(tempHome, 'Applications');
|
||||||
|
const { handleBarInstall } = await loadInstallSubcommand();
|
||||||
|
|
||||||
|
// Simulate an extractor that validates entries and finds no traversal
|
||||||
|
const safeExtract = async (_url: string, dest: string) => {
|
||||||
|
// All entry paths are safe relative paths — no ".." or absolute
|
||||||
|
fs.mkdirSync(path.join(dest, 'CCS Bar.app'), { recursive: true });
|
||||||
|
};
|
||||||
|
|
||||||
|
await handleBarInstall([], {
|
||||||
|
fetchReleaseAsset: async () => ({
|
||||||
|
downloadUrl: FAKE_DOWNLOAD_URL,
|
||||||
|
version: FAKE_VERSION,
|
||||||
|
}),
|
||||||
|
downloadAndExtract: safeExtract,
|
||||||
|
verifyCompat: async () => ({ version: FAKE_VERSION, compatible: true }),
|
||||||
|
getCcsDir: () => path.join(tempHome, '.ccs'),
|
||||||
|
getAppsDir: () => appsDir,
|
||||||
|
});
|
||||||
|
|
||||||
|
const allOutput = consoleOutput.join('\n');
|
||||||
|
expect(allOutput).toMatch(/\[OK\]/);
|
||||||
|
expect(allOutput).not.toMatch(/\[X\]/);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user