Bug 173162 – Align Document::canNavigate on the HTM5 specification

• Status: RESOLVED
• Resolution: FIXED
• Component: Frames

[Frédéric Wang (:fredw)]

See the following test cases:
- web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-3.html
- https://codepen.io/tibit/pen/remQwm/

[Frédéric Wang (:fredw)]

This is the specification:
https://html.spec.whatwg.org/multipage/browsers.html#allowed-to-navigate

This is Chromium's current implementation:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/LocalFrame.cpp?type=cs&q=LocalFrame::CanNavigateWithoutFramebusting

I checked Chromium's git log and these are relevant bugs/CLs/testcases where changes were made:

https://bugs.chromium.org/p/chromium/issues/detail?id=577330
https://codereview.chromium.org/1583953005
http://output.jsbin.com/zenape

https://bugs.chromium.org/p/chromium/issues/detail?id=597641
https://codereview.chromium.org/2399713002
https://codepen.io/tibit/pen/remQwm/

https://bugs.chromium.org/p/chromium/issues/detail?id=716565
https://codereview.chromium.org/2877893002

[Frédéric Wang (:fredw)]

Created attachment 312440 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 312459 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 312459 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312459&action=review

> Source/WebCore/dom/Document.cpp:3044
> +        else if (targetFrame != &m_frame->tree().top() && targetFrame != &m_frame->tree().top() && isSandboxed(SandboxPropagatesToAuxiliaryBrowsingContexts) && (isSandboxed(SandboxPopups) || targetFrame->loader().opener() != m_frame))

Sorry, I made a mistake here when trying to split the patch from bug 158875. It should be !targetFrame->tree().parent() targetFrame != &m_frame->tree().top() && ...

[Frédéric Wang (:fredw)]

(In reply to Frédéric Wang (:fredw) from comment #23 bug 158875)
> >> Source/WebCore/dom/Document.cpp:2982
> >> +        else if (!targetFrame->tree().parent() && targetFrame != &m_frame->tree().top() && isSandboxed(SandboxPropagatesToAuxiliaryBrowsingContexts) && (isSandboxed(SandboxPopups) || targetFrame->loader().opener() != m_frame))
> > 
> > Again, I do not understand, how can a frame NOT have a parent and NOT be the frame tree top?
> > 
> > The implementation of FrameTree::top() looks like so:
> > Frame& FrameTree::top() const
> > {
> >     Frame* frame = &m_thisFrame;
> >     for (Frame* parent = &m_thisFrame; parent; parent = parent->tree().parent())
> >         frame = parent;
> >     return *frame;
> > }
> > 
> > If frame does not have a parent, top() will return the frame itself...
> 
> I believe you are right. I copied that part from Chromium, but they their
> "top" implementation seems a bit more complicated.

OK, I stand corrected. The initial condition was right: We say that the targetFrame is a top frame but not the one of the current frame. This happens for example when you open a new popup and tries to change its URL.

I checked the current tests (W3C and chromium) a bit more. iframe_sandbox_popups_escaping-3.html and iframe_sandbox_popups_nonescaping-3.html will open

http://w3c-test.org/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_helper-3.html

which will attempt to navigate a popup from its opener (!targetFrame->tree().parent() && targetFrame != &m_frame->tree().top()). In the escaping case, isSandboxed(SandboxPropagatesToAuxiliaryBrowsingContexts) is false so the error does not happen. In the non-escaping case, isSandboxed(SandboxPopups) is false but targetFrame->loader().opener() == m_frame so again the error does not happen.

The tests from 
https://bugs.chromium.org/p/chromium/issues/detail?id=577330 and https://bugs.chromium.org/p/chromium/issues/detail?id=597641 verify navigation in the same situation (allow-popups + allow-popups-to-escape-sandbox or allow-popups alone). The latter also verifies "closing the popup", which exercises the same canNavigate code path. So I don't think chromium's tests bring more than the W3C one here.

[Frédéric Wang (:fredw)]

Created attachment 312646 [details]
Patch

In the past, when targetFrame->tree().isDescendantOf(m_frame) was true, canNavigate returned true. Now, if targetFrame == m_frame it follows the early return. Otherwise targetFrame is not a top frame, so none of the other failure conditions happen. Then it follow the normal case, which will again return true ("A document can navigate its decendant frames").

In the past, when targetFrame->tree().isDescendantOf(m_frame) was false, the failure was "'allow-top-navigation' flag is not set" when navigating the top frame of m_frame or "disallowed from navigating its ancestors" otherwise. Now, if targetFrame is not a top frame, the first condition is executed with failure "disallowed from navigating its ancestors". Otherwise, if targetFrame is the top frame of m_frame, then we follow the second condition, with failure "'allow-top-navigation' flag is not set". Otherwise, we *may* follow the third condition and raise a failure about "navigate a popup".

Hence this patch releases the condition as follows: We allow a frame to navigate a popup, when allow-popups-to-escape-sandbox is not set or (allow-popup is set and the frame is the opener of the popup) .

[Frédéric Wang (:fredw)]

Created attachment 312772 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 313525 [details]
Patch

[Chris Dumez]

Comment on attachment 313525 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313525&action=review

> Source/WebCore/dom/Document.cpp:-3107
> -        if (targetFrame->tree().isDescendantOf(m_frame))

Can we try to match the spec [1] as closely as possible? I think this would be close:

// From https://html.spec.whatwg.org/multipage/browsers.html#allowed-to-navigate.
// 1. If A is not the same browsing context as B, and A is not one of the ancestor browsing contexts of B, and B is not a top-level browsing context, and A's active document's active sandboxing
// flag set has its sandboxed navigation browsing context flag set, then abort these steps negatively.
if (m_frame != targetFrame && isSandboxed(SandboxNavigation) &&  targetFrame->tree().parent() && !targetFrame->tree().isDescendantOf(m_frame)) {
    printNavigationErrorMessage(targetFrame, url(), ASCIILiteral("The frame attempting navigation is sandboxed, and is therefore disallowed from navigating its ancestors."));
    return false;
}

// 2. Otherwise, if B is a top-level browsing context, and is one of the ancestor browsing contexts of A, and A's active document's active sandboxing flag set has its sandboxed
// top-level navigation browsing context flag set, then abort these steps negatively.
if (targetFrame == &m_frame->tree().top() && isSandboxed(SandboxTopNavigation)) {
    printNavigationErrorMessage(targetFrame, url(), ASCIILiteral("The frame attempting navigation is sandboxed, and is therefore disallowed from navigating its ancestors."));
    return false;
}

// 3. Otherwise, if B is a top-level browsing context, and is neither A nor one of the ancestor browsing contexts of A, and A's Document's active sandboxing flag set has its
// sandboxed navigation browsing context flag set, and A is not the one permitted sandboxed navigator of B, then abort these steps negatively.
if (!targetFrame->tree().parent() && m_frame != targetFrame && targetFrame != &m_frame->tree().top() && isSandboxed(SandboxNavigation) && targetFrame->loader().opener() != m_frame) {
    printNavigationErrorMessage(targetFrame, url(), ASCIILiteral("The frame attempting navigation is sandboxed, and is not allowed to navigate this popup"));
    return false;
}

// 4. Otherwise, terminate positively!


If for some reason, we *really* cannot match the spec, I would propose the following factoring which should be equivalent to what you proposed but a little bit more efficient (and I think a little clearer):
if (!targetFrame->tree().parent()) {
    if (targetFrame == &m_frame->tree().top())
        failureReason = "The frame attempting navigation of the top-level window is sandboxed, but the 'allow-top-navigation' flag is not set.";
    else if (isSandboxed(SandboxPropagatesToAuxiliaryBrowsingContexts) && (isSandboxed(SandboxPopups) || targetFrame->loader().opener() != m_frame))
        failureReason = "The frame attempting navigation is sandboxed and is trying to navigate a popup, but is not the popup's opener and is not set to propagate sandboxing to popups.";
} else if (!targetFrame->tree().isDescendantOf(m_frame))
        failureReason = "The frame attempting navigation is sandboxed, and is therefore disallowed from navigating its ancestors.";

But in your code, I don't fully understand the use of isSandboxed(SandboxPropagatesToAuxiliaryBrowsingContexts)  and isSandboxed(SandboxPopups), which do not seem to be part of the spec text for allowing navigating. They're about allowing popup *opening* and propagation of sandbox flags from opener to popups AFAICT.


[1] https://html.spec.whatwg.org/multipage/browsers.html#allowed-to-navigate

[Chris Dumez]

Sorry about the review delay, I had a crazy week.

[Frédéric Wang (:fredw)]

(In reply to Chris Dumez from comment #9)
> But in your code, I don't fully understand the use of
> isSandboxed(SandboxPropagatesToAuxiliaryBrowsingContexts)  and
> isSandboxed(SandboxPopups), which do not seem to be part of the spec text
> for allowing navigating. They're about allowing popup *opening* and
> propagation of sandbox flags from opener to popups AFAICT.
> 

OK, I'll try your suggestion. As I said, in the ChangeLog, I was not able to write tests (or to find some in the W3C or Chromium repos) about these cases as other security protections make these hard to do:

- a frame with allow-popups disabled but being a popup opener.
- or a frame trying to access the popup opened by another unrelated frame.

so I suspect we could indeed remove isSandboxed(SandboxPropagatesToAuxiliaryBrowsingContexts) and isSandboxed(SandboxPopups). At least the two W3C tests should still work, I expect.

https://bugs.chromium.org/p/chromium/issues/detail?id=597641 gives some spec references on the "one permitted sandboxed navigator" but I have the impression that Chromium is over-protecting navigation of popups, based on which situations such popups are permitted to be opened.

I have to check whether I can actually remove some conditions in canNavigate because when I initially tried (e.g. the one about "Frame-busting is generally allowed..."), this broke existing tests in WebKit.

[Frédéric Wang (:fredw)]

Created attachment 313822 [details]
Patch

[Chris Dumez]

Comment on attachment 313822 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313822&action=review

> Source/WebCore/dom/Document.cpp:3104
>      if (!isSandboxed(SandboxTopNavigation) && targetFrame == &m_frame->tree().top())

Why is this still here?

> Source/WebCore/dom/Document.cpp:3108
>      if (isSandboxed(SandboxNavigation)) {

if I remember correctly, this is only supposed to affect some of the if checks below (see the code I posted earlier).

> Source/WebCore/dom/Document.cpp:3118
> +        if (m_frame != targetFrame && targetFrame == &m_frame->tree().top()) {

This was supposed to have the isSandboxed(SandboxTopNavigation) check, no isSandboxed(SandboxNavigation) check.

[Frédéric Wang (:fredw)]

Comment on attachment 313822 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313822&action=review

>> Source/WebCore/dom/Document.cpp:3118
>> +        if (m_frame != targetFrame && targetFrame == &m_frame->tree().top()) {
> 
> This was supposed to have the isSandboxed(SandboxTopNavigation) check, no isSandboxed(SandboxNavigation) check.

Sorry, I missed that. Which explains the two points above... I'll upload a new patch. BTW, Note that I added m_frame != targetFrame here compared to your suggestion, because I understand that the spec says a frame is not "ancestor" of itself. Otherwise a SandboxTopNavigation top frame won't be able to navigate itself.

[Frédéric Wang (:fredw)]

Created attachment 313904 [details]
Patch

[Build Bot]

Comment on attachment 313904 [details]
Patch

Attachment 313904 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4005284

New failing tests:
http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change.html
http/tests/fileapi/create-blob-url-from-data-url.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation-1.html
fast/frames/sandboxed-iframe-close-top.html
http/tests/security/xss-DENIED-window-open-parent.html
fast/files/null-origin-string.html

[Build Bot]

Created attachment 313905 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 313904 [details]
Patch

Attachment 313904 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4005309

New failing tests:
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation-1.html
http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change.html
http/tests/fileapi/create-blob-url-from-data-url.html
fast/frames/sandboxed-iframe-close-top.html
http/tests/security/xss-DENIED-window-open-parent.html

[Build Bot]

Created attachment 313906 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 313904 [details]
Patch

Attachment 313904 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4005308

New failing tests:
http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change.html
http/tests/fileapi/create-blob-url-from-data-url.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation-1.html
fast/frames/sandboxed-iframe-close-top.html
http/tests/security/xss-DENIED-window-open-parent.html
fast/files/null-origin-string.html

[Build Bot]

Created attachment 313907 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 313904 [details]
Patch

Attachment 313904 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4005720

New failing tests:
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation-1.html
http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change.html
http/tests/fileapi/create-blob-url-from-data-url.html
fast/frames/sandboxed-iframe-close-top.html
http/tests/security/xss-DENIED-window-open-parent.html

[Build Bot]

Created attachment 313909 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Frédéric Wang (:fredw)]

(In reply to Build Bot from comment #20)
> New failing tests:
> http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change.html
> http/tests/fileapi/create-blob-url-from-data-url.html
> imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-
> element/iframe_sandbox_allow_top_navigation-1.html
> fast/frames/sandboxed-iframe-close-top.html

These tests seem to fail because of the removal of "frame-busting".

> http/tests/security/xss-DENIED-window-open-parent.html

This one too, although it's just the message error which is different from the reference.

> fast/files/null-origin-string.html

This one seems unrelated to this patch.

[Frédéric Wang (:fredw)]

Created attachment 314004 [details]
Patch

(In reply to Chris Dumez from comment #13)
> > Source/WebCore/dom/Document.cpp:3104
> >      if (!isSandboxed(SandboxTopNavigation) && targetFrame == &m_frame->tree().top())
> 
> Why is this still here?
(In reply to Frédéric Wang (:fredw) from comment #11)
> I have to check whether I can actually remove some conditions in canNavigate
> because when I initially tried (e.g. the one about "Frame-busting is
> generally allowed..."), this broke existing tests in WebKit.

So indeed, removing "frame-busting" breaks tests (comment 20) and it seems this is still used by many pages, so I suspect we won't be able to remove it so easily. For the record, Chromium people are trying to get rid of it but have failed so far:

https://bugs.chromium.org/p/chromium/issues/detail?id=640057
https://github.com/WICG/interventions/issues/16
https://bugs.chromium.org/p/chromium/issues/detail?id=624061
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/LocalFrame.cpp?type=cs&q=Frame-busting

[Build Bot]

Attachment 314004 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:23:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Frédéric Wang (:fredw)]

Created attachment 314005 [details]
Patch

[Build Bot]

Comment on attachment 314005 [details]
Patch

Attachment 314005 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4012659

New failing tests:
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-3.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-3.html

[Build Bot]

Created attachment 314006 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 314005 [details]
Patch

Attachment 314005 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4012667

New failing tests:
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-3.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-3.html

[Build Bot]

Created attachment 314007 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 314005 [details]
Patch

Attachment 314005 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4012669

New failing tests:
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-3.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-3.html

[Build Bot]

Created attachment 314008 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 314005 [details]
Patch

Attachment 314005 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4012691

New failing tests:
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-3.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-3.html

[Build Bot]

Created attachment 314009 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Frédéric Wang (:fredw)]

Created attachment 314011 [details]
Patch

[Chris Dumez]

Comment on attachment 314011 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314011&action=review

> Source/WebCore/dom/Document.cpp:3156
> +    if (!isSandboxed(SandboxTopNavigation) && targetFrame == &m_frame->tree().top())

So we allow a subframe to navigate its top frame if it has 'allow-top-navigation', even if it is cross-origin? I suppose you kept this to address layout test failures? Is this how other browsers behave?

[Frédéric Wang (:fredw)]

Comment on attachment 314011 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314011&action=review

>> Source/WebCore/dom/Document.cpp:3156
>> +    if (!isSandboxed(SandboxTopNavigation) && targetFrame == &m_frame->tree().top())
> 
> So we allow a subframe to navigate its top frame if it has 'allow-top-navigation', even if it is cross-origin? I suppose you kept this to address layout test failures? Is this how other browsers behave?

Yes, see comment 24 and 25.

Chromium first checks navigation without frame busting in https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/LocalFrame.cpp?l=913 similarly to what we do and then uses frame busting (with a warning message) with their current default setting:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/LocalFrame.cpp?l=844

[Chris Dumez]

Comment on attachment 314011 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314011&action=review

> Source/WebCore/dom/Document.cpp:-3103
> -    if (!isSandboxed(SandboxTopNavigation) && targetFrame == &m_frame->tree().top())

Since we need to keep this logic, let's keep it here instead of moving it below.

> Source/WebCore/dom/Document.cpp:-3107
> -        if (targetFrame->tree().isDescendantOf(m_frame))

Dropping this makes us stricter and I worry about making our behavior stricter at this point. Can we please keep this? The spec says to return true in step 4 but we currently do not (we have some extra origin checks). Therefore, keeping this check here keeps us closer to the spec as far as I can tell. So something like:
if (isSandboxed(SandboxNavigation) && targetFrame->tree().isDescendantOf(m_frame))
    return true;

> Source/WebCore/dom/Document.cpp:3126
>      // This is the normal case. A document can navigate its decendant frames,

Can you please fix the "decendant" typo while you're here.

>> Source/WebCore/dom/Document.cpp:3156
>> +    if (!isSandboxed(SandboxTopNavigation) && targetFrame == &m_frame->tree().top())
> 
> So we allow a subframe to navigate its top frame if it has 'allow-top-navigation', even if it is cross-origin? I suppose you kept this to address layout test failures? Is this how other browsers behave?

Looks like this is fine but I'd rather we keep it where it was, at the beginning of this method.

[Frédéric Wang (:fredw)]

Comment on attachment 314011 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314011&action=review

>> Source/WebCore/dom/Document.cpp:-3103
>> -    if (!isSandboxed(SandboxTopNavigation) && targetFrame == &m_frame->tree().top())
> 
> Since we need to keep this logic, let's keep it here instead of moving it below.

Done.

>> Source/WebCore/dom/Document.cpp:-3107
>> -        if (targetFrame->tree().isDescendantOf(m_frame))
> 
> Dropping this makes us stricter and I worry about making our behavior stricter at this point. Can we please keep this? The spec says to return true in step 4 but we currently do not (we have some extra origin checks). Therefore, keeping this check here keeps us closer to the spec as far as I can tell. So something like:
> if (isSandboxed(SandboxNavigation) && targetFrame->tree().isDescendantOf(m_frame))
>     return true;

OK I guess this is fine. Done too.

>> Source/WebCore/dom/Document.cpp:3126
>>      // This is the normal case. A document can navigate its decendant frames,
> 
> Can you please fix the "decendant" typo while you're here.

Done.

[Frédéric Wang (:fredw)]

Committed r218921: <http://trac.webkit.org/changeset/218921>