angular-schule
#185 Fixing critical vulnerabilities on 2.0.0-beta.2, caused by [email protected]
Just upgraded to the latest version of gh-pages available at the moment.
After upgrading, removed the node_modules, package-lock.json, and then ran a fresh npm i.
Building with npm build runs just fine. And after fixing a single test, npm test (building first) passes all tests too.
Please feel free to request any changes.
Thank you so much, Isahann! @JohannesHoppe will review and merge this one.
@isahann Thanks for starting this PR! 😃 🙏 Do you think you have the resources to figure out how we can address the breaking change?
@isahann Thanks for starting this PR! 😃 🙏 Do you think you have the resources to figure out how we can address the breaking change?
I've just downloaded the repo, haven't checked much of it besides the package.json and the breaking test.... But I'll try to take a better look into it...
@JohannesHoppe Any updates from the Monday tests? Would be awesome to see this critical vulnerability fixed.
I'm also looking forward this new version to be available, any update on this PR?
Looking forward to merge this PR - folks. Current version is causing havoc with the critical vulnerabilities
@JohannesHoppe any updates on this? I've recently updated to v2.0.3, but I see that it's still using [email protected]...
This PR addressing a critical vulnerability has been open for over a year. @JohannesHoppe, could you please confirm whether this repository is still being maintained? My team needs to decide whether to continue relying on this package or migrate away in order to resolve the issue.
Hi @BGBRWR,
thank you very much for your patience, and sorry for the long delay in responding - I’ve been on a long holiday and just got back.
The main issue with PR #186 is that it bumps gh-pages from v3 to v6. That introduces some tricky changes, especially because gh-pages updated its dependency on commander. Unfortunately, that results in a different version of commander than the one we’re currently using. I tried to update this in the past, but commander itself introduced breaking changes in how values are parsed from the command line. From what I remember, it opens the door to all kinds of regressions.
Another challenge is that no tests are failing with this PR, which is actually a sign that we don’t have test coverage for this area. Until now we’ve just assumed infrastructure like commander wouldn’t change in breaking ways.
That leaves us with three possible paths forward:
- Upgrade to gh-pages v6 and test everything thoroughly (high risk, since ~66k repos depend on angular-cli-ghpages).
- Fork the current gh-pages version and only patch security-relevant dependencies, without updating commander.
- Copy the best parts of gh-pages into our repo (not very polite, but would make us independent).
Each option is quite intensive. To make progress, I’ll start by increasing the test coverage this weekend, so we can document the current status quo. That way we’ll have a foundation for moving forward.
Thanks again for your patience and understanding!
Short Update: Analysis of Breaking Changes
Thank you all for your patience! I've completed a first analysis of the dependency upgrades and discovered several critical issues that need careful handling.
Current versions:
gh-pages: ^3.1.0 → ^6.1.1 (3 major versions) commander: ^3.0.0 → ^14.0.2 (11 major versions)
1. Critical Finding: gh-pages Promise Bug (FIXED in v5.0.0)
The Problem We've Been Working Around:
In gh-pages v3.1.0, the publish() method has a critical bug where early error cases don't return a promise, causing errors to be silently swallowed if you use await.
Evidence:
- v3.1.0 source: https://github.com/tschaub/gh-pages/blob/v3.1.0/lib/index.js#L78-L85
try {
if (!fs.statSync(basePath).isDirectory()) {
done(new Error('The "base" option must be an existing directory'));
return; // ❌ NO PROMISE RETURNED!
}
} catch (err) {
done(err);
return; // ❌ NO PROMISE RETURNED!
}
Our Workaround: We use a callback-based approach instead of promises - see https://github.com/angular-schule/angular-cli-ghpages/blob/v2.0.3/src/engine/engine.ts#L323-L333:
// do NOT (!!) await ghPages.publish,
// the promise is implemented in such a way that it always succeeds – even on errors!
return new Promise((resolve, reject) => {
ghPages.publish(dir, options, error => {
if (error) {
return reject(error);
}
resolve(undefined);
});
});
Fixed in v5.0.0+:
- v6.1.1 source: https://github.com/tschaub/gh-pages/blob/v6.1.1/lib/index.js#L90-L98
- Now correctly returns Promise.reject(err) in all error cases
- https://github.com/tschaub/gh-pages/blob/main/changelog.md#v500
Impact: Our callback workaround should still work in v6, but we need intensive testing of error cases after upgrade and remove the workaround if possible.
2. Critical Finding: CNAME and .nojekyll File Creation (NEW in v6.1.0)
The Change:
gh-pages v6.1.0 added native support for creating CNAME and .nojekyll files, which we've been creating manually since the beginning.
Evidence:
- https://github.com/tschaub/gh-pages/pull/533 - "Add --nojekyll and --cname options"
- v6.1.1 source: https://github.com/tschaub/gh-pages/blob/v6.1.1/lib/index.js#L186-L193
.then((git) => {
if (options.nojekyll) {
log('Creating .nojekyll');
fs.createFileSync(path.join(git.cwd, '.nojekyll'));
}
if (options.cname) {
log('Creating CNAME for %s', options.cname);
fs.writeFileSync(path.join(git.cwd, 'CNAME'), options.cname);
}
// ...
})
Our Current Implementation: We create these files in https://github.com/angular-schule/angular-cli-ghpages/blob/v2.0.3/src/engine/engine.ts#L232-L284 BEFORE calling gh-pages.
Impact:
- Files will be created twice (ours in dist/, theirs in cache)
- No conflict expected (hopefully), but inefficient
- Remove our code if possible and if it's backwards compatible
3. Critical Finding: commander Boolean --no- Options Default Handling (v9)
THE MOST CRITICAL ISSUE!
Commander v9 completely changed how boolean options with --no- prefix handle default values.
Evidence:
- https://github.com/tj/commander.js/releases/tag/v9.0.0
- https://github.com/tj/commander.js/blob/master/CHANGELOG.md#900-2022-01-28: "default value specified for boolean option now always used as default value"
- https://github.com/tj/commander.js/pull/1652 - "Rework option defaults and add preset"
Our Broken Code:
https://github.com/angular-schule/angular-cli-ghpages/blob/v2.0.3/src/angular-cli-ghpages#L56-L67
.option('-T, --no-dotfiles', 'Includes dotfiles by default...')
.option('--no-notfound', 'By default a 404.html file is created...')
.option('--no-nojekyll', 'By default a .nojekyll file is created...')
The Problem:
Our defaults are in https://github.com/angular-schule/angular-cli-ghpages/blob/v2.0.3/src/engine/defaults.ts#L8-L10:
dotfiles: true, // Default should be true
notfound: true, // Default should be true
nojekyll: true, // Default should be true
But commander options DON'T have these defaults specified!
- v3 behavior: --no-dotfiles automatically sets dotfiles: false, default is implicitly true
- v9+ behavior: Default value MUST be explicitly provided or it's undefined
Impact:
- With v3: Works by old default behaviour (commander auto-handles --no- prefix magic)
- With v9+: Defaults will be undefined instead of true!
- This will BREAK existing deployments that rely on default behavior (creating .nojekyll, 404.html)
Fix Required:
// Option 1: Pass defaults explicitly
.option('-T, --no-dotfiles', 'description', defaults.dotfiles)
.option('--no-notfound', 'description', defaults.notfound)
.option('--no-nojekyll', 'description', defaults.nojekyll)
4. Critical Finding: commander v9: Option Property Casing Change
Problem: In commander v9, the property name for accessing option values changed to match the exact casing of the flag.
- Before v9: A flag -n, --name would be accessed as opts().Name (capitalized)
- After v9: The same flag is accessed as opts().name (lowercase, matching the flag)
Our Code Impact: We define flags as: -d, --dir, -r, --repo, -m, --message, -b, --branch, -n, --name, -e, --email, -c, --cname, -a, --add
We access them as: commander.dir, commander.repo, commander.name, etc. (lowercase)
User Impact - BREAKING: If users were passing options in a way that relied on the old casing behavior, their scripts will break:
# This might have worked in v3 (depending on internal behavior):
angular-cli-ghpages --Dir=dist --Repo=https://...
# v9+: Only lowercase works:
angular-cli-ghpages --dir=dist --repo=https://...
Our Risk:
- Our internal code uses lowercase, so we're safe
- User scripts that use different casing will break
- This is a silent breaking change - no error, just wrong values
Cannot prevent or fix - this is commander's new behavior! (So do we really want to upgrade, see below!)
5. Critical Finding: commander v9: Excess Arguments Error
Problem:
Commander v9 made allowExcessArguments: false the default, meaning the CLI now throws an error if users provide more arguments than expected. see https://github.com/tj/commander.js/pull/1409
- Before v9: Extra arguments were silently ignored
- After v9: Extra arguments cause the program to exit with an error
User Impact - BREAKING: Users who accidentally (or intentionally) passed extra arguments will now get errors:
# Worked in v3 (extra-arg ignored):
angular-cli-ghpages --dir=dist extra-arg
# v9+: Errors out:
angular-cli-ghpages --dir=dist extra-arg
# Error: unexpected argument 'extra-arg'
We could explicitly set allowExcessArguments: true to restore v3 behaviour:
commander
.allowExcessArguments(true) // Restore v3 behaviour
.option(...)
Minor Breaking Change: commander Default Export Removed (v12)
The Issue:
Our standalone CLI uses the default export which was removed in commander v12. This is not an issue, we can just update the import.
Evidence:
- https://github.com/tj/commander.js/blob/master/CHANGELOG.md
- Current code: https://github.com/angular-schule/angular-cli-ghpages/blob/v2.0.3/src/angular-cli-ghpages#L9
To many breaking changes in commander!
I must admit, I'm quite concerned about the extent of breaking changes in commander across major versions. The fundamental changes to boolean option handling, default value behaviour, and potentially more (??) represent significant shifts in the library's API contract. Given that we use only a small subset of commander's features (basic option parsing for our standalone CLI), it may be worth considering forking commander v3, stripping it down to just what we need, and maintaining a minimal, stable version internally. This would give us long-term stability without the risk of future breaking changes disrupting our users. In the end, including an argument parser should prevent additional work, but this has flipped, and letting an LLM write a small version of this is easier than maintaining this burden. Additionally, this minimises supply chain risk. --> Chances are high that i will just internally fork it so I never have to think about this again!
Other Small Breaking Changes (not an issue)
- gh-pages v6.0.0: Node.js 16+ required, see https://github.com/tschaub/gh-pages/pull/507
- commander v12: Node.js 18+ required, see in https://github.com/tj/commander.js/blob/master/CHANGELOG.md