Skip to content

Handle very large url pattern input#52836

Merged
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
roberto-apple:eng/Handle-very-large-url-pattern-input
Oct 25, 2025
Merged

Handle very large url pattern input#52836
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
roberto-apple:eng/Handle-very-large-url-pattern-input

Conversation

@roberto-apple
Copy link
Copy Markdown
Contributor

@roberto-apple roberto-apple commented Oct 22, 2025 •
edited by webkit-early-warning-system
Loading

f5e47e0

Handle very large url pattern input
https://bugs.webkit.org/show_bug.cgi?id=301302
rdar://163099884

Reviewed by Ryosuke Niwa and Matthieu Dubet.

When creating a URLPattern with a string as an input, it is possible to
create it with a string so large that the underlying token list (vector)
will intentionally crash when performing an append operation and the
capacity cannot be increased. Check the result of each token append
operation and throw an exception if an append cannot be performed.

Test: fast/dom/DOMURL/url-pattern-very-large-string.html

* LayoutTests/fast/dom/DOMURL/url-pattern-very-large-string-expected.txt: Added.
* LayoutTests/fast/dom/DOMURL/url-pattern-very-large-string.html: Added.
* Source/WebCore/Modules/url-pattern/URLPatternTokenizer.cpp:
(WebCore::URLPatternUtilities::Tokenizer::addToken):
(WebCore::URLPatternUtilities::Tokenizer::tokenize):
* Source/WebCore/Modules/url-pattern/URLPatternTokenizer.h:

Canonical link: https://commits.webkit.org/302131@main

df19ca3

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows Apple Internal
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win ❌ 🛠 ios-apple
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ❌ 🧪 win-tests ✅ 🛠 mac-apple
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe ❌ 🛠 vision-apple
  🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@roberto-apple roberto-apple self-assigned this Oct 22, 2025
@roberto-apple roberto-apple added the New Bugs Unclassified bugs are placed in this component until the correct component can be determined. label Oct 22, 2025
@webkit-early-warning-system
Copy link
Copy Markdown
Collaborator

webkit-early-warning-system commented Oct 22, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 1864a72)

Details

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows Apple Internal
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win ⏳ 🛠 ios-apple
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ❌ 🧪 win-tests ⏳ 🛠 mac-apple
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe ✅ 🛠 vision-apple
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@roberto-apple roberto-apple marked this pull request as ready for review October 23, 2025 00:56
@roberto-apple roberto-apple requested a review from rniwa as a code owner October 23, 2025 00:56
@mdubet
Copy link
Copy Markdown
Contributor

mdubet commented Oct 23, 2025 •
edited
Loading

The test could use testharness.js and assert_throws_js() to properly assert the correct exception is thrown.

@roberto-apple roberto-apple force-pushed the eng/Handle-very-large-url-pattern-input branch from 1864a72 to 8b6fc1e Compare October 23, 2025 23:32
@webkit-early-warning-system
Copy link
Copy Markdown
Collaborator

webkit-early-warning-system commented Oct 24, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 8b6fc1e)

Details

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows Apple Internal
⏳ 🧪 style ⏳ 🛠 ios ⏳ 🛠 mac ⏳ 🛠 wpe ✅ 🛠 win ✅ 🛠 ios-apple
⏳ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ❌ 🧪 win-tests ✅ 🛠 mac-apple
⏳ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe ⏳ 🛠 vision-apple
⏳ 🧪 webkitpy ✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ⏳ 🛠 wpe-cairo
⏳ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ⏳ 🛠 gtk
⏳ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
⏳ 🧪 services ✅ 🛠 vision-sim ⏳ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ⏳ 🛠 mac-safer-cpp ⏳ 🛠 jsc-armv7
✅ 🛠 tv-sim ⏳ 🧪 jsc-armv7-tests
✅ 🛠 watch
✅ 🛠 watch-sim

@jelee53
Copy link
Copy Markdown
Contributor

jelee53 commented Oct 24, 2025 •
edited
Loading

I wonder if it's helpful to implement add a cap / max_url_length for the input length as well. That way we can immediately reject longer URL's without having to perform these additional append operations and increasing stack usage to see if it will eventually fail.

I think I saw somewhere that the max URL length that Safari supports is ~80K characters.

EDIT: Chatted offline with Roberto and it looks like there is no maximum cap for URL's. Disregard this comment :)

jelee53
jelee53 reviewed Oct 24, 2025
View reviewed changes
LayoutTests/fast/url/url-pattern-very-large-string.html
debug("Attemping to set large string as input to URLPattern...");
let urlPattern = new URLPattern(largeString);
});
});
Copy link
Copy Markdown
Contributor

@jelee53 jelee53 Oct 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: A test description might be helpful.
test(function() { }, " .. test description here ..");

mdubet
mdubet approved these changes Oct 24, 2025
View reviewed changes
LayoutTests/fast/url/url-pattern-very-large-string.html
<html>
<head>
<meta charset="utf-8">
<script src="../../resources/js-test-pre.js"></script>
Copy link
Copy Markdown
Contributor

@mdubet mdubet Oct 24, 2025

Choose a reason for hiding this comment

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

I don't think you need the js-test-pre/post with testharness.js

@roberto-apple roberto-apple force-pushed the eng/Handle-very-large-url-pattern-input branch from 8b6fc1e to bd864bf Compare October 24, 2025 17:35
@webkit-early-warning-system
Copy link
Copy Markdown
Collaborator

webkit-early-warning-system commented Oct 24, 2025 •
edited
Loading

EWS run on previous version of this PR (hash bd864bf)

Details

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows Apple Internal
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win ✅ 🛠 ios-apple
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ❌ 🧪 win-tests ✅ 🛠 mac-apple
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 loading-orange 🧪 api-mac ✅ 🧪 api-wpe ✅ 🛠 vision-apple
✅ 🧪 ios-wk2-wpt loading-orange 🧪 mac-wk1 ✅ 🛠 wpe-cairo
  🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
  🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
  🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
⏳ 🧪 vision-wk2 loading-orange 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ❌ 🛠 mac-safer-cpp
loading-orange 🛠 tv-sim
loading-orange 🛠 watch
✅ 🛠 watch-sim

@roberto-apple roberto-apple force-pushed the eng/Handle-very-large-url-pattern-input branch from bd864bf to df19ca3 Compare October 25, 2025 02:21
@webkit-early-warning-system
Copy link
Copy Markdown
Collaborator

webkit-early-warning-system commented Oct 25, 2025 •
edited
Loading

EWS run on current version of this PR (hash df19ca3)

Details

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows Apple Internal
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win ❌ 🛠 ios-apple
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ❌ 🧪 win-tests ✅ 🛠 mac-apple
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe ❌ 🛠 vision-apple
  🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@roberto-apple roberto-apple added the merge-queue Applied to send a pull request to merge-queue label Oct 25, 2025
@roberto-apple
Handle very large url pattern input
f5e47e0 
https://bugs.webkit.org/show_bug.cgi?id=301302
rdar://163099884

Reviewed by Ryosuke Niwa and Matthieu Dubet.

When creating a URLPattern with a string as an input, it is possible to
create it with a string so large that the underlying token list (vector)
will intentionally crash when performing an append operation and the
capacity cannot be increased. Check the result of each token append
operation and throw an exception if an append cannot be performed.

Test: fast/dom/DOMURL/url-pattern-very-large-string.html

* LayoutTests/fast/dom/DOMURL/url-pattern-very-large-string-expected.txt: Added.
* LayoutTests/fast/dom/DOMURL/url-pattern-very-large-string.html: Added.
* Source/WebCore/Modules/url-pattern/URLPatternTokenizer.cpp:
(WebCore::URLPatternUtilities::Tokenizer::addToken):
(WebCore::URLPatternUtilities::Tokenizer::tokenize):
* Source/WebCore/Modules/url-pattern/URLPatternTokenizer.h:

Canonical link: https://commits.webkit.org/302131@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Handle-very-large-url-pattern-input branch from df19ca3 to f5e47e0 Compare October 25, 2025 07:47
@webkit-commit-queue
Copy link
Copy Markdown
Collaborator

webkit-commit-queue commented Oct 25, 2025

Committed 302131@main (f5e47e0): https://commits.webkit.org/302131@main

Reviewed commits have been landed. Closing PR #52836 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit f5e47e0 into WebKit:main Oct 25, 2025
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Oct 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jelee53 jelee53 jelee53 left review comments

@rniwa rniwa rniwa approved these changes

@mdubet mdubet mdubet approved these changes

@youennf youennf Awaiting requested review from youennf

Assignees

@roberto-apple roberto-apple

Labels

New Bugs Unclassified bugs are placed in this component until the correct component can be determined.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@roberto-apple @webkit-early-warning-system @mdubet @jelee53 @webkit-commit-queue @rniwa