fix(logger): use a constant console format string on the stack-trace path (#100) - #204
Merged
Conversation
…path (#100) CodeQL js/tainted-format-string (alert 82): the sanitized-but-user-controlled log line was passed as the first argument of console.error on the stack-trace path, so %s/%d in a user-supplied message or error text would be interpreted as format specifiers and consume the stack argument. Pass a constant "%s\n%s" format string instead; the line and the stack are now plain arguments and user text is never interpreted.
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens stack-trace logging against tainted format strings.
Changes:
- Uses a constant
console.errorformat string. - Updates and adds logger safety tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/lib/logger.ts |
Safely formats error lines and stacks. |
tests/unit/logger.test.ts |
Verifies the new console call shape. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| logger.error("failure", err); | ||
| expect(errorSpy).toHaveBeenCalledTimes(1); | ||
| const line = errorSpy.mock.calls[0][0] as string; | ||
| // Stack path uses a constant format string; the line is the first argument |
Member
Author
There was a problem hiding this comment.
Reworded in 7c41788: the comment now states the call shape ("%s\n%s", line, stack) and that the log line is read from index 1.
| const err = new RangeError("out of bounds"); | ||
| logger.error("bad range", err); | ||
| const line = errorSpy.mock.calls[0][0] as string; | ||
| // Stack path uses a constant format string; the line is the first argument |
Member
Author
There was a problem hiding this comment.
Reworded in 7c41788: the comment now states the call shape ("%s\n%s", line, stack) and that the log line is read from index 1.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
14 tasks
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.
Part of the CodeQL triage tracked in #100 (section 2).
Problem
CodeQL
js/tainted-format-string(alert 82, high): on the stack-trace path,logger.tspassed the log line as the first argument ofconsole.error(full, "\n", safeStack). The line is sanitized against log injection (CR/LF and control chars stripped), but it still contains user-controlled text (message, context values, error message), and the first argument of a multi-argumentconsole.*call is a format string. A%s/%dinside a user-supplied message or error text would be interpreted as a format specifier and consume the stack argument, corrupting the log record.Fix
Pass a constant format string instead:
User text now only ever appears as a formatting argument, never in the format-string position. Output is unchanged apart from no longer being reformattable by user input (verified against a real, unmocked run:
%s/%dstay literal, the stack prints on its own line).Triage context (rest of the 13 open alerts)
This was the only alert of the 13 needing a code change. The other 12 were dismissed with written justifications, per the checklist in #100:
js/log-injection(logger.ts:96/99/102, factory.ts:58, base-provider.ts:261) - false positive: every value passes through an inline sanitizer stripping CR/LF and control chars immediately before the sink; CodeQL does not model the custom sanitizer.js/log-injection(logger.ts:85) - by design: the stack-trace argument is intentionally multi-line; control chars are stripped and stacks are only printed outside production.js/sql-injection(mssql.ts, mongodb.ts x3) - by design: Studio is a database IDE; these lines execute the user's own query/operation against their own configured connection (MSSQL viarequest.querywith optionalrequest.inputbind parameters, MongoDB via the official driver's structured API).js/path-injection(sqlite.ts:176-177) - by design: the SQLite path is user-configured, trusted server-side input (documented indocs/providers/sqlite.md); NUL bytes are rejected and the path resolved; parent-dir creation is intentional first-run behavior.Testing (TDD)
stack-trace path keeps user text out of the console format-string position- asserts the constant"%s\n%s"first argument and that user text (including literal%s/%d) lands in the arguments.Verification
All local gates pass:
format/lint/typecheck/knip/test(20 groups) /build, plustest:coverage && coverage:check(100.00%, 25753/25753 lines).