Skip to content

Add warning against Unicode Bidi characters in our range-diff#2440

Merged
Urgau merged 1 commit into
rust-lang:masterfrom
Urgau:gh-range-diff-unicode-bidi-warning
Jun 20, 2026
Merged

Add warning against Unicode Bidi characters in our range-diff#2440
Urgau merged 1 commit into
rust-lang:masterfrom
Urgau:gh-range-diff-unicode-bidi-warning

Conversation

@Urgau

@Urgau Urgau commented Jun 19, 2026

Copy link
Copy Markdown
Member

Like GitHub, let's show a warning if we detect bidirectional characters in the patch.

Dark Light
image image

Links:

Suggested by @camelid

@Urgau Urgau requested a review from Kobzol June 19, 2026 21:53
@Urgau Urgau force-pushed the gh-range-diff-unicode-bidi-warning branch from ab5bde1 to ed57738 Compare June 19, 2026 22:09

@Kobzol Kobzol left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure if I'm a fan of adding a new dependency (and more code) for something so niche, tbh. But I won't block it.

View changes since this review

@Urgau

Urgau commented Jun 20, 2026

Copy link
Copy Markdown
Member Author

It could be done without the dependency on memchr, but since we already have that dependency 18 times in our Cargo.lock, I think it's fine.

As for adding something this "niche", I would agree at first, but since GitHub decided to add such warning, that we had a rustc CVE for it (CVE-2021-42574), and that we added a deny-by-default lint for it (text_direction_codepoint_in_literal). I don't think it's that niche.

@Urgau Urgau added this pull request to the merge queue Jun 20, 2026
Merged via the queue into rust-lang:master with commit f875d47 Jun 20, 2026
3 checks passed
@Urgau Urgau deleted the gh-range-diff-unicode-bidi-warning branch June 20, 2026 10:21
@camelid

camelid commented Jun 22, 2026

Copy link
Copy Markdown
Member

Thanks @Urgau!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants