diff --git a/packages/cli-upload/package.json b/packages/cli-upload/package.json index 62ac41307..e82be7b36 100644 --- a/packages/cli-upload/package.json +++ b/packages/cli-upload/package.json @@ -35,6 +35,6 @@ "dependencies": { "@percy/cli-command": "1.32.7-beta.0", "fast-glob": "^3.2.11", - "image-size": "~1.0.2" + "probe-image-size": "^7.4.0" } } diff --git a/packages/cli-upload/src/upload.js b/packages/cli-upload/src/upload.js index abd3106fe..b1b3f710f 100644 --- a/packages/cli-upload/src/upload.js +++ b/packages/cli-upload/src/upload.js @@ -6,6 +6,29 @@ import * as UploadConfig from './config.js'; const ALLOWED_FILE_TYPES = /\.(png|jpg|jpeg)$/i; const ALLOWED_TOKEN_TYPES = ['web', 'generic']; +// The dimension reader recognises about ten formats; this command accepts two. +const SUPPORTED_IMAGE_TYPES = new Set(['png', 'jpg']); + +// A JPEG frame header sits behind however much metadata the encoder wrote, so +// dimensions cannot be read at a fixed offset. This is the limit the previous +// `image-size` dependency applied, kept so that no file it could read becomes +// unreadable. +const MAX_HEADER_BYTES = 512 * 1024; + +// Reads the leading bytes of a file, rather than pulling a multi-megabyte image +// into memory just to read its dimensions. +function readImageHeader(absolutePath) { + let fd = fs.openSync(absolutePath, 'r'); + + try { + let length = Math.min(fs.fstatSync(fd).size, MAX_HEADER_BYTES); + let header = Buffer.alloc(length); + return header.subarray(0, fs.readSync(fd, header, 0, length, 0)); + } finally { + fs.closeSync(fd); + } +} + // All BYOS screenshots have a fixed comparison tag export const BYOS_TAG = { name: 'Uploaded Screenshot', @@ -85,7 +108,10 @@ export const upload = command('upload', { exit(1, 'Invalid Token Type. Only "web" and "self-managed" token types are allowed.'); } - let { default: imageSize } = await import('image-size'); + // `sync.js` is the buffer-parser entrypoint — none of the http or stream + // machinery. The extension is required because the package publishes no + // `exports` map and this one is ESM. + let { default: probeImageSize } = await import('probe-image-size/sync.js'); let { getImageResources } = await import('./utils.js'); // the internal discovery queue shares a concurrency with the snapshots queue @@ -97,7 +123,21 @@ export const upload = command('upload', { log.info(`Skipping unsupported file type: ${relativePath}`); } else { let absolutePath = path.resolve(args.dirname, relativePath); - let img = { relativePath, absolutePath, ...imageSize(absolutePath) }; + let probed = probeImageSize(readImageHeader(absolutePath)); + + // covers a file whose extension disagrees with its contents — including a + // readable image in a format this command does not accept + if (!probed || !SUPPORTED_IMAGE_TYPES.has(probed.type)) { + log.info(`Skipping file with unreadable image data: ${relativePath}`); + continue; + } + + let img = { + relativePath, + absolutePath, + width: probed.width, + height: probed.height + }; let { dir, name, ext } = path.parse(relativePath); img.type = ext === '.png' ? 'png' : 'jpeg'; img.name = path.join(dir, name); diff --git a/packages/cli-upload/test/upload.test.js b/packages/cli-upload/test/upload.test.js index c66af5c92..fe31eaddf 100644 --- a/packages/cli-upload/test/upload.test.js +++ b/packages/cli-upload/test/upload.test.js @@ -2,10 +2,46 @@ import { fs, logger, api, setupTest } from '@percy/cli-command/test/helpers'; import upload from '@percy/cli-upload'; import { BYOS_TAG } from '../src/upload.js'; -// http://png-pixel.com/ -const pixel = Buffer.from(( - 'R0lGODlhAQABAIAAAP///wAAACH5BAEAAAAALAAAAAABAAEAAAICRAEAOw==' -), 'base64').toString(); +// Real image bytes, kept as buffers rather than strings — the PNG signature +// starts with 0x89, which does not survive a round trip through UTF-8. +const b64 = str => Buffer.from(str, 'base64'); + +// 1x1 red PNG +const PNG_PIXEL = b64( + 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAIAAACQd1PeAAAADElEQVR4nGP4z8AAAAMBAQDJ' + + '/pLvAAAAAElFTkSuQmCC' +); + +// 1x1 red JPEG +const JPEG_PIXEL = b64( + '/9j/4AAQSkZJRgABAQAAAQABAAD/2wBDABALDA4MChAODQ4SERATGCgaGBYWGDEjJR0oOjM9' + + 'PDkzODdASFxOQERXRTc4UG1RV19iZ2hnPk1xeXBkeFxlZ2P/2wBDARESEhgVGC8aGi9jQjhC' + + 'Y2NjY2NjY2NjY2NjY2NjY2NjY2NjY2NjY2NjY2NjY2NjY2NjY2NjY2NjY2NjY2NjY2P/wAAR' + + 'CAABAAEDASIAAhEBAxEB/8QAHwAAAQUBAQEBAQEAAAAAAAAAAAECAwQFBgcICQoL/8QAtRAA' + + 'AgEDAwIEAwUFBAQAAAF9AQIDAAQRBRIhMUEGE1FhByJxFDKBkaEII0KxwRVS0fAkM2JyggkK' + + 'FhcYGRolJicoKSo0NTY3ODk6Q0RFRkdISUpTVFVWV1hZWmNkZWZnaGlqc3R1dnd4eXqDhIWG' + + 'h4iJipKTlJWWl5iZmqKjpKWmp6ipqrKztLW2t7i5usLDxMXGx8jJytLT1NXW19jZ2uHi4+Tl' + + '5ufo6erx8vP09fb3+Pn6/8QAHwEAAwEBAQEBAQEBAQAAAAAAAAECAwQFBgcICQoL/8QAtREA' + + 'AgECBAQDBAcFBAQAAQJ3AAECAxEEBSExBhJBUQdhcRMiMoEIFEKRobHBCSMzUvAVYnLRChYk' + + 'NOEl8RcYGRomJygpKjU2Nzg5OkNERUZHSElKU1RVVldYWVpjZGVmZ2hpanN0dXZ3eHl6goOE' + + 'hYaHiImKkpOUlZaXmJmaoqOkpaanqKmqsrO0tba3uLm6wsPExcbHyMnK0tPU1dbX2Nna4uPk' + + '5ebn6Onq8vP09fb3+Pn6/9oADAMBAAIRAxEAPwDFoooryz7w/9k=' +); + +// A GIF — readable by the parser, but not a format this command accepts. +const GIF_PIXEL = b64('R0lGODlhAQABAIAAAP///wAAACH5BAEAAAAALAAAAAABAAEAAAICRAEAOw=='); + +// An ICNS buffer with valid magic bytes and a zero-valued entry length — the +// CVE-2025-71330 proof of concept. `image-size` looped forever on it because a +// zero-length entry never advanced the read offset. +const icnsZeroLengthEntry = () => { + let buffer = Buffer.alloc(64); + buffer.write('icns', 0, 'ascii'); + buffer.writeUInt32BE(64, 4); // file length + buffer.write('ic09', 8, 'ascii'); // first entry type + buffer.writeUInt32BE(0, 12); // first entry length + return buffer; +}; describe('percy upload', () => { beforeEach(async () => { @@ -15,13 +51,18 @@ describe('percy upload', () => { process.env.PERCY_FORCE_PKG_VALUE = JSON.stringify({ name: '@percy/client', version: '1.0.0' }); await setupTest({ filesystem: { - 'images/test-1.png': pixel, - 'images/test-2.jpg': pixel, - 'images/test-3.jpeg': pixel, - 'images/test-4.gif': pixel, + 'images/.keep': '', './nope': 'not here' } }); + + // written as buffers rather than through `filesystem` above, which only + // creates files from strings — and image bytes are not valid UTF-8 + fs.writeFileSync('images/test-1.png', PNG_PIXEL); + fs.writeFileSync('images/test-2.jpg', JPEG_PIXEL); + fs.writeFileSync('images/test-3.jpeg', JPEG_PIXEL); + fs.writeFileSync('images/test-4.gif', GIF_PIXEL); + fs.unlinkSync('images/.keep'); }); afterEach(() => { @@ -157,6 +198,48 @@ describe('percy upload', () => { ])); }); + it('skips files whose contents are not a readable image', async () => { + fs.writeFileSync('images/test-5.png', 'this is not a png'); + await upload(['./images']); + + expect(logger.stderr).toEqual([]); + expect(logger.stdout).toEqual(jasmine.arrayContaining([ + '[percy] Skipping file with unreadable image data: test-5.png', + '[percy] Uploading 3 snapshots...', + '[percy] Snapshot uploaded: test-1.png' + ])); + }); + + // Regression for CVE-2025-71330. The previous `image-size` dependency picked + // its parser from magic bytes while this command filters on extension, so an + // ICNS buffer named `.png` reached a parser that looped forever on it. The + // upload now completes and the crafted file is skipped. + it('skips a crafted ICNS file named as a png without hanging', async () => { + fs.writeFileSync('images/crafted.png', icnsZeroLengthEntry()); + await upload(['./images']); + + expect(logger.stderr).toEqual([]); + expect(logger.stdout).toEqual(jasmine.arrayContaining([ + '[percy] Skipping file with unreadable image data: crafted.png', + '[percy] Uploading 3 snapshots...', + '[percy] Finalized build #1: https://percy.io/test/test/123' + ])); + }); + + // the extension filter passes this through, and the parser reads GIFs happily, + // so only the format gate keeps it out + it('skips a readable image whose format is not png or jpeg', async () => { + fs.writeFileSync('images/gif-named.png', GIF_PIXEL); + await upload(['./images']); + + expect(logger.stderr).toEqual([]); + expect(logger.stdout).toEqual(jasmine.arrayContaining([ + '[percy] Skipping file with unreadable image data: gif-named.png', + '[percy] Uploading 3 snapshots...', + '[percy] Finalized build #1: https://percy.io/test/test/123' + ])); + }); + it('does not upload snapshots and prints matching files with --dry-run', async () => { await upload(['./images', '--dry-run']); diff --git a/yarn.lock b/yarn.lock index 46b0f213e..3932687b8 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3830,7 +3830,7 @@ debounce@^1.2.0: resolved "https://registry.npmjs.org/debounce/-/debounce-1.2.0.tgz" integrity sha512-mYtLl1xfZLi1m4RtQYlZgJUNQjl4ZxVnHzIR8nLLgi4q1YT8o/WM+MK/f8yfcc9s5Ir5zRaPZyZU6xs1Syoocg== -debug@2.6.9: +debug@2, debug@2.6.9: version "2.6.9" resolved "https://registry.npmjs.org/debug/-/debug-2.6.9.tgz" integrity sha512-bC7ElrdJaJnPbAP+1EotYvqZsb3ecl5wi6Bfi6BJTUcNowp6cvspg0jXznRTKDjm/E7AdgFBVeAPVMNcKGsHMA== @@ -3844,7 +3844,7 @@ debug@4, debug@^4.0.1, debug@^4.1.0, debug@^4.1.1, debug@^4.3.1, debug@^4.3.3, d dependencies: ms "^2.1.3" -debug@^3.2.7: +debug@^3.2.6, debug@^3.2.7: version "3.2.7" resolved "https://registry.npmjs.org/debug/-/debug-3.2.7.tgz" integrity sha512-CFjzYYAi4ThfiQvizrFQevTTXHtnCqWfe7x1AhgEscTz6ZbLbfoLRLPugTQyBth6f8ZERVUSyWHFD/7Wu4t1XQ== @@ -5432,7 +5432,7 @@ humanize-ms@^1.2.1: dependencies: ms "^2.0.0" -iconv-lite@0.4.24, iconv-lite@^0.4.24: +iconv-lite@0.4.24, iconv-lite@^0.4.24, iconv-lite@^0.4.4: version "0.4.24" resolved "https://registry.npmjs.org/iconv-lite/-/iconv-lite-0.4.24.tgz" integrity sha512-v3MXnZAcvnywkTUEZomIActle7RXXeedOR31wwl7VlyoXO4Qi9arvSenNQWne1TcRwhCL1HwLI21bEqdpj8/rA== @@ -5468,13 +5468,6 @@ ignore@^5.0.4, ignore@^5.1.1, ignore@^5.2.0: resolved "https://registry.npmjs.org/ignore/-/ignore-5.2.0.tgz" integrity sha512-CmxgYGiEPCLhfLnpPp1MoRmifwEIOgjcHXxOBjv7mY96c+eWScsOP9c112ZyLdWHi0FxHjI+4uVhKYp/gcdRmQ== -image-size@~1.0.2: - version "1.0.2" - resolved "https://registry.npmjs.org/image-size/-/image-size-1.0.2.tgz" - integrity sha512-xfOoWjceHntRb3qFCrh5ZFORYH8XCdYpASltMhZ/Q0KZiOwjdE/Yl2QCiWdwD+lygV5bMCvauzgu5PxBX/Yerg== - dependencies: - queue "6.0.2" - import-fresh@^3.0.0, import-fresh@^3.2.1: version "3.3.0" resolved "https://registry.npmjs.org/import-fresh/-/import-fresh-3.3.0.tgz" @@ -6875,6 +6868,15 @@ natural-compare@^1.4.0: resolved "https://registry.npmjs.org/natural-compare/-/natural-compare-1.4.0.tgz" integrity sha1-Sr6/7tdUHywnrPspvbvRXI1bpPc= +needle@^2.5.2: + version "2.9.1" + resolved "https://registry.yarnpkg.com/needle/-/needle-2.9.1.tgz#22d1dffbe3490c2b83e301f7709b6736cd8f2684" + integrity sha512-6R9fqJ5Zcmf+uYaFgdIHmLwNldn5HbK8L5ybn7Uz+ylX/rnOsSp1AHcvQSrCaFN+qNM1wpymHqD7mVasEOlHGQ== + dependencies: + debug "^3.2.6" + iconv-lite "^0.4.4" + sax "^1.2.4" + negotiator@0.6.2: version "0.6.2" resolved "https://registry.npmjs.org/negotiator/-/negotiator-0.6.2.tgz" @@ -7666,6 +7668,15 @@ pretty-format@^29.5.0: ansi-styles "^5.0.0" react-is "^18.0.0" +probe-image-size@^7.4.0: + version "7.4.0" + resolved "https://registry.yarnpkg.com/probe-image-size/-/probe-image-size-7.4.0.tgz#c189ae04e9aad1e3fa3a0998a956a7e9aab7f66d" + integrity sha512-cdEprVtZxV+awMde9X+4jILBFYh4CARxVrQaMl4wY4YcPWbul9jntXrIW95NInBDyJwcVUP3U0T6yukN8rMBaQ== + dependencies: + lodash.merge "^4.6.2" + needle "^2.5.2" + stream-parser "~0.3.1" + proc-log@^2.0.0, proc-log@^2.0.1: version "2.0.1" resolved "https://registry.npmjs.org/proc-log/-/proc-log-2.0.1.tgz" @@ -7778,13 +7789,6 @@ queue-microtask@^1.2.2: resolved "https://registry.npmjs.org/queue-microtask/-/queue-microtask-1.2.3.tgz" integrity sha512-NuaNSa6flKT5JaSYQzJok04JzTL1CA6aGhv5rfLW3PgqA+M2ChpZQnAC8h8i4ZFkBS8X5RqkDBHA7r4hej3K9A== -queue@6.0.2: - version "6.0.2" - resolved "https://registry.npmjs.org/queue/-/queue-6.0.2.tgz" - integrity sha512-iHZWu+q3IdFZFX36ro/lKBkSvfkztY5Y7HMiPlOUjhupPcG2JMfst2KKEpu5XndviX/3UhFbRngUPNKtgvtZiA== - dependencies: - inherits "~2.0.3" - quick-lru@^4.0.1: version "4.0.1" resolved "https://registry.npmjs.org/quick-lru/-/quick-lru-4.0.1.tgz" @@ -8151,6 +8155,11 @@ safe-regex-test@^1.0.3: resolved "https://registry.npmjs.org/safer-buffer/-/safer-buffer-2.1.2.tgz" integrity sha512-YZo3K82SD7Riyi0E1EQPojLz7kpepnSQI9IyPbHHg1XXXevb5dJI7tpyN2ADxGcQbHG7vcyRHk0cbwqcQriUtg== +sax@^1.2.4: + version "1.6.1" + resolved "https://registry.yarnpkg.com/sax/-/sax-1.6.1.tgz#4c23cf608c0b693ab54b4b5888e92cfe977b9843" + integrity sha512-42tBVwLWnaQvW5zc4HbZrTuWccECCZfBi92FDuwtqxasH+JbPB3/FOKb1m222K42R4WxuxzzMsTswfzgtSu64Q== + "semver@2 || 3 || 4 || 5", semver@^5.6.0: version "5.7.1" resolved "https://registry.npmjs.org/semver/-/semver-5.7.1.tgz" @@ -8490,6 +8499,13 @@ statuses@~1.5.0: resolved "https://registry.npmjs.org/statuses/-/statuses-1.5.0.tgz" integrity sha1-Fhx9rBd2Wf2YEfQ3cfqZOBR4Yow= +stream-parser@~0.3.1: + version "0.3.1" + resolved "https://registry.yarnpkg.com/stream-parser/-/stream-parser-0.3.1.tgz#1618548694420021a1182ff0af1911c129761773" + integrity sha512-bJ/HgKq41nlKvlhccD5kaCr/P+Hu0wPNKPJOH7en+YrJu/9EgqUF+88w5Jb6KNcjOFMhfX4B2asfeAtIGuHObQ== + dependencies: + debug "2" + streamroller@^3.0.2: version "3.0.2" resolved "https://registry.npmjs.org/streamroller/-/streamroller-3.0.2.tgz"