Skip to content

Vulnerability in downstream package #486

Description

Regular Expression Denial of Service (ReDoS) in cross-spawn

I am getting this audit result using the latest tfx-cli package (0.18.0)

cross-spawn <6.0.6
Severity: high
Regular Expression Denial of Service (ReDoS) in cross-spawn - GHSA-3xgq-45jj-v275
fix available via npm audit fix --force
Will install tfx-cli@0.5.14, which is a breaking change
node_modules/cross-spawn
execa 0.5.0 - 0.9.0
Depends on vulnerable versions of cross-spawn
node_modules/execa
clipboardy <=1.2.3
Depends on vulnerable versions of execa
node_modules/clipboardy
tfx-cli >=0.6.0
Depends on vulnerable versions of clipboardy
node_modules/tfx-cli

Fixing this appears to be pretty simple. Upgrading clipboardy to 4.0.0 would resolve the issue. It looks like the api has changed slightly, but since it is only used on one line in tfcommand.ts it should be quite simple to upgrade.

Activity

  1. tarunramsinghani commented on Jan 29, 2025

    @tarunramsinghani
    Contributor

    Fixed via #487

  2. tarunramsinghani commented on Feb 4, 2025

    @tarunramsinghani
    Contributor

    Reopening. The proper fix is under development.

  3. CrypticGuy commented on Apr 15, 2025

    @CrypticGuy

    Hi Tarun Ramsinghani (@tarunramsinghani) , any update on this.
    The npm audit recommends to install v0.19.0, which is deprecated itself and I tried running it and got an error related to clipboardy.
    If you need someone to take it up, let me know, I can try contributing for this issue. Not sure, how tricky that would be, but can give it a try.

  4. pbcahill commented on Apr 15, 2025

    @pbcahill

    FYI to workaround the vulnerability while waiting for the actual fix, this is what we added to our package.json to override the cross-spawn transitive dependency version:

    "overrides": {
      "execa": {
        "cross-spawn": "^6.0.6"
      }
    }
    

    Definitely not ideal long term, but seems to still work fine for building/publishing our custom ADO extensions.

  5. CrypticGuy commented on Apr 26, 2025

    @CrypticGuy

    Issue: Integrating clipboardy v4 (ESM) with TypeScript/CommonJS codebase

    Problem Description

    tfx-cli uses clipboardy to copy output to the clipboard. After upgrading to clipboardy v4.0.0, we faced module compatibility issues since v4 is an ES Module (ESM) while our project uses CommonJS.

    Error:

    Error [ERR_REQUIRE_ESM]: require() of ES Module .../node_modules/clipboardy/index.js from .../lib/tfcommand.js not supported.
    Instead change the require of index.js to a dynamic import() which is available in all CommonJS modules.
    

    Attempted Solutions

    1. Direct ESM import in TypeScript

    import clipboardy from "clipboardy";

    Result: Failed. TypeScript compiled this to require() due to our "module": "commonjs" in tsconfig.json, still causing the ESM error.

    2. Dynamic import compatibility layer

    let clipboardyModule: ClipboardyModule | null = null;
    const getClipboardy = async () => {
      if (!clipboardyModule) {
        clipboardyModule = await import('clipboardy');
      }
      return clipboardyModule.default;
    };
    
    export async function write(text: string): Promise<void> {
      const clipboardy = await getClipboardy();
      return clipboardy.write(text);
    }

    Result: Failed. When compiled, TypeScript converted dynamic imports in ways that still triggered the ESM error.

    On further investigation and head-banging with some LLMs as well, it seems like the issue could be due to some tsconfig configurations. On resolving the dynamic module approach, I noticed that the compiled code still uses the require approach for fetching clipboardy instead of using the import method.
    So, to bypass the compilation and usage process entirely thought of using a child process bridge for resolving this.

    Implemented a child_process bridge:

    1. Created an ESM helper script:
    // clipboardy-helper.mjs
    import clipboardy from 'clipboardy';
    
    const action = process.argv[2];
    const text = process.argv[3];
    
    async function run() {
      try {
        switch (action) {
          case 'write':
            await clipboardy.write(text);
            process.stdout.write('success');
            break;
          case 'read':
            process.stdout.write(await clipboardy.read());
            break;
          // other methods...
        }
      } catch (error) {
        process.stderr.write(error.message);
        process.exit(1);
      }
    }
    
    run();
    1. Built a compatibility layer using child_process:
    // lib/clipboardyCompat.ts
    import { execFile } from 'child_process';
    import * as path from 'path';
    
    const helperPath = path.join(__dirname, 'clipboardy-helper.mjs');
    
    function runHelper(action: string, text?: string): Promise<string> {
      return new Promise((resolve, reject) => {
        execFile('node', [helperPath, action, text], (error, stdout, stderr) => {
          if (error) reject(new Error(stderr || error.message));
          else resolve(stdout);
        });
      });
    }
    
    export async function write(text: string): Promise<void> {
      await runHelper('write', text);
    }
    
    export async function read(): Promise<string> {
      return runHelper('read');
    }
    // other methods...
    1. Modified postbuild script to copy the .mjs file:
      "postbuild": "ncp app/tfx-cli.js _build/tfx-cli.js && ncp package.json _build/package.json && ncp app/exec/build/tasks/_resources _build/exec/build/tasks/_resources && ncp app/lib/clipboardy-helper.mjs _build/lib/clipboardy-helper.mjs",

    Questions

    1. Is this child_process approach a reasonable solution, or should we consider alternatives?
    2. Are there performance concerns with spawning a child process for clipboard operations? And if they are, are they significant enough for just one functionality?

    cc: Tarun Ramsinghani (@tarunramsinghani)

  6. CrypticGuy commented on May 18, 2025

    @CrypticGuy

    Tarun Ramsinghani (@tarunramsinghani) gentle reminder for reviewing the approach. If it looks good, I can go ahead with raising the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions