Support Cookie modifications in Declarative WebRequest API

Support Cookie modifications in Declarative WebRequest API


BUG=112155
TEST=no
TBR=ben@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149838

[battre, 2012-05-30 16:57:51]

Hi.

This is the beginning of a CL to support cookie modifications in the Declarative
WebRequest API. I have quite a lot of cookie header parsing, modifying and
reassembling ahead of me. Therefore, I wanted to get a quick update on whether
you like the API first.

It is sufficient to only look at
chrome/common/extensions/api/declarative_web_request.json for now.

Here are some of the usecases I would like to cover:

// Remove a cookie
new declarativeWebRequest.ModifyCookie({name: "login", modification: "remove"})

// Remove all cookies, also used for HTTPS Everywhere in combination with a
filter for the HTTP scheme to get rid of cookies that were not properly marked
as secure.
new declarativeWebRequest.ModifyCookie({modification: "remove"})

// Mark all incoming cookies a secure (usecase: HTTPS Everywhere)
new declarativeWebRequest.ModifyCookie({modification: "markAsSecure"})

// Make all cookies session cookies makeSessionCookie (not sure whether we need
this)
new declarativeWebRequest.ModifyCookie({modification: "markAsSecure"})

// Simulate the existence of a cookie (I suppose we could also save a cookie in
the cookie store --> not sure whether we need this or want to keep it for
completeness sake):
new declarativeWebRequest.SetCookie({direction: "request", value: "Foo=Bar"})
new declarativeWebRequest.SetCookie({direction: "response", value:
"Foo=Bar;Expiration=...;..."})

I am not sure whether there are other important use cases I have missed, or
whether this is overly generic.

Best regards,
Dominic

[Aaron Boodman, 2012-05-30 17:40:22]

+abarth

One thing we need to carefully consider is whether this is increasing the
privilege of this API. It's important that the declarativeWebRequest API never
gain privilege equivalent to host permissions.

What is the scariest thing someone could do with the ability to declaratively
set and modify cookies?

[battre, 2012-05-30 17:50:20]

On 2012/05/30 17:40:22, Aaron Boodman wrote:
> +abarth
> 
> One thing we need to carefully consider is whether this is increasing the
> privilege of this API. It's important that the declarativeWebRequest API never
> gain privilege equivalent to host permissions.
> 
> What is the scariest thing someone could do with the ability to declaratively
> set and modify cookies?

That is an interesting question. We provide full access to cookies in the
conventional web request api already.

[Aaron Boodman, 2012-05-30 17:55:04]

As for the API, could we do something like:

object DeclarativeWebRequest.CookieDetails {
  enum direction { "request", "response" },
  string name;
  string value;
  number expiration;  // zero indicates session
  boolean secure;
  // whatever else
};

new declarativeWebRequest.AddCookie(CookieDetails);
new declarativeWebRequest.EditCookie(CookieDetails);
new declarativeWebRequest.RemoveCookie(string name);

In the future, it would be cool to expand these three actions to be able to do
pattern-based modifications...

[Aaron Boodman, 2012-05-30 17:56:58]

On Wed, May 30, 2012 at 10:55 AM,  <aa@chromium.org> wrote:
> new declarativeWebRequest.RemoveCookie(string name);

Oops, I guess this one would be:

new declarativeWebRequest.Remove({string name, string direction});

[battre, 2012-06-29 15:19:40]

I think this is now ready for review. Matt, could you please take a look?

Thanks,
Dominic

[Matt Perry, 2012-07-09 23:26:12]

https://chromiumcodereview.appspot.com/10447090/diff/9007/chrome/browser/exte...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc
(right):

https://chromiumcodereview.appspot.com/10447090/diff/9007/chrome/browser/exte...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc:876:
WebRequestCookieAction::WebRequestCookieAction(
It looks like the logic for the 2 types of cookies are completely different.
What's the advantage of putting them in a combined Action class?

https://chromiumcodereview.appspot.com/10447090/diff/9007/chrome/browser/exte...
File chrome/browser/extensions/api/web_request/web_request_api_helpers.cc
(right):

https://chromiumcodereview.appspot.com/10447090/diff/9007/chrome/browser/exte...
chrome/browser/extensions/api/web_request/web_request_api_helpers.cc:352: static
void ParseRequestCookieLine(
We already have cookie parsing code in net/. Can we reuse that? Ditto for the
cookie data structures.

https://chromiumcodereview.appspot.com/10447090/diff/9007/chrome/browser/exte...
chrome/browser/extensions/api/web_request/web_request_api_helpers.cc:548: //
Resemble and store new cookie line.
Reassemble*

https://chromiumcodereview.appspot.com/10447090/diff/9007/chrome/browser/exte...
chrome/browser/extensions/api/web_request/web_request_api_helpers.cc:799: //
|deltas| to |cookies|. Returns whether any cookie was deleteds.
deleted*

https://chromiumcodereview.appspot.com/10447090/diff/9007/chrome/test/data/ex...
File chrome/test/data/extensions/api_test/webrequest/test_declarative.js
(right):

https://chromiumcodereview.appspot.com/10447090/diff/9007/chrome/test/data/ex...
chrome/test/data/extensions/api_test/webrequest/test_declarative.js:374:
"function hasCookie(name, value) {" +
indent is off

https://chromiumcodereview.appspot.com/10447090/diff/9007/chrome/test/data/ex...
chrome/test/data/extensions/api_test/webrequest/test_declarative.js:409: "var
cookies = document.cookie.split('; ');" +
indent is off

[battre, 2012-08-02 17:17:34]

Reviving this CL now that all preparations are done.

Matt, PTAL.

Thanks,
Dominic

http://codereview.chromium.org/10447090/diff/9007/chrome/browser/extensions/a...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc
(right):

http://codereview.chromium.org/10447090/diff/9007/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc:876:
WebRequestCookieAction::WebRequestCookieAction(
On 2012/07/09 23:26:13, Matt Perry wrote:
> It looks like the logic for the 2 types of cookies are completely different.
> What's the advantage of putting them in a combined Action class?

Just saving a couple of lines of builder plate code. I have split this.

Done.

http://codereview.chromium.org/10447090/diff/9007/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_request/web_request_api_helpers.cc
(right):

http://codereview.chromium.org/10447090/diff/9007/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_api_helpers.cc:352: static
void ParseRequestCookieLine(
On 2012/07/09 23:26:13, Matt Perry wrote:
> We already have cookie parsing code in net/. Can we reuse that? Ditto for the
> cookie data structures.

net/ has parsing code for *response* cookie lines. I did not see any code for
parsing *request* cookie lines (why would we want to parse cookies descriptions
that we generate ourselves?).

For requests, cookies are generated via URLRequestHttpJob::DoLoadCookies() ->
CookieMonster::GetCookiesWithInfoAsync() ->
CookieMonster::GetCookiesWithInfoTask::Run() ->
CookieMonster::GetCookiesWithInfo(), which operates on CanonicalCookies, which
is complete overkill for this (and has no parser for request cookie lines.)

http://codereview.chromium.org/10447090/diff/9007/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_api_helpers.cc:548: //
Resemble and store new cookie line.
On 2012/07/09 23:26:13, Matt Perry wrote:
> Reassemble*

Done.

http://codereview.chromium.org/10447090/diff/9007/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_api_helpers.cc:799: //
|deltas| to |cookies|. Returns whether any cookie was deleteds.
On 2012/07/09 23:26:13, Matt Perry wrote:
> deleted*

Done.

http://codereview.chromium.org/10447090/diff/9007/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/webrequest/test_declarative.js
(right):

http://codereview.chromium.org/10447090/diff/9007/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/webrequest/test_declarative.js:374:
"function hasCookie(name, value) {" +
On 2012/07/09 23:26:13, Matt Perry wrote:
> indent is off

Done.

http://codereview.chromium.org/10447090/diff/9007/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/webrequest/test_declarative.js:409: "var
cookies = document.cookie.split('; ');" +
On 2012/07/09 23:26:13, Matt Perry wrote:
> indent is off

Done.

[Matt Perry, 2012-08-02 17:37:51]

LGTM with typo fixed

http://codereview.chromium.org/10447090/diff/9007/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_request/web_request_api_helpers.cc
(right):

http://codereview.chromium.org/10447090/diff/9007/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_api_helpers.cc:352: static
void ParseRequestCookieLine(
On 2012/08/02 17:17:34, battre wrote:
> On 2012/07/09 23:26:13, Matt Perry wrote:
> > We already have cookie parsing code in net/. Can we reuse that? Ditto for
the
> > cookie data structures.
> 
> net/ has parsing code for *response* cookie lines. I did not see any code for
> parsing *request* cookie lines (why would we want to parse cookies
descriptions
> that we generate ourselves?).
> 
> For requests, cookies are generated via URLRequestHttpJob::DoLoadCookies() ->
> CookieMonster::GetCookiesWithInfoAsync() ->
> CookieMonster::GetCookiesWithInfoTask::Run() ->
> CookieMonster::GetCookiesWithInfo(), which operates on CanonicalCookies, which
> is complete overkill for this (and has no parser for request cookie lines.)

I see. And I presume there's no way to pull the structured cookie data out of
the request without needing to parse it? Oh well.

http://codereview.chromium.org/10447090/diff/47001/chrome/browser/extensions/...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc
(right):

http://codereview.chromium.org/10447090/diff/47001/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc:955:
return WebRequestAction::ACTION_MODIFY_REQUEST_COOKIE;
oops

[commit-bot: I haz the power, 2012-08-03 09:12:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/10447090/54001

[commit-bot: I haz the power, 2012-08-03 09:12:34]

Failed to apply patch for
chrome/common/extensions/api/declarative_web_request.json:
While running patch -p1 --forward --force;
patching file chrome/common/extensions/api/declarative_web_request.json
Hunk #1 FAILED at 23.
Hunk #2 FAILED at 31.
Hunk #3 FAILED at 39.
Hunk #4 succeeded at 180 (offset 9 lines).
Hunk #5 succeeded at 354 (offset 9 lines).
3 out of 5 hunks FAILED -- saving rejects to file
chrome/common/extensions/api/declarative_web_request.json.rej

[commit-bot: I haz the power, 2012-08-03 09:18:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/10447090/50005

[commit-bot: I haz the power, 2012-08-03 09:18:48]

Presubmit check for 10447090-50005 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 2.4s to calculate.

[battre, 2012-08-03 09:33:53]

TBRing ben for OWNER (triggerd by
chrome/renderer/resources/extensions/declarative_webrequest_custom_bindings.js)

[commit-bot: I haz the power, 2012-08-03 09:34:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/10447090/50005

[commit-bot: I haz the power, 2012-08-03 11:42:34]

Change committed as 149838

[Nico, 2013-08-22 17:15:35]

https://chromiumcodereview.appspot.com/10447090/diff/50005/chrome/browser/ext...
File chrome/browser/extensions/api/web_request/web_request_api_helpers.cc
(right):

https://chromiumcodereview.appspot.com/10447090/diff/50005/chrome/browser/ext...
chrome/browser/extensions/api/web_request/web_request_api_helpers.cc:373: while
(i != header_value.end() && *i != '"') ++i;
http://www.viva64.com/en/b/0205/#ID0ELSBI points out that this is likely wrong.
Can you take a look? (see also
https://code.google.com/p/chromium/issues/detail?id=271530 /
https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0AuaiI7KNlghJdEMzV...
)

[Nico, 2013-08-29 01:11:44]

battre: Can you look at this, please?


On Thu, Aug 22, 2013 at 10:15 AM, <thakis@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/10447090/diff/**
> 50005/chrome/browser/**extensions/api/web_request/**
>
web_request_api_helpers.cc<https://chromiumcodereview.appspot.com/10447090/diff/50005/chrome/browser/extensions/api/web_request/web_request_api_helpers.cc>
> File
> chrome/browser/extensions/api/**web_request/web_request_api_**helpers.cc
> (right):
>
> https://chromiumcodereview.**appspot.com/10447090/diff/**
> 50005/chrome/browser/**extensions/api/web_request/**
>
web_request_api_helpers.cc#**newcode373<https://chromiumcodereview.appspot.com/10447090/diff/50005/chrome/browser/extensions/api/web_request/web_request_api_helpers.cc#newcode373>
> chrome/browser/extensions/api/**web_request/web_request_api_**
> helpers.cc:373:
> while (i != header_value.end() && *i != '"') ++i;
>
http://www.viva64.com/en/b/**0205/#ID0ELSBI<http://www.viva64.com/en/b/0205/#...
out that this is likely
> wrong. Can you take a look? (see also
>
https://code.google.com/p/**chromium/issues/detail?id=**271530<https://code.g...
> https://docs.google.com/a/**chromium.org/spreadsheet/ccc?**key=**
>
0AuaiI7KNlghJdEMzVC1KcGFuVVdpe**XY5R1A4SV93VlE#gid=0<https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0AuaiI7KNlghJdEMzVC1KcGFuVVdpeXY5R1A4SV93VlE#gid=0>
> )
>
>
https://chromiumcodereview.**appspot.com/10447090/<https://chromiumcodereview...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[battre, 2013-08-29 08:06:11]

Sorry for not getting back on this. It has been fixed 3 weeks ago:
https://chromiumcodereview.appspot.com/21907006/

Best regards,
Dominic


On Thu, Aug 29, 2013 at 3:11 AM, Nico Weber <thakis@chromium.org> wrote:

> battre: Can you look at this, please?
>
>
> On Thu, Aug 22, 2013 at 10:15 AM, <thakis@chromium.org> wrote:
>
>>
>> https://chromiumcodereview.**appspot.com/10447090/diff/**
>> 50005/chrome/browser/**extensions/api/web_request/**
>>
web_request_api_helpers.cc<https://chromiumcodereview.appspot.com/10447090/diff/50005/chrome/browser/extensions/api/web_request/web_request_api_helpers.cc>
>> File
>> chrome/browser/extensions/api/**web_request/web_request_api_**helpers.cc
>> (right):
>>
>> https://chromiumcodereview.**appspot.com/10447090/diff/**
>> 50005/chrome/browser/**extensions/api/web_request/**
>>
web_request_api_helpers.cc#**newcode373<https://chromiumcodereview.appspot.com/10447090/diff/50005/chrome/browser/extensions/api/web_request/web_request_api_helpers.cc#newcode373>
>> chrome/browser/extensions/api/**web_request/web_request_api_**
>> helpers.cc:373:
>> while (i != header_value.end() && *i != '"') ++i;
>>
http://www.viva64.com/en/b/**0205/#ID0ELSBI<http://www.viva64.com/en/b/0205/#...
out that this is likely
>> wrong. Can you take a look? (see also
>>
https://code.google.com/p/**chromium/issues/detail?id=**271530<https://code.g...
>> https://docs.google.com/a/**chromium.org/spreadsheet/ccc?**key=**
>>
0AuaiI7KNlghJdEMzVC1KcGFuVVdpe**XY5R1A4SV93VlE#gid=0<https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0AuaiI7KNlghJdEMzVC1KcGFuVVdpeXY5R1A4SV93VlE#gid=0>
>> )
>>
>>
https://chromiumcodereview.**appspot.com/10447090/<https://chromiumcodereview...
>>
>
>

-- 
Google Germany GmbH - Dienerstr. 12 - 80331 München - Germany
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Graham Law, Christine Elizabeth Flores

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2013-08-29 14:28:38]

Thanks!


On Thu, Aug 29, 2013 at 1:05 AM, Dominic Battre <battre@chromium.org> wrote:

> Sorry for not getting back on this. It has been fixed 3 weeks ago:
> https://chromiumcodereview.appspot.com/21907006/
>
> Best regards,
> Dominic
>
>
>
> On Thu, Aug 29, 2013 at 3:11 AM, Nico Weber <thakis@chromium.org> wrote:
>
>> battre: Can you look at this, please?
>>
>>
>> On Thu, Aug 22, 2013 at 10:15 AM, <thakis@chromium.org> wrote:
>>
>>>
>>> https://chromiumcodereview.**appspot.com/10447090/diff/**
>>> 50005/chrome/browser/**extensions/api/web_request/**
>>>
web_request_api_helpers.cc<https://chromiumcodereview.appspot.com/10447090/diff/50005/chrome/browser/extensions/api/web_request/web_request_api_helpers.cc>
>>> File
>>> chrome/browser/extensions/api/**web_request/web_request_api_**helpers.cc
>>> (right):
>>>
>>> https://chromiumcodereview.**appspot.com/10447090/diff/**
>>> 50005/chrome/browser/**extensions/api/web_request/**
>>>
web_request_api_helpers.cc#**newcode373<https://chromiumcodereview.appspot.com/10447090/diff/50005/chrome/browser/extensions/api/web_request/web_request_api_helpers.cc#newcode373>
>>> chrome/browser/extensions/api/**web_request/web_request_api_**
>>> helpers.cc:373:
>>> while (i != header_value.end() && *i != '"') ++i;
>>>
http://www.viva64.com/en/b/**0205/#ID0ELSBI<http://www.viva64.com/en/b/0205/#...
out that this is likely
>>> wrong. Can you take a look? (see also
>>>
https://code.google.com/p/**chromium/issues/detail?id=**271530<https://code.g...
>>> https://docs.google.com/a/**chromium.org/spreadsheet/ccc?**key=**
>>>
0AuaiI7KNlghJdEMzVC1KcGFuVVdpe**XY5R1A4SV93VlE#gid=0<https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0AuaiI7KNlghJdEMzVC1KcGFuVVdpeXY5R1A4SV93VlE#gid=0>
>>> )
>>>
>>>
https://chromiumcodereview.**appspot.com/10447090/<https://chromiumcodereview...
>>>
>>
>>
>
> --
> Google Germany GmbH - Dienerstr. 12 - 80331 München - Germany
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
> Geschäftsführer: Graham Law, Christine Elizabeth Flores
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.