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.

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

Add iframe depth limit #7219

Merged

Conversation

nt1m
Copy link
Member

@nt1m nt1m commented Dec 6, 2022

65071a6

Add iframe depth limit
https://bugs.webkit.org/show_bug.cgi?id=67940
rdar://101560112

Reviewed by Darin Adler and Alex Christensen.

* LayoutTests/fast/frames/frame-depth-limit-expected.txt: Added.
* LayoutTests/fast/frames/frame-depth-limit.html: Added.
* LayoutTests/fast/frames/resources/self-referential-iframe.html: Added.
* Source/WebCore/loader/SubframeLoader.cpp:
(WebCore::FrameLoader::SubframeLoader::loadSubframe):
* Source/WebCore/page/FrameTree.cpp:
(WebCore::FrameTree::depth const):
* Source/WebCore/page/FrameTree.h:
* Source/WebCore/page/Page.h:

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

bd2d9d3

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  πŸ§ͺ win
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ›  gtk βœ… πŸ›  wincairo
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ§ͺ gtk-wk2
❌ πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk1   πŸ§ͺ api-gtk
βœ… πŸ›  tv βœ… πŸ§ͺ mac-wk2
βœ… πŸ›  tv-sim   πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch βœ… πŸ§ͺ mac-wk2-stress
βœ… πŸ›  watch-sim

@nt1m nt1m requested a review from cdumez as a code owner December 6, 2022 20:47
@nt1m nt1m self-assigned this Dec 6, 2022
@nt1m nt1m changed the title ??? Limit iframe depth Dec 6, 2022
@nt1m nt1m added the DOM For bugs specific to XML/HTML DOM elements (including parsing). label Dec 6, 2022
Source/WebCore/page/Page.h Outdated Show resolved Hide resolved
@geoffreygaren
Copy link
Contributor

Can you add a regression test?

@nt1m nt1m marked this pull request as draft December 6, 2022 22:52
@nt1m nt1m changed the title Limit iframe depth Add an iframe depth limit Dec 7, 2022
@nt1m nt1m added Frames For bugs specific to the use of HTML frame/iframe, as well as timing and frame targeting issues. and removed DOM For bugs specific to XML/HTML DOM elements (including parsing). labels Dec 7, 2022
@nt1m nt1m marked this pull request as ready for review December 7, 2022 18:07
Source/WebCore/page/FrameTree.h Outdated Show resolved Hide resolved
LayoutTests/fast/frames/frame-depth-limit.html Outdated Show resolved Hide resolved
LayoutTests/fast/frames/frame-depth-limit.html Outdated Show resolved Hide resolved
LayoutTests/fast/frames/frame-depth-limit.html Outdated Show resolved Hide resolved
Source/WebCore/page/FrameTree.h Outdated Show resolved Hide resolved
Source/WebCore/page/FrameTree.cpp Show resolved Hide resolved
@nt1m nt1m changed the title Add an iframe depth limit Add iframe depth limit Dec 7, 2022
@nt1m nt1m force-pushed the eng/Limit-iframe-depth branch 2 times, most recently from 0f0f187 to bd2d9d3 Compare December 8, 2022 00:51
@nt1m nt1m added request-merge-queue Request a pull request to be added to merge-queue once ready merge-queue Applied to send a pull request to merge-queue and removed request-merge-queue Request a pull request to be added to merge-queue once ready labels Dec 8, 2022
https://bugs.webkit.org/show_bug.cgi?id=67940
rdar://101560112

Reviewed by Darin Adler and Alex Christensen.

* LayoutTests/fast/frames/frame-depth-limit-expected.txt: Added.
* LayoutTests/fast/frames/frame-depth-limit.html: Added.
* LayoutTests/fast/frames/resources/self-referential-iframe.html: Added.
* Source/WebCore/loader/SubframeLoader.cpp:
(WebCore::FrameLoader::SubframeLoader::loadSubframe):
* Source/WebCore/page/FrameTree.cpp:
(WebCore::FrameTree::depth const):
* Source/WebCore/page/FrameTree.h:
* Source/WebCore/page/Page.h:

Canonical link: https://commits.webkit.org/257550@main
@webkit-commit-queue
Copy link
Collaborator

Committed 257550@main (65071a6): https://commits.webkit.org/257550@main

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

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Dec 8, 2022
@csaavedra
Copy link
Member

Why is the test not hitting the self-reference limit of 2 imposed by HTMLFrameOwnerElement::isProhibitedSelfReference(const URL& completeURL) ?

@nt1m nt1m deleted the eng/Limit-iframe-depth branch December 8, 2022 16:08
@nt1m
Copy link
Member Author

nt1m commented Dec 8, 2022

Why is the test not hitting the self-reference limit of 2 imposed by HTMLFrameOwnerElement::isProhibitedSelfReference(const URL& completeURL) ?

That function seems to check for exact URL equality including query strings. The test changes the query string each time.

Either way, that protection is pretty weak, nothing prevents a website from generating identical self-referencing iframes server-side while changing the URL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frames For bugs specific to the use of HTML frame/iframe, as well as timing and frame targeting issues.
Projects
None yet
8 participants