Skip to content

fix(core): let BS hosts inject a complete Maestro scope root #2357

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

Open
rishigupta1599 wants to merge 1 commit into
base: master
Choose a base branch
from
+228 −29
Open

fix(core): let BS hosts inject a complete Maestro scope root #2357

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
88 changes: 67 additions & 21 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 @@ -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 <aa-tmp>/<device>/logs/maestro_debug_*, which drops the
// sessionId path segment entirely (the CLI appends it, so no tmp-root value can
// absorb <device>/logs) and drops the <device>_ 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);
Expand All @@ -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;
Expand All @@ -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.
Expand All @@ -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) {
Expand Down
29 changes: 23 additions & 6 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 @@ -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 <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
Expand All @@ -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
Expand All @@ -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);
Expand Down
140 changes: 138 additions & 2 deletions packages/core/test/api.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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',
Expand Down Expand Up @@ -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 <aa-tmp>/<device>/logs/maestro_debug_*,
// which has no sessionId segment and no <device>_ 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
Expand Down
Loading