Hash the URI as part of the hyperlink ID (#7940)

It turns out that we missed part of the OSC 8 spec which indicated that
_hyperlinks with the same ID but different URIs are logically distinct._

> Character cells that have the same target URI and the same nonempty id
> are always underlined together on mouseover.
> The same id is only used for connecting character cells whose URIs is
> also the same. Character cells pointing to different URIs should never
> be underlined together when hovering over.

This pull request fixes that oversight by appending the (hashed) URI to
the generated ID.

When Terminal receives one of these links over ConPTY, it will hash the
URL a second time and therefore append a second hashed ID. This is taken
as an acceptable cost.

Fixes #7698

(cherry picked from commit df7c3cc)

Author: DHowett

src/buffer/out/textBuffer.cpp

@@ -2281,32 +2281,35 @@ std::wstring TextBuffer::GetHyperlinkUriFromId(uint16_t id) const
 // - The user-defined id
 // Return value:
 // - The internal hyperlink ID
-uint16_t TextBuffer::GetHyperlinkId(std::wstring_view params)
+uint16_t TextBuffer::GetHyperlinkId(std::wstring_view uri, std::wstring_view id)
 {
-    uint16_t id = 0;
-    if (params.empty())
+    uint16_t numericId = 0;
+    if (id.empty())
     {
         // no custom id specified, return our internal count
-        id = _currentHyperlinkId;
+        numericId = _currentHyperlinkId;
         ++_currentHyperlinkId;
     }
     else
     {
         // assign _currentHyperlinkId if the custom id does not already exist
-        const auto result = _hyperlinkCustomIdMap.emplace(params, _currentHyperlinkId);
+        std::wstring newId{ id };
+        // hash the URL and add it to the custom ID - GH#7698
+        newId += L"%" + std::to_wstring(std::hash<std::wstring_view>{}(uri));
+        const auto result = _hyperlinkCustomIdMap.emplace(newId, _currentHyperlinkId);
         if (result.second)
         {
             // the custom id did not already exist
             ++_currentHyperlinkId;
         }
-        id = (*(result.first)).second;
+        numericId = (*(result.first)).second;
     }
     // _currentHyperlinkId could overflow, make sure its not 0
     if (_currentHyperlinkId == 0)
     {
         ++_currentHyperlinkId;
     }
-    return id;
+    return numericId;
 }
 
 // Method Description:

src/buffer/out/textBuffer.hpp

@@ -143,7 +143,7 @@ class TextBuffer final
 
     void AddHyperlinkToMap(std::wstring_view uri, uint16_t id);
     std::wstring GetHyperlinkUriFromId(uint16_t id) const;
-    uint16_t GetHyperlinkId(std::wstring_view params);
+    uint16_t GetHyperlinkId(std::wstring_view uri, std::wstring_view id);
     void RemoveHyperlinkFromMap(uint16_t id);
     std::wstring GetCustomIdFromId(uint16_t id) const;
     void CopyHyperlinkMaps(const TextBuffer& OtherBuffer);

src/cascadia/TerminalCore/TerminalApi.cpp

@@ -574,7 +574,7 @@ CATCH_LOG_RETURN_FALSE()
 bool Terminal::AddHyperlink(std::wstring_view uri, std::wstring_view params) noexcept
 {
     auto attr = _buffer->GetCurrentAttributes();
-    const auto id = _buffer->GetHyperlinkId(params);
+    const auto id = _buffer->GetHyperlinkId(uri, params);
     attr.SetHyperlinkId(id);
     _buffer->SetCurrentAttributes(attr);
     _buffer->AddHyperlinkToMap(uri, id);

src/cascadia/UnitTests_TerminalCore/TerminalApiTest.cpp

@@ -37,6 +37,7 @@ namespace TerminalCoreUnitTests
 
         TEST_METHOD(AddHyperlink);
         TEST_METHOD(AddHyperlinkCustomId);
+        TEST_METHOD(AddHyperlinkCustomIdDifferentUri);
     };
 };
 
@@ -296,15 +297,45 @@ void TerminalCoreUnitTests::TerminalApiTest::AddHyperlinkCustomId()
     stateMachine.ProcessString(L"\x1b]8;id=myId;test.url\x9c");
     VERIFY_IS_TRUE(tbi.GetCurrentAttributes().IsHyperlink());
     VERIFY_ARE_EQUAL(tbi.GetHyperlinkUriFromId(tbi.GetCurrentAttributes().GetHyperlinkId()), L"test.url");
-    VERIFY_ARE_EQUAL(tbi.GetHyperlinkId(L"myId"), tbi.GetCurrentAttributes().GetHyperlinkId());
+    VERIFY_ARE_EQUAL(tbi.GetHyperlinkId(L"test.url", L"myId"), tbi.GetCurrentAttributes().GetHyperlinkId());
 
     // Send any other text
     stateMachine.ProcessString(L"Hello World");
     VERIFY_IS_TRUE(tbi.GetCurrentAttributes().IsHyperlink());
     VERIFY_ARE_EQUAL(tbi.GetHyperlinkUriFromId(tbi.GetCurrentAttributes().GetHyperlinkId()), L"test.url");
-    VERIFY_ARE_EQUAL(tbi.GetHyperlinkId(L"myId"), tbi.GetCurrentAttributes().GetHyperlinkId());
+    VERIFY_ARE_EQUAL(tbi.GetHyperlinkId(L"test.url", L"myId"), tbi.GetCurrentAttributes().GetHyperlinkId());
 
     // Process the closing osc 8 sequences
     stateMachine.ProcessString(L"\x1b]8;;\x9c");
     VERIFY_IS_FALSE(tbi.GetCurrentAttributes().IsHyperlink());
 }
+
+void TerminalCoreUnitTests::TerminalApiTest::AddHyperlinkCustomIdDifferentUri()
+{
+    // This is a nearly literal copy-paste of ScreenBufferTests::TestAddHyperlinkCustomId, adapted for the Terminal
+
+    Terminal term;
+    DummyRenderTarget emptyRT;
+    term.Create({ 100, 100 }, 0, emptyRT);
+
+    auto& tbi = *(term._buffer);
+    auto& stateMachine = *(term._stateMachine);
+
+    // Process the opening osc 8 sequence
+    stateMachine.ProcessString(L"\x1b]8;id=myId;test.url\x9c");
+    VERIFY_IS_TRUE(tbi.GetCurrentAttributes().IsHyperlink());
+    VERIFY_ARE_EQUAL(tbi.GetHyperlinkUriFromId(tbi.GetCurrentAttributes().GetHyperlinkId()), L"test.url");
+    VERIFY_ARE_EQUAL(tbi.GetHyperlinkId(L"test.url", L"myId"), tbi.GetCurrentAttributes().GetHyperlinkId());
+
+    const auto oldAttributes{ tbi.GetCurrentAttributes() };
+
+    // Send any other text
+    stateMachine.ProcessString(L"\x1b]8;id=myId;other.url\x9c");
+    VERIFY_IS_TRUE(tbi.GetCurrentAttributes().IsHyperlink());
+    VERIFY_ARE_EQUAL(tbi.GetHyperlinkUriFromId(tbi.GetCurrentAttributes().GetHyperlinkId()), L"other.url");
+    VERIFY_ARE_EQUAL(tbi.GetHyperlinkId(L"other.url", L"myId"), tbi.GetCurrentAttributes().GetHyperlinkId());
+
+    // This second URL should not change the URL of the original ID!
+    VERIFY_ARE_EQUAL(tbi.GetHyperlinkUriFromId(oldAttributes.GetHyperlinkId()), L"test.url");
+    VERIFY_ARE_NOT_EQUAL(oldAttributes.GetHyperlinkId(), tbi.GetCurrentAttributes().GetHyperlinkId());
+}

src/host/getset.cpp

@@ -1567,7 +1567,7 @@ void DoSrvAddHyperlink(SCREEN_INFORMATION& screenInfo,
                        const std::wstring_view params)
 {
     auto attr = screenInfo.GetAttributes();
-    const auto id = screenInfo.GetTextBuffer().GetHyperlinkId(params);
+    const auto id = screenInfo.GetTextBuffer().GetHyperlinkId(uri, params);
     attr.SetHyperlinkId(id);
     screenInfo.GetTextBuffer().SetCurrentAttributes(attr);
     screenInfo.GetTextBuffer().AddHyperlinkToMap(uri, id);

src/host/ut_host/ScreenBufferTests.cpp

@@ -213,6 +213,7 @@ class ScreenBufferTests
 
     TEST_METHOD(TestAddHyperlink);
     TEST_METHOD(TestAddHyperlinkCustomId);
+    TEST_METHOD(TestAddHyperlinkCustomIdDifferentUri);
 
     TEST_METHOD(UpdateVirtualBottomWhenCursorMovesBelowIt);
 
@@ -5956,19 +5957,46 @@ void ScreenBufferTests::TestAddHyperlinkCustomId()
     stateMachine.ProcessString(L"\x1b]8;id=myId;test.url\x9c");
     VERIFY_IS_TRUE(tbi.GetCurrentAttributes().IsHyperlink());
     VERIFY_ARE_EQUAL(tbi.GetHyperlinkUriFromId(tbi.GetCurrentAttributes().GetHyperlinkId()), L"test.url");
-    VERIFY_ARE_EQUAL(tbi.GetHyperlinkId(L"myId"), tbi.GetCurrentAttributes().GetHyperlinkId());
+    VERIFY_ARE_EQUAL(tbi.GetHyperlinkId(L"test.url", L"myId"), tbi.GetCurrentAttributes().GetHyperlinkId());
 
     // Send any other text
     stateMachine.ProcessString(L"Hello World");
     VERIFY_IS_TRUE(tbi.GetCurrentAttributes().IsHyperlink());
     VERIFY_ARE_EQUAL(tbi.GetHyperlinkUriFromId(tbi.GetCurrentAttributes().GetHyperlinkId()), L"test.url");
-    VERIFY_ARE_EQUAL(tbi.GetHyperlinkId(L"myId"), tbi.GetCurrentAttributes().GetHyperlinkId());
+    VERIFY_ARE_EQUAL(tbi.GetHyperlinkId(L"test.url", L"myId"), tbi.GetCurrentAttributes().GetHyperlinkId());
 
     // Process the closing osc 8 sequences
     stateMachine.ProcessString(L"\x1b]8;;\x9c");
     VERIFY_IS_FALSE(tbi.GetCurrentAttributes().IsHyperlink());
 }
 
+void ScreenBufferTests::TestAddHyperlinkCustomIdDifferentUri()
+{
+    auto& g = ServiceLocator::LocateGlobals();
+    auto& gci = g.getConsoleInformation();
+    auto& si = gci.GetActiveOutputBuffer();
+    auto& tbi = si.GetTextBuffer();
+    auto& stateMachine = si.GetStateMachine();
+
+    // Process the opening osc 8 sequence with a custom id
+    stateMachine.ProcessString(L"\x1b]8;id=myId;test.url\x9c");
+    VERIFY_IS_TRUE(tbi.GetCurrentAttributes().IsHyperlink());
+    VERIFY_ARE_EQUAL(tbi.GetHyperlinkUriFromId(tbi.GetCurrentAttributes().GetHyperlinkId()), L"test.url");
+    VERIFY_ARE_EQUAL(tbi.GetHyperlinkId(L"test.url", L"myId"), tbi.GetCurrentAttributes().GetHyperlinkId());
+
+    const auto oldAttributes{ tbi.GetCurrentAttributes() };
+
+    // Send any other text
+    stateMachine.ProcessString(L"\x1b]8;id=myId;other.url\x9c");
+    VERIFY_IS_TRUE(tbi.GetCurrentAttributes().IsHyperlink());
+    VERIFY_ARE_EQUAL(tbi.GetHyperlinkUriFromId(tbi.GetCurrentAttributes().GetHyperlinkId()), L"other.url");
+    VERIFY_ARE_EQUAL(tbi.GetHyperlinkId(L"other.url", L"myId"), tbi.GetCurrentAttributes().GetHyperlinkId());
+
+    // This second URL should not change the URL of the original ID!
+    VERIFY_ARE_EQUAL(tbi.GetHyperlinkUriFromId(oldAttributes.GetHyperlinkId()), L"test.url");
+    VERIFY_ARE_NOT_EQUAL(oldAttributes.GetHyperlinkId(), tbi.GetCurrentAttributes().GetHyperlinkId());
+}
+
 void ScreenBufferTests::UpdateVirtualBottomWhenCursorMovesBelowIt()
 {
     auto& g = ServiceLocator::LocateGlobals();

src/host/ut_host/TextBufferTests.cpp

@@ -2546,30 +2546,33 @@ void TextBufferTests::HyperlinkTrim()
 
     // Set a hyperlink id in the first row and add a hyperlink to our map
     const COORD pos{ 70, 0 };
-    const auto id = _buffer->GetHyperlinkId(customId);
+    const auto id = _buffer->GetHyperlinkId(url, customId);
     TextAttribute newAttr{ 0x7f };
     newAttr.SetHyperlinkId(id);
     _buffer->GetRowByOffset(pos.Y).GetAttrRow().SetAttrToEnd(pos.X, newAttr);
     _buffer->AddHyperlinkToMap(url, id);
 
     // Set a different hyperlink id somewhere else in the buffer
     const COORD otherPos{ 70, 5 };
-    const auto otherId = _buffer->GetHyperlinkId(otherCustomId);
+    const auto otherId = _buffer->GetHyperlinkId(otherUrl, otherCustomId);
     newAttr.SetHyperlinkId(otherId);
     _buffer->GetRowByOffset(otherPos.Y).GetAttrRow().SetAttrToEnd(otherPos.X, newAttr);
     _buffer->AddHyperlinkToMap(otherUrl, otherId);
 
     // Increment the circular buffer
     _buffer->IncrementCircularBuffer();
 
+    const auto finalCustomId = fmt::format(L"{}%{}", customId, std::hash<std::wstring_view>{}(url));
+    const auto finalOtherCustomId = fmt::format(L"{}%{}", otherCustomId, std::hash<std::wstring_view>{}(otherUrl));
+
     // The hyperlink reference that was only in the first row should be deleted from the map
     VERIFY_ARE_EQUAL(_buffer->_hyperlinkMap.find(id), _buffer->_hyperlinkMap.end());
     // Since there was a custom id, that should be deleted as well
-    VERIFY_ARE_EQUAL(_buffer->_hyperlinkCustomIdMap.find(customId), _buffer->_hyperlinkCustomIdMap.end());
+    VERIFY_ARE_EQUAL(_buffer->_hyperlinkCustomIdMap.find(finalCustomId), _buffer->_hyperlinkCustomIdMap.end());
 
     // The other hyperlink reference should not be deleted
     VERIFY_ARE_EQUAL(_buffer->_hyperlinkMap[otherId], otherUrl);
-    VERIFY_ARE_EQUAL(_buffer->_hyperlinkCustomIdMap[otherCustomId], otherId);
+    VERIFY_ARE_EQUAL(_buffer->_hyperlinkCustomIdMap[finalOtherCustomId], otherId);
 }
 
 // This tests that when we increment the circular buffer, non-obsolete hyperlink references
@@ -2587,7 +2590,7 @@ void TextBufferTests::NoHyperlinkTrim()
 
     // Set a hyperlink id in the first row and add a hyperlink to our map
     const COORD pos{ 70, 0 };
-    const auto id = _buffer->GetHyperlinkId(customId);
+    const auto id = _buffer->GetHyperlinkId(url, customId);
     TextAttribute newAttr{ 0x7f };
     newAttr.SetHyperlinkId(id);
     _buffer->GetRowByOffset(pos.Y).GetAttrRow().SetAttrToEnd(pos.X, newAttr);
@@ -2600,7 +2603,9 @@ void TextBufferTests::NoHyperlinkTrim()
     // Increment the circular buffer
     _buffer->IncrementCircularBuffer();
 
+    const auto finalCustomId = fmt::format(L"{}%{}", customId, std::hash<std::wstring_view>{}(url));
+
     // The hyperlink reference should not be deleted from the map since it is still present in the buffer
     VERIFY_ARE_EQUAL(_buffer->GetHyperlinkUriFromId(id), url);
-    VERIFY_ARE_EQUAL(_buffer->_hyperlinkCustomIdMap[customId], id);
+    VERIFY_ARE_EQUAL(_buffer->_hyperlinkCustomIdMap[finalCustomId], id);
 }