From 862606e806a476bdf9f2d3d572487d3d34297243 Mon Sep 17 00:00:00 2001 From: Pranav Zinzurde Date: Thu, 27 Aug 2026 15:20:44 +0530 Subject: [PATCH 1/4] fix(core): accept a host-supplied Maestro scope root (PER-10620) Adds PERCY_MAESTRO_BS_SCOPE_ROOT. When the BrowserStack host sets it, the value is the scope root: the relay globs {root}/**/{name}.png, the manual walker recurses the same root, and the realpath containment check anchors on it. Non-absolute values are ignored so a malformed injection falls back to the composed convention. Android, iOS and self-hosted keep their existing behaviour when the variable is unset. Co-Authored-By: Claude Opus 5 (1M context) --- packages/core/src/maestro-screenshot-file.js | 54 ++++--- packages/core/src/maestro-screenshot.js | 19 ++- packages/core/test/api.test.js | 141 ++++++++++++++++++- 3 files changed, 187 insertions(+), 27 deletions(-) diff --git a/packages/core/src/maestro-screenshot-file.js b/packages/core/src/maestro-screenshot-file.js index 17a12e4d5..5fc46f484 100644 --- a/packages/core/src/maestro-screenshot-file.js +++ b/packages/core/src/maestro-screenshot-file.js @@ -15,31 +15,42 @@ export function appAutomateTmpDir() { return path.isAbsolute(dir) ? dir : '/tmp'; } +export function bsScopeRootOverride() { + let raw = process.env.PERCY_MAESTRO_BS_SCOPE_ROOT; + if (!raw) return null; + let dir = raw.replace(/[/\\]+$/, ''); + return path.isAbsolute(dir) ? dir : null; +} + /* istanbul ignore next — defensive manual directory walker invoked only when fast-glob import fails (broken install / FS corruption). Unit tests exercise the primary glob path; integration tests on BS hosts exercise the walker against real session layouts. Path-traversal sinks inside this function are suppressed at file level in .semgrepignore with the same rationale (upstream SAFE_ID validation, depth cap, exact filename match). */ -async function manualScreenshotWalk(platform, sessionId, name) { +async function manualScreenshotWalk(platform, sessionId, name, scopeRoot) { const files = []; - try { - if (platform === 'ios') { - const sessionDir = `${appAutomateTmpDir()}/${sessionId}`; - const walk = async (dir, depth) => { - if (depth > 15) return; // sanity cap - let entries; - try { entries = await fs.promises.readdir(dir, { withFileTypes: true }); } catch { return; } - for (const entry of entries) { - const full = path.join(dir, entry.name); - if (entry.isDirectory()) { - await walk(full, depth + 1); - } else if (entry.isFile() && entry.name === `${name}.png` && full.includes('_maestro_debug_')) { - files.push(full); - } + const walkFrom = async (root, accept) => { + const walk = async (dir, depth) => { + if (depth > 15) return; // sanity cap + let entries; + try { entries = await fs.promises.readdir(dir, { withFileTypes: true }); } catch { return; } + for (const entry of entries) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) { + await walk(full, depth + 1); + } else if (entry.isFile() && entry.name === `${name}.png` && accept(full)) { + files.push(full); } - }; - await walk(sessionDir, 0); + } + }; + await walk(root, 0); + }; + try { + if (scopeRoot) { + await walkFrom(scopeRoot, () => true); + } else if (platform === 'ios') { + await walkFrom(`${appAutomateTmpDir()}/${sessionId}`, full => full.includes('_maestro_debug_')); } else { const baseDir = `${appAutomateTmpDir()}/${sessionId}_test_suite/logs`; const logDirs = await fs.promises.readdir(baseDir); @@ -66,7 +77,8 @@ async function manualScreenshotWalk(platform, sessionId, name) { // ServerError(404) when the file is missing or resolves outside scopeRoot. // Callers pass `filePath` already shape-validated, plus the resolved `scopeRoot` // and `selfHosted` flag. -export async function locateScreenshot({ platform, sessionId, name, filePath, scopeRoot, selfHosted }) { +export async function locateScreenshot({ platform, sessionId, name, filePath, scopeRoot, selfHosted, recursiveScope }) { + let scopedGlob = selfHosted || !!recursiveScope; let chosenFile; if (filePath) { chosenFile = filePath; @@ -81,7 +93,7 @@ export async function locateScreenshot({ platform, sessionId, name, filePath, sc // (scopeRoot = PERCY_MAESTRO_SCREENSHOT_DIR). `name` is SAFE_ID-validated // by the caller, so it cannot contain separators or traversal chars. let searchPattern; - if (selfHosted) { + if (scopedGlob) { // fast-glob requires forward-slashes in patterns on every platform; on // Windows scopeRoot contains backslashes, so normalize before embedding. // Production-code Windows portability — verified by the CI Windows runner. @@ -111,7 +123,9 @@ export async function locateScreenshot({ platform, sessionId, name, filePath, sc // See manualScreenshotWalk() at file top + the file-level .semgrepignore. /* istanbul ignore next — only fires when fast-glob import throws (broken install / FS corruption); integration-test territory. */ - files = selfHosted ? [] : await manualScreenshotWalk(platform, sessionId, name); + files = selfHosted + ? [] + : await manualScreenshotWalk(platform, sessionId, name, recursiveScope ? scopeRoot : null); } if (!files || files.length === 0) { diff --git a/packages/core/src/maestro-screenshot.js b/packages/core/src/maestro-screenshot.js index 3ae4e131a..1ef8c25cf 100644 --- a/packages/core/src/maestro-screenshot.js +++ b/packages/core/src/maestro-screenshot.js @@ -4,7 +4,7 @@ import { normalize } from '@percy/config/utils'; import { ServerError } from './server.js'; import { encodeURLSearchParams } from './utils.js'; import { handleSyncJob } from './snapshot.js'; -import { locateScreenshot, appAutomateTmpDir } from './maestro-screenshot-file.js'; +import { locateScreenshot, appAutomateTmpDir, bsScopeRootOverride } from './maestro-screenshot-file.js'; import { validateRegionInputs, resolveRegions } from './maestro-regions.js'; import { deriveDeviceInsets } from './maestro-hierarchy.js'; @@ -98,6 +98,7 @@ export async function handleMaestroScreenshot(req, res, percy) { // realpath + prefix check inside locateScreenshot enforces the security // invariant at whichever root applies; the boundary is relocated, not removed. let scopeRoot; + let recursiveScope = false; if (selfHosted) { // Reject filePath outright in self-hosted mode. The SDK never emits it (it // sends a relative SCREENSHOT_NAME); honoring an absolute filePath against @@ -124,10 +125,18 @@ export async function handleMaestroScreenshot(req, res, percy) { throw new ServerError(400, `PERCY_MAESTRO_SCREENSHOT_DIR is not an existing directory: ${dir}`); } scopeRoot = dir; + recursiveScope = true; } else { - scopeRoot = platform === 'ios' - ? `${appAutomateTmpDir()}/${sessionId}` - : `${appAutomateTmpDir()}/${sessionId}_test_suite`; + let overrideRoot = bsScopeRootOverride(); + if (overrideRoot) { + scopeRoot = overrideRoot; + recursiveScope = true; + percy.log.debug(`maestro screenshot scope root overridden: ${scopeRoot}`); + } else { + scopeRoot = platform === 'ios' + ? `${appAutomateTmpDir()}/${sessionId}` + : `${appAutomateTmpDir()}/${sessionId}_test_suite`; + } } // Validate regions input shape early (before file I/O and ADB work) so @@ -137,7 +146,7 @@ export async function handleMaestroScreenshot(req, res, percy) { // Locate the screenshot on disk (supplied filePath, BS session glob, or // self-hosted PERCY_MAESTRO_SCREENSHOT_DIR recursive glob) and confirm it // resolves under scopeRoot. Throws ServerError(404) when missing/out-of-root. - let realPath = await locateScreenshot({ platform, sessionId, name, filePath: suppliedFilePath, scopeRoot, selfHosted }); + let realPath = await locateScreenshot({ platform, sessionId, name, filePath: suppliedFilePath, scopeRoot, selfHosted, recursiveScope }); // Read and base64-encode the screenshot let fileContent = await fs.promises.readFile(realPath); diff --git a/packages/core/test/api.test.js b/packages/core/test/api.test.js index 90e176c76..03a0d3f51 100644 --- a/packages/core/test/api.test.js +++ b/packages/core/test/api.test.js @@ -5,7 +5,7 @@ import { logger, api, setupTest, fs } from './helpers/index.js'; import Percy from '@percy/core'; import WebdriverUtils from '@percy/webdriver-utils'; import { getPercyDomPath, _applyHttpReadOnlyStripping } from '../src/api.js'; -import { appAutomateTmpDir } from '../src/maestro-screenshot-file.js'; +import { appAutomateTmpDir, bsScopeRootOverride } from '../src/maestro-screenshot-file.js'; describe('API Server', () => { let percy; @@ -25,7 +25,7 @@ describe('API Server', () => { // suite (works in isolation; returns [] mid-suite), so route the // self-hosted root to the REAL filesystem — this also tests the true // production glob path. Only paths under this unique root are affected. - await setupTest({ filesystem: { $bypass: [p => typeof p === 'string' && (p.includes('percy-self-hosted-real') || p.includes('percy-bs-tmp-real'))] } }); + await setupTest({ filesystem: { $bypass: [p => typeof p === 'string' && (p.includes('percy-self-hosted-real') || p.includes('percy-bs-tmp-real') || p.includes('percy-bs-scope-'))] } }); percy = new Percy({ token: 'PERCY_TOKEN', @@ -1938,6 +1938,143 @@ describe('API Server', () => { }); }); + describe('PERCY_MAESTRO_BS_SCOPE_ROOT override', () => { + const SCOPE_ROOT = path.join(os.tmpdir(), 'percy-bs-scope-real-root'); + const REALMOBILE_DIR = path.join(SCOPE_ROOT, 'maestro_debug_LoginFlow_LoginFlow_0'); + let priorScope, priorTmp; + + beforeEach(() => { + priorScope = process.env.PERCY_MAESTRO_BS_SCOPE_ROOT; + priorTmp = process.env.PERCY_APP_AUTOMATE_TMP_DIR; + process.env.PERCY_MAESTRO_BS_SCOPE_ROOT = SCOPE_ROOT; + fs.rmSync(SCOPE_ROOT, { recursive: true, force: true }); + fs.mkdirSync(REALMOBILE_DIR, { recursive: true }); + fs.writeFileSync(path.join(REALMOBILE_DIR, `${SS_NAME}.png`), 'PNGBYTES-SCOPE-ROOT'); + }); + + afterEach(() => { + if (priorScope === undefined) delete process.env.PERCY_MAESTRO_BS_SCOPE_ROOT; + else process.env.PERCY_MAESTRO_BS_SCOPE_ROOT = priorScope; + if (priorTmp === undefined) delete process.env.PERCY_APP_AUTOMATE_TMP_DIR; + else process.env.PERCY_APP_AUTOMATE_TMP_DIR = priorTmp; + fs.rmSync(SCOPE_ROOT, { recursive: true, force: true }); + }); + + it('finds a screenshot the platform convention cannot reach', async () => { + spyOn(percy, 'upload').and.resolveTo(); + await percy.start(); + + await expectAsync(postMaestro({ name: SS_NAME, sessionId: SID, platform: 'ios' })) + .toBeResolvedTo(jasmine.objectContaining({ success: true })); + + let [payload] = percy.upload.calls.mostRecent().args; + expect(payload.tiles[0].content).toBe(Buffer.from('PNGBYTES-SCOPE-ROOT').toString('base64')); + }); + + it('applies to android too — the root is the whole convention', async () => { + spyOn(percy, 'upload').and.resolveTo(); + await percy.start(); + + await expectAsync(postMaestro({ name: SS_NAME, sessionId: SID, platform: 'android' })) + .toBeResolvedTo(jasmine.objectContaining({ success: true })); + + let [payload] = percy.upload.calls.mostRecent().args; + expect(payload.tiles[0].content).toBe(Buffer.from('PNGBYTES-SCOPE-ROOT').toString('base64')); + }); + + it('wins over PERCY_APP_AUTOMATE_TMP_DIR when both are set', async () => { + process.env.PERCY_APP_AUTOMATE_TMP_DIR = '/tmp'; + spyOn(percy, 'upload').and.resolveTo(); + await percy.start(); + + await expectAsync(postMaestro({ name: SS_NAME, sessionId: SID, platform: 'ios' })) + .toBeResolvedTo(jasmine.objectContaining({ success: true })); + + let [payload] = percy.upload.calls.mostRecent().args; + expect(payload.tiles[0].content).toBe(Buffer.from('PNGBYTES-SCOPE-ROOT').toString('base64')); + }); + + it('tolerates a trailing slash on the override', async () => { + process.env.PERCY_MAESTRO_BS_SCOPE_ROOT = `${SCOPE_ROOT}/`; + spyOn(percy, 'upload').and.resolveTo(); + await percy.start(); + + await expectAsync(postMaestro({ name: SS_NAME, sessionId: SID, platform: 'ios' })) + .toBeResolvedTo(jasmine.objectContaining({ success: true })); + + let [payload] = percy.upload.calls.mostRecent().args; + expect(payload.tiles[0].content).toBe(Buffer.from('PNGBYTES-SCOPE-ROOT').toString('base64')); + }); + + it('ignores a non-absolute override and keeps the composed convention', async () => { + process.env.PERCY_MAESTRO_BS_SCOPE_ROOT = 'relative/path'; + spyOn(percy, 'upload').and.resolveTo(); + await percy.start(); + + await expectAsync(postMaestro({ name: SS_NAME, sessionId: SID, platform: 'android' })) + .toBeResolvedTo(jasmine.objectContaining({ success: true })); + + let [payload] = percy.upload.calls.mostRecent().args; + expect(payload.tiles[0].content).toBe(Buffer.from('PNGBYTES-ANDROID').toString('base64')); + }); + + it('re-anchors filePath containment on the overridden root', async () => { + fs.writeFileSync(path.join(REALMOBILE_DIR, `${FILEPATH_NAME}.png`), 'PNGBYTES-SCOPE-FILEPATH'); + spyOn(percy, 'upload').and.resolveTo(); + await percy.start(); + + await expectAsync(postMaestro({ + name: FILEPATH_NAME, + sessionId: SID, + platform: 'ios', + filePath: path.join(REALMOBILE_DIR, `${FILEPATH_NAME}.png`) + })).toBeResolvedTo(jasmine.objectContaining({ success: true })); + + let [payload] = percy.upload.calls.mostRecent().args; + expect(payload.tiles[0].content).toBe(Buffer.from('PNGBYTES-SCOPE-FILEPATH').toString('base64')); + + await expectAsync(postMaestro({ + name: FILEPATH_NAME, + sessionId: SID, + platform: 'ios', + filePath: `${IOS_FILEPATH_DIR}/${FILEPATH_NAME}.png` + })).toBeRejectedWithError(/Screenshot not found/); + }); + + it('404s when a globbed file resolves outside the overridden root (symlink escape)', async () => { + const outside = path.join(os.tmpdir(), 'percy-bs-scope-real-OUTSIDE.png'); + const link = path.join(REALMOBILE_DIR, 'EscapeScreen.png'); + try { fs.unlinkSync(link); } catch {} + fs.writeFileSync(outside, 'OUTSIDE'); + fs.symlinkSync(outside, link); + await percy.start(); + + await expectAsync(postMaestro({ name: 'EscapeScreen', sessionId: SID, platform: 'ios' })) + .toBeRejectedWithError(/resolved outside session dir/); + + fs.rmSync(outside, { force: true }); + }); + + it('404s when the overridden root does not exist', async () => { + process.env.PERCY_MAESTRO_BS_SCOPE_ROOT = path.join(os.tmpdir(), 'percy-bs-scope-missing'); + await percy.start(); + + await expectAsync(postMaestro({ name: SS_NAME, sessionId: SID, platform: 'ios' })) + .toBeRejectedWithError(/Screenshot not found/); + }); + + it('pins the trim + null semantics of the exported helper', () => { + process.env.PERCY_MAESTRO_BS_SCOPE_ROOT = `${SCOPE_ROOT}///`; + expect(bsScopeRootOverride()).toBe(SCOPE_ROOT); + process.env.PERCY_MAESTRO_BS_SCOPE_ROOT = '/'; + expect(bsScopeRootOverride()).toBeNull(); + process.env.PERCY_MAESTRO_BS_SCOPE_ROOT = ''; + expect(bsScopeRootOverride()).toBeNull(); + delete process.env.PERCY_MAESTRO_BS_SCOPE_ROOT; + expect(bsScopeRootOverride()).toBeNull(); + }); + }); + // PNG-header fill: relay reads IHDR from the screenshot and populates // payload.tag.width / payload.tag.height when missing. Source of truth // for tag dims is the PNG bytes themselves — what Percy stores and From bd5a3aa15083119cb900198f0d3ca46e719464c6 Mon Sep 17 00:00:00 2001 From: Pranav Zinzurde Date: Thu, 27 Aug 2026 15:58:21 +0530 Subject: [PATCH 2/4] fix(core): bound and label the scoped Maestro glob Keys fast-glob's dot option on the same predicate as the search pattern so the primary lookup and the walker fallback agree beneath an operator-chosen root, bounds both root-recursive globs to the walker's existing depth cap, and names the variable actually in play in the containment-failure message instead of always saying "session dir". Adds specs for a dot-prefixed directory under the override root, a name shared by two session subtrees, and a containment escape on the composed BrowserStack path. Co-Authored-By: Claude Opus 5 (1M context) --- packages/core/src/maestro-screenshot-file.js | 15 ++--- packages/core/test/api.test.js | 65 +++++++++++++++++++- 2 files changed, 72 insertions(+), 8 deletions(-) diff --git a/packages/core/src/maestro-screenshot-file.js b/packages/core/src/maestro-screenshot-file.js index 5fc46f484..053f07f81 100644 --- a/packages/core/src/maestro-screenshot-file.js +++ b/packages/core/src/maestro-screenshot-file.js @@ -2,6 +2,8 @@ import fs from 'fs'; import path from 'path'; import { ServerError } from './server.js'; +const MAX_SEARCH_DEPTH = 15; + // Root under which BrowserStack App Automate hosts write Maestro session // artifacts (screenshots + debug output). Historically /tmp; hosts that // relocate it inject PERCY_APP_AUTOMATE_TMP_DIR with the new root, so the @@ -32,7 +34,7 @@ async function manualScreenshotWalk(platform, sessionId, name, scopeRoot) { const files = []; const walkFrom = async (root, accept) => { const walk = async (dir, depth) => { - if (depth > 15) return; // sanity cap + if (depth > MAX_SEARCH_DEPTH) return; let entries; try { entries = await fs.promises.readdir(dir, { withFileTypes: true }); } catch { return; } for (const entry of entries) { @@ -111,11 +113,7 @@ export async function locateScreenshot({ platform, sessionId, name, filePath, sc let files; try { let { default: glob } = await import('fast-glob'); - // Self-hosted needs `dot: true` because Maestro's default output dir is - // `.maestro/` — a dot-prefixed entry fast-glob hides by default. BS - // layouts have no dot-prefixed segments, so omitting it there keeps the - // byte-identical behavior. - files = await glob(searchPattern, selfHosted ? { dot: true } : undefined); + files = await glob(searchPattern, scopedGlob ? { dot: true, deep: MAX_SEARCH_DEPTH } : undefined); } catch { // Fast-glob import / glob call failed — fall back to manual walker (BS // only; self-hosted has no fixed-layout convention, so empty → 404 with @@ -168,7 +166,10 @@ export async function locateScreenshot({ platform, sessionId, name, filePath, sc const realPathFwd = realPath.replace(/\\/g, '/'); const realPrefixFwd = realPrefix.replace(/\\/g, '/'); if (!realPathFwd.startsWith(`${realPrefixFwd}/`)) { - throw new ServerError(404, `Screenshot not found: ${name}.png (resolved outside ${selfHosted ? 'PERCY_MAESTRO_SCREENSHOT_DIR' : 'session dir'})`); + const scopeLabel = selfHosted + ? 'PERCY_MAESTRO_SCREENSHOT_DIR' + : recursiveScope ? 'PERCY_MAESTRO_BS_SCOPE_ROOT' : 'session dir'; + throw new ServerError(404, `Screenshot not found: ${name}.png (resolved outside ${scopeLabel})`); } return realPath; diff --git a/packages/core/test/api.test.js b/packages/core/test/api.test.js index 03a0d3f51..d807e0502 100644 --- a/packages/core/test/api.test.js +++ b/packages/core/test/api.test.js @@ -1879,6 +1879,21 @@ describe('API Server', () => { fs.rmSync(CUSTOM_ROOT, { recursive: true, force: true }); }); + it('404s when a globbed file resolves outside the session root (symlink escape)', async () => { + const dir = path.join(CUSTOM_ROOT, `${SID}_test_suite`, 'logs', 'run1', 'screenshots'); + const outside = path.join(os.tmpdir(), 'percy-bs-tmp-real-OUTSIDE.png'); + const link = path.join(dir, 'EscapeScreen.png'); + try { fs.unlinkSync(link); } catch {} + fs.writeFileSync(outside, 'OUTSIDE'); + fs.symlinkSync(outside, link); + await percy.start(); + + await expectAsync(postMaestro({ name: 'EscapeScreen', sessionId: SID, platform: 'android' })) + .toBeRejectedWithError(/resolved outside session dir/); + + fs.rmSync(outside, { force: true }); + }); + it('globs under the overridden tmp root instead of the default', async () => { spyOn(percy, 'upload').and.resolveTo(); await percy.start(); @@ -2050,7 +2065,7 @@ describe('API Server', () => { await percy.start(); await expectAsync(postMaestro({ name: 'EscapeScreen', sessionId: SID, platform: 'ios' })) - .toBeRejectedWithError(/resolved outside session dir/); + .toBeRejectedWithError(/resolved outside PERCY_MAESTRO_BS_SCOPE_ROOT/); fs.rmSync(outside, { force: true }); }); @@ -2063,6 +2078,54 @@ describe('API Server', () => { .toBeRejectedWithError(/Screenshot not found/); }); + it('finds a screenshot under a dot-prefixed directory beneath the root', async () => { + const DOT_DIR = path.join(SCOPE_ROOT, '.maestro', 'maestro_debug_DotFlow'); + fs.mkdirSync(DOT_DIR, { recursive: true }); + fs.writeFileSync(path.join(DOT_DIR, 'DotScreen.png'), 'PNGBYTES-DOT'); + spyOn(percy, 'upload').and.resolveTo(); + await percy.start(); + + await expectAsync(postMaestro({ name: 'DotScreen', sessionId: SID, platform: 'ios' })) + .toBeResolvedTo(jasmine.objectContaining({ success: true })); + + let [payload] = percy.upload.calls.mostRecent().args; + expect(payload.tiles[0].content).toBe(Buffer.from('PNGBYTES-DOT').toString('base64')); + }); + + it('resolves a name shared by two session subtrees to the newest file', async () => { + const pngOf = (w, h) => { + const buf = Buffer.alloc(24); + Buffer.from([0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A]).copy(buf, 0); + buf.writeUInt32BE(13, 8); + Buffer.from('IHDR', 'ascii').copy(buf, 12); + buf.writeUInt32BE(w, 16); + buf.writeUInt32BE(h, 20); + return buf; + }; + + const OLDER = path.join(SCOPE_ROOT, 'session-a', 'maestro_debug_Flow'); + const NEWER = path.join(SCOPE_ROOT, 'session-b', 'maestro_debug_Flow'); + fs.mkdirSync(OLDER, { recursive: true }); + fs.mkdirSync(NEWER, { recursive: true }); + const olderFile = path.join(OLDER, 'SharedName.png'); + const newerFile = path.join(NEWER, 'SharedName.png'); + fs.writeFileSync(olderFile, pngOf(720, 1280)); + fs.writeFileSync(newerFile, pngOf(1080, 2400)); + const now = Date.now() / 1000; + fs.utimesSync(olderFile, now - 60, now - 60); + fs.utimesSync(newerFile, now, now); + + spyOn(percy, 'upload').and.resolveTo(); + await percy.start(); + + await expectAsync(postMaestro({ name: 'SharedName', sessionId: SID, platform: 'ios' })) + .toBeResolvedTo(jasmine.objectContaining({ success: true })); + + let [payload] = percy.upload.calls.mostRecent().args; + expect(payload.tag.width).toBe(1080); + expect(payload.tag.height).toBe(2400); + }); + it('pins the trim + null semantics of the exported helper', () => { process.env.PERCY_MAESTRO_BS_SCOPE_ROOT = `${SCOPE_ROOT}///`; expect(bsScopeRootOverride()).toBe(SCOPE_ROOT); From 494da754159964c9c0e027c070aa42fe5cadd9e0 Mon Sep 17 00:00:00 2001 From: Pranav Zinzurde Date: Thu, 27 Aug 2026 16:20:29 +0530 Subject: [PATCH 3/4] fix(core): keep sessionId in scope under the Maestro root override When the injected root contains a subdirectory named for the session, that subtree becomes the scope root, so the glob, the walker fallback and the realpath containment check all narrow to one session. Roots without such a subdirectory are used as given, which is the layout the override exists for. Adds specs for a sibling session holding a newer file of the same name, a filePath pointing into another session's subtree, and a session-named entry that is not a directory. Co-Authored-By: Claude Opus 5 (1M context) --- packages/core/src/maestro-screenshot.js | 7 ++- packages/core/test/api.test.js | 57 +++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 2 deletions(-) diff --git a/packages/core/src/maestro-screenshot.js b/packages/core/src/maestro-screenshot.js index 1ef8c25cf..1ad2acaa8 100644 --- a/packages/core/src/maestro-screenshot.js +++ b/packages/core/src/maestro-screenshot.js @@ -129,9 +129,12 @@ export async function handleMaestroScreenshot(req, res, percy) { } else { let overrideRoot = bsScopeRootOverride(); if (overrideRoot) { - scopeRoot = overrideRoot; recursiveScope = true; - percy.log.debug(`maestro screenshot scope root overridden: ${scopeRoot}`); + let sessionScoped = path.join(overrideRoot, sessionId); + let sessionStat; + try { sessionStat = await fs.promises.stat(sessionScoped); } catch { sessionStat = null; } + scopeRoot = sessionStat?.isDirectory() ? sessionScoped : overrideRoot; + percy.log.debug(`maestro screenshot scope root: ${scopeRoot}`); } else { scopeRoot = platform === 'ios' ? `${appAutomateTmpDir()}/${sessionId}` diff --git a/packages/core/test/api.test.js b/packages/core/test/api.test.js index d807e0502..b1c61c7eb 100644 --- a/packages/core/test/api.test.js +++ b/packages/core/test/api.test.js @@ -2078,6 +2078,63 @@ describe('API Server', () => { .toBeRejectedWithError(/Screenshot not found/); }); + it('narrows to the session subtree when the root contains one', async () => { + const SCOPED_SID = 'scopedsession'; + const MINE = path.join(SCOPE_ROOT, SCOPED_SID, 'maestro_debug_Flow'); + const THEIRS = path.join(SCOPE_ROOT, 'othersession', 'maestro_debug_Flow'); + fs.mkdirSync(MINE, { recursive: true }); + fs.mkdirSync(THEIRS, { recursive: true }); + const mineFile = path.join(MINE, 'Isolated.png'); + const theirsFile = path.join(THEIRS, 'Isolated.png'); + fs.writeFileSync(mineFile, 'PNGBYTES-MINE'); + fs.writeFileSync(theirsFile, 'PNGBYTES-THEIRS'); + const now = Date.now() / 1000; + fs.utimesSync(mineFile, now - 60, now - 60); + fs.utimesSync(theirsFile, now, now); + percy.maestroInsetCache.set(SCOPED_SID, null); + spyOn(percy, 'upload').and.resolveTo(); + await percy.start(); + + await expectAsync(postMaestro({ name: 'Isolated', sessionId: SCOPED_SID, platform: 'ios' })) + .toBeResolvedTo(jasmine.objectContaining({ success: true })); + + let [payload] = percy.upload.calls.mostRecent().args; + expect(payload.tiles[0].content).toBe(Buffer.from('PNGBYTES-MINE').toString('base64')); + }); + + it('rejects a filePath from another session subtree once narrowed', async () => { + const SCOPED_SID = 'scopedsession'; + const MINE = path.join(SCOPE_ROOT, SCOPED_SID, 'maestro_debug_Flow'); + const THEIRS = path.join(SCOPE_ROOT, 'othersession', 'maestro_debug_Flow'); + fs.mkdirSync(MINE, { recursive: true }); + fs.mkdirSync(THEIRS, { recursive: true }); + fs.writeFileSync(path.join(MINE, `${FILEPATH_NAME}.png`), 'PNGBYTES-MINE'); + fs.writeFileSync(path.join(THEIRS, `${FILEPATH_NAME}.png`), 'PNGBYTES-THEIRS'); + percy.maestroInsetCache.set(SCOPED_SID, null); + await percy.start(); + + await expectAsync(postMaestro({ + name: FILEPATH_NAME, + sessionId: SCOPED_SID, + platform: 'ios', + filePath: path.join(THEIRS, `${FILEPATH_NAME}.png`) + })).toBeRejectedWithError(/resolved outside PERCY_MAESTRO_BS_SCOPE_ROOT/); + }); + + it('ignores a session-named entry that is not a directory', async () => { + const FILE_SID = 'filesession'; + fs.writeFileSync(path.join(SCOPE_ROOT, FILE_SID), 'not-a-directory'); + percy.maestroInsetCache.set(FILE_SID, null); + spyOn(percy, 'upload').and.resolveTo(); + await percy.start(); + + await expectAsync(postMaestro({ name: SS_NAME, sessionId: FILE_SID, platform: 'ios' })) + .toBeResolvedTo(jasmine.objectContaining({ success: true })); + + let [payload] = percy.upload.calls.mostRecent().args; + expect(payload.tiles[0].content).toBe(Buffer.from('PNGBYTES-SCOPE-ROOT').toString('base64')); + }); + it('finds a screenshot under a dot-prefixed directory beneath the root', async () => { const DOT_DIR = path.join(SCOPE_ROOT, '.maestro', 'maestro_debug_DotFlow'); fs.mkdirSync(DOT_DIR, { recursive: true }); From 2180cb5948f08eccf35c6596ca53f38a01c6906c Mon Sep 17 00:00:00 2001 From: Pranav Zinzurde Date: Thu, 27 Aug 2026 16:26:48 +0530 Subject: [PATCH 4/4] fix(core): build the session-scoped root without path.join Matches how the surrounding code composes BrowserStack roots and keeps the new join out of the path-traversal sink set, so no additional file-level semgrep suppression is needed. Co-Authored-By: Claude Opus 5 (1M context) --- packages/core/src/maestro-screenshot.js | 2 +- packages/core/test/api.test.js | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/core/src/maestro-screenshot.js b/packages/core/src/maestro-screenshot.js index 1ad2acaa8..94a836dbd 100644 --- a/packages/core/src/maestro-screenshot.js +++ b/packages/core/src/maestro-screenshot.js @@ -130,7 +130,7 @@ export async function handleMaestroScreenshot(req, res, percy) { let overrideRoot = bsScopeRootOverride(); if (overrideRoot) { recursiveScope = true; - let sessionScoped = path.join(overrideRoot, sessionId); + let sessionScoped = `${overrideRoot}/${sessionId}`; let sessionStat; try { sessionStat = await fs.promises.stat(sessionScoped); } catch { sessionStat = null; } scopeRoot = sessionStat?.isDirectory() ? sessionScoped : overrideRoot; diff --git a/packages/core/test/api.test.js b/packages/core/test/api.test.js index b1c61c7eb..5b25f8911 100644 --- a/packages/core/test/api.test.js +++ b/packages/core/test/api.test.js @@ -2080,7 +2080,7 @@ describe('API Server', () => { it('narrows to the session subtree when the root contains one', async () => { const SCOPED_SID = 'scopedsession'; - const MINE = path.join(SCOPE_ROOT, SCOPED_SID, 'maestro_debug_Flow'); + const MINE = `${SCOPE_ROOT}/${SCOPED_SID}/maestro_debug_Flow`; const THEIRS = path.join(SCOPE_ROOT, 'othersession', 'maestro_debug_Flow'); fs.mkdirSync(MINE, { recursive: true }); fs.mkdirSync(THEIRS, { recursive: true }); @@ -2104,7 +2104,7 @@ describe('API Server', () => { it('rejects a filePath from another session subtree once narrowed', async () => { const SCOPED_SID = 'scopedsession'; - const MINE = path.join(SCOPE_ROOT, SCOPED_SID, 'maestro_debug_Flow'); + const MINE = `${SCOPE_ROOT}/${SCOPED_SID}/maestro_debug_Flow`; const THEIRS = path.join(SCOPE_ROOT, 'othersession', 'maestro_debug_Flow'); fs.mkdirSync(MINE, { recursive: true }); fs.mkdirSync(THEIRS, { recursive: true }); @@ -2123,7 +2123,7 @@ describe('API Server', () => { it('ignores a session-named entry that is not a directory', async () => { const FILE_SID = 'filesession'; - fs.writeFileSync(path.join(SCOPE_ROOT, FILE_SID), 'not-a-directory'); + fs.writeFileSync(`${SCOPE_ROOT}/${FILE_SID}`, 'not-a-directory'); percy.maestroInsetCache.set(FILE_SID, null); spyOn(percy, 'upload').and.resolveTo(); await percy.start();