From 26ca18a05b14bd22dfa5e2a881d9f4f63d45147d Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Fri, 4 Apr 2025 17:32:04 -0400 Subject: [PATCH] LibWebView: Do not use AK::format to format search engine URLs This is to prepare for custom search engines. If we use AK::format, it would be trivial for a user (or bad actor) to come up with a template search engine URL that ultimately crashes the browser due to internal assertions in AK::format. For example: https://example.com/crash={1} Rather than coming up with a complicated pre-format validator, let's just not use AK::format. Custom URLs will signify their template query parameters with "%s". So we can do the same with our built-in engines. When it comes time to format the URL, we will do a simple string replacement. --- Libraries/LibWebView/SearchEngine.cpp | 46 +++++++++++--------------- Libraries/LibWebView/SearchEngine.h | 5 +-- Libraries/LibWebView/URL.cpp | 6 ++-- Libraries/LibWebView/URL.h | 5 +-- Tests/LibWebView/TestWebViewURL.cpp | 18 +++++----- UI/AppKit/Interface/LadybirdWebView.mm | 4 +-- UI/AppKit/Interface/TabController.mm | 6 +--- UI/Qt/LocationEdit.cpp | 6 +--- UI/Qt/Tab.cpp | 4 +-- 9 files changed, 44 insertions(+), 56 deletions(-) diff --git a/Libraries/LibWebView/SearchEngine.cpp b/Libraries/LibWebView/SearchEngine.cpp index 767f84a7c3d..1c42bddb092 100644 --- a/Libraries/LibWebView/SearchEngine.cpp +++ b/Libraries/LibWebView/SearchEngine.cpp @@ -5,21 +5,22 @@ */ #include +#include #include namespace WebView { static auto builtin_search_engines = to_array({ - { "Bing"_string, "https://www.bing.com/search?q={}"_string }, - { "Brave"_string, "https://search.brave.com/search?q={}"_string }, - { "DuckDuckGo"_string, "https://duckduckgo.com/?q={}"_string }, - { "Ecosia"_string, "https://ecosia.org/search?q={}"_string }, - { "Google"_string, "https://www.google.com/search?q={}"_string }, - { "Kagi"_string, "https://kagi.com/search?q={}"_string }, - { "Mojeek"_string, "https://www.mojeek.com/search?q={}"_string }, - { "Startpage"_string, "https://startpage.com/search?q={}"_string }, - { "Yahoo"_string, "https://search.yahoo.com/search?p={}"_string }, - { "Yandex"_string, "https://yandex.com/search/?text={}"_string }, + { "Bing"_string, "https://www.bing.com/search?q=%s"_string }, + { "Brave"_string, "https://search.brave.com/search?q=%s"_string }, + { "DuckDuckGo"_string, "https://duckduckgo.com/?q=%s"_string }, + { "Ecosia"_string, "https://ecosia.org/search?q=%s"_string }, + { "Google"_string, "https://www.google.com/search?q=%s"_string }, + { "Kagi"_string, "https://kagi.com/search?q=%s"_string }, + { "Mojeek"_string, "https://www.mojeek.com/search?q=%s"_string }, + { "Startpage"_string, "https://startpage.com/search?q=%s"_string }, + { "Yahoo"_string, "https://search.yahoo.com/search?p=%s"_string }, + { "Yandex"_string, "https://yandex.com/search/?text=%s"_string }, }); ReadonlySpan search_engines() @@ -34,29 +35,20 @@ Optional find_search_engine_by_name(StringView name) }); } -Optional find_search_engine_by_query_url(StringView query_url) -{ - return find_value(builtin_search_engines, [&](auto const& engine) { - return engine.query_url == query_url; - }); -} - -String format_search_query_for_display(StringView query_url, StringView query) +String SearchEngine::format_search_query_for_display(StringView query) const { static constexpr auto MAX_SEARCH_STRING_LENGTH = 32; - if (auto search_engine = find_search_engine_by_query_url(query_url); search_engine.has_value()) { - return MUST(String::formatted("Search {} for \"{:.{}}{}\"", - search_engine->name, - query, - MAX_SEARCH_STRING_LENGTH, - query.length() > MAX_SEARCH_STRING_LENGTH ? "..."sv : ""sv)); - } - - return MUST(String::formatted("Search for \"{:.{}}{}\"", + return MUST(String::formatted("Search {} for \"{:.{}}{}\"", + name, query, MAX_SEARCH_STRING_LENGTH, query.length() > MAX_SEARCH_STRING_LENGTH ? "..."sv : ""sv)); } +String SearchEngine::format_search_query_for_navigation(StringView query) const +{ + return MUST(query_url.replace("%s"sv, URL::percent_encode(query), ReplaceMode::All)); +} + } diff --git a/Libraries/LibWebView/SearchEngine.h b/Libraries/LibWebView/SearchEngine.h index 225fb6182cc..e29ec04f34f 100644 --- a/Libraries/LibWebView/SearchEngine.h +++ b/Libraries/LibWebView/SearchEngine.h @@ -13,13 +13,14 @@ namespace WebView { struct SearchEngine { + String format_search_query_for_display(StringView query) const; + String format_search_query_for_navigation(StringView query) const; + String name; String query_url; }; ReadonlySpan search_engines(); Optional find_search_engine_by_name(StringView name); -Optional find_search_engine_by_query_url(StringView query_url); -String format_search_query_for_display(StringView query_url, StringView query); } diff --git a/Libraries/LibWebView/URL.cpp b/Libraries/LibWebView/URL.cpp index d4c110c4410..9e760ea8027 100644 --- a/Libraries/LibWebView/URL.cpp +++ b/Libraries/LibWebView/URL.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2023-2025, Tim Flynn + * Copyright (c) 2023-2025, Tim Flynn * Copyright (c) 2023, Cameron Youell * Copyright (c) 2025, Manuel Zahariev * @@ -13,13 +13,13 @@ namespace WebView { -Optional sanitize_url(StringView location, Optional search_engine, AppendTLD append_tld) +Optional sanitize_url(StringView location, Optional const& search_engine, AppendTLD append_tld) { auto search_url_or_error = [&]() -> Optional { if (!search_engine.has_value()) return {}; - return URL::Parser::basic_parse(MUST(String::formatted(*search_engine, URL::percent_encode(location)))); + return URL::Parser::basic_parse(search_engine->format_search_query_for_navigation(location)); }; location = location.trim_whitespace(); diff --git a/Libraries/LibWebView/URL.h b/Libraries/LibWebView/URL.h index aa617019e14..780eb4c3571 100644 --- a/Libraries/LibWebView/URL.h +++ b/Libraries/LibWebView/URL.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2023, Tim Flynn + * Copyright (c) 2023-2025, Tim Flynn * * SPDX-License-Identifier: BSD-2-Clause */ @@ -9,6 +9,7 @@ #include #include #include +#include namespace WebView { @@ -16,7 +17,7 @@ enum class AppendTLD { No, Yes, }; -Optional sanitize_url(StringView, Optional search_engine = {}, AppendTLD = AppendTLD::No); +Optional sanitize_url(StringView, Optional const& search_engine = {}, AppendTLD = AppendTLD::No); Vector sanitize_urls(ReadonlySpan raw_urls, URL::URL const& new_tab_page_url); struct URLParts { diff --git a/Tests/LibWebView/TestWebViewURL.cpp b/Tests/LibWebView/TestWebViewURL.cpp index c2924d5c472..be724c10217 100644 --- a/Tests/LibWebView/TestWebViewURL.cpp +++ b/Tests/LibWebView/TestWebViewURL.cpp @@ -6,8 +6,14 @@ */ #include +#include #include +static WebView::SearchEngine s_test_engine { + .name = "Test"_string, + .query_url = "https://ecosia.org/search?q=%s"_string +}; + static void compare_url_parts(StringView url, WebView::URLParts const& expected) { auto result = WebView::break_url_into_parts(url); @@ -28,9 +34,7 @@ static bool is_sanitized_url_the_same(StringView url) static void expect_url_equals_sanitized_url(StringView test_url, StringView url, WebView::AppendTLD append_tld = WebView::AppendTLD::No) { - StringView const search_engine_url = "https://ecosia.org/search?q={}"sv; - - auto sanitized_url = WebView::sanitize_url(url, search_engine_url, append_tld); + auto sanitized_url = WebView::sanitize_url(url, s_test_engine, append_tld); EXPECT(sanitized_url.has_value()); EXPECT_EQ(sanitized_url->to_string(), test_url); @@ -38,13 +42,11 @@ static void expect_url_equals_sanitized_url(StringView test_url, StringView url, static void expect_search_url_equals_sanitized_url(StringView url) { - StringView const search_engine_url = "https://ecosia.org/search?q={}"sv; - auto const search_url = String::formatted(search_engine_url, URL::percent_encode(url)); - - auto sanitized_url = WebView::sanitize_url(url, search_engine_url); + auto search_url = s_test_engine.format_search_query_for_navigation(url); + auto sanitized_url = WebView::sanitize_url(url, s_test_engine); EXPECT(sanitized_url.has_value()); - EXPECT_EQ(sanitized_url->to_string(), search_url.value()); + EXPECT_EQ(sanitized_url->to_string(), search_url); } TEST_CASE(invalid_url) diff --git a/UI/AppKit/Interface/LadybirdWebView.mm b/UI/AppKit/Interface/LadybirdWebView.mm index 4190eaeb02c..ae8c0b2d9db 100644 --- a/UI/AppKit/Interface/LadybirdWebView.mm +++ b/UI/AppKit/Interface/LadybirdWebView.mm @@ -631,7 +631,7 @@ static void copy_data_to_clipboard(StringView data, NSPasteboardType pasteboard_ TemporaryChange change_url { m_context_menu_search_text, move(selected_text) }; if (m_context_menu_search_text.has_value()) { - auto action_text = WebView::format_search_query_for_display(search_engine->query_url, *m_context_menu_search_text); + auto action_text = search_engine->format_search_query_for_display(*m_context_menu_search_text); [search_selected_text_menu_item setTitle:Ladybird::string_to_ns_string(action_text)]; [search_selected_text_menu_item setHidden:NO]; } else { @@ -1135,7 +1135,7 @@ static void copy_data_to_clipboard(StringView data, NSPasteboardType pasteboard_ if (!search_engine.has_value()) return; - auto url_string = MUST(String::formatted(search_engine->query_url, URL::percent_encode(*m_context_menu_search_text))); + auto url_string = search_engine->format_search_query_for_navigation(*m_context_menu_search_text); auto url = URL::Parser::basic_parse(url_string); VERIFY(url.has_value()); [self.observer onCreateNewTab:url.release_value() activateTab:Web::HTML::ActivateTab::Yes]; diff --git a/UI/AppKit/Interface/TabController.mm b/UI/AppKit/Interface/TabController.mm index 49d44ad6153..4c07ade18be 100644 --- a/UI/AppKit/Interface/TabController.mm +++ b/UI/AppKit/Interface/TabController.mm @@ -301,11 +301,7 @@ static NSString* const TOOLBAR_TAB_OVERVIEW_IDENTIFIER = @"ToolbarTabOverviewIde - (BOOL)navigateToLocation:(String)location { - Optional search_engine_url; - if (auto const& search_engine = WebView::Application::settings().search_engine(); search_engine.has_value()) - search_engine_url = search_engine->query_url; - - if (auto url = WebView::sanitize_url(location, search_engine_url); url.has_value()) { + if (auto url = WebView::sanitize_url(location, WebView::Application::settings().search_engine()); url.has_value()) { [self loadURL:*url]; } diff --git a/UI/Qt/LocationEdit.cpp b/UI/Qt/LocationEdit.cpp index 642e9d7a7c9..e7ce80a4420 100644 --- a/UI/Qt/LocationEdit.cpp +++ b/UI/Qt/LocationEdit.cpp @@ -36,16 +36,12 @@ LocationEdit::LocationEdit(QWidget* parent) clearFocus(); - Optional search_engine_url; - if (auto const& search_engine = WebView::Application::settings().search_engine(); search_engine.has_value()) - search_engine_url = search_engine->query_url; - auto query = ak_string_from_qstring(text()); auto ctrl_held = QApplication::keyboardModifiers() & Qt::ControlModifier; auto append_tld = ctrl_held ? WebView::AppendTLD::Yes : WebView::AppendTLD::No; - if (auto url = WebView::sanitize_url(query, search_engine_url, append_tld); url.has_value()) + if (auto url = WebView::sanitize_url(query, WebView::Application::settings().search_engine(), append_tld); url.has_value()) set_url(url.release_value()); }); diff --git a/UI/Qt/Tab.cpp b/UI/Qt/Tab.cpp index 62949bf1b3e..a5a7cbc63a1 100644 --- a/UI/Qt/Tab.cpp +++ b/UI/Qt/Tab.cpp @@ -459,7 +459,7 @@ Tab::Tab(BrowserWindow* window, RefPtr parent_client, if (!search_engine.has_value()) return; - auto url_string = MUST(String::formatted(search_engine->query_url, URL::percent_encode(*m_page_context_menu_search_text))); + auto url_string = search_engine->format_search_query_for_navigation(*m_page_context_menu_search_text); auto url = URL::Parser::basic_parse(url_string); VERIFY(url.has_value()); @@ -531,7 +531,7 @@ Tab::Tab(BrowserWindow* window, RefPtr parent_client, TemporaryChange change_url { m_page_context_menu_search_text, AK::move(selected_text) }; if (m_page_context_menu_search_text.has_value()) { - auto action_text = WebView::format_search_query_for_display(search_engine->query_url, *m_page_context_menu_search_text); + auto action_text = search_engine->format_search_query_for_display(*m_page_context_menu_search_text); search_selected_text_action->setText(qstring_from_ak_string(action_text)); search_selected_text_action->setVisible(true); } else {