Bug 142422 – Content extensions need triggers dependent on 3rd party and content type

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Alex Christensen]

This is one of many ways to implement this.  Some of them are:

1) Prepending non-ascii characters to the url containing the flags.  This is 
2) Putting the flags in the compiled action description.  This would create many false positives that would be filtered out after interpreting.
3) Make many smaller DFAs for each combination of flags.
4) Put the flags in the byte code if needed.  This is similar to 2.

I implemented number 4.  Comments?

[Alex Christensen]

Created attachment 248123 [details]
Patch

[Benjamin Poulain]

Comment on attachment 248123 [details]
Patch

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

> Source/WebCore/contentextensions/ContentExtensionParser.cpp:80
> +    JSValue imagesOnlyObject = triggerObject.get(&exec, Identifier(&exec, "images-only"));
> +    if (imagesOnlyObject && !exec.hadException() && imagesOnlyObject.isBoolean())
> +        trigger.imageOnly = imagesOnlyObject.toBoolean(&exec);
> +    
> +    JSValue thirdPartyObject = triggerObject.get(&exec, Identifier(&exec, "third-party"));
> +    if (thirdPartyObject && !exec.hadException() && thirdPartyObject.isBoolean())
> +        trigger.thirdPartyOnly = thirdPartyObject.toBoolean(&exec);

I am unconvinced we should handle flags at the top level. Say you have "image-only:true" and "stylesheet-only:true", what should that match?

Maybe we should have:
    resourceType: ["image", "stylesheet", "script"]
    loadType: ["first-party", "third-party", ""]

> Source/WebCore/contentextensions/DFABytecode.h:49
> +    // The flags to check before appending (1 byte),

I think we'll need more than that :)

> Source/WebCore/loader/ResourceLoadInfo.cpp:33
> +ResourceType toResourceType(CachedResource::Type type)

Sam had a clear spec somewhere giving all the types we should expose.

> Source/WebCore/loader/ResourceLoadInfo.cpp:70
> +    // This definition of third party needs to be refined. This is just a proof-of-concept.

My guess is we should the exact same definition we use for cookies.

[Benjamin Poulain]

Sam, do you know if we defer to some CFNetwork API to know if a request is third party?

[Sam Weinig]

(In reply to comment #2)
> Comment on attachment 248123 [details]
> > Source/WebCore/loader/ResourceLoadInfo.cpp:33
> > +ResourceType toResourceType(CachedResource::Type type)
> 
> Sam had a clear spec somewhere giving all the types we should expose.

https://fetch.spec.whatwg.org/#concept-request-context

[Sam Weinig]

(In reply to comment #3)
> Sam, do you know if we defer to some CFNetwork API to know if a request is
> third party?

For third party cookie blocking, yes, but only because that is where cookies are handled. For this, I think we should just do it based on the same origin policy. That means, that if the SecurityOrigin of the resource being requested does not match the SecurityOrigin of the top level frame, it is third party. We can revise the meaning later if necessary.

[Alex Christensen]

Created attachment 248445 [details]
Patch

[Alex Christensen]

Created attachment 248448 [details]
Patch

[Alex Christensen]

Rebased patch.  Tests conflicted with http://trac.webkit.org/changeset/181405

[Benjamin Poulain]

Comment on attachment 248445 [details]
Patch

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

Ok, I may be wrong but from a quick look I think this is incorrect.

Since you are merging the flags into the actions at the machine level, you need to differentiate 2 actions of same type with different conditions.
Say I have 2 rules:
    1) BLock *.swf on thirdparty
    2) Block foobar.pdf always

When you merge the actions in serializeActions(), you must differentiate those two.
Since now you are adding the flags after merging the rules, both filters would be conditional on third-party.

> Source/WebCore/contentextensions/ContentExtensionParser.cpp:49
> +static bool getTypeFlags(ExecState& exec, const JSValue& typeValue, ResourceFlags& flags, uint16_t(*reader)(const String&))

reader -> stringToType?

> Source/WebCore/contentextensions/ContentExtensionParser.cpp:56
> +        WTFLogAlways("Invalid array");

We'll have to improve those error messages. Here is it not clear which array is invalid.

> Source/WebCore/contentextensions/DFABytecode.h:51
> +    AppendConditionalAction,

A bit weird to have "Conditional Action"

AppendActionConditionallyOnFlags
TestFlagsAndAppendAction

?

> Source/WebCore/loader/ResourceLoadInfo.cpp:38
> +    case CachedResource::SVGDocumentResource:

You don't use the SVGDocument type?

> Source/WebCore/loader/ResourceLoadInfo.cpp:62
> +    case CachedResource::TextTrackResource:
> +        return ResourceType::Media;

Hum, I don't know if that's right.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:513
> +    URL mainDocumentURL;
> +    if (frame() && frame()->document())
> +        mainDocumentURL = frame()->document()->url();

If you are in a embedded frame (e.g. iFrame), shouldn't mainDocumentURL be the one of the top level frame?

[Alex Christensen]

Comment on attachment 248445 [details]
Patch

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

>> Source/WebCore/loader/ResourceLoadInfo.cpp:38
>> +    case CachedResource::SVGDocumentResource:
> 
> You don't use the SVGDocument type?

Oops.  You're right.

>> Source/WebCore/loader/ResourceLoadInfo.cpp:62
>> +        return ResourceType::Media;
> 
> Hum, I don't know if that's right.

It seems like TextTrackResource would always be related to media, which we would need a separate device to block.

>> Source/WebCore/loader/cache/CachedResourceLoader.cpp:513
>> +        mainDocumentURL = frame()->document()->url();
> 
> If you are in a embedded frame (e.g. iFrame), shouldn't mainDocumentURL be the one of the top level frame?

I think so.  Should we also use the mainFrame's userContentController?

[Alex Christensen]

(In reply to comment #9)
> Ok, I may be wrong but from a quick look I think this is incorrect.
> 
> Since you are merging the flags into the actions at the machine level, you
> need to differentiate 2 actions of same type with different conditions.
> Say I have 2 rules:
>     1) BLock *.swf on thirdparty
>     2) Block foobar.pdf always
> 
> When you merge the actions in serializeActions(), you must differentiate
> those two.
> Since now you are adding the flags after merging the rules, both filters
> would be conditional on third-party.
The conditionality is in the trigger in the DFA byte code.  In this case it would behave correctly: It would append the block action unconditionally for foobar.pdf and it would append the same block action after testing flags for *.swf.
That is a good test case, though.  I'm including it.

[Alex Christensen]

Created attachment 248460 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 248460 [details]
Patch

Clearing flags on attachment: 248460

Committed r181421: <http://trac.webkit.org/changeset/181421>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.