diff --git a/.changeset/sdk-6948-binary-url-fallback.md b/.changeset/sdk-6948-binary-url-fallback.md new file mode 100644 index 00000000..442acf26 --- /dev/null +++ b/.changeset/sdk-6948-binary-url-fallback.md @@ -0,0 +1,7 @@ +--- +"@wdio/browserstack-service": patch +--- + +fix(cli): fall back to `BROWSERSTACK_BINARY_URL` when `update_cli` fails (SDK-6948) + +`update_cli` runs during bootstrap against production `api.browserstack.com`, so on an internal staging run (`BROWSERSTACK_STAGING_ENV`) it answers `401`. The SDK then found no CLI binary and fell back to the non-CLI flow, or silently reused a stale cached binary. When `BROWSERSTACK_BINARY_URL` is set and `update_cli` fails (non-2xx or network error), the binary is now downloaded from that URL. If that download fails, a cached binary is used when there is one. A 2xx reply with neither `url` nor `updated_cli_version` keeps the cached binary, and only downloads from the URL when none is cached. `CLIUtils.requestToUpdateCLI` now rejects on a non-2xx reply (with `response.statusCode`) instead of resolving the error body. With `BROWSERSTACK_BINARY_URL` unset, behaviour is unchanged: a non-2xx reply keeps the existing binary, and network errors propagate. diff --git a/packages/browserstack-service/src/cli/cliUtils.ts b/packages/browserstack-service/src/cli/cliUtils.ts index 03b335f2..a5608bba 100644 --- a/packages/browserstack-service/src/cli/cliUtils.ts +++ b/packages/browserstack-service/src/cli/cliUtils.ts @@ -227,56 +227,101 @@ export class CLIUtils { PerformanceTester.start(PerformanceEvents.SDK_CLI_CHECK_UPDATE) logger.info(`Current CLI Path Found: ${existingCliPath}`) - const queryParams: Record = { - sdk_version: CLIUtils.getSdkVersion(), - os: platform(), - os_arch: arch(), - cli_version: '0', - sdk_language: this.getSdkLanguage(), - } - if (!isNullOrEmpty(existingCliPath)) { - // If binary is busy (being executed by another process), skip version check - // and API call entirely — use existing binary as-is - if (this.isBinaryBusy(existingCliPath)) { - logger.warn(`Existing binary is currently in use, skipping update: ${existingCliPath}`) - PerformanceTester.end(PerformanceEvents.SDK_CLI_CHECK_UPDATE) - return existingCliPath + // Single close for the span on every path, including a rejected download. + try { + const queryParams: Record = { + sdk_version: CLIUtils.getSdkVersion(), + os: platform(), + os_arch: arch(), + cli_version: '0', + sdk_language: this.getSdkLanguage(), } - const version = await this.runShellCommand( - `${existingCliPath} version`, - ) - if (version.toLowerCase().includes('text file busy')) { - logger.warn(`Binary busy during version check, skipping update: ${existingCliPath}`) - PerformanceTester.end(PerformanceEvents.SDK_CLI_CHECK_UPDATE) - return existingCliPath + if (!isNullOrEmpty(existingCliPath)) { + // If binary is busy (being executed by another process), skip version check + // and API call entirely — use existing binary as-is + if (this.isBinaryBusy(existingCliPath)) { + logger.warn(`Existing binary is currently in use, skipping update: ${existingCliPath}`) + return existingCliPath + } + const version = await this.runShellCommand( + `${existingCliPath} version`, + ) + if (version.toLowerCase().includes('text file busy')) { + logger.warn(`Binary busy during version check, skipping update: ${existingCliPath}`) + return existingCliPath + } + queryParams.cli_version = version } - queryParams.cli_version = version - } - const response = await this.requestToUpdateCLI(queryParams, config) - if (nestedKeyValue(response, ['updated_cli_version'])) { - logger.debug( - `Need to update binary, current binary version: ${queryParams.cli_version}`, - ) - const browserStackBinaryUrl = process.env.BROWSERSTACK_BINARY_URL || null - if (!isNullOrEmpty(browserStackBinaryUrl)) { - logger.debug( - `Using BROWSERSTACK_BINARY_URL: ${browserStackBinaryUrl}`, + + const fallbackToBinaryUrl = async (reason: unknown) => { + logger.warn( + `update_cli request failed (${reason}); falling back to BROWSERSTACK_BINARY_URL`, ) - response.url = browserStackBinaryUrl + try { + return await this.downloadLatestBinary(browserStackBinaryUrl as string, cliDir) + } catch (downloadErr) { + if (isNullOrEmpty(existingCliPath)) { + throw downloadErr + } + logger.warn( + `BROWSERSTACK_BINARY_URL download failed (${util.format(downloadErr)}); using cached binary ${existingCliPath}`, + ) + return existingCliPath + } } - const finalBinaryPath = await this.downloadLatestBinary( - nestedKeyValue(response, ['url']), - cliDir, - nestedKeyValue(response, ['updated_cli_version']), - ) + let response + try { + response = await this.requestToUpdateCLI(queryParams, config) + } catch (err) { + // update_cli runs during bootstrap — before the binary is spawned and before GRR + // localizes the API hosts — so it always targets the production api host. On an internal + // staging run (BROWSERSTACK_STAGING_ENV) the staging creds are rejected there (401). If an + // explicit binary URL was supplied, use it so the run is not blocked on this call. Opt-in + // only: with no BROWSERSTACK_BINARY_URL behaviour is exactly as before. + const statusCode = (err as { response?: { statusCode?: number } })?.response?.statusCode + if (!isNullOrEmpty(browserStackBinaryUrl)) { + return await fallbackToBinaryUrl(statusCode ?? (err as Error)?.message) + } + if (statusCode !== undefined) { + // A non-2xx reply used to be returned as a body without `updated_cli_version`, + // which kept the existing binary — preserve that. + return existingCliPath + } + throw err + } + + // A 2xx reply with neither `updated_cli_version` nor `url` means there is nothing to update: + // keep a cached binary, and only fetch from the explicit binary URL when there is none. + if (!isNullOrEmpty(browserStackBinaryUrl) && isNullOrEmpty(existingCliPath) && + !nestedKeyValue(response, ['updated_cli_version']) && !nestedKeyValue(response, ['url'])) { + return await fallbackToBinaryUrl(JSON.stringify(response)) + } + + if (nestedKeyValue(response, ['updated_cli_version'])) { + logger.debug( + `Need to update binary, current binary version: ${queryParams.cli_version}`, + ) + + if (!isNullOrEmpty(browserStackBinaryUrl)) { + logger.debug( + `Using BROWSERSTACK_BINARY_URL: ${browserStackBinaryUrl}`, + ) + response.url = browserStackBinaryUrl + } + + return await this.downloadLatestBinary( + nestedKeyValue(response, ['url']), + cliDir, + nestedKeyValue(response, ['updated_cli_version']), + ) + } + return existingCliPath + } finally { PerformanceTester.end(PerformanceEvents.SDK_CLI_CHECK_UPDATE) - return finalBinaryPath } - PerformanceTester.end(PerformanceEvents.SDK_CLI_CHECK_UPDATE) - return existingCliPath } static getCliDir() { @@ -428,6 +473,16 @@ export class CLIUtils { `${APIUtils.BROWSERSTACK_AUTOMATE_API_URL}/${UPDATED_CLI_ENDPOINT}?${params.toString()}`, requestInit, ) + if (!response.ok) { + // Check the status before parsing: an LB error page (HTML/empty 5xx) must still carry + // its status code rather than surface as a SyntaxError. + const body = await response.json().catch(() => null) + logger.debug(`response ${response.status} ${JSON.stringify(body)}`) + throw Object.assign( + new Error(`update_cli request failed with status ${response.status}`), + { response: { statusCode: response.status, body } }, + ) + } const jsonResponse = await response.json() logger.debug(`response ${JSON.stringify(jsonResponse)}`) return jsonResponse diff --git a/packages/browserstack-service/tests/cli/cliUtils.test.ts b/packages/browserstack-service/tests/cli/cliUtils.test.ts index aa82167c..99f52393 100644 --- a/packages/browserstack-service/tests/cli/cliUtils.test.ts +++ b/packages/browserstack-service/tests/cli/cliUtils.test.ts @@ -459,6 +459,172 @@ describe('CLIUtils', () => { mockConfig ) }) + + it('falls back to BROWSERSTACK_BINARY_URL when update_cli fails (SDK-6948)', async () => { + const saved = process.env.BROWSERSTACK_BINARY_URL + process.env.BROWSERSTACK_BINARY_URL = 'https://example.com/staging-binary.zip' + try { + const fallbackPath = '/mock/cli/dir/binary-fallback' + vi.spyOn(CLIUtils, 'runShellCommand').mockResolvedValue('1.0.0') + // update_cli fails (e.g. staging creds against the prod api host -> 401) + vi.spyOn(CLIUtils, 'requestToUpdateCLI').mockRejectedValue({ response: { statusCode: 401 } }) + vi.spyOn(CLIUtils, 'downloadLatestBinary').mockResolvedValue(fallbackPath) + + const result = await CLIUtils.checkAndUpdateCli(mockExistingPath, mockCliDir, mockConfig) + + expect(result).toBe(fallbackPath) + expect(CLIUtils.downloadLatestBinary).toHaveBeenCalledWith('https://example.com/staging-binary.zip', mockCliDir) + } finally { + if (saved === undefined) { delete process.env.BROWSERSTACK_BINARY_URL } else { process.env.BROWSERSTACK_BINARY_URL = saved } + } + }) + + it('re-throws when update_cli fails and no BROWSERSTACK_BINARY_URL is set (unchanged behaviour)', async () => { + const saved = process.env.BROWSERSTACK_BINARY_URL + delete process.env.BROWSERSTACK_BINARY_URL + try { + const err = new Error('Unauthorized') + vi.spyOn(CLIUtils, 'runShellCommand').mockResolvedValue('1.0.0') + vi.spyOn(CLIUtils, 'requestToUpdateCLI').mockRejectedValue(err) + const downloadSpy = vi.spyOn(CLIUtils, 'downloadLatestBinary').mockResolvedValue('/should/not/be/used') + + await expect(CLIUtils.checkAndUpdateCli(mockExistingPath, mockCliDir, mockConfig)).rejects.toBe(err) + expect(downloadSpy).not.toHaveBeenCalled() + } finally { + if (saved !== undefined) { process.env.BROWSERSTACK_BINARY_URL = saved } + } + }) + }) + + describe('checkAndUpdateCli update_cli 401 (SDK-6948)', () => { + const mockCliDir = '/mock/cli/dir' + const mockExistingPath = '/mock/cli/dir/binary-existing' + const config = { user: 'testuser', key: 'testkey' } as Options.Testrunner + const savedBinaryUrl = process.env.BROWSERSTACK_BINARY_URL + const savedFetch = global.fetch + const savedProxy = { http: process.env.HTTP_PROXY, https: process.env.HTTPS_PROXY } + + beforeEach(() => { + // _fetch routes through undici's ProxyAgent when a proxy var is set, bypassing global.fetch + delete process.env.HTTP_PROXY + delete process.env.HTTPS_PROXY + vi.spyOn(PerformanceTester, 'start').mockImplementation(() => {}) + vi.spyOn(PerformanceTester, 'end').mockImplementation(() => {}) + vi.spyOn(CLIUtils, 'runShellCommand').mockResolvedValue('1.0.0') + vi.spyOn(CLIUtils, 'isBinaryBusy').mockReturnValue(false) + global.fetch = vi.fn().mockResolvedValue({ + ok: false, + status: 401, + json: vi.fn().mockResolvedValue({ message: 'Unauthorized' }) + }) + }) + + afterEach(() => { + global.fetch = savedFetch + if (savedProxy.http !== undefined) { process.env.HTTP_PROXY = savedProxy.http } + if (savedProxy.https !== undefined) { process.env.HTTPS_PROXY = savedProxy.https } + if (savedBinaryUrl === undefined) { + delete process.env.BROWSERSTACK_BINARY_URL + } else { + process.env.BROWSERSTACK_BINARY_URL = savedBinaryUrl + } + vi.restoreAllMocks() + }) + + it('falls back to BROWSERSTACK_BINARY_URL when update_cli answers 401 with a JSON body', async () => { + process.env.BROWSERSTACK_BINARY_URL = 'https://example.com/staging-binary.zip' + const download = vi.spyOn(CLIUtils, 'downloadLatestBinary').mockResolvedValue('/mock/cli/dir/binary-fallback') + + const result = await CLIUtils.checkAndUpdateCli('', mockCliDir, config) + + expect(result).toBe('/mock/cli/dir/binary-fallback') + expect(download).toHaveBeenCalledWith('https://example.com/staging-binary.zip', mockCliDir) + expect(global.fetch).toHaveBeenCalledTimes(1) + }) + + it('keeps the existing binary on a 401 when BROWSERSTACK_BINARY_URL is unset (unchanged behaviour)', async () => { + delete process.env.BROWSERSTACK_BINARY_URL + const download = vi.spyOn(CLIUtils, 'downloadLatestBinary').mockResolvedValue('/should/not/be/used') + + const result = await CLIUtils.checkAndUpdateCli(mockExistingPath, mockCliDir, config) + + expect(result).toBe(mockExistingPath) + expect(download).not.toHaveBeenCalled() + expect(global.fetch).toHaveBeenCalledTimes(1) + }) + + it('keeps the cached binary when a 2xx body has neither url nor updated_cli_version', async () => { + process.env.BROWSERSTACK_BINARY_URL = 'https://example.com/staging-binary.zip' + global.fetch = vi.fn().mockResolvedValue({ + ok: true, + status: 200, + json: vi.fn().mockResolvedValue({ success: true }) + }) + const download = vi.spyOn(CLIUtils, 'downloadLatestBinary').mockResolvedValue('/should/not/be/used') + + const result = await CLIUtils.checkAndUpdateCli(mockExistingPath, mockCliDir, config) + + expect(result).toBe(mockExistingPath) + expect(download).not.toHaveBeenCalled() + expect(global.fetch).toHaveBeenCalledTimes(1) + }) + + it('falls back when a 2xx body has neither url nor updated_cli_version and no binary is cached', async () => { + process.env.BROWSERSTACK_BINARY_URL = 'https://example.com/staging-binary.zip' + global.fetch = vi.fn().mockResolvedValue({ + ok: true, + status: 200, + json: vi.fn().mockResolvedValue({ message: 'Unauthorized' }) + }) + const download = vi.spyOn(CLIUtils, 'downloadLatestBinary').mockResolvedValue('/mock/cli/dir/binary-fallback') + + const result = await CLIUtils.checkAndUpdateCli('', mockCliDir, config) + + expect(result).toBe('/mock/cli/dir/binary-fallback') + expect(download).toHaveBeenCalledWith('https://example.com/staging-binary.zip', mockCliDir) + expect(global.fetch).toHaveBeenCalledTimes(1) + }) + + it('uses the cached binary when the fallback download fails', async () => { + process.env.BROWSERSTACK_BINARY_URL = 'https://example.com/staging-binary.zip' + vi.spyOn(CLIUtils, 'downloadLatestBinary').mockRejectedValue(new Error('download failed')) + + const result = await CLIUtils.checkAndUpdateCli(mockExistingPath, mockCliDir, config) + + expect(result).toBe(mockExistingPath) + expect(PerformanceTester.end).toHaveBeenCalledTimes(1) + }) + + it('returns an empty path on a 401 with no cached binary and no BROWSERSTACK_BINARY_URL', async () => { + delete process.env.BROWSERSTACK_BINARY_URL + const download = vi.spyOn(CLIUtils, 'downloadLatestBinary').mockResolvedValue('/should/not/be/used') + + const result = await CLIUtils.checkAndUpdateCli('', mockCliDir, config) + + expect(result).toBe('') + expect(download).not.toHaveBeenCalled() + expect(global.fetch).toHaveBeenCalledTimes(1) + }) + + it('propagates a rejected fallback download and still closes the span', async () => { + process.env.BROWSERSTACK_BINARY_URL = 'https://example.com/staging-binary.zip' + const downloadErr = new Error('download failed') + vi.spyOn(CLIUtils, 'downloadLatestBinary').mockRejectedValue(downloadErr) + + await expect(CLIUtils.checkAndUpdateCli('', mockCliDir, config)).rejects.toBe(downloadErr) + expect(PerformanceTester.end).toHaveBeenCalledTimes(1) + expect(PerformanceTester.end).toHaveBeenCalledWith(PerformanceEvents.SDK_CLI_CHECK_UPDATE) + }) + + it('treats an empty BROWSERSTACK_BINARY_URL as unset', async () => { + process.env.BROWSERSTACK_BINARY_URL = '' + const download = vi.spyOn(CLIUtils, 'downloadLatestBinary').mockResolvedValue('/should/not/be/used') + + const result = await CLIUtils.checkAndUpdateCli(mockExistingPath, mockCliDir, config) + + expect(result).toBe(mockExistingPath) + expect(download).not.toHaveBeenCalled() + }) }) describe('setupCliPath', () => { @@ -521,17 +687,24 @@ describe('CLIUtils', () => { user: 'testuser', key: 'testkey' } as Options.Testrunner + const savedProxy = { http: process.env.HTTP_PROXY, https: process.env.HTTPS_PROXY } beforeEach(() => { vi.resetAllMocks() + // _fetch routes through undici's ProxyAgent when a proxy var is set, bypassing global.fetch + delete process.env.HTTP_PROXY + delete process.env.HTTPS_PROXY // Mock fetch to return a mock response global.fetch = vi.fn().mockResolvedValue({ + ok: true, json: vi.fn().mockResolvedValue({ status: 'success' }) }) }) afterEach(() => { + if (savedProxy.http !== undefined) { process.env.HTTP_PROXY = savedProxy.http } + if (savedProxy.https !== undefined) { process.env.HTTPS_PROXY = savedProxy.https } vi.clearAllMocks() }) @@ -543,6 +716,7 @@ describe('CLIUtils', () => { const mockJsonResponse = { updated_cli_version: '2.0.0' } global.fetch = vi.fn().mockResolvedValue({ + ok: true, json: vi.fn().mockResolvedValue(mockJsonResponse) }) @@ -566,6 +740,7 @@ describe('CLIUtils', () => { it('returns response from fetch', async () => { const mockResponse = { updated_cli_version: '2.0.0' } global.fetch = vi.fn().mockResolvedValue({ + ok: true, json: vi.fn().mockResolvedValue(mockResponse) }) @@ -574,6 +749,32 @@ describe('CLIUtils', () => { expect(result).toEqual(mockResponse) }) + it('rejects with the status code on a non-2xx reply (SDK-6948)', async () => { + global.fetch = vi.fn().mockResolvedValue({ + ok: false, + status: 401, + json: vi.fn().mockResolvedValue({ message: 'Unauthorized' }) + }) + + await expect(CLIUtils.requestToUpdateCLI({}, mockConfig)) + .rejects + .toMatchObject({ response: { statusCode: 401, body: { message: 'Unauthorized' } } }) + expect(global.fetch).toHaveBeenCalledTimes(1) + }) + + it('keeps the status code when a non-2xx reply has a non-JSON body (SDK-6948)', async () => { + global.fetch = vi.fn().mockResolvedValue({ + ok: false, + status: 502, + json: vi.fn().mockRejectedValue(new SyntaxError('Unexpected token <')) + }) + + await expect(CLIUtils.requestToUpdateCLI({}, mockConfig)) + .rejects + .toMatchObject({ response: { statusCode: 502, body: null } }) + expect(global.fetch).toHaveBeenCalledTimes(1) + }) + it('handles errors from fetch', async () => { const mockError = new Error('Network error') global.fetch = vi.fn().mockRejectedValue(mockError)