From 0b084ed05bb28aa826efc99287dd49202176b6b9 Mon Sep 17 00:00:00 2001 From: rishigupta1599 Date: Thu, 30 Jul 2026 14:29:09 +0530 Subject: [PATCH 1/3] fix(core): let BS hosts inject a complete Maestro scope root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #2353 assumed the App Automate tmp-dir relocation only moved the root while preserving the {root}/{sessionId}{_test_suite} layout beneath it. That holds on Android, where the suite moved to //test-suites/_test_suite/logs/*/screenshots — so relocating the root via PERCY_APP_AUTOMATE_TMP_DIR composes correctly. It does not hold on iOS. realmobile's relocation moved Maestro debug output to //logs/maestro_debug_*, which drops the sessionId path segment entirely (the CLI appends it, so no tmp-root value can absorb /logs) and drops the _ prefix that the *_maestro_debug_* glob keys on. Screenshot lookups there 404 regardless of what PERCY_APP_AUTOMATE_TMP_DIR is set to. Add PERCY_MAESTRO_BS_SCOPE_ROOT for layouts the convention can no longer express. When set, the value IS the scope root: the relay globs {root}/**/{name}.png with no layout assumption, the manual-walker fallback recurses the same root, and the realpath containment check anchors on it — the security boundary is relocated, never widened. Non-absolute values are ignored so a malformed injection falls back to the composed convention rather than producing a cwd-relative root. Android and self-hosted (PERCY_MAESTRO_SCREENSHOT_DIR) are untouched. Co-Authored-By: Claude Opus 5 (1M context) --- packages/core/src/maestro-screenshot-file.js | 88 +++++++++--- packages/core/src/maestro-screenshot.js | 29 +++- packages/core/test/api.test.js | 140 ++++++++++++++++++- 3 files changed, 228 insertions(+), 29 deletions(-) diff --git a/packages/core/src/maestro-screenshot-file.js b/packages/core/src/maestro-screenshot-file.js index 17a12e4d5..ceae52385 100644 --- a/packages/core/src/maestro-screenshot-file.js +++ b/packages/core/src/maestro-screenshot-file.js @@ -15,31 +15,64 @@ export function appAutomateTmpDir() { return path.isAbsolute(dir) ? dir : '/tmp'; } +// Complete session scope root, injected by hosts whose layout no longer fits +// the {appAutomateTmpDir()}/{sessionId}{_test_suite} convention at all — where +// relocating the root alone can't express the new shape. +// +// realmobile's AAP-18965 relocation is the motivating case: iOS Maestro debug +// output moved to //logs/maestro_debug_*, which drops the +// sessionId path segment entirely (the CLI appends it, so no tmp-root value can +// absorb /logs) and drops the _ prefix the *_maestro_debug_* +// glob keys on. Android kept its shape under the relocation and stays on +// PERCY_APP_AUTOMATE_TMP_DIR. +// +// When set, this value IS the scope root: the relay globs {root}/**/{name}.png +// with no layout assumption, and the realpath containment check anchors on it — +// so the security boundary is relocated, never widened. Non-absolute values are +// ignored (falling through to the composed convention) rather than producing a +// cwd-relative root; trailing separators are trimmed for the same +// prefix-composition reason as appAutomateTmpDir(). BrowserStack-mode only — +// self-hosted scoping stays on PERCY_MAESTRO_SCREENSHOT_DIR. +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); - } + // Shared recursive walker for both root-relative layouts. `accept` gates + // which matching files count, so the iOS convention can keep its + // `_maestro_debug_` guard while an explicit scope root — where the host has + // already narrowed the tree to one session — takes any depth. + 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); @@ -60,13 +93,19 @@ async function manualScreenshotWalk(platform, sessionId, name) { // 1. `filePath` supplied (BrowserStack new SDK — absolute path under the BS // session root; rejected upstream in self-hosted mode). // 2. BrowserStack glob (the BS-infra SCREENSHOTS_DIR layout). -// 3. Self-hosted recursive glob under scopeRoot (PERCY_MAESTRO_SCREENSHOT_DIR). +// 3. Recursive glob under scopeRoot — self-hosted +// (PERCY_MAESTRO_SCREENSHOT_DIR) or a BS host that injected an explicit +// scope root (PERCY_MAESTRO_BS_SCOPE_ROOT); `recursiveScope` selects it. // Either way, the shared realpath + scopeRoot prefix check below enforces the // security invariant. Returns the canonicalized absolute path, or throws // 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 }) { + // Self-hosted always scopes recursively under its root; BS does so only when + // the host injected an explicit scope root. Derived here rather than trusted + // from the caller so `selfHosted` alone keeps its existing meaning. + let scopedGlob = selfHosted || !!recursiveScope; let chosenFile; if (filePath) { chosenFile = filePath; @@ -80,8 +119,11 @@ export async function locateScreenshot({ platform, sessionId, name, filePath, sc // Self-hosted: recursive glob under the customer's --test-output-dir // (scopeRoot = PERCY_MAESTRO_SCREENSHOT_DIR). `name` is SAFE_ID-validated // by the caller, so it cannot contain separators or traversal chars. + // BS explicit scope root (PERCY_MAESTRO_BS_SCOPE_ROOT): same recursive + // glob — the host already narrowed scopeRoot to this session's dir, so + // there is no convention left to key on. 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. @@ -108,10 +150,14 @@ export async function locateScreenshot({ platform, sessionId, name, filePath, sc // Fast-glob import / glob call failed — fall back to manual walker (BS // only; self-hosted has no fixed-layout convention, so empty → 404 with // the actionable PERCY_MAESTRO_SCREENSHOT_DIR guidance from the caller). + // On BS the walker mirrors the glob: an explicit scope root recurses that + // root, otherwise it follows the platform convention. // 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..2999a39a3 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'; @@ -91,13 +91,18 @@ export async function handleMaestroScreenshot(req, res, percy) { // Resolve the file-find scope root. On BrowserStack (sessionId present), the // root is the BS host's {appAutomateTmpDir()}/{sessionId}{_test_suite} - // convention (PERCY_APP_AUTOMATE_TMP_DIR, defaulting to /tmp). Self-hosted + // convention (PERCY_APP_AUTOMATE_TMP_DIR, defaulting to /tmp), unless the + // host injected a complete scope root via PERCY_MAESTRO_BS_SCOPE_ROOT — for + // layouts that convention can no longer express. Self-hosted // (sessionId absent) requires PERCY_MAESTRO_SCREENSHOT_DIR (read from // process.env, never the request body) to be an absolute, existing directory // — typically the customer's `maestro test --test-output-dir ` path. The // realpath + prefix check inside locateScreenshot enforces the security // invariant at whichever root applies; the boundary is relocated, not removed. let scopeRoot; + // Recursive-glob mode: no layout convention to key on, so the relay searches + // the whole scope root. True for self-hosted and for a BS explicit root. + 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 +129,22 @@ 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`; + // Host-injected complete scope root wins over the composed convention. No + // existence pre-check here (unlike self-hosted): this is host config, not + // customer config, so a stale/missing root should surface as the same 404 + // the containment check emits rather than a 400 aimed at the customer. + 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 +154,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 df0f6ec3d..0eef435e5 100644 --- a/packages/core/test/api.test.js +++ b/packages/core/test/api.test.js @@ -5,7 +5,7 @@ import { logger, 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', @@ -1915,6 +1915,142 @@ describe('API Server', () => { }); }); + // PERCY_MAESTRO_BS_SCOPE_ROOT override — for BS layouts the + // {tmpRoot}/{sessionId}{_test_suite} convention can no longer express at + // all. realmobile's AAP-18965 relocation is the motivating case: iOS + // Maestro debug output moved to //logs/maestro_debug_*, + // which has no sessionId segment and no _ prefix, so relocating + // the tmp root alone can't compose either the scope root or the glob. + // Real-fs root for the same fast-glob binding-staleness reason as above. + describe('PERCY_MAESTRO_BS_SCOPE_ROOT override', () => { + // Mirrors the realmobile shape: the host points at the per-session dir + // directly, and the debug dir underneath matches no CLI convention. + 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; + // Not the /tmp IOS_DIR fixture the composed convention would have hit + 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; + // Fell through to the default /tmp glob, not a cwd-relative root + expect(payload.tiles[0].content).toBe(Buffer.from('PNGBYTES-ANDROID').toString('base64')); + }); + + it('re-anchors filePath containment on the overridden root', async () => { + // Same-root filePath resolves; a file outside it does not — the + // boundary moved with the root rather than being removed. + 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 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 75d079cefc4a9162e8226123f968753a55793e8d Mon Sep 17 00:00:00 2001 From: Aryan Kumar Date: Fri, 14 Aug 2026 13:44:36 +0530 Subject: [PATCH 2/3] chore(core): shorten code comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Trims the comments added by this PR from 55 lines to 37. Keeps what the code doesn't already say: why the convention can't express the realmobile layout, that the override IS the scope root so the boundary is relocated rather than widened, why there is no existence pre-check on host config, and what `accept` gates in the shared walker. Folds the '/'-rejection rationale into the existing non-absolute sentence instead of adding a line for it. Comments only — no code change. Co-Authored-By: Claude Opus 5 (1M context) --- packages/core/src/maestro-screenshot-file.js | 49 ++++++++------------ packages/core/src/maestro-screenshot.js | 16 +++---- packages/core/test/api.test.js | 11 ++--- 3 files changed, 29 insertions(+), 47 deletions(-) diff --git a/packages/core/src/maestro-screenshot-file.js b/packages/core/src/maestro-screenshot-file.js index ceae52385..cbb483237 100644 --- a/packages/core/src/maestro-screenshot-file.js +++ b/packages/core/src/maestro-screenshot-file.js @@ -15,24 +15,16 @@ export function appAutomateTmpDir() { return path.isAbsolute(dir) ? dir : '/tmp'; } -// Complete session scope root, injected by hosts whose layout no longer fits -// the {appAutomateTmpDir()}/{sessionId}{_test_suite} convention at all — where -// relocating the root alone can't express the new shape. -// -// realmobile's AAP-18965 relocation is the motivating case: iOS Maestro debug -// output moved to //logs/maestro_debug_*, which drops the -// sessionId path segment entirely (the CLI appends it, so no tmp-root value can -// absorb /logs) and drops the _ prefix the *_maestro_debug_* -// glob keys on. Android kept its shape under the relocation and stays on +// Complete session scope root for BS hosts whose layout the +// {appAutomateTmpDir()}/{sessionId}{_test_suite} convention can't express: +// realmobile's AAP-18965 iOS relocation drops both the sessionId segment and +// the _ prefix the glob keys on. Android kept its shape and stays on // PERCY_APP_AUTOMATE_TMP_DIR. // -// When set, this value IS the scope root: the relay globs {root}/**/{name}.png -// with no layout assumption, and the realpath containment check anchors on it — -// so the security boundary is relocated, never widened. Non-absolute values are -// ignored (falling through to the composed convention) rather than producing a -// cwd-relative root; trailing separators are trimmed for the same -// prefix-composition reason as appAutomateTmpDir(). BrowserStack-mode only — -// self-hosted scoping stays on PERCY_MAESTRO_SCREENSHOT_DIR. +// When set this IS the scope root — the relay globs {root}/**/{name}.png and +// the realpath containment check anchors on it, so the boundary is relocated, +// never widened. Non-absolute values are ignored rather than resolved against +// cwd, which also rejects '/' once the trailing separator is trimmed. export function bsScopeRootOverride() { let raw = process.env.PERCY_MAESTRO_BS_SCOPE_ROOT; if (!raw) return null; @@ -48,10 +40,9 @@ export function bsScopeRootOverride() { rationale (upstream SAFE_ID validation, depth cap, exact filename match). */ async function manualScreenshotWalk(platform, sessionId, name, scopeRoot) { const files = []; - // Shared recursive walker for both root-relative layouts. `accept` gates - // which matching files count, so the iOS convention can keep its - // `_maestro_debug_` guard while an explicit scope root — where the host has - // already narrowed the tree to one session — takes any depth. + // `accept` gates which matches count: the iOS convention keeps its + // `_maestro_debug_` guard, while an explicit scope root (already narrowed to + // one session by the host) takes anything. const walkFrom = async (root, accept) => { const walk = async (dir, depth) => { if (depth > 15) return; // sanity cap @@ -94,17 +85,16 @@ async function manualScreenshotWalk(platform, sessionId, name, scopeRoot) { // session root; rejected upstream in self-hosted mode). // 2. BrowserStack glob (the BS-infra SCREENSHOTS_DIR layout). // 3. Recursive glob under scopeRoot — self-hosted -// (PERCY_MAESTRO_SCREENSHOT_DIR) or a BS host that injected an explicit -// scope root (PERCY_MAESTRO_BS_SCOPE_ROOT); `recursiveScope` selects it. +// (PERCY_MAESTRO_SCREENSHOT_DIR) or a BS explicit root +// (PERCY_MAESTRO_BS_SCOPE_ROOT); `recursiveScope` selects it. // Either way, the shared realpath + scopeRoot prefix check below enforces the // security invariant. Returns the canonicalized absolute path, or throws // 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, recursiveScope }) { - // Self-hosted always scopes recursively under its root; BS does so only when - // the host injected an explicit scope root. Derived here rather than trusted - // from the caller so `selfHosted` alone keeps its existing meaning. + // Derived here rather than trusted from the caller so `selfHosted` alone + // keeps its existing meaning. let scopedGlob = selfHosted || !!recursiveScope; let chosenFile; if (filePath) { @@ -119,9 +109,8 @@ export async function locateScreenshot({ platform, sessionId, name, filePath, sc // Self-hosted: recursive glob under the customer's --test-output-dir // (scopeRoot = PERCY_MAESTRO_SCREENSHOT_DIR). `name` is SAFE_ID-validated // by the caller, so it cannot contain separators or traversal chars. - // BS explicit scope root (PERCY_MAESTRO_BS_SCOPE_ROOT): same recursive - // glob — the host already narrowed scopeRoot to this session's dir, so - // there is no convention left to key on. + // BS explicit scope root: same recursive glob — the host already narrowed + // scopeRoot to this session, so there is no convention left to key on. let searchPattern; if (scopedGlob) { // fast-glob requires forward-slashes in patterns on every platform; on @@ -150,8 +139,8 @@ export async function locateScreenshot({ platform, sessionId, name, filePath, sc // Fast-glob import / glob call failed — fall back to manual walker (BS // only; self-hosted has no fixed-layout convention, so empty → 404 with // the actionable PERCY_MAESTRO_SCREENSHOT_DIR guidance from the caller). - // On BS the walker mirrors the glob: an explicit scope root recurses that - // root, otherwise it follows the platform convention. + // The walker mirrors the glob: an explicit scope root recurses that root, + // otherwise it follows the platform convention. // 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. */ diff --git a/packages/core/src/maestro-screenshot.js b/packages/core/src/maestro-screenshot.js index 2999a39a3..36c723ee8 100644 --- a/packages/core/src/maestro-screenshot.js +++ b/packages/core/src/maestro-screenshot.js @@ -91,17 +91,16 @@ export async function handleMaestroScreenshot(req, res, percy) { // Resolve the file-find scope root. On BrowserStack (sessionId present), the // root is the BS host's {appAutomateTmpDir()}/{sessionId}{_test_suite} - // convention (PERCY_APP_AUTOMATE_TMP_DIR, defaulting to /tmp), unless the - // host injected a complete scope root via PERCY_MAESTRO_BS_SCOPE_ROOT — for - // layouts that convention can no longer express. Self-hosted + // convention (PERCY_APP_AUTOMATE_TMP_DIR, defaulting to /tmp), unless the host + // injected a complete root via PERCY_MAESTRO_BS_SCOPE_ROOT. Self-hosted // (sessionId absent) requires PERCY_MAESTRO_SCREENSHOT_DIR (read from // process.env, never the request body) to be an absolute, existing directory // — typically the customer's `maestro test --test-output-dir ` path. The // realpath + prefix check inside locateScreenshot enforces the security // invariant at whichever root applies; the boundary is relocated, not removed. let scopeRoot; - // Recursive-glob mode: no layout convention to key on, so the relay searches - // the whole scope root. True for self-hosted and for a BS explicit root. + // No convention to key on, so search the whole root. True for self-hosted and + // for a BS explicit root. let recursiveScope = false; if (selfHosted) { // Reject filePath outright in self-hosted mode. The SDK never emits it (it @@ -131,10 +130,9 @@ export async function handleMaestroScreenshot(req, res, percy) { scopeRoot = dir; recursiveScope = true; } else { - // Host-injected complete scope root wins over the composed convention. No - // existence pre-check here (unlike self-hosted): this is host config, not - // customer config, so a stale/missing root should surface as the same 404 - // the containment check emits rather than a 400 aimed at the customer. + // No existence pre-check here (unlike self-hosted): host config, not + // customer config, so a stale root should surface as the containment + // check's 404 rather than a 400 aimed at the customer. let overrideRoot = bsScopeRootOverride(); if (overrideRoot) { scopeRoot = overrideRoot; diff --git a/packages/core/test/api.test.js b/packages/core/test/api.test.js index 0eef435e5..4830d0de2 100644 --- a/packages/core/test/api.test.js +++ b/packages/core/test/api.test.js @@ -1915,16 +1915,11 @@ describe('API Server', () => { }); }); - // PERCY_MAESTRO_BS_SCOPE_ROOT override — for BS layouts the - // {tmpRoot}/{sessionId}{_test_suite} convention can no longer express at - // all. realmobile's AAP-18965 relocation is the motivating case: iOS - // Maestro debug output moved to //logs/maestro_debug_*, - // which has no sessionId segment and no _ prefix, so relocating - // the tmp root alone can't compose either the scope root or the glob. + // Mirrors the realmobile AAP-18965 shape: the host points straight at the + // per-session dir, and the debug dir under it has no sessionId segment and + // no _ prefix, so no tmp-root value composes it. // Real-fs root for the same fast-glob binding-staleness reason as above. describe('PERCY_MAESTRO_BS_SCOPE_ROOT override', () => { - // Mirrors the realmobile shape: the host points at the per-session dir - // directly, and the debug dir underneath matches no CLI convention. 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; From b49b1234966beee168253d476423f022e6dc97a0 Mon Sep 17 00:00:00 2001 From: Aryan Kumar Date: Fri, 14 Aug 2026 13:52:49 +0530 Subject: [PATCH 3/3] chore(core): trim comments further MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second pass on review feedback. 37 lines down to 22 (55 before the first pass). bsScopeRootOverride's doc is 10 lines down to 4, keeping only what the code can't show: why the convention can't express the layout, and that the override relocates the containment boundary rather than widening it. Comments only — no code change. Co-Authored-By: Claude Opus 5 (1M context) --- packages/core/src/maestro-screenshot-file.js | 33 +++++++------------- packages/core/src/maestro-screenshot.js | 8 ++--- packages/core/test/api.test.js | 10 +++--- 3 files changed, 18 insertions(+), 33 deletions(-) diff --git a/packages/core/src/maestro-screenshot-file.js b/packages/core/src/maestro-screenshot-file.js index cbb483237..753e57ce9 100644 --- a/packages/core/src/maestro-screenshot-file.js +++ b/packages/core/src/maestro-screenshot-file.js @@ -15,16 +15,10 @@ export function appAutomateTmpDir() { return path.isAbsolute(dir) ? dir : '/tmp'; } -// Complete session scope root for BS hosts whose layout the -// {appAutomateTmpDir()}/{sessionId}{_test_suite} convention can't express: -// realmobile's AAP-18965 iOS relocation drops both the sessionId segment and -// the _ prefix the glob keys on. Android kept its shape and stays on -// PERCY_APP_AUTOMATE_TMP_DIR. -// -// When set this IS the scope root — the relay globs {root}/**/{name}.png and -// the realpath containment check anchors on it, so the boundary is relocated, -// never widened. Non-absolute values are ignored rather than resolved against -// cwd, which also rejects '/' once the trailing separator is trimmed. +// Complete scope root for hosts whose layout {appAutomateTmpDir()}/{sessionId} +// can't express (realmobile's AAP-18965 iOS move). When set this IS the root: +// globbed recursively, and the realpath containment check anchors on it — so +// the boundary moves, never widens. Non-absolute is ignored, which drops '/'. export function bsScopeRootOverride() { let raw = process.env.PERCY_MAESTRO_BS_SCOPE_ROOT; if (!raw) return null; @@ -40,9 +34,8 @@ export function bsScopeRootOverride() { rationale (upstream SAFE_ID validation, depth cap, exact filename match). */ async function manualScreenshotWalk(platform, sessionId, name, scopeRoot) { const files = []; - // `accept` gates which matches count: the iOS convention keeps its - // `_maestro_debug_` guard, while an explicit scope root (already narrowed to - // one session by the host) takes anything. + // `accept` gates matches: iOS keeps its `_maestro_debug_` guard, an explicit + // root (already narrowed to one session) takes anything. const walkFrom = async (root, accept) => { const walk = async (dir, depth) => { if (depth > 15) return; // sanity cap @@ -84,17 +77,14 @@ async function manualScreenshotWalk(platform, sessionId, name, scopeRoot) { // 1. `filePath` supplied (BrowserStack new SDK — absolute path under the BS // session root; rejected upstream in self-hosted mode). // 2. BrowserStack glob (the BS-infra SCREENSHOTS_DIR layout). -// 3. Recursive glob under scopeRoot — self-hosted -// (PERCY_MAESTRO_SCREENSHOT_DIR) or a BS explicit root -// (PERCY_MAESTRO_BS_SCOPE_ROOT); `recursiveScope` selects it. +// 3. Recursive glob under scopeRoot — self-hosted, or a BS explicit root. // Either way, the shared realpath + scopeRoot prefix check below enforces the // security invariant. Returns the canonicalized absolute path, or throws // 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, recursiveScope }) { - // Derived here rather than trusted from the caller so `selfHosted` alone - // keeps its existing meaning. + // Derived, not trusted from the caller, so `selfHosted` keeps its meaning. let scopedGlob = selfHosted || !!recursiveScope; let chosenFile; if (filePath) { @@ -109,8 +99,8 @@ export async function locateScreenshot({ platform, sessionId, name, filePath, sc // Self-hosted: recursive glob under the customer's --test-output-dir // (scopeRoot = PERCY_MAESTRO_SCREENSHOT_DIR). `name` is SAFE_ID-validated // by the caller, so it cannot contain separators or traversal chars. - // BS explicit scope root: same recursive glob — the host already narrowed - // scopeRoot to this session, so there is no convention left to key on. + // BS explicit root: same recursive glob — the host already narrowed + // scopeRoot to this session, so no convention is left to key on. let searchPattern; if (scopedGlob) { // fast-glob requires forward-slashes in patterns on every platform; on @@ -139,8 +129,7 @@ export async function locateScreenshot({ platform, sessionId, name, filePath, sc // Fast-glob import / glob call failed — fall back to manual walker (BS // only; self-hosted has no fixed-layout convention, so empty → 404 with // the actionable PERCY_MAESTRO_SCREENSHOT_DIR guidance from the caller). - // The walker mirrors the glob: an explicit scope root recurses that root, - // otherwise it follows the platform convention. + // The walker mirrors the glob: explicit root recurses it, else convention. // 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. */ diff --git a/packages/core/src/maestro-screenshot.js b/packages/core/src/maestro-screenshot.js index 36c723ee8..aa90f4c20 100644 --- a/packages/core/src/maestro-screenshot.js +++ b/packages/core/src/maestro-screenshot.js @@ -99,8 +99,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; - // No convention to key on, so search the whole root. True for self-hosted and - // for a BS explicit root. + // Search the whole root — self-hosted, or a BS explicit root. let recursiveScope = false; if (selfHosted) { // Reject filePath outright in self-hosted mode. The SDK never emits it (it @@ -130,9 +129,8 @@ export async function handleMaestroScreenshot(req, res, percy) { scopeRoot = dir; recursiveScope = true; } else { - // No existence pre-check here (unlike self-hosted): host config, not - // customer config, so a stale root should surface as the containment - // check's 404 rather than a 400 aimed at the customer. + // No existence pre-check (unlike self-hosted): host config, not customer + // config, so a stale root should 404, not 400 at the customer. let overrideRoot = bsScopeRootOverride(); if (overrideRoot) { scopeRoot = overrideRoot; diff --git a/packages/core/test/api.test.js b/packages/core/test/api.test.js index 4830d0de2..cea66cff0 100644 --- a/packages/core/test/api.test.js +++ b/packages/core/test/api.test.js @@ -1915,10 +1915,9 @@ describe('API Server', () => { }); }); - // Mirrors the realmobile AAP-18965 shape: the host points straight at the - // per-session dir, and the debug dir under it has no sessionId segment and - // no _ prefix, so no tmp-root value composes it. - // Real-fs root for the same fast-glob binding-staleness reason as above. + // Mirrors the realmobile AAP-18965 shape: no sessionId segment and no + // _ prefix, so no tmp-root value composes it. Real-fs root for the + // same fast-glob binding-staleness reason as above. 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'); @@ -2002,8 +2001,7 @@ describe('API Server', () => { }); it('re-anchors filePath containment on the overridden root', async () => { - // Same-root filePath resolves; a file outside it does not — the - // boundary moved with the root rather than being removed. + // In-root filePath resolves, out-of-root does not: boundary moved, not removed. fs.writeFileSync(path.join(REALMOBILE_DIR, `${FILEPATH_NAME}.png`), 'PNGBYTES-SCOPE-FILEPATH'); spyOn(percy, 'upload').and.resolveTo(); await percy.start();