Skip to content

fix(core): accept a host-supplied Maestro scope root #2404

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pranavz28 merged 4 commits into from
Aug 27, 2026
Merged

fix(core): accept a host-supplied Maestro scope root #2404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
862606e
fix(core): accept a host-supplied Maestro scope root (PER-10620)
pranavz28 Aug 27, 2026
bd5a3aa
fix(core): bound and label the scoped Maestro glob
pranavz28 Aug 27, 2026
494da75
fix(core): keep sessionId in scope under the Maestro root override
pranavz28 Aug 27, 2026
2180cb5
fix(core): build the session-scoped root without path.join
pranavz28 Aug 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 41 additions & 26 deletions packages/core/src/maestro-screenshot-file.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -15,31 +17,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 > MAX_SEARCH_DEPTH) return;
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);
Expand All @@ -66,7 +79,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;
Comment thread
pranavz28 marked this conversation as resolved.
let chosenFile;
if (filePath) {
chosenFile = filePath;
Expand All @@ -81,7 +95,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.
Expand All @@ -99,19 +113,17 @@ 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);

@pranavz28 pranavz28 Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Low] deep bound may sit one level short of the walker's cap, and newly bounds the self-hosted path

Two small follow-ups on the new cap — the original "unbounded glob" finding is resolved by this line.

  1. deep: MAX_SEARCH_DEPTH and the walker's if (depth > MAX_SEARCH_DEPTH) return are reported not to be equivalent: testing fast-glob 3.x, deep: 15 reaches one directory level less than the walker does. A file at exactly the boundary depth would then be found by the manual-walker fallback but missed by the primary glob. Flagging the provenance honestly — this is the reviewer's empirical result; I was not able to re-run the check myself.
  2. deep now also applies to the self-hosted path, which previously had no cap. A customer whose --test-output-dir nests deeper than the cap would newly get a 404 instead of a hit.

Suggestion: use deep: MAX_SEARCH_DEPTH + 1, or add a comment recording that the two bounds intentionally differ by a level. The self-hosted cap is worth a release-note line — it is a behaviour change on an existing path, even though it aligns that path with the walker's long-standing limit.

Reviewer: stack-code-reviewer

} 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
// the actionable PERCY_MAESTRO_SCREENSHOT_DIR guidance from the caller).
// 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) {
Expand Down Expand Up @@ -154,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;
Expand Down
22 changes: 17 additions & 5 deletions packages/core/src/maestro-screenshot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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
Expand All @@ -124,10 +125,21 @@ 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) {
recursiveScope = true;
let sessionScoped = `${overrideRoot}/${sessionId}`;

@pranavz28 pranavz28 Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Low] New stat TOCTOU lacks the explanatory comment its sibling carries

This stat sits in the same shape as the self-hosted PERCY_MAESTRO_SCREENSHOT_DIR stat a few lines above, which carries an explicit comment on why the race is acceptable (realpath, not stat, is the security boundary).

Functionally this is fine: the stat only chooses which string becomes scopeRoot, and locateScreenshot still realpaths both sides before the prefix check. But a future reader has to re-derive that, and could mistake it for a security control.

Suggestion: one line mirroring the existing comment at maestro-screenshot.js:116-121.

Reviewer: stack-code-reviewer

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}`
: `${appAutomateTmpDir()}/${sessionId}_test_suite`;
}
}

// Validate regions input shape early (before file I/O and ADB work) so
Expand All @@ -137,7 +149,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);
Expand Down
Loading
Loading