From c5d690d4b3e2c795109977c88991d1bae3042dd4 Mon Sep 17 00:00:00 2001 From: ad-shreya Date: Thu, 13 Aug 2026 20:07:56 +0530 Subject: [PATCH] fix: prevent command injection via server-provided redirectUrl The devops stage environment add command opened the OAuth redirectUrl returned by the target org using exec() with the URL interpolated into a shell string. A malicious endpoint could return a redirectUrl containing shell metacharacters (e.g. `safe" & calc.exe & rem "`) to break out of the quoted URL and execute arbitrary commands, leading to RCE. Fix (defense-in-depth): - Switch from exec() to execFile(), passing the URL as a discrete argument so no shell interprets it. Windows uses `cmd /c start "" `. - Add sanitizeRedirectUrl(): parse with the WHATWG URL API, reject any non-http(s) scheme (blocks javascript:, file:, and non-URL payloads), and return the normalized href so residual quotes/spaces are percent-encoded. Add regression tests covering the reported payload, scheme rejection, and metacharacter normalization. Co-Authored-By: Claude Opus 4.8 (1M context) --- messages/devops.stage.environment.add.md | 4 + src/commands/devops/stage/environment/add.ts | 38 ++++- .../devops/stage/environment/add.test.ts | 132 +++++++++++++++++- 3 files changed, 167 insertions(+), 7 deletions(-) diff --git a/messages/devops.stage.environment.add.md b/messages/devops.stage.environment.add.md index 207d1140..9130687e 100644 --- a/messages/devops.stage.environment.add.md +++ b/messages/devops.stage.environment.add.md @@ -63,3 +63,7 @@ Failed to create environment for stage: %s # error.AuthTimeout Authentication timed out. The environment was created but not yet authenticated. Re-run the command or authenticate manually via the org's DevOps Center setup. + +# error.InvalidRedirectUrl + +The server returned an invalid authentication redirect URL: %s. Expected an http or https URL. diff --git a/src/commands/devops/stage/environment/add.ts b/src/commands/devops/stage/environment/add.ts index 12113c39..72d437f9 100644 --- a/src/commands/devops/stage/environment/add.ts +++ b/src/commands/devops/stage/environment/add.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { exec } from 'node:child_process'; +import { execFile } from 'node:child_process'; import { Messages, Org } from '@salesforce/core'; import { SfCommand, Flags } from '@salesforce/sf-plugins-core'; import { addStageEnvironment, AddStageEnvironmentResult, OrgType } from '../../../../utils/addStageEnvironment.js'; @@ -26,10 +26,40 @@ Messages.importMessagesDirectoryFromMetaUrl(import.meta.url); const messages = Messages.loadMessages('@salesforce/plugin-devops-center', 'devops.stage.environment.add'); const commonErrorMessages = Messages.loadMessages('@salesforce/plugin-devops-center', 'commonErrors'); +/** + * Validates that a server-provided redirect URL is a well-formed http(s) URL, and + * returns its normalized form. Rejects anything else so a malicious endpoint can't + * smuggle shell metacharacters or non-web schemes (javascript:, file:, etc.) into + * the browser-open step. Throws on invalid input. + */ +function sanitizeRedirectUrl(url: string): string { + let parsed: URL; + try { + parsed = new URL(url); + } catch { + throw new Error(messages.getMessage('error.InvalidRedirectUrl', [url])); + } + if (parsed.protocol !== 'https:' && parsed.protocol !== 'http:') { + throw new Error(messages.getMessage('error.InvalidRedirectUrl', [url])); + } + // Return the normalized href: quotes, spaces, and other unsafe characters are + // percent-encoded, so no shell metacharacters survive. + return parsed.href; +} + function openUrl(url: string): void { const platform = process.platform; - const cmd = platform === 'darwin' ? 'open' : platform === 'win32' ? 'start' : 'xdg-open'; - exec(`${cmd} "${url}"`); + // Pass the URL as a separate argument (never interpolated into a shell string), + // so it can't be interpreted as a command even if it contained metacharacters. + // On Windows, `start` is a cmd builtin; its first quoted argument is the window + // title, so pass an empty title before the URL. + if (platform === 'darwin') { + execFile('open', [url]); + } else if (platform === 'win32') { + execFile('cmd', ['/c', 'start', '', url]); + } else { + execFile('xdg-open', [url]); + } } function decodeRedirectUrl(url: string): string { @@ -113,7 +143,7 @@ export default class DevopsStageEnvironmentAdd extends SfCommand { - const url = decodeRedirectUrl(data.redirectUrl); + const url = sanitizeRedirectUrl(decodeRedirectUrl(data.redirectUrl)); if (!noBrowser) { openUrl(url); this.log(messages.getMessage('info.BrowserOpened')); diff --git a/test/commands/devops/stage/environment/add.test.ts b/test/commands/devops/stage/environment/add.test.ts index 8f35915b..802a20a0 100644 --- a/test/commands/devops/stage/environment/add.test.ts +++ b/test/commands/devops/stage/environment/add.test.ts @@ -28,7 +28,7 @@ describe('devops stage environment add', () => { const mockOrg = { id: '1', getOrgId: () => '1', getConnection: () => mockConnection, getUsername: () => 'testOrg' }; const addStageEnvironmentStub = sinon.stub(); const fetchPipelineStagesStub = sinon.stub(); - const execStub = sinon.stub(); + const execFileStub = sinon.stub(); before(async () => { const mod = await esmock('../../../../../src/commands/devops/stage/environment/add.js', { @@ -39,7 +39,7 @@ describe('devops stage environment add', () => { fetchPipelineStages: fetchPipelineStagesStub, }, 'node:child_process': { - exec: execStub, + execFile: execFileStub, }, }); AddEnvironmentCommand = mod.default; @@ -49,7 +49,7 @@ describe('devops stage environment add', () => { sandbox = sinon.createSandbox(); addStageEnvironmentStub.reset(); fetchPipelineStagesStub.reset(); - execStub.reset(); + execFileStub.reset(); queryStub.reset(); queryStub.resolves({ records: [{ IsActive: false }] }); }); @@ -332,4 +332,130 @@ describe('devops stage environment add', () => { } }); }); + + describe('redirect URL is opened safely (RCE regression)', () => { + test + .stdout() + .stderr() + .it('passes the URL as a separate argument, never a shell string', async () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + sandbox.stub(Org, 'create' as any).returns(mockOrg); + fetchPipelineStagesStub.resolves([{ Id: '0Xp000000000001', Name: 'Production' }]); + addStageEnvironmentStub.callsFake( + async (params: { onCreated?: (data: { environmentId: string; redirectUrl: string }) => void }) => { + params.onCreated?.({ + environmentId: '0Hi000000000001', + redirectUrl: 'https://login.salesforce.com/services/oauth2/authorize?client_id=abc', + }); + return { + success: true, + stageId: '0Xp000000000001', + environmentId: '0Hi000000000001', + environmentName: 'Production_Org', + orgType: 'Production', + pipelineId: '0Xo000000000001', + redirectUrl: 'https://login.salesforce.com/services/oauth2/authorize?client_id=abc', + namedCredential: 'Production_Org_NC', + organizationId: '00D000000000001', + }; + } + ); + + await AddEnvironmentCommand.run([ + '--target-org', + 'testOrg', + '--pipeline-id', + '0Xo000000000001', + '--stage-id', + '0Xp000000000001', + '--environment-name', + 'Production_Org', + '--org-type', + 'Production', + ]); + + expect(execFileStub.calledOnce).to.equal(true); + const [, args] = execFileStub.firstCall.args as [string, string[]]; + // The URL must be an element of the args array, not concatenated into a command string. + expect(args).to.include('https://login.salesforce.com/services/oauth2/authorize?client_id=abc'); + }); + + test + .stdout() + .stderr() + .it('rejects a malicious redirectUrl that is not an http(s) URL', async (ctx) => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + sandbox.stub(Org, 'create' as any).returns(mockOrg); + fetchPipelineStagesStub.resolves([{ Id: '0Xp000000000001', Name: 'Production' }]); + addStageEnvironmentStub.callsFake( + async (params: { onCreated?: (data: { environmentId: string; redirectUrl: string }) => void }) => { + // Payload from the vulnerability report: breaks out of the quoted URL on Windows. + params.onCreated?.({ + environmentId: '0Hi000000000001', + redirectUrl: 'safe" & calc.exe & rem "', + }); + return { success: true }; + } + ); + + try { + await AddEnvironmentCommand.run([ + '--target-org', + 'testOrg', + '--pipeline-id', + '0Xo000000000001', + '--stage-id', + '0Xp000000000001', + '--environment-name', + 'Production_Org', + '--org-type', + 'Production', + ]); + expect.fail('should have thrown'); + } catch (e) { + // expected + } + + // The browser-open must never run for a non-URL payload. + expect(execFileStub.called).to.equal(false); + expect(ctx.stderr).to.contain('invalid authentication redirect URL'); + }); + + test + .stdout() + .stderr() + .it('normalizes a crafted http URL so shell metacharacters are percent-encoded', async () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + sandbox.stub(Org, 'create' as any).returns(mockOrg); + fetchPipelineStagesStub.resolves([{ Id: '0Xp000000000001', Name: 'Production' }]); + addStageEnvironmentStub.callsFake( + async (params: { onCreated?: (data: { environmentId: string; redirectUrl: string }) => void }) => { + params.onCreated?.({ + environmentId: '0Hi000000000001', + redirectUrl: 'https://login.salesforce.com/x" & calc.exe & rem "', + }); + return { success: true, environmentId: '0Hi000000000001' }; + } + ); + + await AddEnvironmentCommand.run([ + '--target-org', + 'testOrg', + '--pipeline-id', + '0Xo000000000001', + '--stage-id', + '0Xp000000000001', + '--environment-name', + 'Production_Org', + '--org-type', + 'Production', + ]); + + expect(execFileStub.calledOnce).to.equal(true); + const [, args] = execFileStub.firstCall.args as [string, string[]]; + const opened = args[args.length - 1]; + // No literal double-quote or space survives normalization. + expect(opened).to.not.match(/["\s]/); + }); + }); });