-
Notifications
You must be signed in to change notification settings - Fork 41
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
fix: handle strings the same in cjs, esm, and deno #139
Open
isaacs
wants to merge
6
commits into
yargs:master
Choose a base branch
from
isaacs:isaacs/esm-cjs-consistency
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+92
−67
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
3ed58ce
chore(ts): small tweaks to make typescript happier
isaacs 7a37884
fix: handle strings the same in cjs, esm, and deno
isaacs 8b61045
chore: remove istanbul comments
isaacs fafee28
chore: move cjs to mjs so standardx doesn't check it
isaacs f236663
chore(test): update esm test to verify correct behavior
isaacs 9f97090
chore: fix trailing comma causing lint failure
isaacs File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,7 @@ | |
"fix": "standardx --fix '**/*.ts' && standardx --fix '**/*.js' && standardx --fix '**/*.cjs'", | ||
"pretest": "rimraf build && tsc -p tsconfig.test.json && cross-env NODE_ENV=test npm run build:cjs", | ||
"test": "c8 mocha ./test/*.cjs", | ||
"test:esm": "c8 mocha ./test/esm/cliui-test.mjs", | ||
"test:esm": "c8 mocha ./test/**/*.mjs", | ||
"postest": "check", | ||
"coverage": "c8 report --check-coverage", | ||
"precompile": "rimraf build", | ||
|
@@ -49,9 +49,12 @@ | |
"author": "Ben Coe <[email protected]>", | ||
"license": "ISC", | ||
"dependencies": { | ||
"string-width": "^4.2.0", | ||
"strip-ansi": "^6.0.1", | ||
"wrap-ansi": "^7.0.0" | ||
"string-width": "^5.1.2", | ||
"string-width-cjs": "npm:string-width@^4.2.0", | ||
"strip-ansi": "^7.0.1", | ||
"strip-ansi-cjs": "npm:strip-ansi@^6.0.1", | ||
"wrap-ansi": "^8.1.0", | ||
"wrap-ansi-cjs": "npm:wrap-ansi@^7.0.0" | ||
}, | ||
"devDependencies": { | ||
"@types/node": "^14.0.27", | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
'use strict' | ||
|
||
/* global describe, it */ | ||
|
||
import { createRequire } from 'module' | ||
const require = createRequire(import.meta.url) | ||
require('chai').should() | ||
|
||
const text = `usage: git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e] | ||
<tagname> [<commit> | <object>] | ||
or: git tag -d <tagname>... | ||
or: git tag [-n[<num>]] -l [--contains <commit>] [--no-contains <commit>] | ||
[--points-at <object>] [--column[=<options>] | --no-column] | ||
[--create-reflog] [--sort=<key>] [--format=<format>] | ||
[--merged <commit>] [--no-merged <commit>] [<pattern>...] | ||
or: git tag -v [--format=<format>] <tagname>...` | ||
|
||
|
||
const cliuiCJS = require('../build/index.cjs') | ||
import cliuiESM from '../index.mjs' | ||
describe('consistent wrapping', () => { | ||
it('should produce matching output in cjs and esm', () => { | ||
const uiCJS = cliuiCJS({}) | ||
const uiESM = cliuiESM({}) | ||
uiCJS.div({ | ||
padding: [0, 0, 0, 0], | ||
text, | ||
}) | ||
uiESM.div({ | ||
padding: [0, 0, 0, 0], | ||
text, | ||
}) | ||
uiCJS.toString().should.equal(uiESM.toString()) | ||
}) | ||
}) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Not sure how helpful this is, but this change in
@isaacs/cliui
fork scared me.I'm running this on the Docusaurus repo, trying to detect possible supply chain attacks, and got these aliases being reported:
And there's this "anonymous" guy that published empty packages on npm with the exact same name:
https://www.npmjs.com/package/string-width-cjs
https://www.npmjs.com/package/strip-ansi-cjs
https://www.npmjs.com/package/wrap-ansi-cjs
I don't know how harmful it could be, but this looks suspicious that those packages even get a few weekly downloads.
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 is not a security issue, it's just a dependency alias, which every js package manager should support by now. If yarn is complaining, it's likely a yarn bug.
Those packages likely get downloads because the registry is constantly being mirrored by many third-party registry instances. I don't know if they're malicious or just litter, but they're irrelevant 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.
That a might be a false positive from lockfile-lint, possibly it does not look for package aliases. The author of
lockfile-lint
may be interested.If is interesting that there are some matching packages published. The optimistic view is perhaps someone was investigating working around the bug in earlier versions of yarn. The pessimistic view is someone was investigating exploiting the problem. The failures we have seen are a runtime failure rather than a download so not directly fixable/exploitable in this way. (But someone might possibly see the alias name in a message and think it was missing and try installing it.)
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.
Yes, it is definitely a false positive in whatever tool is generating this warning. I recommend reporting it to them.
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.
@isaacs considering that the npm registry itself is/was vulnerable to npm package aliases and allowed spoofing package names (the way you used them here in the package.json) is enough to warrant that this is a real-world concern and not theoretical, in my opinion. See here for evidence: https://snyk.io/blog/exploring-extensions-of-dependency-confusion-attacks-via-npm-package-aliasing/
@slorber lockfile-lint allows you to accept the risk of package aliases as long as you explicitly call them out, here's how to allow-list one of the packages:
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.
@lirantal That blog post is misleading. It's not a "supply chain security" issue, it's just a website display issue. I'm not saying it's not a bug, and yes it does potentially lead to falsely providing reputation in some way to the other package name, but it's imo quite a stretch to call it evidence of a supply chain security bug for package publishers or consumers using package aliases as they're intended. The website is not part of the supply chain, and the registry and all modern clients of it handle aliases just fine.
The failure of
lockfile-lint
to do so as well leads to false positives like this one creating make-work and wasting everyone's time. Please reconsider.