Fix for change in new captureStackTrace behaviour#2
Conversation
For the newer v8 versions in node 0.11.x Signed-off-by: Tim Smart <tim@fostle.com>
felixge
added a commit
that referenced
this pull request
Jun 19, 2013
Fix for change in new captureStackTrace behaviour
Owner
|
💖 thx! |
niemyjski
added a commit
that referenced
this pull request
May 2, 2026
- index.js: replace !firstLine.match(/^node:/) with !firstLine.startsWith('node:')
for clarity and performance; update comment for guard #2
- parse-test.js: remove [REQUIRES FIX], [REGRESSION], [SECURITY]/ReDoS
markers — tests are verified and shipping; remove '— possible ReDoS'
from timing assert messages
niemyjski
added a commit
that referenced
this pull request
May 2, 2026
* fix: address PR #66/#67 review, fix #29, verify #13 and #25 ISSUE #29: parse() now captures leading source-location lines - CJS SyntaxError stacks (require() on invalid files) begin with '/path/to/file.cjs:line[:col]' before the error message. This line was previously lost; it is now returned as the first CallSite frame. - Detection guards (both required to identify a source loc): 1. /:\s/ — all error messages contain ': ' (colon+space); source location lines never do. 2. /^(?:https?|ftp|data|blob):\/\// — excludes web URL schemes but explicitly permits file:// (valid source loc in some envs). - Verified against actual Node 25.9 stacks; format stable since Node 10. - ESM SyntaxErrors produce a standard 'SyntaxError: message' first line (no source location prepended) — no change needed for that path. BEFORE/AFTER VERIFICATION: - Against master index.js: exactly 5 [REQUIRES FIX] tests fail. - Against patched index.js: all 35 tests pass. CODE CHANGE: map+filter → forEach+push - forEach+push produces identical output (no falsy entries ever pushed; non-matching lines return early without pushing — equivalent to the prior .filter(Boolean) step). - Verified by test 'non-parseable lines produce no entries in output array'. - Eliminates one intermediate array allocation per parse() call. - All forEach/push/Array APIs stable since ES5; fully Node 20 compatible. SECURITY: ReDoS analysis - /at (?:(.+?)\s+\()?(?:(.+?):(\d+)(?::(\d+))?|([^)]+))\)?/ No nested quantifiers. Non-greedy .+? with concrete anchors. - /^(.+?):(\d+)(?::(\d+))?$/ Non-greedy .+? anchored by $ terminus. Tested at 50KB: <1ms. - Both regexes have dedicated adversarial timing tests. OTHER CHANGES: - engines.node: >=20.0.0 (unchanged) - README: 'All other methods' -> 'The other getter-style methods' (bool predicates return false, not null, for boundary frames) - long-stack-trace test: full boundary-frame contract assertions - parse-test: remove dead 'exceptions' param from compare() - Regression test for #13 ([object Object] in function name) - Verification test for #25 (async/await) — resolved since Node 12 Closes #13 Closes #25 Closes #29 * fix: allow file:// source locations in parse(); fix CI on Node 24 RCA: The source-location URL guard /^\w+:\/\// was too broad — it excluded 'file://' scheme URLs which are valid source location prefixes. This caused the 'file:// source location is captured correctly' test to fail on Node 24 (and 25 in CI) because file:///path:line was rejected before being captured as a source-loc frame. Fix: Change the URL exclusion from /^\w+:\/\// to /^(?:https?|ftp|data|blob):\/\// so that file:// is intentionally permitted while network/data URL schemes are still excluded. node: scheme paths (node:internal/...) do not use '://' and are already handled correctly. Also: expand the source-location comment block to document: - All supported path formats (POSIX, Windows, file://) - The two detection guards and why each works - ESM vs CJS SyntaxError behaviour differences - Tested on Node 24.x and 25.x (matches CI matrix) - Verified against @exceptionless/node package test suite * updated git ignore * fix: address Copilot review #9 — node: scheme guard, async throw, comment accuracy, ReDoS threshold - index.js: add 'node:' to URL scheme exclusion guard so paths like 'node:internal/modules/...' are never misclassified as source locations - index.js: tighten comment wording — 'always' was inaccurate for empty-message errors (new Error().stack → 'Error' with no colon+space; the source-loc regex already rejects such lines, but the comment was misleading) - get-test.js: fix async/await test to genuinely cross an async suspension point (await Promise.resolve() before throw) and use try/catch so V8 attaches the async context; match 'async outerAsync' prefix via .includes() - parse-test.js: relax ReDoS timing threshold from 100ms → 1000ms; 100ms was brittle on loaded CI runners — true ReDoS takes seconds to minutes * fix: use Number.isNaN check instead of || null for parseInt results parseInt(...) || null coerces 0 to null because 0 is falsy. Column numbers can legitimately be 0 in some tooling. Replace with explicit NaN guard: Number.isNaN(n) ? null : n Added regression test: 'SyntaxError CJS: column number 0 is preserved'. * fix: correctly exclude node: specifiers from source-loc detection The guard !firstLine.match(/^(?:https?|ftp|data|blob|node):\\/\\//) only matched 'node://' (with slashes) but Node built-in specifiers use 'node:' without '//'. Fix: add separate !firstLine.match(/^node:/) check. Update comment to clarify that node: and node:// are different formats. Add regression test: 'node: specifier is not treated as source loc'. * chore: clean up test markers and use startsWith for node: guard - index.js: replace !firstLine.match(/^node:/) with !firstLine.startsWith('node:') for clarity and performance; update comment for guard #2 - parse-test.js: remove [REQUIRES FIX], [REGRESSION], [SECURITY]/ReDoS markers — tests are verified and shipping; remove '— possible ReDoS' from timing assert messages * Apply suggestion from @niemyjski * Apply suggestion from @niemyjski * fix: consistent Number.isNaN guard for at-frame parseInt; trim comment block - Apply Number.isNaN(n) ? null : n to lineNumber and columnNumber in the at-frame parsing path for consistency with the source-loc frame fix. parseInt() || null coerces 0 to null; 0 is a valid column number. - Trim the 20-line source-loc comment block to 6 lines — this package ships source without minification, so large comments go to consumers. * fix: use node:test built-in timeout option instead of wall-clock assertions
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
2 participants
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.
For the newer v8 versions in node 0.11.x
Also is backwards compatible. There is a failing test, but that was also failing in the same fashion before this patch.