Display Unicode URIs side-by-side with their Punycode encoding #15488

lhecker · 2023-05-31T14:39:55Z

06174a9 didn't properly fix the issue of us showing homoglyphs in our
URI tooltip. This commit introduces a different approach where we
display both, the Punycode and Unicode encoding, whenever we encounter
an IDN. This isn't perfect but simple to implement.

Closes #15432

Validation Steps Performed

https://www.xn--fcbook-3nf5b.com/ (which contains confusing glyphs)
is shown both in its Punycode and Unicode form simultaneously. ✅

lhecker · 2023-05-31T14:41:11Z

src/cascadia/TerminalControl/TermControl.cpp

@@ -3042,56 +3042,85 @@ namespace winrt::Microsoft::Terminal::Control::implementation
        _core.ClearHoveredCell();
    }

-    winrt::fire_and_forget TermControl::_hoveredHyperlinkChanged(IInspectable /*sender*/,
-                                                                 IInspectable /*args*/)
+    // Attackers abuse Unicode characters that happen to look similar to ASCII characters. Cyrillic for instance has


You should suppress whitespace changes!

lhecker · 2023-05-31T14:44:25Z

This is what it looks like:

This is what happens if you use a right-to-left override (RTLO):

lhecker · 2023-05-31T14:46:45Z

src/cascadia/TerminalControl/TermControl.cpp

        }
+
+        const auto uriText = sanitizeURI(_core.HoveredUriText());


I'm not sure if we should encode URIs to Punycode a little earlier than this... If the encoding to Punycode fails, we shouldn't even allow the option to click it. On the other hand, the old code didn't handle that either and this fixes the immediate issue.

Honestly, I am OKAY with this.

Thanks so much.

zadjii-msft · 2023-05-31T15:00:58Z

wait can you ctrl+click invalid URIs?

DHowett · 2023-05-31T16:09:29Z

send a screenshot!

lhecker · 2023-05-31T16:27:09Z

wait can you ctrl+click invalid URIs?

Yep, you can. It just might not work, because the OS validates them. For instance, this doesn't work:

"`n`e]8;;https://www.`u{202e}xn--fcbook-3nf5b.com/`e\TOTALLY REAL FACEBOOK LINK. YOU CAN TRUST ME.`e]8;;`e\ `n`n"

but this does (the a and e in "facebook" are not ASCII):

"`n`e]8;;https://www.`u{202e}fаcеbook.com/`e\TOTALLY REAL FACEBOOK LINK. YOU CAN TRUST ME.`e]8;;`e\ `n`n"

...which is a little concerning. You get this message instead:

send a screenshot!

What would you like to see? A comment above shows 2 situations - I'd be happy to make more.

DHowett · 2023-05-31T20:34:13Z

06174a9 didn't properly fix the issue of us showing homoglyphs

to be clear, it was not intended to

DHowett

Notes and Qs.

Generally, I am 100% happy with this. Just the UI thread concern!

DHowett · 2023-06-01T04:02:31Z

src/cascadia/TerminalControl/TermControl.cpp

        }
+
+        const auto uriText = sanitizeURI(_core.HoveredUriText());


Honestly, I am OKAY with this.

Thanks so much.

DHowett · 2023-06-01T04:03:24Z

src/cascadia/TerminalControl/TermControl.cpp

    {
-        auto weakThis{ get_weak() };
-        co_await wil::resume_foreground(Dispatcher());


This is another instance where we were switching to the foreground to make touching UI objects safe. Are we convinced that this is safe?

The only place _hoveredHyperlinkChanged is called from is the _core.HoveredHyperlinkChanged() event. _HoveredHyperlinkChangedHandlers is only called in ControlCore::_updateHoveredCell, which in turn is only called by SetHoveredCell and ClearHoveredCell. The former is only called by ControlInteractivity::PointerMoved which is a UI-thread-only function and the latter is only called by TermControl::_PointerExitedHandler which is the same. BTW It's somewhat weird that PointerMoved us in ControlInteractivity, but "pointer exited" isn't.

So yeah this can only be called from the UI thread.

That's technically an implementation detail, but I'll allow it ;P

carlos-zamora · 2023-06-05T23:23:13Z

src/cascadia/TerminalControl/Resources/en-US/Resources.resw

@@ -178,6 +178,10 @@
  <data name="HowToOpenRun.Text" xml:space="preserve">
    <value>Ctrl+Click to follow link</value>
  </data>
+  <data name="InvalidUri" xml:space="preserve">
+    <value>Invalid URI</value>
+    <comment>Whenever we encounter an invalid URI or URL we show this string as a warning.</comment>


Suggested change

<comment>Whenever we encounter an invalid URI or URL we show this string as a warning.</comment>

<comment>Whenever we encounter an invalid URI or URL we show this string as an error.</comment>

nit

Hmm I feel like "as a warning" is better in this case. 🤔
After all, this doesn't prevent you from clicking the URL.

src/cascadia/TerminalControl/TermControl.cpp

DHowett

I am okay merging without fixing Carlos' comments honestly

Co-authored-by: Carlos Zamora <[email protected]>

06174a9 didn't properly fix the issue of us showing homoglyphs in our URI tooltip. This commit introduces a different approach where we display both, the Punycode and Unicode encoding, whenever we encounter an IDN. This isn't perfect but simple to implement. Closes #15432 ## Validation Steps Performed * `https://www.xn--fcbook-3nf5b.com/` (which contains confusing glyphs) is shown both in its Punycode and Unicode form simultaneously. ✅ --------- Co-authored-by: Carlos Zamora <[email protected]> (cherry picked from commit f1aa699) Service-Card-Id: 90012448 Service-Version: 1.17

06174a9 didn't properly fix the issue of us showing homoglyphs in our URI tooltip. This commit introduces a different approach where we display both, the Punycode and Unicode encoding, whenever we encounter an IDN. This isn't perfect but simple to implement. Closes #15432 ## Validation Steps Performed * `https://www.xn--fcbook-3nf5b.com/` (which contains confusing glyphs) is shown both in its Punycode and Unicode form simultaneously. ✅ --------- Co-authored-by: Carlos Zamora <[email protected]> (cherry picked from commit f1aa699) Service-Card-Id: 90012449 Service-Version: 1.18

Display Unicode URIs side-by-side with their Punycode encoding

3d75a48

microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-User Interface Issues pertaining to the user interface of the Console or Terminal Priority-1 A description (P1) Product-Terminal The new Windows Terminal. labels May 31, 2023

lhecker commented May 31, 2023

View reviewed changes

DHowett requested changes Jun 1, 2023

View reviewed changes

microsoft-github-policy-service bot added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jun 1, 2023

carlos-zamora approved these changes Jun 5, 2023

View reviewed changes

carlos-zamora assigned DHowett Jun 5, 2023

DHowett approved these changes Jun 8, 2023

View reviewed changes

Update src/cascadia/TerminalControl/TermControl.cpp

2003bde

Co-authored-by: Carlos Zamora <[email protected]>

lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jun 8, 2023

microsoft-github-policy-service bot enabled auto-merge (squash) June 8, 2023 23:17

microsoft-github-policy-service bot merged commit f1aa699 into main Jun 8, 2023

microsoft-github-policy-service bot deleted the dev/lhecker/15432-homoglyph-uris branch June 8, 2023 23:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display Unicode URIs side-by-side with their Punycode encoding #15488

Display Unicode URIs side-by-side with their Punycode encoding #15488

lhecker commented May 31, 2023 •

edited

Loading

lhecker May 31, 2023

lhecker commented May 31, 2023 •

edited

Loading

lhecker May 31, 2023 •

edited

Loading

DHowett Jun 1, 2023

zadjii-msft commented May 31, 2023

DHowett commented May 31, 2023

lhecker commented May 31, 2023

DHowett commented May 31, 2023

DHowett left a comment

DHowett Jun 1, 2023

DHowett Jun 1, 2023

lhecker Jun 2, 2023

DHowett Jun 8, 2023

carlos-zamora Jun 5, 2023

lhecker Jun 8, 2023 •

edited

Loading

DHowett left a comment

	<comment>Whenever we encounter an invalid URI or URL we show this string as a warning.</comment>
	<comment>Whenever we encounter an invalid URI or URL we show this string as an error.</comment>

Display Unicode URIs side-by-side with their Punycode encoding #15488

Display Unicode URIs side-by-side with their Punycode encoding #15488

Conversation

lhecker commented May 31, 2023 • edited Loading

Validation Steps Performed

lhecker May 31, 2023

Choose a reason for hiding this comment

lhecker commented May 31, 2023 • edited Loading

lhecker May 31, 2023 • edited Loading

Choose a reason for hiding this comment

DHowett Jun 1, 2023

Choose a reason for hiding this comment

zadjii-msft commented May 31, 2023

DHowett commented May 31, 2023

lhecker commented May 31, 2023

DHowett commented May 31, 2023

DHowett left a comment

Choose a reason for hiding this comment

DHowett Jun 1, 2023

Choose a reason for hiding this comment

DHowett Jun 1, 2023

Choose a reason for hiding this comment

lhecker Jun 2, 2023

Choose a reason for hiding this comment

DHowett Jun 8, 2023

Choose a reason for hiding this comment

carlos-zamora Jun 5, 2023

Choose a reason for hiding this comment

lhecker Jun 8, 2023 • edited Loading

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

lhecker commented May 31, 2023 •

edited

Loading

lhecker commented May 31, 2023 •

edited

Loading

lhecker May 31, 2023 •

edited

Loading

lhecker Jun 8, 2023 •

edited

Loading