Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: handle strings the same in cjs, esm, and deno #139

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

fix: handle strings the same in cjs, esm, and deno #139

Show file tree
Hide file tree
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 Apr 28, 2023
7a37884
fix: handle strings the same in cjs, esm, and deno
isaacs Apr 29, 2023
8b61045
chore: remove istanbul comments
isaacs Apr 30, 2023
fafee28
chore: move cjs to mjs so standardx doesn't check it
isaacs May 1, 2023
f236663
chore(test): update esm test to verify correct behavior
isaacs May 1, 2023
9f97090
chore: fix trailing comma causing lint failure
isaacs May 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions deno.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
// Bootstrap cliui with CommonJS dependencies:
// Bootstrap cliui with ESM dependencies in Deno's style:
import { cliui, UI } from './build/lib/index.js'
import type { UIOptions } from './build/lib/index.d.ts'
import { wrap, stripAnsi } from './build/lib/string-utils.js'

import stringWidth from 'npm:string-width'
import stripAnsi from 'npm:strip-ansi'
import wrap from 'npm:wrap-ansi'

export default function ui (opts: UIOptions): UI {
return cliui(opts, {
stringWidth: (str: string) => {
return [...str].length
},
stringWidth,
stripAnsi,
wrap
})
Expand Down
11 changes: 6 additions & 5 deletions index.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
// Bootstrap cliui with CommonJS dependencies:
// Bootstrap cliui with ESM dependencies:
import { cliui } from './build/lib/index.js'
import { wrap, stripAnsi } from './build/lib/string-utils.js'

import stringWidth from 'string-width'
import stripAnsi from 'strip-ansi'
import wrap from 'wrap-ansi'

export default function ui (opts) {
return cliui(opts, {
stringWidth: (str) => {
return [...str].length
},
stringWidth,
stripAnsi,
wrap
})
Expand Down
6 changes: 3 additions & 3 deletions lib/cjs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
// Bootstrap cliui with CommonJS dependencies:
import { cliui, UIOptions } from './index.js'
const stringWidth = require('string-width')
const stripAnsi = require('strip-ansi')
const wrap = require('wrap-ansi')
const stringWidth = require('string-width-cjs')
const stripAnsi = require('strip-ansi-cjs')
const wrap = require('wrap-ansi-cjs')
export default function ui (opts: UIOptions) {
return cliui(opts, {
stringWidth,
Expand Down
27 changes: 21 additions & 6 deletions lib/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,9 @@ export class UI {

constructor (opts: UIOptions) {
this.width = opts.width
/* c8 ignore start */
this.wrap = opts.wrap ?? true
/* c8 ignore stop */
this.rows = []
}

Expand Down Expand Up @@ -164,7 +166,10 @@ export class UI {
const fn = align[(row[c].align as 'right'|'center')]
ts = fn(ts, wrapWidth)
if (mixin.stringWidth(ts) < wrapWidth) {
ts += ' '.repeat((width || 0) - mixin.stringWidth(ts) - 1)
/* c8 ignore start */
const w = width || 0
/* c8 ignore stop */
ts += ' '.repeat(w - mixin.stringWidth(ts) - 1)
}
}

Expand Down Expand Up @@ -202,9 +207,11 @@ export class UI {
// the target line, do so.
private renderInline (source: string, previousLine: Line) {
const match = source.match(/^ */)
/* c8 ignore start */
const leadingWhitespace = match ? match[0].length : 0
/* c8 ignore stop */
const target = previousLine.text
const targetTextWidth = mixin.stringWidth(target.trimRight())
const targetTextWidth = mixin.stringWidth(target.trimEnd())

if (!previousLine.span) {
return source
Expand All @@ -223,13 +230,13 @@ export class UI {

previousLine.hidden = true

return target.trimRight() + ' '.repeat(leadingWhitespace - targetTextWidth) + source.trimLeft()
return target.trimEnd() + ' '.repeat(leadingWhitespace - targetTextWidth) + source.trimStart()
}

private rasterize (row: ColumnArray) {
const rrows: string[][] = []
const widths = this.columnWidths(row)
let wrapped
let wrapped: string[]

// word wrap all columns, and create
// a data-structure that is easy to rasterize.
Expand Down Expand Up @@ -274,7 +281,9 @@ export class UI {
}

private negatePadding (col: Column) {
/* c8 ignore start */
let wrapWidth = col.width || 0
/* c8 ignore stop */
if (col.padding) {
wrapWidth -= (col.padding[left] || 0) + (col.padding[right] || 0)
}
Expand Down Expand Up @@ -308,7 +317,9 @@ export class UI {
})

// any unset widths should be calculated.
/* c8 ignore start */
const unsetWidth = unset ? Math.floor(remainingWidth / unset) : 0
/* c8 ignore stop */

return widths.map((w, i) => {
if (w === undefined) {
Expand Down Expand Up @@ -349,12 +360,13 @@ function _minWidth (col: Column) {
}

function getWindowWidth (): number {
/* istanbul ignore next: depends on terminal */
/* c8 ignore start */
if (typeof process === 'object' && process.stdout && process.stdout.columns) {
return process.stdout.columns
}
return 80
}
/* c8 ignore stop */

function alignRight (str: string, width: number): string {
str = str.trim()
Expand All @@ -371,10 +383,11 @@ function alignCenter (str: string, width: number): string {
str = str.trim()
const strWidth = mixin.stringWidth(str)

/* istanbul ignore next */
/* c8 ignore start */
if (strWidth >= width) {
return str
}
/* c8 ignore stop */

return ' '.repeat((width - strWidth) >> 1) + str
}
Expand All @@ -383,7 +396,9 @@ let mixin: Mixin
export function cliui (opts: Partial<UIOptions>, _mixin: Mixin) {
mixin = _mixin
return new UI({
/* c8 ignore start */
width: opts?.width || getWindowWidth(),
wrap: opts?.wrap
/* c8 ignore stop */
})
}
30 changes: 0 additions & 30 deletions lib/string-utils.ts
View file
Open in desktop

This file was deleted.

11 changes: 7 additions & 4 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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"
Comment on lines +52 to +57
Copy link

@slorber slorber Jul 9, 2024 •
edited
Loading

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:

npx lockfile-lint --path yarn.lock --type yarn --allowed-hosts yarn --validate-https --validate-package-names

detected resolved URL for package with a different name: string-width-cjs
    expected: string-width-cjs
    actual: string-width

detected resolved URL for package with a different name: strip-ansi-cjs
    expected: strip-ansi-cjs
    actual: strip-ansi

detected resolved URL for package with a different name: wrap-ansi-cjs
    expected: wrap-ansi-cjs
    actual: wrap-ansi

 ✖ Error: security issues detected!

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.

Copy link
Author

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.

Copy link
Member

@shadowspawn shadowspawn Jul 9, 2024 •
edited
Loading

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.)

Copy link
Author

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.

Copy link

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:

npx lockfile-lint --path package-lock.json --allowed-hosts yarn npm --validate-https --validate-package-names --allowed-package-name-aliases string-width-cjs:string-width

Copy link
Author

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.

},
"devDependencies": {
"@types/node": "^14.0.27",
Expand Down
35 changes: 35 additions & 0 deletions test/cjs-esm-compare.mjs
View file
Open in desktop
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())
})
})
2 changes: 1 addition & 1 deletion test/cliui.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ process.env.FORCE_COLOR = 1

const chalk = require('chalk')
const cliui = require('../build/index.cjs')
const stripAnsi = require('strip-ansi')
const stripAnsi = require('strip-ansi-cjs')

describe('cliui', () => {
describe('resetOutput', () => {
Expand Down
11 changes: 5 additions & 6 deletions test/deno/cliui-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,12 @@ Deno.test('evenly divides text across columns if multiple columns are given', ()
})

// it should wrap each column appropriately.
// TODO: we should flesh out the Deno and ESM implementation
// such that it spreads words out over multiple columns appropriately:
const expected = [
'i am a string ti am a seconi am a third',
'hat should be wd string tha string that',
'rapped t should be should be w',
' wrapped rapped'
'i am a string i am a i am a third',
'that should be second string that',
'wrapped string that should be',
' should be wrapped',
' wrapped'
]

ui.toString().split('\n').forEach((line: string, i: number) => {
Expand Down
11 changes: 6 additions & 5 deletions test/esm/cliui-test.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,14 @@ describe('ESM', () => {
// TODO: we should flesh out the Deno and ESM implementation
// such that it spreads words out over multiple columns appropriately:
const expected = [
'i am a string ti am a seconi am a third',
'hat should be wd string tha string that',
'rapped t should be should be w',
' wrapped rapped'
'i am a string i am a i am a third',
'that should be second string that',
'wrapped string that should be',
' should be wrapped',
' wrapped'
]
ui.toString().split('\n').forEach((line, i) => {
strictEqual(line, expected[i])
})
})
})
})
4 changes: 2 additions & 2 deletions tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@
"target": "es2017",
"moduleResolution": "node",
"module": "es2015"
},
},
"include": [
"lib/**/*.ts"
],
"exclude": [
"lib/cjs.ts"
]
}
}