Bug 22431 - Implement <access> support in WML
Summary: Implement <access> support in WML
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks: 20393
  Show dependency treegraph
 
Reported: 2008-11-22 16:47 PST by Nikolas Zimmermann
Modified: 2008-11-24 16:06 PST (History)
0 users

See Also:


Attachments
Initial patch (8.98 KB, patch)
2008-11-22 16:48 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch (10.54 KB, patch)
2008-11-22 16:51 PST, Nikolas Zimmermann
eric: review-
Details | Formatted Diff | Diff
Updated patch v2 (6.45 KB, patch)
2008-11-24 13:41 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch v3 (6.51 KB, patch)
2008-11-24 13:46 PST, Nikolas Zimmermann
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2008-11-22 16:47:20 PST
Support the <access> element. It just contains two attribute values that need to be propagated to WMLPageState.
Comment 1 Nikolas Zimmermann 2008-11-22 16:48:56 PST
Created attachment 25383 [details]
Initial patch
Comment 2 Nikolas Zimmermann 2008-11-22 16:51:17 PST
Created attachment 25384 [details]
Updated patch

Missed one file, updating patch.
Comment 3 Eric Seidel (no email) 2008-11-24 12:46:34 PST
So WML has it's own security model?  I'm unclear on what "calls" this would allow, once you give a certain other URL "access" to this deck.
Comment 4 Nikolas Zimmermann 2008-11-24 12:52:09 PST
Exactly, some quotes from the WML spec to clarify:

<quote>
The access element specifies access control information for the entire deck.  It is an error for a 
deck to contain more than one access element.  If a deck does not include an access element, 
access control is disabled. When access control is disabled, cards in any deck can access this deck.

A deck's domain and path attributes specify which other decks may access it.  As the 
user agent navigates from one deck to another, it performs access control checks to 
determine whether the destination deck allows access from the current deck. 
</quote>

Does this clarify the situation?
Comment 5 Eric Seidel (no email) 2008-11-24 12:53:54 PST
(In reply to comment #4)
So you can control from what referrers you're allowed to access a deck, client side!?  That seems like a lame-duck security feature that no one would ever use.  I would expect servers to serve different pages based on referrers.
Comment 6 Nikolas Zimmermann 2008-11-24 13:09:35 PST
Honestly, I can't name an example what needs this. But I haven't written the actual implementation. I bet Yichao/Charles Wei may know some use cases. We only implemented things that are needed. There's even a test in manual-tests/wml/ that checks this behaviour.

I'd prefer to just get it in. It doesn't affect WebKit/HTML in any way.
Comment 7 Eric Seidel (no email) 2008-11-24 13:14:45 PST
Comment on attachment 25384 [details]
Updated patch


> +        if (Page* page = document()->page())
> +            page->wmlPageState()->setDeckAccessDomain(attr->value());

> +        if (Page* page = document()->page()) 
> +            page->wmlPageState()->setDeckAccessPath(attr->value());

I think these calls would be better named "restrictAccessToDeckToDomain" and "restrictAccessToDeckToPath"

identifying that adding a new restriction clears all others, and that this is not what domains or paths the current deck can acess, but rather, what domains and paths can acesss the deck.  The names are still not perfect, but I think they're better than what were there before.

That should probably be "containsWMLVariableReference()"
 
> +bool valueContainsVariableReference(const String& text)
> +{
> +    int start = text.find('$');
> +    if (start == -1)
> +        return false;
> +
> +    int end = start + 1;
> +    while (text[end] == '$')
> +        ++end;
> +
> +    return ((end-start) % 2) != 0;
> +}

Also, that function will do the following:
"$" true
"$$" false
"$$$" true
"$$ foobar $bar" false

I think it needs more testing...

>      enum WMLVariableEscapingMode {
>          WMLVariableEscaping_None = 0,
> -        WMLVariableEscaping_Escape = 1,
> -        WMLVariableEscaping_Unescape = 2
> +        WMLVariableEscaping_Escape,
> +        WMLVariableEscaping_Unescape
>      };

WebKit style would say those should be WMLVariableEscapingNone, no _ I think.

r- for the lack of variable testing (and thus incorrectness of said function).  I"m not sure it's really a big deal that it's wrong.  Otherwise the patch looked fine.  Please rename the functions noted to something better. :)
Comment 8 Nikolas Zimmermann 2008-11-24 13:41:03 PST
Created attachment 25447 [details]
Updated patch v2
Comment 9 Nikolas Zimmermann 2008-11-24 13:46:23 PST
Created attachment 25448 [details]
Updated patch v3

Oops, uploaded wrong patch.
Comment 10 Nikolas Zimmermann 2008-11-24 15:44:26 PST
In 'Updated patch v3' it says ' for (unsigned i = startPos; i < length; ++i) {' - it should read 'startPos + 1' - not uploading a new patch though.
Comment 11 Nikolas Zimmermann 2008-11-24 16:06:24 PST
Landed in r38733.