Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 2 commits into from
Jun 8, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d75a48
Display Unicode URIs side-by-side with their Punycode encoding
lhecker May 31, 2023
2003bde
Update src/cascadia/TerminalControl/TermControl.cpp
lhecker Jun 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/actions/spelling/allow/apis.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ IConnection
ICustom
IDialog
IDirect
Idn
IExplorer
IFACEMETHOD
IFile
Expand Down
8 changes: 3 additions & 5 deletions .github/actions/spelling/expect/expect.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ cmt
cmw
cmyk
CNL
cnn
cnt
CNTRL
Codeflow
Expand Down Expand Up @@ -260,7 +261,6 @@ condrv
conechokey
conemu
configurability
confusables
conhost
conime
conimeinfo
Expand Down Expand Up @@ -813,7 +813,6 @@ HIBYTE
hicon
HIDEWINDOW
hinst
Hirots
HISTORYBUFS
HISTORYNODUP
HISTORYSIZE
Expand All @@ -830,6 +829,8 @@ HMK
hmod
hmodule
hmon
homeglyphs
homoglyph
HORZ
hostable
hostlib
Expand Down Expand Up @@ -1287,7 +1288,6 @@ nullability
nullness
nullonfailure
nullopts
NULs
numlock
numpad
NUMSCROLL
Expand Down Expand Up @@ -1776,7 +1776,6 @@ somefile
SOURCEBRANCH
sourced
spammy
spand
SRCCODEPAGE
SRCCOPY
SRCINVERT
Expand Down Expand Up @@ -2302,7 +2301,6 @@ xunit
xutr
XVIRTUALSCREEN
XWalk
xwwyzz
xxyyzz
yact
YCast
Expand Down
4 changes: 4 additions & 0 deletions src/cascadia/TerminalControl/Resources/en-US/Resources.resw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@lhecker lhecker Jun 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

</data>
<data name="NoticeFontNotFound" xml:space="preserve">
<value>Unable to find the selected font "{0}".

Expand Down
69 changes: 49 additions & 20 deletions src/cascadia/TerminalControl/TermControl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3042,31 +3042,64 @@ 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
Copy link
Member Author

Choose a reason for hiding this comment

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

You should suppress whitespace changes!

// its own glyphs for а, с, е, о, р, х, and у that look practically identical to their ASCII counterparts.
// This is called an "IDN homoglyph attack".
//
// But outright showing Punycode URIs only is similarly flawed as they can end up looking similar to valid ASCII URIs.
// xn--cnn.com for instance looks confusingly similar to cnn.com, but actually represents U+407E.
//
// An optimal solution would detect any URI that contains homoglyphs and show them in their Punycode form.
// Such a detector however is not quite trivial and requires constant maintenance, which this project's
// maintainers aren't currently well equipped to handle. As such we do the next best thing and show the
// Punycode encoding side-by-side with the Unicode string for any IDN.
static winrt::hstring sanitizeURI(winrt::hstring uri)
{
auto weakThis{ get_weak() };
co_await wil::resume_foreground(Dispatcher());
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

if (auto self{ weakThis.get() })
if (uri.empty())
{
auto lastHoveredCell = _core.HoveredCell();
if (lastHoveredCell)
return uri;
}

wchar_t punycodeBuffer[256];
wchar_t unicodeBuffer[256];

// These functions return int, but are documented to only return positive numbers.
// Better make sure though. It allows us to pass punycodeLength right into IdnToUnicode.
const auto punycodeLength = std::max(0, IdnToAscii(0, uri.data(), gsl::narrow<int>(uri.size()), &punycodeBuffer[0], 256));
const auto unicodeLength = std::max(0, IdnToUnicode(0, &punycodeBuffer[0], punycodeLength, &unicodeBuffer[0], 256));

if (punycodeLength <= 0 || unicodeLength <= 0)
{
winrt::hstring uriText = _core.HoveredUriText();
if (uriText.empty())
return RS_(L"InvalidUri");
}

const std::wstring_view punycode{ &punycodeBuffer[0], gsl::narrow_cast<size_t>(punycodeLength) };
const std::wstring_view unicode{ &unicodeBuffer[0], gsl::narrow_cast<size_t>(unicodeLength) };

// IdnToAscii/IdnToUnicode return the input string as is if it's all
// plain ASCII. But we don't know if the input URI is Punycode or not.
// --> It's non-Punycode and ASCII if it round-trips.
if (uri == punycode && uri == unicode)
{
co_return;
return uri;
}

try
return winrt::hstring{ fmt::format(FMT_COMPILE(L"{}\n({})"), punycode, unicode) };
}

void TermControl::_hoveredHyperlinkChanged(const IInspectable& /*sender*/, const IInspectable& /*args*/)
{
// DisplayUri will filter out non-printable characters and confusables.
Windows::Foundation::Uri parsedUri{ uriText };
if (!parsedUri)
const auto lastHoveredCell = _core.HoveredCell();
if (!lastHoveredCell)
{
co_return;
return;
}

const auto uriText = sanitizeURI(_core.HoveredUriText());
Copy link
Member Author

@lhecker lhecker May 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Honestly, I am OKAY with this.

Thanks so much.

if (uriText.empty())
{
return;
}
uriText = parsedUri.DisplayUri();

const auto panel = SwapChainPanel();
const auto scale = panel.CompositionScaleX();
Expand All @@ -3089,10 +3122,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
OverlayCanvas().SetLeft(HyperlinkTooltipBorder(), locationInDIPs.x - offset.x);
OverlayCanvas().SetTop(HyperlinkTooltipBorder(), locationInDIPs.y - offset.y);
}
CATCH_LOG();
}
}
}

winrt::fire_and_forget TermControl::_updateSelectionMarkers(IInspectable /*sender*/, Control::UpdateSelectionMarkersEventArgs args)
{
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalControl/TermControl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void _CurrentCursorPositionHandler(const IInspectable& sender, const CursorPositionEventArgs& eventArgs);
void _FontInfoHandler(const IInspectable& sender, const FontInfoEventArgs& eventArgs);

winrt::fire_and_forget _hoveredHyperlinkChanged(IInspectable sender, IInspectable args);
void _hoveredHyperlinkChanged(const IInspectable& sender, const IInspectable& args);
winrt::fire_and_forget _updateSelectionMarkers(IInspectable sender, Control::UpdateSelectionMarkersEventArgs args);

void _coreFontSizeChanged(const int fontWidth,
Expand Down