Bug 74757 – XSLT-created HTML documents do not inherit first party for cookies from originally loaded XML.

• Component: XML
• Status: RESOLVED FIXED

[Thomas Sepez]

This is causing problems downstream along the lines of: http://code.google.com/p/chromium/issues/detail?id=104715

I think the correct thing to do is to copy the original document's first party for cookies in WebCore/xml/XSLTProcessor.cpp's 
PassRefPtr<Document> XSLTProcessor::createDocumentFromSource():

       if (Document* oldDocument = frame->document()) {
            result->setTransformSourceDocument(oldDocument);
            result->setSecurityOrigin(oldDocument->securityOrigin());
            result->setCookieURL(oldDocument->cookieURL());
+           result->setFirstPartyForCookies(oldDocument->firstPartyForCookies());
        }

[Adam Barth]

Yep

[Adam Barth]

We should move cookieURL and firstPartyForCookies into SecurityContext and copy over the whole SecurityContext.

[Thomas Sepez]

I'll take a shot at this if its not too disruptive.  I would feel a lot better if there weren't all these dangling parts outside the context which could fall off at any point in time ...

[Thomas Sepez]

Created attachment 120080 [details]
Patch + tests

Note: confirmed third-party-cookie-blocking-xslt.xml flunks prior to this patch.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

Source/WebCore/dom/SecurityContext.h:75:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/dom/SecurityContext.h:84:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
LayoutTests/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 7 in 12 files


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

[Thomas Sepez]

Created attachment 120085 [details]
Patch, test, whitespace cleanup.

[Adam Barth]

Comment on attachment 120085 [details]
Patch, test, whitespace cleanup.

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

> Source/WebCore/dom/Document.cpp:4577
> -    SecurityContext::setSecurityOrigin(origin);
> +    *static_cast<SecurityContext*>(this) = that;

I wonder if this is exactly right.  For example, this will end up having the two documents pointing to the same ContentSecurityPolicy object.  Is that the right thing?

> Source/WebCore/dom/Document.h:997
> +    void updateSecurityContextFromDocument(const Document&);

Should we keep the warning about how this function is dangerous?

> Source/WebCore/dom/SecurityContext.h:64
> +    // Explicitly override the security origin for this security context.
> +    // Note: It is dangerous to change the security origin of a context that already contains content.
> +    void setSecurityOrigin(PassRefPtr<SecurityOrigin>);

Can we make this private?

> Source/WebCore/dom/SecurityContext.h:67
> +    void setContentSecurityPolicy(PassRefPtr<ContentSecurityPolicy>);

Can we make this private?

> Source/WebCore/dom/SecurityContext.h:80
> +    void setCookieURL(const KURL& url) { m_cookieURL = url; }

Can we make this protected?

> Source/WebCore/dom/SecurityContext.h:93
> +    void setFirstPartyForCookies(const KURL& url) { m_firstPartyForCookies = url; }

Can we make this private?

> Source/WebCore/xml/XSLTProcessor.cpp:91
> +            result->updateSecurityContextFromDocument(*oldDocument);

Maybe this function should take a Document* ?  Generally we pass around document as pointers.

[Thomas Sepez]

Comment on attachment 120085 [details]
Patch, test, whitespace cleanup.

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

>> Source/WebCore/dom/Document.cpp:4577
>> +    *static_cast<SecurityContext*>(this) = that;
> 
> I wonder if this is exactly right.  For example, this will end up having the two documents pointing to the same ContentSecurityPolicy object.  Is that the right thing?

I think this is ok.  The shared objects are refptr'd, and I suspect that the original document is going away shortly. It strkikes me that there is probably a bug here that the CSP headers from the original XML document response don't apply to the transformed document because we lost the original  CSP object.  I'll need to add a test for that.

If you'd feel better about copying the CSP ans securityorigin objects, then I think this method belongs in securitycontext, not document, so that it's "close by" when addl fields get added to securitycontext.

>> Source/WebCore/dom/Document.h:997
>> +    void updateSecurityContextFromDocument(const Document&);
> 
> Should we keep the warning about how this function is dangerous?

Sure.

>> Source/WebCore/dom/SecurityContext.h:64
>> +    void setSecurityOrigin(PassRefPtr<SecurityOrigin>);
> 
> Can we make this private?

ok.

>> Source/WebCore/dom/SecurityContext.h:67
>> +    void setContentSecurityPolicy(PassRefPtr<ContentSecurityPolicy>);
> 
> Can we make this private?

ok.

>> Source/WebCore/dom/SecurityContext.h:80
>> +    void setCookieURL(const KURL& url) { m_cookieURL = url; }
> 
> Can we make this protected?

ok

>> Source/WebCore/dom/SecurityContext.h:93
>> +    void setFirstPartyForCookies(const KURL& url) { m_firstPartyForCookies = url; }
> 
> Can we make this private?

ok.

>> Source/WebCore/xml/XSLTProcessor.cpp:91
>> +            result->updateSecurityContextFromDocument(*oldDocument);
> 
> Maybe this function should take a Document* ?  Generally we pass around document as pointers.

ok.

[Adam Barth]

testing++

[Alexey Proskuryakov]

Comment on attachment 120085 [details]
Patch, test, whitespace cleanup.

One important thing about "firstPartyForCookies" related names in Document is that they are a result of misunderstanding. They are used for more than cookies. An example that I recently discovered is that the same data is used when setting quarantine metadata on downloaded files.

The original name was "mainDocumentURL", which matches the behavior much better. Conceptually, it's what the user thinks the URL they are viewing is (and users cannot see subframe URLs and such). In fact, that was the original name in WebCore.

The refactoring here seems harmless. However, I would suggest to fix the names and comments before doing any refactoring - that would make the intentions much clearer.

I'm marking this r- for procedural reasons, although the patch itself appears quite good. There has been much confusion over this before, so ideally, there should be three patches, one to fix the bug, one to rename firstPartyForCookies, and one to do the refactoring that Adam suggested.

[Adam Barth]

> One important thing about "firstPartyForCookies" related names in Document is that they are a result of misunderstanding. They are used for more than cookies. An example that I recently discovered is that the same data is used when setting quarantine metadata on downloaded files.

firstPartyForCookies should not be used for anything besides cookies.  It's a quirky value that's specific to the needs of cookies.

> The original name was "mainDocumentURL", which matches the behavior much better.

I'm not sure that'a accurate.  When I studied this code previously, state was called policyURL and policyBaseURL (which were the same thing).  mainDocumentURL is a name from NSURLRequest, which is a CFNetwork specific concept (and a layering violation).

> The refactoring here seems harmless. However, I would suggest to fix the names and comments before doing any refactoring - that would make the intentions much clearer.

Let's consider changing the names in a separate patch.  I don't think we should change the names, but I don't want that discussion to block this patch.

[Thomas Sepez]

Comment on attachment 120085 [details]
Patch, test, whitespace cleanup.

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

>>> Source/WebCore/dom/SecurityContext.h:64
>>> +    void setSecurityOrigin(PassRefPtr<SecurityOrigin>);
>> 
>> Can we make this private?
> 
> ok.

Nope.  We can't even make it protected, since it's called by DocumentWriter.  This was public in when it used to live in document, now that this goes directly to the superclass its needs to be public, too.  I'd hate to add a public method to document just to call the protected method on SecurityContext.

>>> Source/WebCore/dom/SecurityContext.h:67
>>> +    void setContentSecurityPolicy(PassRefPtr<ContentSecurityPolicy>);
>> 
>> Can we make this private?
> 
> ok.

Nope.  We can make it protected, since its called by Document.cpp in a few places, though.

>>> Source/WebCore/dom/SecurityContext.h:80
>>> +    void setCookieURL(const KURL& url) { m_cookieURL = url; }
>> 
>> Can we make this protected?
> 
> ok

Nope.  There was a Document::setCookieURL that was public; now that this goes directly to the superclass its needs to be public, too.  I'd hate to add a public method to document just to call the protected method on SecurityContext.

[Adam Barth]

Should the callers of these functions actually be calling a different function (such as one that deals with all the information on SecurityContext) ?

[Alexey Proskuryakov]

Let's fix the bug as Thomas originally suggested, but I want refactoring to be blocked until we are on the same page WRT firstPartyForCookies. It makes no sense to refactor something that we don't agree what it means.

[Alexey Proskuryakov]

> When I studied this code previously, state was called policyURL and policyBaseURL (which were the same thing).

Yes, I could be wrong about the original name. As you agree, it was not about cookies though.

[Adam Barth]

I agree that we should fix this bug, but I'm not sure i subscribe to your point of view about what other changes should be blocked on what.  In any case, lets make progress here and we can discuss what comes next in the future.

[Adam Barth]

(In reply to comment #15)
> > When I studied this code previously, state was called policyURL and policyBaseURL (which were the same thing).
> 
> Yes, I could be wrong about the original name. As you agree, it was not about cookies though.

This sounds like a topic for webkit-dev rather than a bug thread.  I'm pretty against changing the name and using the state for things other than cookies, but I'm happy to discuss the issue with you on webkit-dev.

[Alexey Proskuryakov]

Sounds great to me, thanks.

[Thomas Sepez]

Comment on attachment 120085 [details]
Patch, test, whitespace cleanup.

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

>>> Source/WebCore/dom/SecurityContext.h:93
>>> +    void setFirstPartyForCookies(const KURL& url) { m_firstPartyForCookies = url; }
>> 
>> Can we make this private?
> 
> ok.

Nope.  Needs to be called from FrameLoader.cpp

[Thomas Sepez]

(In reply to comment #13)
> Should the callers of these functions actually be calling a different function (such as one that deals with all the information on SecurityContext) ?

I think this is the heart of the matter which is still unaddressed irrespective of what we do with naming down the road.  When you've got an object like SecurityContext with a no-arg constructor and a bunch of individual setters/getters, it's a often red flag that the invariants about what makes a validd object aren't well understood.  The resulting usage where groups of these calls are used together to accomplish some larger operation also implies that the larger transformations on the object aren't understood either.  Studying the implementation may well reveal the what the design should have been.

But ... I didn't address these to get the refactoring in place without breaking stuff.   I think this ought to be folded in with the renaming in another CL.

My intent in that the next patch will keep the same overall structure as the current one, but include a test for XSLT and CSP which I expect to be broken because of the lack of a setCSP call in XSLTProcessor which would be fixed by copying over the whole context.

[Thomas Sepez]

Comment on attachment 120085 [details]
Patch, test, whitespace cleanup.

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

>>> Source/WebCore/dom/Document.cpp:4577
>>> +    *static_cast<SecurityContext*>(this) = that;
>> 
>> I wonder if this is exactly right.  For example, this will end up having the two documents pointing to the same ContentSecurityPolicy object.  Is that the right thing?
> 
> I think this is ok.  The shared objects are refptr'd, and I suspect that the original document is going away shortly. It strkikes me that there is probably a bug here that the CSP headers from the original XML document response don't apply to the transformed document because we lost the original  CSP object.  I'll need to add a test for that.
> 
> If you'd feel better about copying the CSP ans securityorigin objects, then I think this method belongs in securitycontext, not document, so that it's "close by" when addl fields get added to securitycontext.

Nope.  This is wrong.  The CSP stashes a ponter back to its script execution context, so using the same CSP is only ok so long as the parent doc outlives a subordinate doc, which is not the case here.  So CSP needs a deep-copy  function.  On the bright side, there's a FIXME in WorkerContext.cpp that has also wanting this functionality to allow a fix ...

[Adam Barth]

We should probably keep around the original text of the CSP policy, so the deep copy can just be to take the string representation of the policy and re-parse it for the new object.

[Thomas Sepez]

(In reply to comment #22)
> We should probably keep around the original text of the CSP policy, so the deep copy can just be to take the string representation of the policy and re-parse it for the new object.

Perhaps.  Seems clumsy when you need to account for multiple headers of different types.

[Alexey Proskuryakov]

Thomas, is your plan at this point to land the fix suggested in bug description, and do the rest separately? I think that this is by far the preferable course of action. Am I missing something?

[Thomas Sepez]

Ok.  I'll minimize this, leaving the refactoring out ... and file another bug for the lack of CSP inheritence ... and leave the naming/refactoring to somebody else.

[Thomas Sepez]

Created attachment 120220 [details]
Patch, test, no refactoring

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Inform the scrolling coordinator when scrollbar layers come and go
Using index info to reconstruct a base tree...
<stdin>:474806: trailing whitespace.
        [Chromium] DatabaseTrackerChromium: iterating DatabaseSet races with Database disposal on worker thread 
<stdin>:474827: trailing whitespace.
        Nothing to test, just removing redundant code. Correct behavior tested by 
<stdin>:475346: trailing whitespace.
    
warning: 3 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 167249 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/chromium/test_expectations.txt
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebKit2/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Inform the scrolling coordinator when scrollbar layers come and go

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 158.


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

[Thomas Sepez]

Created attachment 120222 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 120222 [details]
Patch

Looks good to me. Giving Adam a chance to comment.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
From git://git.webkit.org/WebKit
   eac27da..eda9e81  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 103461 = eac27daf25de1abb134b31a11fba2a7be9e63e0c
r103462 = eda9e816dab424eb6ebcc44ebe350e35b490e2eb
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Applying: Inform the scrolling coordinator when scrollbar layers come and go
Using index info to reconstruct a base tree...
<stdin>:474806: trailing whitespace.
        [Chromium] DatabaseTrackerChromium: iterating DatabaseSet races with Database disposal on worker thread 
<stdin>:474827: trailing whitespace.
        Nothing to test, just removing redundant code. Correct behavior tested by 
<stdin>:475346: trailing whitespace.
    
warning: 3 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 167249 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/chromium/test_expectations.txt
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebCore/page/ScrollingCoordinator.h
CONFLICT (content): Merge conflict in Source/WebCore/page/ScrollingCoordinator.h
Auto-merging Source/WebCore/page/mac/ScrollingCoordinatorMac.mm
CONFLICT (content): Merge conflict in Source/WebCore/page/mac/ScrollingCoordinatorMac.mm
Auto-merging Source/WebCore/platform/ScrollView.cpp
Auto-merging Source/WebCore/rendering/RenderLayerCompositor.cpp
CONFLICT (content): Merge conflict in Source/WebCore/rendering/RenderLayerCompositor.cpp
Auto-merging Source/WebKit2/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Auto-merging Tools/Scripts/build-webkit
Auto-merging Tools/Scripts/webkitdirs.pm
CONFLICT (content): Merge conflict in Tools/Scripts/webkitdirs.pm
Failed to merge in the changes.
Patch failed at 0001 Inform the scrolling coordinator when scrollbar layers come and go

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 158.


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

[WebKit Review Bot]

Comment on attachment 120222 [details]
Patch

Clearing flags on attachment: 120222

Committed r103511: <http://trac.webkit.org/changeset/103511>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.