Enable (optional) blocking of webrequests in case a PAC script cannot be fetched or is invalid.

Enable (optional) blocking of webrequests in case a PAC script cannot be fetched or is invalid.

The optional blocking is currently only exposed to the Proxy Settings API, not to command-line parameters or policies.

BUG=79344
TEST=Install the sample proxy extension and enter a non-existing url and a URL that does not point to a valid JavaScript file. In either case, the request should fail.

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

[battre, 2011-04-15 18:06:22]

Hi,

@eroman: Could you please have a look whether you like the approach of blocking
invalid PAC scripts in the net/proxy/* files? I don't know whether the approach
is the best from an architectural perspective. You should have a better insight
with regards to this.

I think I have to handle three cases:
- User sets PAC URL and script does not exist
- User sets PAC URL and script is no valid JavaScript (does not parse)
- User sets PAC URL and script returns an invalid value

These seem to work. Is there anything else?

@bauerb: Could you please have a look at the other part of the CL? It is mostly
mechanic changes.

Thanks,
Dominic

[Bernhard Bauer, 2011-04-15 18:38:47]

http://codereview.chromium.org/6871019/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_proxy_api_helpers.cc (right):

http://codereview.chromium.org/6871019/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_proxy_api_helpers.cc:87: LOG(ERROR) <<
"'pacScript.mandatory' could not be parsed.";
Hm, this should already be covered by the JSON schema, no? In that case we
usually set |bad_message_| to true (which kills the extension process).

http://codereview.chromium.org/6871019/diff/1/chrome/common/extensions/api/ex...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/6871019/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:3858: "mandatory": {"type":
"boolean", "optional": true, "description": "If true, an invalid PAC script will
cause to fall back to direct connections. Defaults to false."}
Nit: missing a subject here (cause what to fall back?)

[battre, 2011-04-26 15:31:30]

Hi Eric,

may I ask you to have a look at this?

Thanks,
Dominic

http://codereview.chromium.org/6871019/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_proxy_api_helpers.cc (right):

http://codereview.chromium.org/6871019/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_proxy_api_helpers.cc:87: LOG(ERROR) <<
"'pacScript.mandatory' could not be parsed.";
On 2011/04/15 18:38:47, Bernhard Bauer wrote:
> Hm, this should already be covered by the JSON schema, no? In that case we
> usually set |bad_message_| to true (which kills the extension process).

I have added a TODO, in order to do this in a separate CL.

http://codereview.chromium.org/6871019/diff/1/chrome/common/extensions/api/ex...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/6871019/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:3858: "mandatory": {"type":
"boolean", "optional": true, "description": "If true, an invalid PAC script will
cause to fall back to direct connections. Defaults to false."}
On 2011/04/15 18:38:47, Bernhard Bauer wrote:
> Nit: missing a subject here (cause what to fall back?)

Done.

[eroman, 2011-04-26 21:19:59]

> Hi Eric,
> may I ask you to have a look at this?

Yep, looking now.

[eroman, 2011-04-26 21:47:17]

Overall the approach looks reasonable, however please introduce a new network
error code for this situation (proxy_connection_failed has a different meaning:
it means our connection to the proxy server itself failed, whereas this meaning
really means we ran out of fallbacks).

Please ping me over IM or a separate email when I lag this badly, since
sometimes the code review emails go into wrong inbox and I lose track of it.

http://codereview.chromium.org/6871019/diff/3001/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/6871019/diff/3001/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:3998: "mandatory": {"type":
"boolean", "optional": true, "description": "If true, an invalid PAC script will
cause the network stack to fall back to direct connections. Defaults to false."}
This seems inverted (if true, request willl NOT fall back to direct).

http://codereview.chromium.org/6871019/diff/3001/chrome/common/extensions/doc...
File chrome/common/extensions/docs/experimental.proxy.html (right):

http://codereview.chromium.org/6871019/diff/3001/chrome/common/extensions/doc...
chrome/common/extensions/docs/experimental.proxy.html:2076: <dd>If true, an
invalid PAC script will cause the network stack to fall back to direct
connections. Defaults to false.</dd>
Ditto.

http://codereview.chromium.org/6871019/diff/3001/net/proxy/proxy_config.cc
File net/proxy/proxy_config.cc (right):

http://codereview.chromium.org/6871019/diff/3001/net/proxy/proxy_config.cc#ne...
net/proxy/proxy_config.cc:214: dict->SetString("pac_url",
pac_url_.possibly_invalid_spec());
The new properties should be mirrored here as well.

http://codereview.chromium.org/6871019/diff/3001/net/proxy/proxy_config.h
File net/proxy/proxy_config.h (right):

http://codereview.chromium.org/6871019/diff/3001/net/proxy/proxy_config.h#new...
net/proxy/proxy_config.h:171: void set_block_everything(bool
enable_block_everything) {
I would rather not have this bool here, since it isn't really part of the
policy, but more of an implementation state variable.

I think you could accomplish this same thing by having a member variable in
ProxyService (of type "int" so it holds an error code). Since the
"block_everything()" bit is only checked from ProxyService anyway, it would
basically work the same.

Another idea I have been toying with, is breaking up ProxyConfig into an ordered
list of "rules", so it reads more like a directed acyclic graph (the fallback
policies would then be explicit in the datstructure, rather than something the
ProxyService decided). Just thinking out loud, not something you would want to
tackle for this change.

http://codereview.chromium.org/6871019/diff/3001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/6871019/diff/3001/net/proxy/proxy_service.cc#n...
net/proxy/proxy_service.cc:556: return ERR_PROXY_CONNECTION_FAILED;
Should use a more specific error code here.

http://codereview.chromium.org/6871019/diff/3001/net/proxy/proxy_service.cc#n...
net/proxy/proxy_service.cc:747: result_code = ERR_PROXY_CONNECTION_FAILED;
Use a more specific error code.

[battre, 2011-04-27 17:02:58]

Thanks for the review. This is the next iteration.

http://codereview.chromium.org/6871019/diff/3001/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/6871019/diff/3001/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:3998: "mandatory": {"type":
"boolean", "optional": true, "description": "If true, an invalid PAC script will
cause the network stack to fall back to direct connections. Defaults to false."}
On 2011/04/26 21:47:17, eroman wrote:
> This seems inverted (if true, request willl NOT fall back to direct).

Done.

http://codereview.chromium.org/6871019/diff/3001/chrome/common/extensions/doc...
File chrome/common/extensions/docs/experimental.proxy.html (right):

http://codereview.chromium.org/6871019/diff/3001/chrome/common/extensions/doc...
chrome/common/extensions/docs/experimental.proxy.html:2076: <dd>If true, an
invalid PAC script will cause the network stack to fall back to direct
connections. Defaults to false.</dd>
On 2011/04/26 21:47:17, eroman wrote:
> Ditto.

Done.

http://codereview.chromium.org/6871019/diff/3001/net/proxy/proxy_config.cc
File net/proxy/proxy_config.cc (right):

http://codereview.chromium.org/6871019/diff/3001/net/proxy/proxy_config.cc#ne...
net/proxy/proxy_config.cc:214: dict->SetString("pac_url",
pac_url_.possibly_invalid_spec());
On 2011/04/26 21:47:17, eroman wrote:
> The new properties should be mirrored here as well.

Done.

http://codereview.chromium.org/6871019/diff/3001/net/proxy/proxy_config.h
File net/proxy/proxy_config.h (right):

http://codereview.chromium.org/6871019/diff/3001/net/proxy/proxy_config.h#new...
net/proxy/proxy_config.h:171: void set_block_everything(bool
enable_block_everything) {
On 2011/04/26 21:47:17, eroman wrote:
> I would rather not have this bool here, since it isn't really part of the
> policy, but more of an implementation state variable.
> 
> I think you could accomplish this same thing by having a member variable in
> ProxyService (of type "int" so it holds an error code). Since the
> "block_everything()" bit is only checked from ProxyService anyway, it would
> basically work the same.
> 
> Another idea I have been toying with, is breaking up ProxyConfig into an
ordered
> list of "rules", so it reads more like a directed acyclic graph (the fallback
> policies would then be explicit in the datstructure, rather than something the
> ProxyService decided). Just thinking out loud, not something you would want to
> tackle for this change.

I have introduced the int. I don't know whether this is overly generic, though.
As we don't use the value beyond a comparison to OK.

Done.

http://codereview.chromium.org/6871019/diff/3001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/6871019/diff/3001/net/proxy/proxy_service.cc#n...
net/proxy/proxy_service.cc:556: return ERR_PROXY_CONNECTION_FAILED;
On 2011/04/26 21:47:17, eroman wrote:
> Should use a more specific error code here.

Done.

http://codereview.chromium.org/6871019/diff/3001/net/proxy/proxy_service.cc#n...
net/proxy/proxy_service.cc:747: result_code = ERR_PROXY_CONNECTION_FAILED;
On 2011/04/26 21:47:17, eroman wrote:
> Use a more specific error code.

Done.

[eroman, 2011-05-03 09:45:24]

LGTM.

Could you also add a test to proxy_service_unittest.cc which exercises the
mandatory PAC boolean? (can be part of a follow-up, so as not to delay this CL
further).

http://codereview.chromium.org/6871019/diff/8002/net/proxy/proxy_config.cc
File net/proxy/proxy_config.cc (right):

http://codereview.chromium.org/6871019/diff/8002/net/proxy/proxy_config.cc#ne...
net/proxy/proxy_config.cc:211: if (pac_mandatory_)
can you add this within the "has_pac_url()" case above? (since it only makes
sense in the context where there is a PAC URL).

http://codereview.chromium.org/6871019/diff/8002/net/proxy/proxy_config.h
File net/proxy/proxy_config.h (right):

http://codereview.chromium.org/6871019/diff/8002/net/proxy/proxy_config.h#new...
net/proxy/proxy_config.h:199: // fails. Only valid if pac_url_ is non-empty.
minor nit: enclose enclose "pac_url_" as in |pac_url_|, similar to above.

[battre, 2011-05-03 09:55:14]

Thanks for the review. I have created http://crbug.com/81368 for the unit test.

http://codereview.chromium.org/6871019/diff/8002/net/proxy/proxy_config.cc
File net/proxy/proxy_config.cc (right):

http://codereview.chromium.org/6871019/diff/8002/net/proxy/proxy_config.cc#ne...
net/proxy/proxy_config.cc:211: if (pac_mandatory_)
On 2011/05/03 09:45:24, eroman wrote:
> can you add this within the "has_pac_url()" case above? (since it only makes
> sense in the context where there is a PAC URL).

Done.

http://codereview.chromium.org/6871019/diff/8002/net/proxy/proxy_config.h
File net/proxy/proxy_config.h (right):

http://codereview.chromium.org/6871019/diff/8002/net/proxy/proxy_config.h#new...
net/proxy/proxy_config.h:199: // fails. Only valid if pac_url_ is non-empty.
On 2011/05/03 09:45:24, eroman wrote:
> minor nit: enclose enclose "pac_url_" as in |pac_url_|, similar to above.

Done.

[Ryan Hamilton, 2011-05-09 23:01:49]

Just a quick question...  When will the API documentation be update here:

http://code.google.com/chrome/extensions/experimental.proxy.html

[battre, 2011-05-10 11:03:11]

On 2011/05/09 23:01:49, Ryan Hamilton wrote:
> Just a quick question...  When will the API documentation be update here:
> 
> http://code.google.com/chrome/extensions/experimental.proxy.html

I guess, when this is moved to stable. 

Check out
http://code.google.com/chrome/extensions/trunk/experimental.proxy.htmlfor now
(notice the additional "trunk/" in the path).

[Ryan Hamilton, 2011-05-10 13:14:12]

Perfect.  thanks!

On Tue, May 10, 2011 at 4:03 AM, <battre@chromium.org> wrote:

> On 2011/05/09 23:01:49, Ryan Hamilton wrote:
>
>> Just a quick question...  When will the API documentation be update here:
>>
>
>  http://code.google.com/chrome/extensions/experimental.proxy.html
>>
>
> I guess, when this is moved to stable.
>
> Check out
> http://code.google.com/chrome/extensions/trunk/experimental.proxy.htmlfornow
> (notice the additional "trunk/" in the path).
>
>
> http://codereview.chromium.org/6871019/
>