Skip to content

Conversation

@nemanjarogic
Copy link

@nemanjarogic nemanjarogic commented Dec 2, 2025

Fix glob package vulnerability by updating packages from which it is referenced (shelljs and mocha).

This only partially fixes the problem, since we depend on internal implementation of these packages and the newest version of mocha is based on glob 10.4.5. However, suggestion is to upgrade to 10.5.0, 11.1.0 or higher.

Additional explanation about changes made in PR:

  1. node make.js test for /node directory for master branch (without any of mine changes) fails with the following error
node_modules/@types/node/buffer.d.ts(552,19): error TS2430: Interface 'Buffer' incorrectly extends interface 'Uint8Array<ArrayBufferLike>'.      
  The types of 'slice(...).buffer' are incompatible between these types.
    Type 'ArrayBufferLike' is not assignable to type 'ArrayBuffer'.
      Type 'SharedArrayBuffer' is not assignable to type 'ArrayBuffer'.
        Types of property '[Symbol.toStringTag]' are incompatible.
          Type '"SharedArrayBuffer"' is not assignable to type '"ArrayBuffer"'.
Unhandled rejection: exec:
Error: exec:
    at Object.error (C:\Repos\Github\azure-pipelines-task-lib\node\node_modules\shelljs\src\common.js:110:27)
    at execSync (C:\Repos\Github\azure-pipelines-task-lib\node\node_modules\shelljs\src\exec.js:120:12)
    at _exec (C:\Repos\Github\azure-pipelines-task-lib\node\node_modules\shelljs\src\exec.js:223:12)
    at C:\Repos\Github\azure-pipelines-task-lib\node\node_modules\shelljs\src\common.js:335:23
    at run (C:\Repos\Github\azure-pipelines-task-lib\node\buildutils.js:15:14)
    at target.test (C:\Repos\Github\azure-pipelines-task-lib\node\make.js:42:5)
  1. node make.js test for /powershell directory for master branch (without any of mine changes) fails with the following error

mocha tool:
Unhandled rejection: mocha not found.  might need to run npm install
Error: mocha not found.  might need to run npm install
    at Object.ensureTool (C:\Repos\Github\azure-pipelines-task-lib\powershell\make-util.js:89:15)
    at target.test (C:\Repos\Github\azure-pipelines-task-lib\powershell\make.js:69:10)
    at global.target.<computed> [as test] (C:\Repos\Github\azure-pipelines-task-lib\powershell\node_modules\shelljs\make.js:36:40)
    at C:\Repos\Github\azure-pipelines-task-lib\powershell\node_modules\shelljs\make.js:48:27
    at Array.forEach (<anonymous>)
    at Timeout._onTimeout (C:\Repos\Github\azure-pipelines-task-lib\powershell\node_modules\shelljs\make.js:46:10)
    at listOnTimeout (node:internal/timers:588:17)
    at process.processTimers (node:internal/timers:523:7)
  1. The mentioned problems are not the only ones, because consequent errors will be shown when errors above are fixed. Therefore, changes in this PR also fix testing infrastructure.

Work item: ADO#2335511

@nemanjarogic nemanjarogic requested review from a team as code owners December 2, 2025 13:16
@nemanjarogic
Copy link
Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@nemanjarogic
Copy link
Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@nemanjarogic
Copy link
Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@nemanjarogic
Copy link
Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@nemanjarogic
Copy link
Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

"outDir": "../_test",
"moduleResolution": "node"
"moduleResolution": "node",
"skipLibCheck": true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nemanjarogic ,
With skipLibCheck: true, any inconsistencies inside .d.ts files will not be flagged by the TypeScript compiler. Could we consider removing this setting to ensure stricter type checking?
For more details, please refer to the discussion here: microsoft/azure-pipelines-extensions#1295 (comment)

Copy link
Author

@nemanjarogic nemanjarogic Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great if I can avoid this, but not sure how. The problem is not related to any file we have in this repository.
To make it even worse, I can reproduce this problem even on master branch.
image

When I try to run tests locally, they fail (I ensured that all folders like node_modules, _build, etc. are deleted after switching to master branch), however they work when executed on pipeline. I created a dummy PR to confirm this.

Therefore, if we don't have a concrete suggestion how to actually fix this, I would prefer to leave skipLibCheck. It ignores only files from dependencies, and not files in our repository, so I don't see that as a major issue. I am aware of cons, but need to unblock my PR. If needed this can be fixed as a separate work item from repo owners.

process.env['TASKLIB_INPROC_UNITS'] = '1'; // export task-lib internals for internal unit testing
set('+e'); // Don't throw an exception when tests fail
run('mocha ' + testPath);
run('npx mocha ' + testPath);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why add npx here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because without it I was getting
'mocha' is not recognized as an internal or external command

node/internal.ts Outdated
}

_debug(name + '=' + varval);
if (!skipDebug) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add a comment why skipDebug is required here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't seem to be related to glob vulnerability fix, should this be part of separate repo?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left that in PR description, but will leave it here since description will be modified (and this change is no longer needed since I managed to avoid es upgrade).

skipDebug parameter in node/internal.ts is introduced to avoid circular dependency and compilator error that happens while resolving debugMode field (debugMode -> getVariable() -> _debug() -> debugMode). With ES5 fields

const debugMode = _getVariable('system.debug')?.toLowerCase() === 'true';
const shouldCheckDebugMode = _getVariable('DistributedTask.Tasks.Node.SkipDebugLogsWhenDebugModeOff')?.toLowerCase() === 'true';

were converted to

var debugMode = ...
var shouldCheckDebugMode = ...

However, with ES2020 that is no longer case and we have compilator error due to circular dependency.

P.S. Even if this works with ES5, I think we have bug here, because debugMode and shouldCheckDebugMode will always be falsy when this happens (they are not initialized).

@nemanjarogic
Copy link
Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants