[android_webview] Add more params to request intercepting.

[android_webview] Add more params to request intercepting.

This adds the following to the shouldInterceptRequest params:
 - isMainFrame
 - hasUserGesture
 - method
 - headers
This adds the following to InterceptedRequestData:
 - status code
 - response phrase
 - headers

BUG=387086
android_webview-only CL, trybots are happy.
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278806

[mkosiba (inactive), 2014-05-15 12:59:52]

PTAL

[agrieve, 2014-05-15 14:01:18]

https://codereview.chromium.org/284123004/diff/1/android_webview/java/src/org...
File
android_webview/java/src/org/chromium/android_webview/InterceptedRequestData.java
(right):

https://codereview.chromium.org/284123004/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/InterceptedRequestData.java:36:
assert statusCode < 100 || statusCode >= 600;
Is this backwards?

[mnaganov (inactive), 2014-05-15 14:23:16]

Looks good in general, I have the same question as Andy.

https://codereview.chromium.org/284123004/diff/1/android_webview/browser/inte...
File android_webview/browser/intercepted_request_data.h (right):

https://codereview.chromium.org/284123004/diff/1/android_webview/browser/inte...
android_webview/browser/intercepted_request_data.h:37: // returned |headers| was
not updated.
nit: were?

[mkosiba (inactive), 2014-05-15 14:31:49]

https://codereview.chromium.org/284123004/diff/1/android_webview/browser/inte...
File android_webview/browser/intercepted_request_data.h (right):

https://codereview.chromium.org/284123004/diff/1/android_webview/browser/inte...
android_webview/browser/intercepted_request_data.h:37: // returned |headers| was
not updated.
On 2014/05/15 14:23:17, Mikhail Naganov (Cr) wrote:
> nit: were?

Done.

https://codereview.chromium.org/284123004/diff/1/android_webview/java/src/org...
File
android_webview/java/src/org/chromium/android_webview/InterceptedRequestData.java
(right):

https://codereview.chromium.org/284123004/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/InterceptedRequestData.java:36:
assert statusCode < 100 || statusCode >= 600;
On 2014/05/15 14:01:19, agrieve wrote:
> Is this backwards?

oh, yeath.. hmm.. I guess tests don't actually run with asserts then :/

[mnaganov (inactive), 2014-05-15 14:55:25]

lgtm

https://codereview.chromium.org/284123004/diff/1/android_webview/java/src/org...
File
android_webview/java/src/org/chromium/android_webview/InterceptedRequestData.java
(right):

https://codereview.chromium.org/284123004/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/InterceptedRequestData.java:36:
assert statusCode < 100 || statusCode >= 600;
On 2014/05/15 14:31:50, mkosiba wrote:
> On 2014/05/15 14:01:19, agrieve wrote:
> > Is this backwards?
> 
> oh, yeath.. hmm.. I guess tests don't actually run with asserts then :/

For me, the test runner usually complains something like that:

"Unable to enable java asserts for <pylib.device.device_utils.DeviceUtils object
at 0x3969450>, non rooted device"

Probably, it needs an "eng" build?

[mnaganov (inactive), 2014-05-15 14:56:26]

One more remark, though: I guess, you should create a Chromium bug with some
description distilled from internal bugs.

[benm (inactive), 2014-05-15 15:55:29]

On 2014/05/15 14:56:26, Mikhail Naganov (Cr) wrote:
> One more remark, though: I guess, you should create a Chromium bug with some
> description distilled from internal bugs.

Yes, please file an external issue.

[benm (inactive), 2014-05-15 16:17:27]

On 2014/05/15 15:55:29, benm wrote:
> On 2014/05/15 14:56:26, Mikhail Naganov (Cr) wrote:
> > One more remark, though: I guess, you should create a Chromium bug with some
> > description distilled from internal bugs.
> 
> Yes, please file an external issue.

So we are allowing the embedder to view the headers that would've been sent and
then push new header name/values into the arrays to inject headers? If that's
the case the two arrays feel a bit awkward, can we just use a map?

[benm (inactive), 2014-05-15 16:17:39]

https://codereview.chromium.org/284123004/diff/40001/android_webview/browser/...
File
android_webview/browser/net/android_stream_reader_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/284123004/diff/40001/android_webview/browser/...
android_webview/browser/net/android_stream_reader_url_request_job_unittest.cc:134:
const int HeaderAlteringStreamReaderDelegate::kResponseCode = 401;
are there any headers that we shouldn't allow the app to spoof, or are their
special headers like Referer that we should test here (as there's special
handing for them in chromium).

(I seem to recall there is a header that CTS ensures the app can't overwrite via
the loadUrl overload...)

[mkosiba (inactive), 2014-05-15 16:50:43]

On 2014/05/15 16:17:27, benm wrote:
> On 2014/05/15 15:55:29, benm wrote:
> > On 2014/05/15 14:56:26, Mikhail Naganov (Cr) wrote:
> > > One more remark, though: I guess, you should create a Chromium bug with
some
> > > description distilled from internal bugs.
> > 
> > Yes, please file an external issue.
> 
> So we are allowing the embedder to view the headers that would've been sent
and
> then push new header name/values into the arrays to inject headers? If that's
> the case the two arrays feel a bit awkward, can we just use a map?

huh? We use a map (look at AwContentsClient). The two arrays are there because
they
are simpler/more efficient to shove through JNI.

[mkosiba (inactive), 2014-05-15 17:03:20]

https://codereview.chromium.org/284123004/diff/40001/android_webview/browser/...
File
android_webview/browser/net/android_stream_reader_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/284123004/diff/40001/android_webview/browser/...
android_webview/browser/net/android_stream_reader_url_request_job_unittest.cc:134:
const int HeaderAlteringStreamReaderDelegate::kResponseCode = 401;
On 2014/05/15 16:17:40, benm wrote:
> are there any headers that we shouldn't allow the app to spoof, or are their
> special headers like Referer that we should test here (as there's special
> handing for them in chromium).

That's a good question. I'm not sure there is a lot of 'security' value in
preventing the app from spoofing headers. They can do that already by
intercepting the request and issuing a completely separate request from a custom
network stack.

There might be cases were they can shoot themselves in the foot (maybe something
like appending to content-length?). There is a list of 'dangerous' headers that
web pages shouldn't set here:
http://www.w3.org/TR/XMLHttpRequest/#the-setrequestheader()-method but most of
that seem like 'stuff we need to prevent untrusted code from doing to maintain
the security model' rather than 'touch any of these and everything breaks'.

I'll expand the test a bit but the app can't overwrite any headers. It can only
append so the worst that can happen is that the header contains more than one
value.


> (I seem to recall there is a header that CTS ensures the app can't overwrite
via
> the loadUrl overload...)

testCanInjectHeaders? It seems to check the exact opposite.

[benm (inactive), 2014-05-15 17:43:15]

Got it. Sorry I think I was mixing up request/response headers when I looked
last time. Can you be more explicit about what is what? I guess the
requestHeaders that you pass into shouldInterceptRequest could be an immutable
map?

The CTS I was thinking of is
testAppInjectedXRequestedWithHeaderIsNotOverwritten. But that's verifying the
opposite too... :-)

https://codereview.chromium.org/284123004/diff/40001/android_webview/browser/...
File android_webview/browser/intercepted_request_data.h (right):

https://codereview.chromium.org/284123004/diff/40001/android_webview/browser/...
android_webview/browser/intercepted_request_data.h:38: virtual bool GetHeaders(
GetResponseHeaders

https://codereview.chromium.org/284123004/diff/40001/android_webview/browser/...
File android_webview/browser/net/android_stream_reader_url_request_job.h
(right):

https://codereview.chromium.org/284123004/diff/40001/android_webview/browser/...
android_webview/browser/net/android_stream_reader_url_request_job.h:70: virtual
void UpdateHeaders(JNIEnv* env,
should this be AppendResponseHeaders?

https://codereview.chromium.org/284123004/diff/40001/android_webview/java/src...
File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
(right):

https://codereview.chromium.org/284123004/diff/40001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:162:
public Map<String, String> headers;
call it requestHeaders please

https://codereview.chromium.org/284123004/diff/40001/android_webview/java/src...
File
android_webview/java/src/org/chromium/android_webview/AwContentsIoThreadClient.java
(right):

https://codereview.chromium.org/284123004/diff/40001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwContentsIoThreadClient.java:46:
boolean hasUserGesture, String method, String[] headerNames, String[]
headerValues) {
requestHeaders

https://codereview.chromium.org/284123004/diff/40001/android_webview/java/src...
File
android_webview/java/src/org/chromium/android_webview/InterceptedRequestData.java
(right):

https://codereview.chromium.org/284123004/diff/40001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/InterceptedRequestData.java:23:
private String[] mHeaderNames;
mResonseHeaders/mResponseValues

https://codereview.chromium.org/284123004/diff/40001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/InterceptedRequestData.java:33:
int statusCode, String reasonPhrase, Map<String, String> headers) {
call it responseHeaders

[mkosiba (inactive), 2014-06-19 17:54:15]

https://codereview.chromium.org/284123004/diff/40001/android_webview/browser/...
File android_webview/browser/intercepted_request_data.h (right):

https://codereview.chromium.org/284123004/diff/40001/android_webview/browser/...
android_webview/browser/intercepted_request_data.h:38: virtual bool GetHeaders(
On 2014/05/15 17:43:15, benm wrote:
> GetResponseHeaders

Done.

https://codereview.chromium.org/284123004/diff/40001/android_webview/browser/...
File android_webview/browser/net/android_stream_reader_url_request_job.h
(right):

https://codereview.chromium.org/284123004/diff/40001/android_webview/browser/...
android_webview/browser/net/android_stream_reader_url_request_job.h:70: virtual
void UpdateHeaders(JNIEnv* env,
On 2014/05/15 17:43:15, benm wrote:
> should this be AppendResponseHeaders?

Done.

https://codereview.chromium.org/284123004/diff/40001/android_webview/java/src...
File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
(right):

https://codereview.chromium.org/284123004/diff/40001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:162:
public Map<String, String> headers;
On 2014/05/15 17:43:15, benm wrote:
> call it requestHeaders please

Done.

https://codereview.chromium.org/284123004/diff/40001/android_webview/java/src...
File
android_webview/java/src/org/chromium/android_webview/AwContentsIoThreadClient.java
(right):

https://codereview.chromium.org/284123004/diff/40001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwContentsIoThreadClient.java:46:
boolean hasUserGesture, String method, String[] headerNames, String[]
headerValues) {
On 2014/05/15 17:43:15, benm wrote:
> requestHeaders

Done.

https://codereview.chromium.org/284123004/diff/40001/android_webview/java/src...
File
android_webview/java/src/org/chromium/android_webview/InterceptedRequestData.java
(right):

https://codereview.chromium.org/284123004/diff/40001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/InterceptedRequestData.java:23:
private String[] mHeaderNames;
On 2014/05/15 17:43:15, benm wrote:
> mResonseHeaders/mResponseValues

Done.

https://codereview.chromium.org/284123004/diff/40001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/InterceptedRequestData.java:33:
int statusCode, String reasonPhrase, Map<String, String> headers) {
On 2014/05/15 17:43:15, benm wrote:
> call it responseHeaders

Done.

[benm (inactive), 2014-06-20 12:53:19]

lgtm

thanks!

[benm (inactive), 2014-06-20 12:54:35]

On 2014/06/20 12:53:19, benm wrote:
> lgtm
> 
> thanks!

(please don't forget an external bug, though)

[mkosiba (inactive), 2014-06-20 19:55:31]

The CQ bit was checked by mkosiba@chromium.org

[commit-bot: I haz the power, 2014-06-20 19:57:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mkosiba@chromium.org/284123004/130001

[commit-bot: I haz the power, 2014-06-20 20:00:01]

Change committed as 278806