Add warning for BIDI characters in page renders and in diffs

[zeripath]

Fix #17514

Given the comments I've adjusted this somewhat. The numbers of characters detected are increased and include things like the use of U+300 to make à instead of à and non-breaking spaces.

There is a button which can be used to escape the content to show it.

Rendered page header when hidden characters are detected

Rendered page when BiDi characters are detected on lines only containing LTR characters

Diff view

Diff view is escaped by default

but can be unescaped:

Diff Split view

with unescape as above

Blame view

unescape as above.

Signed-off-by: Andrew Thornton art27@cantab.net

[codecov-commenter]

Codecov Report

Merging #17562 (f66923f) into main (36a0f4e) will increase coverage by 0.00%.
The diff coverage is 52.35%.

@@           Coverage Diff            @@
##             main   #17562    +/-   ##
========================================
  Coverage   45.63%   45.64%            
========================================
  Files         828      829     +1     
  Lines       91814    91977   +163     
========================================
+ Hits        41902    41982    +80     
- Misses      43182    43251    +69     
- Partials     6730     6744    +14     

Impacted Files	Coverage Δ
routers/web/repo/blame.go	0.00% <0.00%> (ø)
routers/web/repo/lfs.go	0.00% <0.00%> (ø)
routers/web/repo/view.go	39.96% <47.61%> (+0.46%)	⬆️
modules/charset/escape.go	50.70% <50.70%> (ø)
services/gitdiff/gitdiff.go	73.02% <83.33%> (+0.19%)	⬆️
routers/web/repo/wiki.go	39.41% <100.00%> (ø)
modules/git/utils.go	68.05% <0.00%> (-2.78%)	⬇️
modules/git/repo_base_nogogit.go	80.00% <0.00%> (-2.50%)	⬇️
modules/queue/queue_channel.go	95.00% <0.00%> (-1.67%)	⬇️
modules/queue/queue_bytefifo.go	59.28% <0.00%> (-0.60%)	⬇️
... and 5 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36a0f4e...f66923f. Read the comment docs.

[gwymor]

Please don't implement this like this. This makes things worse for right-to-left-language coders by default.

[zeripath]

Please don't implement this like this. This makes things worse for right-to-left-language coders by default.

Please could you advise how else to do this then?

[gwymor]

I don't think that "Trojan Source" is really a threat that warrants patching.

While I'd rather no mitigation, the method Konstantin Ryabitsev used in b4 seems reasonable:

I implemented this solution in b4 master, so it should error out only when it finds control characters without any "other letter" unicode character category present in the same line (where Hebrew, Arabic, etc live). There's probably still a way to take advantage of this, but hopefully it's a lot less trivial now and less likely to go unnoticed by the reviewer.

In practice, I think all Trojan Source is going to accomplish is make a lot of software worse for people who speak RTL languages, without much of a real security gain to show for it.

[zeripath]

Have you even tried this PR?

[zeripath]

OK I've adjusted this to only put the warning if there is a line with a BIDI character but no RTL definite characters on there.

[maweil]

OK I've adjusted this to only put the warning if there is a line with a BIDI character but no RTL definite characters on there.

I think your previous implementation was superior from a security standpoint.
As also noted in the discussion for b4, there are probably ways around this check.

Displaying the warning all the time seems also to be what GitHub opted for (see https://github.blog/changelog/2021-10-31-warning-about-bidirectional-unicode-text/). Maybe instead of a red background, the message should use an orange or yellow one (to indicate a warning instead of an error). This is just my opinion, though.

I hope more people give their feedback regarding this PR.

[zeripath]

I've updated with another change.

[silverwind]

Not sure if a warning is really necessary. I think it's sufficient to render the characters in a highlighted manner, like VSCode does it.

[zeripath]

Not sure if a warning is really necessary. I think it's sufficient to render the characters in a highlighted manner, like VSCode does it.

It really would be helpful if people could try the PR.

The diff and blame views do escape the characters by default and therefore do not display a warning.

Rendering the file as markdown etc. the characters are rendered as expected but the warning is added.

[silverwind]

Some error message tweaks:

diff --git a/templates/repo/view_file.tmpl b/templates/repo/view_file.tmpl
index 356ae44c1..cb860ff3d 100644
--- a/templates/repo/view_file.tmpl
+++ b/templates/repo/view_file.tmpl
@@ -68,9 +68,9 @@
 		</div>
 	</h4>
 	<div class="ui attached table unstackable segment">
 		{{if .EscapeStatus.BadBIDI}}
-			<div class="ui error message">
+			<div class="ui error message m-0">
 				<span class="close icon">{{svg "octicon-x" 16 "close inside"}}</span>
 				<div class="header">
 					{{.i18n.Tr "repo.bidi_bad_header"}}
 				</div>
diff --git a/web_src/less/_base.less b/web_src/less/_base.less
index c09f3a2bd..81eac977a 100644
--- a/web_src/less/_base.less
+++ b/web_src/less/_base.less
@@ -586,8 +586,12 @@ a.ui.card:hover,
 .ui.link.list.list .item a:not(.ui):active {
   color: var(--color-text-dark);
 }

+.ui.error.message .header {
+  color: var(--color-red);
+}
+
 .dont-break-out {
   overflow-wrap: break-word;
   word-wrap: break-word;
   word-break: break-all;

[silverwind]

Is it intentional that nothing is visible in the code in the screenshots above? Source file is https://github.com/nickboucher/trojan-source/blob/main/JavaScript/commenting-out.js. GitHub does have some warning signs on the lines, thought I guess if they are too much work, we don't have to add them.

Edit: I see the toggle button works as expected. I would suggest rewording to "Use the Escape button above to ...".

[lunny]

Please send back port.

[wxiaoguang]

Do we really need a backport? In my mind this is a new feature, and it's huge.