Bug 27721 – [WML] Deck access control is completly broken

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

Deck access control (a very important security feature in the WML world) is broken. Affects manual-tests/wml/deck-access-control.wml.

[Nikolas Zimmermann]

Created attachment 33943 [details]
Initial patch

[Eric Seidel (no email)]

Comment on attachment 33943 [details]
Initial patch

Does WML execute script?  Is it possible to change these values?  If so, setting to an invalid value will not clear domain or path since you if (value.isEmpty()) return before setting.  Is that intentional behavior?

What does this do, and why?
    if (!initialized) {
 58         if (WMLPageState* wmlPageState = wmlPageStateForDocument(this)) {
 59             if (Page* page = wmlPageState->page())
 60                 page->goBack();
 61         }
 62     }
Why not use early return there?

typo:
 71     // Remember that we'e successfully entered the deck

Seems:
bool initialized
is the wrong name.  It's accessAllowed or something like that.

Change seems large.  Too large for my little brain at this moment.  maybe we could/should add the layout tests first...

r- hoping that we could split this to make it easier to review.

[Nikolas Zimmermann]

(In reply to comment #2)
> (From update of attachment 33943 [details])
> Does WML execute script?  Is it possible to change these values?  If so,
> setting to an invalid value will not clear domain or path since you if
> (value.isEmpty()) return before setting.  Is that intentional behavior?
No, WML doesn't have any JS support at all. WMLScript support is not desired to be implemented. So it's all static.
 
> What does this do, and why?
This is deck-access control. A deck is a document, a card is contained within a specific deck.
Suppose Card 1 of Deck A tries to enter Card 1 of Deck B. Deck B contains "<access domain='not-existant.com'...>", then Deck A is not allowed to enter Deck B. Deck B immediately goes back to the invoking card, and an error is displayed on the original page, that it failed to enter the target deck (task-execution-failure). This is specified WML behaviour.

>     if (!initialized) {
>  58         if (WMLPageState* wmlPageState = wmlPageStateForDocument(this)) {
>  59             if (Page* page = wmlPageState->page())
>  60                 page->goBack();
>  61         }
>  62     }
> Why not use early return there?
Fixed.

> 
> typo:
>  71     // Remember that we'e successfully entered the deck
> 
> Seems:
> bool initialized
> is the wrong name.  It's accessAllowed or something like that.
Fixed.

Uploading a slighly revised version soon, which also fixes a bug regarding intrinsic event execution upon task-execution-failure.

[Nikolas Zimmermann]

Created attachment 34433 [details]
Updated patch

[Nikolas Zimmermann]

Created attachment 34434 [details]
Updated patch - incorporating Georges comments

[Nikolas Zimmermann]

Sending        LayoutTests/ChangeLog
Adding         LayoutTests/http/tests/wml/access-target-domain-deny-expected.txt
Adding         LayoutTests/http/tests/wml/access-target-domain-deny.html
Adding         LayoutTests/http/tests/wml/access-target-expected.txt
Adding         LayoutTests/http/tests/wml/access-target-path-deny-expected.txt
Adding         LayoutTests/http/tests/wml/access-target-path-deny.html
Adding         LayoutTests/http/tests/wml/access-target.html
Adding         LayoutTests/http/tests/wml/resources/access-target-domain-deny.js
Adding         LayoutTests/http/tests/wml/resources/access-target-domain-deny.wml
Adding         LayoutTests/http/tests/wml/resources/access-target-path-deny.js
Adding         LayoutTests/http/tests/wml/resources/access-target-path-deny.wml
Adding         LayoutTests/http/tests/wml/resources/access-target.js
Adding         LayoutTests/http/tests/wml/resources/access-target.wml
Adding         LayoutTests/http/tests/wml/resources/locked-deck.wml
Adding         LayoutTests/http/tests/wml/resources/unreachable-domain.wml
Adding         LayoutTests/http/tests/wml/resources/unreachable-path.wml
Adding         LayoutTests/wml/access-target-deny-expected.txt
Adding         LayoutTests/wml/access-target-deny.html
Adding         LayoutTests/wml/access-target-expected.txt
Adding         LayoutTests/wml/access-target.html
Adding         LayoutTests/wml/resources/access-target-deny.js
Adding         LayoutTests/wml/resources/access-target-deny.wml
Adding         LayoutTests/wml/resources/access-target.js
Adding         LayoutTests/wml/resources/access-target.wml
Adding         LayoutTests/wml/resources/locked-deck.wml
Sending        LayoutTests/wml/variable-reference-invalid-character-expected.txt
Sending        WebCore/ChangeLog
Sending        WebCore/inspector/ConsoleMessage.h
Sending        WebCore/inspector/InspectorController.h
Sending        WebCore/loader/FrameLoader.cpp
Sending        WebCore/loader/FrameLoaderTypes.h
Sending        WebCore/page/Console.cpp
Sending        WebCore/page/Console.h
Sending        WebCore/page/Console.idl
Sending        WebCore/wml/WMLAccessElement.cpp
Sending        WebCore/wml/WMLAccessElement.h
Sending        WebCore/wml/WMLCardElement.cpp
Sending        WebCore/wml/WMLDocument.cpp
Sending        WebCore/wml/WMLErrorHandling.cpp
Sending        WebCore/wml/WMLPageState.cpp
Sending        WebCore/wml/WMLPageState.h
Sending        WebKit/mac/ChangeLog
Sending        WebKit/mac/WebView/WebFramePrivate.h
Sending        WebKit/win/ChangeLog
Sending        WebKit/win/Interfaces/IWebFramePrivate.idl
Transmitting file data .............................................
Committed revision 46966.
http://trac.webkit.org/changeset/46966

[Pavel Feldman]

Guys, is this console.lastWMLErrorMessage web-facing? Or is it only used in the layout tests? If latter, let us pass it via the layoutTestController instead. The reason I ask is that I saw some nasty ifdef(INSPECTOR) checks within WebCore/inspector/ConsoleMessage.cpp, while my assumption was that ConsoleMessage.cpp was inspector's property (and hence resides in the inspector folder).

I'd also like to remove InspectorController::consoleMessages() as a whole so that other clients would not use the interface. Your code seems to be the only client.