@W-23807282 - prevent command injection via server-provided redirectUrl (RCE)

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.

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<AddStageEnviron
         environmentName,
         orgType,
         onCreated: (data) => {
-          const url = decodeRedirectUrl(data.redirectUrl);
+          const url = sanitizeRedirectUrl(decodeRedirectUrl(data.redirectUrl));
           if (!noBrowser) {
             openUrl(url);
             this.log(messages.getMessage('info.BrowserOpened'));

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]/);
+      });
+  });
 });