Bug 89750 - Support paths in Content Security Policy directives.
: Support paths in Content Security Policy directives.
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 90564
: 85558
  Show dependency treegraph
 
Reported: 2012-06-22 01:51 PST by
Modified: 2012-09-20 11:11 PST (History)


Attachments
WIP. (20.64 KB, patch)
2012-06-22 02:06 PST, Mike West
no flags Review Patch | Details | Formatted Diff | Diff
Slightly less WIP. (27.86 KB, patch)
2012-07-03 08:19 PST, Mike West
no flags Review Patch | Details | Formatted Diff | Diff
Slightly less WIP. (27.86 KB, patch)
2012-07-03 08:28 PST, Mike West
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-08 (714.20 KB, application/zip)
2012-07-03 09:10 PST, WebKit Review Bot
no flags Details
Patch (22.63 KB, patch)
2012-08-17 14:49 PST, Mike West
no flags Review Patch | Details | Formatted Diff | Diff
Patch (22.51 KB, patch)
2012-08-26 04:13 PST, Mike West
no flags Review Patch | Details | Formatted Diff | Diff
Fixing Mac. (22.54 KB, patch)
2012-08-26 05:08 PST, Mike West
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-03 (595.98 KB, application/zip)
2012-08-26 05:45 PST, WebKit Review Bot
no flags Details
Patch (32.17 KB, patch)
2012-09-18 09:11 PST, Mike West
no flags Review Patch | Details | Formatted Diff | Diff
No backtics. :( (32.17 KB, patch)
2012-09-18 09:21 PST, Mike West
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-06-22 01:51:13 PST
In CSP 1.0, paths are simply ignored: `script-src http://example.com/path/to/a/file` would allow script to be loaded from `http://example.com/path/to/a/file/javascript.js`, but also from `http://example.com/javascript.js`. A proposal for CSP 1.1 is to accept more granular source definitions (http://www.w3.org/Security/wiki/Content_Security_Policy#Proposals_for_Version_1.1).

The basic concept is straightforward: I'll upload a strawman patch in a moment for discussion.
------- Comment #1 From 2012-06-22 02:06:44 PST -------
Created an attachment (id=148980) [details]
WIP.
------- Comment #2 From 2012-06-22 02:14:11 PST -------
(From update of attachment 148980 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=148980&action=review

> Source/WebCore/page/ContentSecurityPolicy.cpp:184
> +        return path.startsWith(m_path, false);

There are some open questions that should be discussed on public-webappsec@ before this lands, even as an experiment.

1. This implementation is strictly string-matching: it doesn't do any fancy directory parsing (or even non-fancy splitting on `/`). Is that enough?
2. Currently, paths behave as though they had a wildcard at the end. That is: `/path` matches `/path/to/file.js`, but also `/pathsareawesome/file.js`. I'm tempted to say that this is good enough, since ending the rule with a `/` would remove ambiguity (e.g. `/path/`), but it's something that should be discussed.
3. Query strings and anchors are part of the path: should they be?

> LayoutTests/http/tests/security/contentSecurityPolicy/1.1/source-list-parsing-paths-01.html:12
> +    ['no', 'script-src 127.0.0.1:*/sec/', 'resources/script.js'],

This shows the wildcard-like behavior noted above. I think it makes sense, but I'd like some other opinions. :)
------- Comment #3 From 2012-06-22 10:46:21 PST -------
(From update of attachment 148980 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=148980&action=review

> Source/WebCore/page/ContentSecurityPolicy.cpp:511
> -    m_list.append(CSPSource(m_origin->protocol(), m_origin->host(), m_origin->port(), false, false));
> +    m_list.append(CSPSource(m_origin->protocol(), m_origin->host(), m_origin->port(), "", false, false));

You can use String() here, which is more efficient because it doesn't cause a malloc.
------- Comment #4 From 2012-06-22 10:49:58 PST -------
> 1. This implementation is strictly string-matching: it doesn't do any fancy directory parsing (or even non-fancy splitting on `/`). Is that enough?

One thing to consider is URL canonicalization.  For example, what happens if the source-list or the URL itself contains a %-encoded character?  We probably want to treat "equivalent" URLs equivalently.

> 2. Currently, paths behave as though they had a wildcard at the end. That is: `/path` matches `/path/to/file.js`, but also `/pathsareawesome/file.js`. I'm tempted to say that this is good enough, since ending the rule with a `/` would remove ambiguity (e.g. `/path/`), but it's something that should be discussed.

Hum...  I wonder if we should add an implicit / if there isn't one in the policy.  That seems least surprising.

> 3. Query strings and anchors are part of the path: should they be?

That doesn't quite seem right.  I wonder if we should ignore everything after a ? or a # in the source-list, just like we previously ignored everything after a /.

> > LayoutTests/http/tests/security/contentSecurityPolicy/1.1/source-list-parsing-paths-01.html:12
> > +    ['no', 'script-src 127.0.0.1:*/sec/', 'resources/script.js'],
> 
> This shows the wildcard-like behavior noted above. I think it makes sense, but I'd like some other opinions. :)

That's an interesting idea, but probably not something there's a strong need to support.
------- Comment #5 From 2012-06-22 11:27:47 PST -------
We should probably move this discussion to the public-webappsec list. But, since we're here:

(In reply to comment #4)
> > 1. This implementation is strictly string-matching: it doesn't do any fancy directory parsing (or even non-fancy splitting on `/`). Is that enough?
> 
> One thing to consider is URL canonicalization.  For example, what happens if the source-list or the URL itself contains a %-encoded character?  We probably want to treat "equivalent" URLs equivalently.

You're right. I didn't even think about that.

> > 2. Currently, paths behave as though they had a wildcard at the end. That is: `/path` matches `/path/to/file.js`, but also `/pathsareawesome/file.js`. I'm tempted to say that this is good enough, since ending the rule with a `/` would remove ambiguity (e.g. `/path/`), but it's something that should be discussed.
> 
> Hum...  I wonder if we should add an implicit / if there isn't one in the policy.  That seems least surprising.

How would a user specify a file named "path" if we added an implicit `/`? Do we want users to be able to specify individual files? I think we probably do.

> > 3. Query strings and anchors are part of the path: should they be?
> 
> That doesn't quite seem right.  I wonder if we should ignore everything after a ? or a # in the source-list, just like we previously ignored everything after a /.

I'm with you on anchors. Let's throw those out.

I'm wondering if there's value in query strings, however. Maybe there's javascript that returns different results based on a user ID that's passed in as a GET param? Perhaps a developer could use CSP to ensure that only the most recent version of her javascript loads if a version is encoded in a GET param? Who knows what crazy things programmers do? :)
------- Comment #6 From 2012-06-22 11:32:32 PST -------
Questions 2 and 3 are related.  If we let folks specify individual files, then it makes sense to let them specify query parameters as well.
------- Comment #7 From 2012-06-22 11:33:34 PST -------
Ok.  If we're going to allow explicit files, then we might want to use a * to indicate a wild card.  Similar to what we do with host names, we should probably only allow the * at the end and immediately after a / character.
------- Comment #8 From 2012-06-25 18:56:34 PST -------
(In reply to comment #7)
> Ok.  If we're going to allow explicit files, then we might want to use a * to indicate a wild card.  Similar to what we do with host names, we should probably only allow the * at the end and immediately after a / character.

I've been thinking about this a bit, and I've reconsidered. I don't think file-level whitelisting is critical, and it adds significant complexity to the processing (for instance, what do we do if a developer names a file `*`? :) ).

I'd now suggest that the first pass we make at this feature keep the granularity at the directory level. We can always add the complexity of individual file-level sources later, and developers have the simple workaround of putting that one file they care about in a directory that's whitelisted.

With that in mind, I'd like to upload a patch that would implicitly end the path with a `/` if one isn't already present, and throw a warning if the path contains either `?` or `#`. I think that has the best potential to be clearly understood, and meets at least 80% of the use case.

WDYT?
------- Comment #9 From 2012-06-25 22:09:25 PST -------
Ok.  Sounds good.
------- Comment #10 From 2012-07-03 08:19:48 PST -------
Created an attachment (id=150613) [details]
Slightly less WIP.
------- Comment #11 From 2012-07-03 08:28:28 PST -------
Created an attachment (id=150615) [details]
Slightly less WIP.
------- Comment #12 From 2012-07-03 08:30:27 PST -------
(From update of attachment 150615 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=150615&action=review

I think this addresses the core of your feedback, Adam. Would you mind taking another look? I'd appreciate suggestions on the path parsing bit; it seems less efficient than it could be.

> Source/WebCore/page/ContentSecurityPolicy.cpp:501
> +    path = KURL(m_scriptExecutionContext->url(), String(begin, end - begin)).path();

This seems like an expensive way of parsing the path, but it's the best I could find (that didn't involve replicating huge swaths of code). Are there better mechanisms for canonicalizing paths?
------- Comment #13 From 2012-07-03 08:35:58 PST -------
(From update of attachment 150615 [details])
Attachment 150615 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13125518
------- Comment #14 From 2012-07-03 09:10:36 PST -------
(From update of attachment 150615 [details])
Attachment 150615 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13128503

New failing tests:
http/tests/security/contentSecurityPolicy/source-list-parsing-06.html
http/tests/security/contentSecurityPolicy/source-list-parsing-05.html
------- Comment #15 From 2012-07-03 09:10:40 PST -------
Created an attachment (id=150624) [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
------- Comment #16 From 2012-07-03 09:13:04 PST -------
(From update of attachment 150615 [details])
Attachment 150615 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13129527
------- Comment #17 From 2012-07-03 10:26:27 PST -------
(From update of attachment 150615 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=150615&action=review

> Source/WebCore/page/ContentSecurityPolicy.cpp:188
> +        return path.startsWith(m_path, false);

This test is overly simplistic.  You need to do some sort of comparison that ignores encoding differences.

> Source/WebCore/page/ContentSecurityPolicy.cpp:230
> +    String m_directiveName;
> +    ScriptExecutionContext* m_scriptExecutionContext;

I'd prefer not to store this state on CSPSourceList.  Perhaps the source list can report its error to some higher level object that has a back pointer to ScriptExecutionContext?

> Source/WebCore/page/ContentSecurityPolicy.cpp:486
> +    while (position < end) {
> +      skipWhile<isPathComponentCharacter>(position, end);

four-space indent pls.

> Source/WebCore/page/ContentSecurityPolicy.cpp:496
> +        logInvalidPath(String(begin, end - begin));

Let's worry about logging errors in an later patch.  This line is pushing us to have this object keep a bunch of state that it doesn't really need or want.

>> Source/WebCore/page/ContentSecurityPolicy.cpp:501
>> +    path = KURL(m_scriptExecutionContext->url(), String(begin, end - begin)).path();
> 
> This seems like an expensive way of parsing the path, but it's the best I could find (that didn't involve replicating huge swaths of code). Are there better mechanisms for canonicalizing paths?

I'm not sure what you're trying to accomplish here.  We should just store String(begin, end - begin) into path and use a comparison function that understands %-escaping.  It's not the case that KURL canonicalizes paths anyway, so this isn't really saving us any work.
------- Comment #18 From 2012-07-03 12:17:46 PST -------
Thanks, Adam! I'll fiddle with this more if you can point me in the right direction (see below).

(In reply to comment #17)
> (From update of attachment 150615 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150615&action=review
> 
> > Source/WebCore/page/ContentSecurityPolicy.cpp:188
> > +        return path.startsWith(m_path, false);
> 
> This test is overly simplistic.  You need to do some sort of comparison that ignores encoding differences.

Ideally, I think it would be this simplistic here. It seems like it would be best to do any necessary processing once at parse time, rather than every time we check the URL.

> > Source/WebCore/page/ContentSecurityPolicy.cpp:486
> > +    while (position < end) {
> > +      skipWhile<isPathComponentCharacter>(position, end);
> 
> four-space indent pls.

Arg. I need to figure out how to make vim treat files inside my webkit repository strangely. :)

> > Source/WebCore/page/ContentSecurityPolicy.cpp:496
> > +        logInvalidPath(String(begin, end - begin));
> 
> Let's worry about logging errors in an later patch.  This line is pushing us to have this object keep a bunch of state that it doesn't really need or want.

Agreed, I'll rip this out.

> 
> >> Source/WebCore/page/ContentSecurityPolicy.cpp:501
> >> +    path = KURL(m_scriptExecutionContext->url(), String(begin, end - begin)).path();
> > 
> > This seems like an expensive way of parsing the path, but it's the best I could find (that didn't involve replicating huge swaths of code). Are there better mechanisms for canonicalizing paths?
> 
> I'm not sure what you're trying to accomplish here.  We should just store String(begin, end - begin) into path and use a comparison function that understands %-escaping.  It's not the case that KURL canonicalizes paths anyway, so this isn't really saving us any work.

KURL does some work on the path that I'd like to avoid replicating (processing `..` for instance: I'm looking specifically at KURL::parse, around http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/platform/KURL.cpp&type=cs&l=1360). I don't know the code that well, however, so it's very possible I've misinterpreted what it's doing there.

Do you have a specific comparison function in mind? I'm creating a KURL here because I'm given a KURL in all the `*Matches()` functions, and I thought it best to do the work here once rather than repeating any processing in the test, but I'm certainly happy to reuse any components I've overlooked.
------- Comment #19 From 2012-07-03 13:35:18 PST -------
I wonder if we should use the same algorithm that's used for cookies:

http://tools.ietf.org/html/rfc6265#section-5.1.4

Specifically, cookie punt on all this canonicalization stuff.
------- Comment #20 From 2012-07-03 14:30:47 PST -------
(In reply to comment #19)
> I wonder if we should use the same algorithm that's used for cookies:
> 
> http://tools.ietf.org/html/rfc6265#section-5.1.4
> 
> Specifically, cookie punt on all this canonicalization stuff.

That seems like a reasonable place to start. We can always add complexity in later.
------- Comment #21 From 2012-07-04 09:25:13 PST -------
Moving the test-skipping out to a separate patch.
------- Comment #22 From 2012-08-04 15:22:01 PST -------
*** Bug 89288 has been marked as a duplicate of this bug. ***
------- Comment #23 From 2012-08-17 14:49:02 PST -------
Created an attachment (id=159210) [details]
Patch
------- Comment #24 From 2012-08-17 14:52:37 PST -------
Picking this back up after a month or so.

The patch is a bit smaller now, and I think I've found a good solution for URL-decoding paths before comparison: 'decodeURLEscapeSequences' seems to be exactly what I was looking for back in June.

Would you mind taking a look, Adam? If this is an implementation that still makes sense to you, I'll take a stab at speccing it out.

Thanks!

-mike
------- Comment #25 From 2012-08-17 15:15:12 PST -------
(From update of attachment 159210 [details])
Attachment 159210 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13530165
------- Comment #26 From 2012-08-26 04:13:22 PST -------
Created an attachment (id=160599) [details]
Patch
------- Comment #27 From 2012-08-26 05:08:55 PST -------
Created an attachment (id=160601) [details]
Fixing Mac.
------- Comment #28 From 2012-08-26 05:45:22 PST -------
(From update of attachment 160601 [details])
Attachment 160601 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13594807

New failing tests:
http/tests/cache/post-with-cached-subresources.php
------- Comment #29 From 2012-08-26 05:45:25 PST -------
Created an attachment (id=160603) [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
------- Comment #30 From 2012-08-26 08:02:05 PST -------
(In reply to comment #28)
> (From update of attachment 160601 [details] [details])
> Attachment 160601 [details] [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/13594807
> 
> New failing tests:
> http/tests/cache/post-with-cached-subresources.php

Hrm. I'm fairly confident that this isn't a failure generated by this patch, and it passes here on my mac locally. I'll try it on my linux box at the office tomorrow, but I suspect it's just flake.
------- Comment #31 From 2012-08-26 09:06:18 PST -------
Yeah http/tests/cache/post-with-cached-subresources.php is very flaky.  We need to mark it as such.
------- Comment #32 From 2012-08-27 11:33:28 PST -------
(From update of attachment 160601 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=160601&action=review

> Source/WebCore/ChangeLog:10
> +        `http://example.com/path/to/a/file/javascript.js`, but also from

``````````````````

> Source/WebCore/page/ContentSecurityPolicy.cpp:518
> +        skipWhile<isPathComponentCharacter>(position, end);
> +
> +        // path/to/file.js
> +        //     ^
> +        if (position < end && skipExactly(position, end, '/'))
> +            continue;

Why not add '/' to isPathComponentCharacter?
------- Comment #33 From 2012-08-27 11:34:56 PST -------
This looks great.  My only concern here is how to deal with CSP_NEXT.  Previously, my thought was that we would expose CSP 1.0 in the Content-Security-Policy header and CSP 1.1 features in X-WebKit-CSP.  However, we wouldn't be able to do that the way you've structured the ifdefs.
------- Comment #34 From 2012-08-27 11:35:43 PST -------
Let me ask on the webappsec call tomorrow what the other vendors want to do here.
------- Comment #35 From 2012-08-27 11:46:56 PST -------
(In reply to comment #33)
> This looks great.  My only concern here is how to deal with CSP_NEXT.  Previously, my thought was that we would expose CSP 1.0 in the Content-Security-Policy header and CSP 1.1 features in X-WebKit-CSP.  However, we wouldn't be able to do that the way you've structured the ifdefs.

I agree with you that it'd be best to expose 1.0 through the unprefixed header.

If we put a "prefixed" boolean on CSPSourceListDirective, one option would be to choose between implementation of both ignorePath() and parsePath(), and ifdef the implementation of the latter if CSP_NEXT isn't enabled.

Basically, whatever you decide on the call, we can make work. :)

That said, I'd _much_ rather get the unprefixed header out the door than nail down the 1.1 path processing detail.

(In reply to comment #32)
> (From update of attachment 160601 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160601&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        `http://example.com/path/to/a/file/javascript.js`, but also from
> 
> ``````````````````

'''''''''''''''''

I was so careful!

> 
> > Source/WebCore/page/ContentSecurityPolicy.cpp:518
> > +        skipWhile<isPathComponentCharacter>(position, end);
> > +
> > +        // path/to/file.js
> > +        //     ^
> > +        if (position < end && skipExactly(position, end, '/'))
> > +            continue;
> 
> Why not add '/' to isPathComponentCharacter?

I think I originally thought we'd want to do some extra validation here, flagging 'path//to/whatever', but it's probably not necessary.
------- Comment #36 From 2012-08-28 15:33:22 PST -------
I asked on the webappsec telecon today.  They seemed agreeable to keeping the 1.1 features on the X-WebKit-CSP header.  I'm going to send a follow email to give folks some time to think about it.
------- Comment #37 From 2012-08-28 22:48:47 PST -------
(In reply to comment #36)
> I asked on the webappsec telecon today.  They seemed agreeable to keeping the 1.1 features on the X-WebKit-CSP header.  I'm going to send a follow email to give folks some time to think about it.

Great. While that discussion's open, would you like me to take a stab at updating the relevant bits of the spec in line with this patch?
------- Comment #38 From 2012-08-29 09:46:42 PST -------
Please!
------- Comment #39 From 2012-09-06 11:32:58 PST -------
(In reply to comment #38)
> Please!

Added in https://dvcs.w3.org/hg/content-security-policy/rev/abb64ba225c4 and kicked off a very quiet (so far) discussion at http://lists.w3.org/Archives/Public/public-webappsec/2012Sep/0001.html.

This implementation somewhat depends on how we decide to proceed with the 1.0/1.1 headers. There hasn't been a ton of discussion on that thread... Now that we're more formally working towards moving to to CR, perhaps you could poke folks on the next call? :)
------- Comment #40 From 2012-09-06 11:40:18 PST -------
IMHO, we should use the approach outlined in http://lists.w3.org/Archives/Public/public-webappsec/2012Aug/0007.html

Specifically, we should keep all the CSP 1.1 specific behavior in the prefixed header *except* path restrictions, which we should implement in the Content-Security-Policy header.  The email messages outlines why, but the gist is that it's an easy position to walk away from by loosening the semantics later.
------- Comment #41 From 2012-09-06 11:48:27 PST -------
(In reply to comment #40)
> IMHO, we should use the approach outlined in http://lists.w3.org/Archives/Public/public-webappsec/2012Aug/0007.html
> 
> Specifically, we should keep all the CSP 1.1 specific behavior in the prefixed header *except* path restrictions, which we should implement in the Content-Security-Policy header.  The email messages outlines why, but the gist is that it's an easy position to walk away from by loosening the semantics later.

I think that makes sense.

If we go that route, would we also want to change the behavior for ports that haven't opted-in to CSP_NEXT? That would be a fairly trivial change to this patch, and I think it's the direction we want to go, but I'm not sure to what extent we can bring other ports with us.

More broadly, once 1.0 hits CR and 1.1 moves to some sort of official draft status (I assume?), do we need to maintain the CSP_NEXT flag?
------- Comment #42 From 2012-09-06 12:04:34 PST -------
> If we go that route, would we also want to change the behavior for ports that haven't opted-in to CSP_NEXT?

Probably.

> That would be a fairly trivial change to this patch, and I think it's the direction we want to go, but I'm not sure to what extent we can bring other ports with us.

IMHO, we should make sure the Content-Security-Policy header works the same in every port.

> More broadly, once 1.0 hits CR and 1.1 moves to some sort of official draft status (I assume?), do we need to maintain the CSP_NEXT flag?

We can ask other folks for input, but my guess is that once CSP 1.1 hits FPWD, we can drop the CSP_NEXT flag and just keep the 1.1 directives in the X-WebKit-CSP header.
------- Comment #43 From 2012-09-18 09:11:10 PST -------
Created an attachment (id=164573) [details]
Patch
------- Comment #44 From 2012-09-18 09:21:09 PST -------
Created an attachment (id=164575) [details]
No backtics. :(
------- Comment #45 From 2012-09-18 09:24:15 PST -------
Hey Adam, I think this implements what we've discussed. Paths are supported outside the CSP_NEXT flag, which means they'll be available for all ports, both on the current prefixed header and the upcoming canonical header. Also, the changelog is completely devoid of the cursed '`'.

As it turns out, it's not at all dependent on the canonical header patch that's also in-flight. It still probably makes sense to land them together in order to minimize the amount of flux we impose on ports, but if you'd like to land this now and the other later, I wouldn't complain. :)

WDYT?
------- Comment #46 From 2012-09-18 13:08:44 PST -------
(From update of attachment 164575 [details])
Looks great.  Let's wait for after the M23 branch before landing.
------- Comment #47 From 2012-09-20 02:24:14 PST -------
(From update of attachment 164575 [details])
(In reply to comment #46)
> (From update of attachment 164575 [details] [details])
> Looks great.  Let's wait for after the M23 branch before landing.

http://trac.webkit.org/browser/branches/chromium/1271 has been cut, so I think this should be safe to land. CQ?
------- Comment #48 From 2012-09-20 11:10:57 PST -------
(From update of attachment 164575 [details])
Clearing flags on attachment: 164575

Committed r129143: <http://trac.webkit.org/changeset/129143>
------- Comment #49 From 2012-09-20 11:11:04 PST -------
All reviewed patches have been landed.  Closing bug.