Skip to content

util: fix OSC 8 hyperlink stripping in stripVTControlCharacters - #64319

Open
dushyant-hada-90 wants to merge 6 commits into
nodejs:mainfrom
dushyant-hada-90:fix/strip-vt-osc-8-uri-chars
Open

util: fix OSC 8 hyperlink stripping in stripVTControlCharacters#64319
dushyant-hada-90 wants to merge 6 commits into
nodejs:mainfrom
dushyant-hada-90:fix/strip-vt-osc-8-uri-chars

Conversation

@dushyant-hada-90

Copy link
Copy Markdown

Fixes: #64313

Summary

The bundled ansi-regex OSC pattern used by stripVTControlCharacters() relied on a restrictive URI character class that failed when OSC 8 hyperlink URIs contained RFC 3986-valid characters such as (, ), !, +, [ and ].

This updates OSC handling to match sequences generically as:

ESC ] ... ST

aligned with ansi-regex v6.2.0.

Problem

stripVTControlCharacters() failed to correctly strip OSC 8 hyperlinks whose URI contained characters omitted from the previous regex character class:

[-a-zA-Z\d/#&.:=?%@~_]

Example:

import { stripVTControlCharacters } from 'node:util';

const link =
  '\u001B]8;;https://example.com/(foo\u0007label\u001B]8;;\u0007';

console.log(stripVTControlCharacters(link));

Actual

ttps://example.com/(foo\u0007label

Expected

label

The OSC match terminated early at (, leaving most of the escape sequence in the output.

Changes

  • Replace the restrictive OSC URI matcher with generic OSC matching up to the first string terminator (BEL, ESC \, or 0x9C)
  • Sync the bundled regex behavior with ansi-regex v6.2.0
  • Preserve existing CSI handling logic

Tests

Added regression coverage in:

test/parallel/test-util-stripvtcontrolcharacters.js

for OSC 8 hyperlinks containing:

  • (
  • )
  • !
  • +
  • [ ]

across all supported OSC string terminators.

Notes

  • Existing CSI handling is unchanged
  • Fast-path behavior is unchanged
  • Embedded OSC terminators inside URIs retain previous behavior

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Jul 6, 2026
Comment thread lib/internal/util/inspect.js Outdated
'[\\dA-PR-TZcf-nq-uy=><~]))', 'g',
'(?:\\u001B\\][\\s\\S]*?(?:\\u0007|\\u001B\\u005C|\\u009C))' +
'|[\\u001B\\u009B][[\\]()#;?]*' +
'(?:(?:\\d{1,4}(?:;\\d{0,4})*)?' +

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not complete consistent with
(?:;\d{0,4})*
(?:[;:]\d{0,4})*

Comment thread lib/internal/util/inspect.js Outdated
@dushyant-hada-90
dushyant-hada-90 force-pushed the fix/strip-vt-osc-8-uri-chars branch from e3d3cb8 to 9da6ef2 Compare July 8, 2026 05:40
@dushyant-hada-90

Copy link
Copy Markdown
Author

Pushed updates addressing the feedback:

Accepted the [;:] CSI parameter separator suggestion (@Archkon) and reworded the commit to follow commit message guidelines
Added regression tests for the remaining RFC 3986 characters ($ ' * ,), colon-delimited CSI sub-parameters, and an unterminated-OSC edge case

Verified the final regex against every added test case and confirmed it now matches upstream ansi-regex v6.2.0 exactly.
Ready for another look

@dushyant-hada-90
dushyant-hada-90 requested a review from Archkon July 8, 2026 05:45
Comment thread lib/internal/util/inspect.js Outdated
@dushyant-hada-90
dushyant-hada-90 force-pushed the fix/strip-vt-osc-8-uri-chars branch 2 times, most recently from 711b204 to 7a426b8 Compare July 20, 2026 17:37
@dushyant-hada-90

Copy link
Copy Markdown
Author

Hi @Archkon, @avivkeller
I've addressed all the feedback from both rounds. The resolved threads are all closed and the regex now matches ansi-regex v6.2.0 exactly. Would really appreciate a re-review or an approval when you get a chance. Happy to make any further changes!

Comment thread lib/internal/util/inspect.js
@avivkeller avivkeller added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jul 25, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 25, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.14%. Comparing base (00917ba) to head (a40efe2).
⚠️ Report is 78 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main   #64319    +/-   ##
========================================
  Coverage   90.14%   90.14%            
========================================
  Files         741      743     +2     
  Lines      242076   242322   +246     
  Branches    45558    45645    +87     
========================================
+ Hits       218216   218445   +229     
+ Misses      15385    15361    -24     
- Partials     8475     8516    +41     
Files with missing lines Coverage Δ
lib/internal/util/inspect.js 96.93% <100.00%> (-0.01%) ⬇️

... and 80 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

dushyant-hada-90 and others added 6 commits July 27, 2026 00:57
The bundled ansi-regex OSC pattern used a restrictive URI
character class that failed when URIs contained RFC 3986-valid
characters such as parentheses.

Match OSC sequences generically as ESC ] ... ST, aligned with
ansi-regex v6.2.0.

Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: dushyant <dushyanthada90@gmail.com>
Co-authored-by: Archkon <180910180+Archkon@users.noreply.github.com>
Signed-off-by: dushyant <dushyanthada90@gmail.com>
Signed-off-by: dushyant <dushyanthada90@gmail.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: dushyant <dushyanthada90@gmail.com>
Remove the extra non-capturing group around CSI parameters and
the final byte so the bundled pattern matches ansi-regex at
72bc570 exactly.

Refs: https://github.com/chalk/ansi-regex/blob/72bc570aaf25fca25541b49c6a8564f3ec63e835/index.js

Signed-off-by: dushyant <dushyanthada90@gmail.com>
The previous comment claimed truncated OSC sequences were left
unmatched rather than partially stripped. The CSI alternative can
still consume a short prefix, which is why the expected remainder
is 'ttps://...'. Update the comment to describe that behavior.

Signed-off-by: dushyant <dushyanthada90@gmail.com>
@dushyant-hada-90
dushyant-hada-90 force-pushed the fix/strip-vt-osc-8-uri-chars branch from a40efe2 to 8bcabf1 Compare July 26, 2026 19:29
@dushyant-hada-90

dushyant-hada-90 commented Jul 26, 2026

Copy link
Copy Markdown
Author

I have updated the commit message to comply with the Node.js commit guidelines, since the previous message was failing CI.
Kindly re-approve CI when convenient.
@avivkeller

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

util.stripVTControlCharacters() fails to strip OSC 8 hyperlinks whose URI contains characters like "("

4 participants