From c33bd163fa7041ca1b9cf5870b35655991f7f625 Mon Sep 17 00:00:00 2001 From: Ruturaj-Browserstack Date: Thu, 14 May 2026 14:59:29 +0530 Subject: [PATCH 1/2] fix: source per-user creds from BrowserStackConfig instead of process.env MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two runtime paths were reading BROWSERSTACK_USERNAME / BROWSERSTACK_ACCESS_KEY directly from process.env. In remote (multi-tenant) mode the per-user creds flow through the BrowserStackConfig object passed to each tool, not env vars, so these reads silently return undefined (401) or — worse — return a stale value left over from container startup, leaking another user's credentials. Changes: 1. triggerEspressoBuild (appautomate-utils/native-execution/appautomate.ts) Now takes a BrowserStackConfig parameter and derives auth via getBrowserStackAuth(config). Matches the existing pattern in the sibling triggerXcuiBuild. Caller in src/tools/appautomate.ts updated to thread config through. Throws a clear error when config is missing creds rather than silently base64-encoding "undefined:undefined". 2. getMavenCommandForWindows (sdk-utils/bstack/commands.ts) Now takes username/accessKey parameters, matching the sibling getMavenCommandForUnix. Previously the Windows branch read process.env while the Unix branch correctly took params. In remote mode the Windows variant emitted "undefined" into the Maven command shown to the user. Tests: - tests/tools/triggerEspressoBuild.test.ts — verifies creds come from config, not process.env (sets process.env to a sentinel and asserts the Authorization header decodes to the config values). Also covers the missing-creds error path. - tests/tools/sdk-utils-commands.test.ts — verifies getSDKPrefixCommand (nodejs / java unix / java windows) renders the passed username/accessKey and never the literal "undefined" or "process.env". Out of scope (separate follow-ups): - Module-level user-data globals in src/lib/inmemory-store.ts, src/tools/sdk-utils/percy-web/fetchPercyToken.ts, src/lib/tm-base-url.ts. All consumer tools are already on the remote wrapper's INITIALLY_DISABLED_TOOLS list; refactoring to request-scoped state is a larger change. - Code-template emitters under src/tools/*/appium-sdk/languages/*.ts and the WebdriverIO config snippets in sdk-utils/bstack/constants.ts and sdk-utils/percy-automate/constants.ts contain the literal text "process.env.BROWSERSTACK_USERNAME" — these are emitted as code for the user to paste into their own integration; they are not runtime reads and are intentionally left as-is. The prettier reformat on update-testcase.ts is an unrelated drive-by cleanup applied by `npm run format` (part of the build gate). --- .../native-execution/appautomate.ts | 12 ++-- src/tools/appautomate.ts | 1 + src/tools/sdk-utils/bstack/commands.ts | 9 +-- .../testmanagement-utils/update-testcase.ts | 11 ++-- tests/tools/sdk-utils-commands.test.ts | 49 +++++++++++++++ tests/tools/triggerEspressoBuild.test.ts | 61 +++++++++++++++++++ 6 files changed, 127 insertions(+), 16 deletions(-) create mode 100644 tests/tools/sdk-utils-commands.test.ts create mode 100644 tests/tools/triggerEspressoBuild.test.ts diff --git a/src/tools/appautomate-utils/native-execution/appautomate.ts b/src/tools/appautomate-utils/native-execution/appautomate.ts index 00a2f8b6..19e6fe09 100644 --- a/src/tools/appautomate-utils/native-execution/appautomate.ts +++ b/src/tools/appautomate-utils/native-execution/appautomate.ts @@ -2,6 +2,7 @@ import fs from "fs"; import FormData from "form-data"; import { apiClient } from "../../../lib/apiClient.js"; import { customFuzzySearch } from "../../../lib/fuzzy.js"; +import { getBrowserStackAuth } from "../../../lib/get-auth.js"; import { BrowserStackConfig } from "../../../lib/types.js"; interface Device { @@ -258,18 +259,15 @@ export async function triggerEspressoBuild( test_suite_url: string, devices: string[], project: string, + config: BrowserStackConfig, ): Promise { - const auth = { - username: process.env.BROWSERSTACK_USERNAME || "", - password: process.env.BROWSERSTACK_ACCESS_KEY || "", - }; + const authHeader = + "Basic " + Buffer.from(getBrowserStackAuth(config)).toString("base64"); const response = await apiClient.post({ url: "https://api-cloud.browserstack.com/app-automate/espresso/v2/build", headers: { - Authorization: - "Basic " + - Buffer.from(`${auth.username}:${auth.password}`).toString("base64"), + Authorization: authHeader, "Content-Type": "application/json", }, body: { diff --git a/src/tools/appautomate.ts b/src/tools/appautomate.ts index 192218f4..edcb246c 100644 --- a/src/tools/appautomate.ts +++ b/src/tools/appautomate.ts @@ -233,6 +233,7 @@ async function runAppTestsOnBrowserStack( test_suite_url, deviceStrings, args.project, + config, ); return { diff --git a/src/tools/sdk-utils/bstack/commands.ts b/src/tools/sdk-utils/bstack/commands.ts index 1f600d72..d2b0815f 100644 --- a/src/tools/sdk-utils/bstack/commands.ts +++ b/src/tools/sdk-utils/bstack/commands.ts @@ -43,7 +43,8 @@ const GRADLE_SETUP_INSTRUCTIONS = ` // Generates Maven archetype command for Windows platform function getMavenCommandForWindows( - framework: string, + username: string, + accessKey: string, mavenFramework: string, ): string { return ( @@ -54,8 +55,8 @@ function getMavenCommandForWindows( `-DgroupId="${MAVEN_ARCHETYPE_GROUP_ID}" ` + `-DartifactId="${MAVEN_ARCHETYPE_ARTIFACT_ID}" ` + `-Dversion="${MAVEN_ARCHETYPE_VERSION}" ` + - `-DBROWSERSTACK_USERNAME="${process.env.BROWSERSTACK_USERNAME}" ` + - `-DBROWSERSTACK_ACCESS_KEY="${process.env.BROWSERSTACK_ACCESS_KEY}" ` + + `-DBROWSERSTACK_USERNAME="${username}" ` + + `-DBROWSERSTACK_ACCESS_KEY="${accessKey}" ` + `-DBROWSERSTACK_FRAMEWORK="${mavenFramework}"` ); } @@ -85,7 +86,7 @@ function getJavaSDKInstructions( const platformLabel = isWindows ? "Windows" : "macOS/Linux"; const mavenCommand = isWindows - ? getMavenCommandForWindows(framework, mavenFramework) + ? getMavenCommandForWindows(username, accessKey, mavenFramework) : getMavenCommandForUnix(username, accessKey, mavenFramework); return `---STEP--- diff --git a/src/tools/testmanagement-utils/update-testcase.ts b/src/tools/testmanagement-utils/update-testcase.ts index ba4ed60f..7546a9d4 100644 --- a/src/tools/testmanagement-utils/update-testcase.ts +++ b/src/tools/testmanagement-utils/update-testcase.ts @@ -97,10 +97,7 @@ export const UpdateTestCaseSchema = z.object({ "Replacement list of linked Jira/Asana/Azure issue IDs for the test case.", ), custom_fields: z - .record( - z.string(), - z.union([z.string(), z.number(), z.boolean()]), - ) + .record(z.string(), z.union([z.string(), z.number(), z.boolean()])) .optional() .describe( "Map of custom field name/id to value. Valid field names and value types are per-project; discover them via the project's form fields.", @@ -118,7 +115,11 @@ export const UpdateTestCaseSchema = z.object({ * pass the raw value through so the backend can surface its own error. */ function normalizeDefaultFieldValue( - fieldValues: Array<{ internal_name?: string | null; name?: string; value: any }>, + fieldValues: Array<{ + internal_name?: string | null; + name?: string; + value: any; + }>, input: string, emit: "name" | "internal_name", ): string | undefined { diff --git a/tests/tools/sdk-utils-commands.test.ts b/tests/tools/sdk-utils-commands.test.ts new file mode 100644 index 00000000..1cb95729 --- /dev/null +++ b/tests/tools/sdk-utils-commands.test.ts @@ -0,0 +1,49 @@ +import { describe, it, expect, afterEach, beforeEach } from "vitest"; +import { getSDKPrefixCommand } from "../../src/tools/sdk-utils/bstack/commands"; + +describe("getSDKPrefixCommand", () => { + const originalPlatform = Object.getOwnPropertyDescriptor(process, "platform"); + + afterEach(() => { + if (originalPlatform) { + Object.defineProperty(process, "platform", originalPlatform); + } + }); + + beforeEach(() => { + // Guard: ensure these env vars never leak into the rendered command. + // The fix forwards `username` / `accessKey` parameters; reading process.env + // would silently return undefined or a stale value in remote (multi-tenant) mode. + delete process.env.BROWSERSTACK_USERNAME; + delete process.env.BROWSERSTACK_ACCESS_KEY; + }); + + it("nodejs: embeds passed username/accessKey, never reads process.env", () => { + const out = getSDKPrefixCommand("nodejs", "testng", "u-from-config", "k-from-config"); + expect(out).toContain("--username u-from-config"); + expect(out).toContain("--key k-from-config"); + expect(out).not.toContain("undefined"); + expect(out).not.toContain("process.env"); + }); + + it("java/unix: Maven command uses passed username/accessKey", () => { + Object.defineProperty(process, "platform", { value: "darwin" }); + const out = getSDKPrefixCommand("java", "testng", "u-from-config", "k-from-config"); + expect(out).toContain('-DBROWSERSTACK_USERNAME="u-from-config"'); + expect(out).toContain('-DBROWSERSTACK_ACCESS_KEY="k-from-config"'); + expect(out).not.toContain("undefined"); + expect(out).not.toContain("process.env"); + }); + + it("java/windows: Maven command uses passed username/accessKey (regression)", () => { + // Regression for a bug where the Windows branch read process.env.BROWSERSTACK_* + // while the Unix branch correctly took params. In remote mode this leaked the + // string "undefined" into the Maven command shown to the user. + Object.defineProperty(process, "platform", { value: "win32" }); + const out = getSDKPrefixCommand("java", "testng", "u-from-config", "k-from-config"); + expect(out).toContain('-DBROWSERSTACK_USERNAME="u-from-config"'); + expect(out).toContain('-DBROWSERSTACK_ACCESS_KEY="k-from-config"'); + expect(out).not.toContain("undefined"); + expect(out).not.toContain("process.env"); + }); +}); diff --git a/tests/tools/triggerEspressoBuild.test.ts b/tests/tools/triggerEspressoBuild.test.ts new file mode 100644 index 00000000..5fe1a251 --- /dev/null +++ b/tests/tools/triggerEspressoBuild.test.ts @@ -0,0 +1,61 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +vi.mock("../../src/lib/apiClient", () => ({ + apiClient: { + post: vi.fn(), + }, +})); + +vi.mock("../../src/logger", () => ({ + default: { error: vi.fn(), info: vi.fn(), debug: vi.fn() }, +})); + +import { apiClient } from "../../src/lib/apiClient"; +import { triggerEspressoBuild } from "../../src/tools/appautomate-utils/native-execution/appautomate"; + +const mockConfig = { + "browserstack-username": "config-user", + "browserstack-access-key": "config-key", +}; + +describe("triggerEspressoBuild — credential sourcing", () => { + beforeEach(() => { + vi.clearAllMocks(); + // Guard: even if process.env happens to be set, the function must not use it. + process.env.BROWSERSTACK_USERNAME = "env-user-should-be-ignored"; + process.env.BROWSERSTACK_ACCESS_KEY = "env-key-should-be-ignored"; + }); + + it("uses creds from config, not process.env", async () => { + (apiClient.post as any).mockResolvedValue({ data: { build_id: "BUILD-1" } }); + + const buildId = await triggerEspressoBuild( + "app.apk", + "tests.apk", + ["Samsung Galaxy S20-12.0"], + "p1", + mockConfig, + ); + + expect(buildId).toBe("BUILD-1"); + + const call = (apiClient.post as any).mock.calls[0][0]; + const authHeader = call.headers.Authorization as string; + expect(authHeader.startsWith("Basic ")).toBe(true); + + const decoded = Buffer.from(authHeader.slice("Basic ".length), "base64").toString(); + expect(decoded).toBe("config-user:config-key"); + expect(decoded).not.toContain("env-user-should-be-ignored"); + }); + + it("throws a clear error when config is missing creds (does not silently auth)", async () => { + await expect( + triggerEspressoBuild("app.apk", "tests.apk", ["d"], "p", { + "browserstack-username": "", + "browserstack-access-key": "", + } as any), + ).rejects.toThrow(/credentials not set/i); + + expect(apiClient.post).not.toHaveBeenCalled(); + }); +}); From 996f5624e88dbe74b4c647fdf2770798f9e5b76a Mon Sep 17 00:00:00 2001 From: Ruturaj-Browserstack Date: Thu, 14 May 2026 15:02:31 +0530 Subject: [PATCH 2/2] fix(tm-base-url): skip cache in remote MCP mode to avoid cross-region leak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The module-level `cachedBaseUrl` is process-shared. In remote (multi-tenant) mode, the first user to call `getTMBaseURL` populates it with their region's URL, and every subsequent user — including users on a different region's BrowserStack account — receives that same URL. Their requests then hit the wrong region's Test Management backend. Guard both the read and write of `cachedBaseUrl` behind `!appConfig.REMOTE_MCP`. In stdio (single-tenant) mode behavior is unchanged: discover once, cache, reuse. In remote mode the cache stays empty and each caller re-discovers their own region. Per-credentials caching (instead of skipping the cache entirely) would be faster but requires a larger change; the immediate priority is correctness. Adds tests/lib/tm-base-url.test.ts with two regression tests: one verifying the stdio mode still caches, one verifying remote mode re-discovers across calls with different region responses. --- src/lib/tm-base-url.ts | 12 +++++- tests/lib/tm-base-url.test.ts | 69 +++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 2 deletions(-) create mode 100644 tests/lib/tm-base-url.test.ts diff --git a/src/lib/tm-base-url.ts b/src/lib/tm-base-url.ts index 88c4e201..ad0c9f2a 100644 --- a/src/lib/tm-base-url.ts +++ b/src/lib/tm-base-url.ts @@ -2,6 +2,7 @@ import { apiClient } from "./apiClient.js"; import logger from "../logger.js"; import { BrowserStackConfig } from "./types.js"; import { getBrowserStackAuth } from "./get-auth.js"; +import appConfig from "../config.js"; const TM_BASE_URLS = [ "https://test-management.browserstack.com", @@ -14,7 +15,10 @@ let cachedBaseUrl: string | null = null; export async function getTMBaseURL( config: BrowserStackConfig, ): Promise { - if (cachedBaseUrl) { + // Skip the module-level cache in remote (multi-tenant) mode: it is process-shared, + // so the first user's region would be served to every subsequent user — breaking + // requests for users on a different region's BrowserStack account. + if (!appConfig.REMOTE_MCP && cachedBaseUrl) { logger.debug(`Using cached TM base URL: ${cachedBaseUrl}`); return cachedBaseUrl; } @@ -37,7 +41,11 @@ export async function getTMBaseURL( }); if (res.ok) { - cachedBaseUrl = baseUrl; + // Only populate the cache in single-tenant (stdio) mode; in remote mode + // the cache must stay empty so each user discovers their own region. + if (!appConfig.REMOTE_MCP) { + cachedBaseUrl = baseUrl; + } logger.info(`Selected TM base URL: ${baseUrl}`); return baseUrl; } diff --git a/tests/lib/tm-base-url.test.ts b/tests/lib/tm-base-url.test.ts new file mode 100644 index 00000000..f4ba3940 --- /dev/null +++ b/tests/lib/tm-base-url.test.ts @@ -0,0 +1,69 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +vi.mock("../../src/lib/apiClient", () => ({ + apiClient: { + get: vi.fn(), + }, +})); + +vi.mock("../../src/logger", () => ({ + default: { error: vi.fn(), info: vi.fn(), debug: vi.fn() }, +})); + +// Re-imported per-test by resetting modules so the module-level +// `cachedBaseUrl` and the mocked `appConfig.REMOTE_MCP` stay isolated +// across cases. +async function loadModule(remoteMcp: boolean) { + vi.resetModules(); + vi.doMock("../../src/config", () => ({ + __esModule: true, + default: { REMOTE_MCP: remoteMcp }, + })); + const apiClientMod = await import("../../src/lib/apiClient"); + const tmMod = await import("../../src/lib/tm-base-url"); + return { apiClient: apiClientMod.apiClient, getTMBaseURL: tmMod.getTMBaseURL }; +} + +const mockConfig = { + "browserstack-username": "u", + "browserstack-access-key": "k", +}; + +describe("getTMBaseURL — multi-tenant cache discipline", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("stdio mode (REMOTE_MCP=false): caches the discovered base URL across calls", async () => { + const { apiClient, getTMBaseURL } = await loadModule(false); + (apiClient.get as any).mockResolvedValueOnce({ ok: true }); + + const first = await getTMBaseURL(mockConfig); + expect(first).toBe("https://test-management.browserstack.com"); + expect(apiClient.get).toHaveBeenCalledTimes(1); + + // Second call must hit the cache; no additional HTTP call. + const second = await getTMBaseURL(mockConfig); + expect(second).toBe(first); + expect(apiClient.get).toHaveBeenCalledTimes(1); + }); + + it("remote mode (REMOTE_MCP=true): never caches, re-discovers each call", async () => { + const { apiClient, getTMBaseURL } = await loadModule(true); + // First user — region 1 succeeds on the first URL. + (apiClient.get as any).mockResolvedValueOnce({ ok: true }); + const userA = await getTMBaseURL(mockConfig); + expect(userA).toBe("https://test-management.browserstack.com"); + + // Second user (different region) — first URL fails, EU succeeds. + (apiClient.get as any) + .mockResolvedValueOnce({ ok: false }) + .mockResolvedValueOnce({ ok: true }); + const userB = await getTMBaseURL(mockConfig); + expect(userB).toBe("https://test-management-eu.browserstack.com"); + + // Three HTTP calls total: one for user A, two for user B. + // If the cache leaked across users, user B would have been served userA's URL with zero new calls. + expect(apiClient.get).toHaveBeenCalledTimes(3); + }); +});