-
Notifications
You must be signed in to change notification settings - Fork 279
Fix glob package vulnerability #1138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| "outDir": "../_test", | ||
| "moduleResolution": "node" | ||
| "moduleResolution": "node", | ||
| "skipLibCheck": true |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.

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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why add npx here?
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Fix glob package vulnerability by updating packages from which it is referenced (
shelljsandmocha).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 to10.5.0,11.1.0or higher.Additional explanation about changes made in PR:
node make.js testfor /node directory for master branch (without any of mine changes) fails with the following errornode make.js testfor /powershell directory for master branch (without any of mine changes) fails with the following errorWork item: ADO#2335511