feat(tracer): resolve identifiers assigned an object literal - #689
Open
Edneam wants to merge 2 commits into
Open
feat(tracer): resolve identifiers assigned an object literal#689Edneam wants to merge 2 commits into
Edneam wants to merge 2 commits into
Conversation
VariableTracer could only resolve identifiers back to their literal
value for Literal and TemplateLiteral initializers. An identifier
assigned an object literal (const opts = {...}) had no way to be
resolved back to its ObjectExpression node, so the log-usage probe's
pino()/winston.createLogger() config detection only worked when the
config object was passed inline, e.g. pino({ customLevels: {...} }),
not when passed as a separately-declared variable, e.g.:
const opts = { customLevels: {...} };
pino(opts);
Add a VariableTracer.objectIdentifiers map, populated for top-level
object-literal assignments alongside the existing literalIdentifiers
map, and use it in log-usage.ts to resolve the pino()/
winston.createLogger() config argument when it's an Identifier
instead of an inline ObjectExpression.
Fixes NodeSecure#639
🦋 Changeset detectedLatest commit: 214eb2c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
clemgbld
approved these changes
Aug 2, 2026
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Closes #639.
VariableTracercan resolve an identifier back to its value forLiteralandTemplateLiteralinitializers (literalIdentifiers), but not for an identifier assigned an object literal:This has a concrete effect on the
log-usageprobe:pino()andwinston.createLogger()config detection (customLevels,useOnlyCustomLevels,levels) only works when the config object is passed inline:but silently falls through to the default method list (or, for
winston.createLogger, produces no warning at all) when the same object is passed via a variable, which is an extremely common pattern:Changes
VariableTracer.ts: add a new publicobjectIdentifiers: Map<string, ESTree.ObjectExpression>, populated in the existingObjectExpressionbranch of#walkVariableDeclaratorInitializationfor top-level assignments only (const x = {...}, not nested object values), mirroring howliteralIdentifiersis populated forLiteral/TemplateLiteral.log-usage.ts: in bothcreatePinoTracerListenerandcreateWinstonCreateLoggerTracerListener, resolve the config argument viatracer.objectIdentifierswhen it's anIdentifierinstead of bailing out when it isn't an inlineObjectExpression.I deliberately did not touch
literalIdentifiersitself (adding an object type there) since its.value: stringfield is consumed as a plain string by several other probes (sql-injection,isRequire,isWeakBcrypt,isMonkeyPatch, etc.) — a separate map keeps this change isolated to the object-resolution use case with no risk to existing consumers.Tests
test/VariableTracer/assignments.spec.ts: two new tests confirmingobjectIdentifiersis populated for a top-level object literal assignment and correctly not populated for a nested object value.test/probes/log-usage.spec.ts: two new tests (one forpino(), one forwinston.createLogger()) confirming the config-via-variable case now resolves the custom levels correctly. Verified against a version of the fix reverted locally that without it, the pino case falls back to default methods and the winston case produces zero warnings — confirming this is a real, previously-silent gap.Verification
All pass (179/179) when run with a
RegExp.escapepolyfill preloaded to work around this sandbox only having Node 22 available (the repo requires Node >=24; I don't have Node 24 in this environment, and confirmed theRegExp.escape is not a functionfailures reproduce identically on a cleanmastercheckout — they're unrelated to this change, from an unrelated obfuscator check that fires on every.analyse()call).Full suite on Node 22, no polyfill: 739 tests, 701 pass, 38 fail — same 38 pre-existing
RegExp.escape-related failures as on a cleanmastercheckout (0 new failures from this change).tsc/eslintalso couldn't run locally (both reference private@openally/config.*packages not resolvable outside NodeSecure's own setup), so I can't run those two locally either, but neither touches runtime logic.Added a changeset (
minor, matching the existing convention for new detection capability).