Skip to content

fix: do not redact notice logs going to stdout #8629

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

Merged
merged 1 commit into from
Oct 2, 2025
+24 −40
Merged

fix: do not redact notice logs going to stdout #8629

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
64 changes: 24 additions & 40 deletions lib/utils/display.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,9 @@ const setBlocking = (stream) => {
return stream
}

// These are important
// This is the key that is returned to the user for errors
const ERROR_KEY = 'error'
// This is the key producers use to indicate that there
// is a json error that should be merged into the finished output
// This is the key producers use to indicate that there is a json error that should be merged into the finished output
const JSON_ERROR_KEY = 'jsonError'

const isPlainObject = (v) => v && typeof v === 'object' && !Array.isArray(v)
Expand Down Expand Up @@ -120,15 +118,12 @@ const getJsonBuffer = ({ [JSON_ERROR_KEY]: metaError }, buffer) => {

const res = getArrayOrObject(items)

// This skips any error checking since we can only set an error property
// on an object that can be stringified
// This skips any error checking since we can only set an error property on an object that can be stringified
// XXX(BREAKING_CHANGE): remove this in favor of always returning an object with result and error keys
if (isPlainObject(res) && errors.length) {
// This is not ideal. JSON output has always been keyed at the root with an `error`
// key, so we cant change that without it being a breaking change. At the same time
// some commands output arbitrary keys at the top level of the output, such as package
// names. So the output could already have the same key. The choice here is to overwrite
// it with our error since that is (probably?) more important.
// This is not ideal.
// JSON output has always been keyed at the root with an `error` key, so we cant change that without it being a breaking change. At the same time some commands output arbitrary keys at the top level of the output, such as package names.
// So the output could already have the same key. The choice here is to overwrite it with our error since that is (probably?) more important.
// XXX(BREAKING_CHANGE): all json output should be keyed under well known keys, eg `result` and `error`
if (res[ERROR_KEY]) {
log.warn('', `overwriting existing ${ERROR_KEY} on json output`)
Expand Down Expand Up @@ -227,9 +222,7 @@ class Display {
import('chalk'),
import('supports-color'),
])
// we get the chalk level based on a null stream meaning chalk will only use
// what it knows about the environment to get color support since we already
// determined in our definitions that we want to show colors.
// we get the chalk level based on a null stream meaning chalk will only use what it knows about the environment to get color support since we already determined in our definitions that we want to show colors.
const level = Math.max(createSupportsColor(null).level, 1)
this.#noColorChalk = new Chalk({ level: 0 })
this.#stdoutColor = stdoutColor
Expand Down Expand Up @@ -265,8 +258,7 @@ class Display {

// HANDLERS

// Arrow function assigned to a private class field so it can be passed
// directly as a listener and still reference "this"
// Arrow function assigned to a private class field so it can be passed directly as a listener and still reference "this"
#logHandler = withMeta((level, meta, ...args) => {
switch (level) {
case log.KEYS.resume:
Expand All @@ -289,8 +281,7 @@ class Display {
}
})

// Arrow function assigned to a private class field so it can be passed
// directly as a listener and still reference "this"
// Arrow function assigned to a private class field so it can be passed directly as a listener and still reference "this"
#outputHandler = withMeta((level, meta, ...args) => {
this.#json = typeof meta.json === 'boolean' ? meta.json : this.#json
switch (level) {
Expand All @@ -316,18 +307,14 @@ class Display {
if (this.#outputState.buffering) {
this.#outputState.buffer.push([level, meta, ...args])
} else {
// HACK: Check if the argument looks like a run-script banner. This can be
// replaced with proc-log.META in @npmcli/run-script
// HACK: Check if the argument looks like a run-script banner. This can be replaced with proc-log.META in @npmcli/run-script
if (typeof args[0] === 'string' && args[0].startsWith('\n> ') && args[0].endsWith('\n')) {
if (this.#silent || ['exec', 'explore'].includes(this.#command)) {
// Silent mode and some specific commands always hide run script banners
break
} else if (this.#json) {
// In json mode, change output to stderr since we don't want to break json
// parsing on stdout if the user is piping to jq or something.
// XXX: in a future (breaking?) change it might make sense for run-script to
// always output these banners with proc-log.output.error if we think they
// align closer with "logging" instead of "output"
// In json mode, change output to stderr since we don't want to break json parsing on stdout if the user is piping to jq or something.
// XXX: in a future (breaking?) change it might make sense for run-script to always output these banners with proc-log.output.error if we think they align closer with "logging" instead of "output"
level = output.KEYS.error
}
}
Expand All @@ -352,14 +339,12 @@ class Display {
break

case input.KEYS.read: {
// The convention when calling input.read is to pass in a single fn that returns
// the promise to await. resolve and reject are provided by proc-log
// The convention when calling input.read is to pass in a single fn that returns the promise to await. resolve and reject are provided by proc-log
const [res, rej, p] = args
return input.start(() => p()
.then(res)
.catch(rej)
// Any call to procLog.input.read will render a prompt to the user, so we always
// add a single newline of output to stdout to move the cursor to the next line
// Any call to procLog.input.read will render a prompt to the user, so we always add a single newline of output to stdout to move the cursor to the next line
.finally(() => output.standard('')))
}
}
Expand All @@ -383,10 +368,7 @@ class Display {

#tryWriteLog (level, meta, ...args) {
try {
// Also (and this is a really inexcusable kludge), we patch the
// log.warn() method so that when we see a peerDep override
// explanation from Arborist, we can replace the object with a
// highly abbreviated explanation of what's being overridden.
// Also (and this is a really inexcusable kludge), we patch the log.warn() method so that when we see a peerDep override explanation from Arborist, we can replace the object with a highly abbreviated explanation of what's being overridden.
// TODO: this could probably be moved to arborist now that display is refactored
const [heading, message, expl] = args
if (level === log.KEYS.warn && heading === 'ERESOLVE' && expl && typeof expl === 'object') {
Expand All @@ -400,8 +382,7 @@ class Display {
// if it crashed once, it might again!
this.#writeLog(log.KEYS.verbose, meta, '', `attempt to log crashed`, ...args, ex)
} catch (ex2) {
// This happens if the object has an inspect method that crashes so just console.error
// with the errors but don't do anything else that might error again.
// This happens if the object has an inspect method that crashes so just console.error with the errors but don't do anything else that might error again.
// eslint-disable-next-line no-console
console.error(`attempt to log crashed`, ex, ex2)
}
Expand All @@ -421,7 +402,12 @@ class Display {
this.#logColors[level](level),
title ? this.#logColors.title(title) : null,
]
this.#write(this.#stderr, { prefix }, ...args)
const writeOpts = { prefix }
// notice logs typically come from `npm-notice` headers in responses. Some of them have 2fa login links so we skip redaction.
if (level === 'notice') {
writeOpts.redact = false
}
this.#write(this.#stderr, writeOpts, ...args)
}
}
}
Expand Down Expand Up @@ -480,9 +466,8 @@ class Progress {
this.#render()
}

// If we are currently rendering the spinner we clear it
// before writing our line and then re-render the spinner after.
// If not then all we need to do is write the line
// If we are currently rendering the spinner we clear it before writing our line and then re-render the spinner after.
// If not then all we need to do is write the line.
write (write) {
if (this.#spinning) {
this.#clearSpinner()
Expand Down Expand Up @@ -510,8 +495,7 @@ class Progress {
if (!this.#rendering) {
return
}
// We always attempt to render immediately but we only request to move to the next
// frame if it has been longer than our spinner frame duration since our last update
// We always attempt to render immediately but we only request to move to the next frame if it has been longer than our spinner frame duration since our last update
this.#renderFrame(Date.now() - this.#lastUpdate >= this.#spinner.duration)
clearInterval(this.#interval)
this.#interval = setInterval(() => this.#renderFrame(true), this.#spinner.duration)
Expand Down
Loading