SSL Fix: Step 3....

SSLPolicy Fix: Step 3.

Plumbing the security origin of the frame making the request to SSL land.

R=wtc
BUG=8706
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=11851

[abarth-chromium, 2009-03-16 22:57:23]

Lots of files, but a conceptually simple change.

[wtc, 2009-03-17 01:08:39]

LGTM.

1. This CL uses "null" in many places.  This makes me feel
more strongly that we should define a constant for that
string, for people like me who're not familiar with how
security origins are serialized.

2. I wonder if all we need from main_frame_origin and
frame_origin is a bool that indicates whether the scheme
is secure (HTTPS) or not.

http://codereview.chromium.org/48038/diff/36/1007
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/48038/diff/36/1007#newcode966
Line 966: if (SSLManager::ShouldDelayRequest(this, request, ui_loop_)) {
Perhaps rename the method ShouldStartRequest and make it
return the opposite sense?  Then we can write the code like
this, which seems to read better:

  if (!SSLManager::ShouldStartRequest(...)) {
    return;
  }
  request->Start();

Whatever you decide is fine by me.

http://codereview.chromium.org/48038/diff/36/1005
File chrome/browser/renderer_host/resource_dispatcher_host.h (right):

http://codereview.chromium.org/48038/diff/36/1005#newcode120
Line 120: std::string frame_origin;
Would be nice to add short comments for these two members.

http://codereview.chromium.org/48038/diff/36/1005#newcode375
Line 375: struct BlockedRequest {
Do we still need a struct when it has only one member?

Perhaps a TODO if you don't want to deal with this now.

http://codereview.chromium.org/48038/diff/36/1001
File chrome/browser/ssl/ssl_manager.cc (right):

http://codereview.chromium.org/48038/diff/36/1001#newcode474
Line 474: // We cheat here and talk to the SSLPolicy on the IO channel because
we need
"IO channel" => "IO thread".

Is it not thread-safe to talk to the SSLPolicy on the IO
thread?

http://codereview.chromium.org/48038/diff/36/1001#newcode478
Line 478: info->main_frame_origin))
So you're not using info->frame_origin.  Why?

http://codereview.chromium.org/48038/diff/36/1002
File chrome/browser/ssl/ssl_manager.h (right):

http://codereview.chromium.org/48038/diff/36/1002#newcode346
Line 346: // Called before a URL request has been started.  Returns whether the
resource
"Called before a URL request has been started" sounds a little
strange to me.  How about "Called before a URL request is
about to be started"?

http://codereview.chromium.org/48038/diff/36/1003
File chrome/browser/ssl/ssl_policy.cc (right):

http://codereview.chromium.org/48038/diff/36/1003#newcode460
Line 460: return GURL(main_frame_origin).SchemeIsSecure() &&
!url.SchemeIsSecure();
So all we need from main_frame_origin is whether it's scheme
is secure or not.  Perhaps we just need to plumb a bool
instead of the security origin?

I still don't see where we use frame_origin...  Are you going
to use it in the new code you're going to write?

http://codereview.chromium.org/48038/diff/36/1014
File chrome/renderer/renderer_glue.cc (right):

http://codereview.chromium.org/48038/diff/36/1014#newcode255
Line 255: std::string main_frame_origin = webframe->GetTop() ?
This expression is also in Step 4.  Perhaps the right
accessor is GetMainFrameSecurityOrigin()?  (Or we add
this function and still keep GetTop().)

[abarth-chromium, 2009-03-17 01:53:08]

> 1. This CL uses "null" in many places.

Yes.  We should define a constant.  I'm not quite sure where it would go.  Let's
add a TODO for this.

> 2. I wonder if all we need from main_frame_origin and
> frame_origin is a bool that indicates whether the scheme
> is secure (HTTPS) or not.

We need the full origins for the following reason:  Once a HTTPS frame loads an
HTTP script, that insecure script can manipulate all the other HTTPS frames in
the same origin.  The new SSLPolicy will take this into account and propagate
the AUTHENTICATION_BROKEN security style to all the windows with the same
origin.

http://codereview.chromium.org/48038/diff/36/1007
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/48038/diff/36/1007#newcode966
Line 966: if (SSLManager::ShouldDelayRequest(this, request, ui_loop_)) {
Good idea.  Changed to ShouldStartRequest.

http://codereview.chromium.org/48038/diff/36/1005
File chrome/browser/renderer_host/resource_dispatcher_host.h (right):

http://codereview.chromium.org/48038/diff/36/1005#newcode120
Line 120: std::string frame_origin;
Fixed.

http://codereview.chromium.org/48038/diff/36/1005#newcode375
Line 375: struct BlockedRequest {
We don't need the struct.  I added a TODO.

http://codereview.chromium.org/48038/diff/36/1001
File chrome/browser/ssl/ssl_manager.cc (right):

http://codereview.chromium.org/48038/diff/36/1001#newcode474
Line 474: // We cheat here and talk to the SSLPolicy on the IO channel because
we need
Fixed.

The SSLPolicy doesn't exist on the IO thread.  If you try to interact with it
much, you'll start hitting DCHECKs about threading in SSLManager.

http://codereview.chromium.org/48038/diff/36/1001#newcode478
Line 478: info->main_frame_origin))
On 2009/03/17 01:08:39, wtc wrote:
> So you're not using info->frame_origin.  Why?

I'm keeping the current behavior.  You're right that this behavior is wrong,
which is part of the point of these changes.  :)

I wanted to keep all the functional changes separate from the plumbing so we can
think about them carefully.

http://codereview.chromium.org/48038/diff/36/1002
File chrome/browser/ssl/ssl_manager.h (right):

http://codereview.chromium.org/48038/diff/36/1002#newcode346
Line 346: // Called before a URL request has been started.  Returns whether the
resource
Fixed.

http://codereview.chromium.org/48038/diff/36/1003
File chrome/browser/ssl/ssl_policy.cc (right):

http://codereview.chromium.org/48038/diff/36/1003#newcode460
Line 460: return GURL(main_frame_origin).SchemeIsSecure() &&
!url.SchemeIsSecure();
We need all the information in frame_origin and main_frame_origin to do the
right thing.  If you like, you can see how these are used in SSLPolicy in
http://codereview.chromium.org/46094.  At the moment, we've plumbed all this
state to here but haven't done anything useful with it.  :)

http://codereview.chromium.org/48038/diff/36/1014
File chrome/renderer/renderer_glue.cc (right):

http://codereview.chromium.org/48038/diff/36/1014#newcode255
Line 255: std::string main_frame_origin = webframe->GetTop() ?
I've fixed this to the more compact
webframe->GetTop()->GetSecurityOrigin()

[wtc, 2009-03-17 17:45:39]

Patch Set 3 looks good to me.