Closed Bug 1133763 Opened 5 years ago Closed 5 years ago

HTTPS requests can't be synthesized using FetchEvent.respondWith

Categories

(Core :: DOM: Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: ferjm, Assigned: ehsan)

References

Details

Attachments

(8 files, 11 obsolete files)

4.68 KB, patch
nsm
: review+
Details | Diff | Splinter Review
2.00 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
5.05 KB, patch
nsm
: review+
Details | Diff | Splinter Review
6.38 KB, patch
nsm
: review+
Details | Diff | Splinter Review
7.74 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
4.79 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
4.51 KB, patch
jdm
: review+
Details | Diff | Splinter Review
9.40 KB, patch
michal
: review+
Details | Diff | Splinter Review
STR:
1. Navigate to https://ferjm.github.io/cachebug/index.html
2. Go offline.
3. Reload https://ferjm.github.io/cachebug/index.html

Expected:
4. The content of https://ferjm.github.io/cachebug/index.html is shown

Current:
5. The "Unable to connect" page is shown.
 
I am using a slightly modified version of bkelly's patch queue from [1] that you can find at [2].

The code for this test case is at [3]

[1] http://blog.wanderview.com/sw-builds/
[2] https://github.com/ferjm/swpatchqueue
[3] https://github.com/ferjm/cachebug/tree/gh-pages
See Also: → 1133520
Actually, I think we just need to finish implementing bug 931249 for this.  Sorry I didn't realize the problem earlier when we were talking on IRC.
Depends on: 931249
The log that ferjm showed me had the the fetch listener responding appropriately and intercepting the channel, but the HTTP cache tried to process the security-info attribute and couldn't locate it, so it doomed the cache entry. This was happening on a github pages site, so it was using HTTPS. Honza, how can we populate the synthesized cache entry with a valid security info object if we don't get one until OnStartRequest during non-intercepted requests? Alternatively, should we just not doom cache entries for intercepted HTTPS requests that are missing the security info?
Flags: needinfo?(honzab.moz)
If the SW onfetch handler intercepted the request and returned a Response from the Cache API, why is the request going to the http cache at all?
If I understood correctly, that's the way the intercepted request response works. We create a fake http cache entry and fill it with the response that we send from the service worker.
FWIW this works with an http origin. It is not really useful for service workers though.
(In reply to Josh Matthews [:jdm] from comment #2)
> The log that ferjm showed me had the the fetch listener responding
> appropriately and intercepting the channel, but the HTTP cache tried to
> process the security-info attribute and couldn't locate it, so it doomed the
> cache entry. This was happening on a github pages site, so it was using
> HTTPS. Honza, how can we populate the synthesized cache entry with a valid
> security info object if we don't get one until OnStartRequest during
> non-intercepted requests? 

There is no way.  Only appcache is abusing a compatibility hack, see https://bugzilla.mozilla.org/show_bug.cgi?id=1009531#c22

This has been a dilemma for a long time already.  The channel (or to be exact - the CacheEntry) should inherit the security info from the context (channel or principal) that intercepted the load.  I.e. the either secinfo of the document you are running in or the content script that implements the interception.  Somebody on the sec team may know better.

Or, if we can take the intercepted content as trusted, we can bypass the secinfo presence check.  What implications is that going to have further (on the mixed content blocker for instance) I really don't know.  But I presume bad...

> Alternatively, should we just not doom cache
> entries for intercepted HTTPS requests that are missing the security info?

We must.  I can lookup bug#'s for you what happens when we don't.
Flags: needinfo?(honzab.moz)
Summary: Cached resources are not been used while offline → HTTPS requests can't be synthesized using FetchEvent.respondWith
Assignee: nobody → josh
What's your plan, Josh?
There are two cases that I see:
* the ServiceWorker intercepts an HTTPS request and creates a new Response with a synthesized body manually
* the ServiceWorker intercepts an HTTPS request, and synthesizes a response using the result of another non-intercepted request (or a cached Response object)

In the second case, we're talking about storing the channel's security info object in the Response object, and updating the intercepted channel's security info with that stored object. When there is no such object available, I think we'll try to fall back to the document's object? Does it matter if the intercepted request is cross-origin with the document? I don't really understand what the securityInfo object is.
The securityInfo object holds information about what certificate the server we obtained the resource from provided, between else.  Having secInfo means the cert and the connection have been verified, also if it has EV status etc.  And having secInfo also means the resource has actually been securely downloaded.  You cannot just fake it here and there.

I would spec the following: 
1) an unsecured script cannot synthesize secured (https) resources.  
2) a secure script cannot synthesize cross-origin secured resources.

Period.  (Simply said: https can only be synthesized by a same-origin script)

If you have these preconditions, you can easily use the secInfo of the script or the document that embeds the script.

Claim that a resource comes from https://a.com/ means that it either has been downloaded from that site via a secured connection (you have secInfo naturally) or produced by something that has been also been securely (and "trustworthly") downloaded from that very same site.  The same applies to 'data:' URLs that derive security context, except that data: from its nature cannot be different-origin.
Thanks, that's really helpful. I'll need to talk with some people to see whether condition 2 will cause any problems.
I've talked with Ehsan, and we looked at the spec, and it looks like we're going to be ok here. The spec explicitly disallows manually creating responses for cross-origin requests - you must either provide a response which is the result of another cross-origin request (and therefore includes the security info), or a previously-cached cross-origin response (which should therefore be able to reconstruct the security info from the cached data). It looks like this bug will need to focus on storing the security info object on the InternalReponse object, as well as serializing and deserializing the security info object when a response stored/retrieved in the cache.
Assignee: josh → ehsan
Attachment #8572920 - Flags: review?(bkelly)
Attachment #8572921 - Flags: review?(honzab.moz)
Comment on attachment 8572917 [details] [diff] [review]
Part 1: Remember the security info associated with HTTP fetches and record it inside InternalResponse

Review of attachment 8572917 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with 1 change. Please copy the security string in the InternalResponse copy constructor, otherwise clone()d responses won't work.
Attachment #8572917 - Flags: review?(nsm.nikhil) → review+
This version of the patch fixes the test failures on the try server.
Attachment #8573251 - Flags: review?(josh)
Attachment #8572922 - Attachment is obsolete: true
Attachment #8572922 - Flags: review?(josh)
Comment on attachment 8572920 [details] [diff] [review]
Part 4: Store the response's security info in the cache database

Review of attachment 8572920 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/cache/DBSchema.cpp
@@ +1026,5 @@
>  
>    rv = BindId(state, 13, aResponseBodyId);
>    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>  
> +  rv = state->BindUTF8StringParameter(14, aResponse.securityInfo());

you can name the parameters, no longer need to keep track of numbering, didn't you know?
Comment on attachment 8572921 [details] [diff] [review]
Part 5: Allow the security info on HTTP channels to be overridden

Review of attachment 8572921 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsIHttpChannel.idl
@@ +183,5 @@
> +     *
> +     * NOTE: This is only intended to be used for intercepted channels that
> +     * will not get their own security info from the transport layer.
> +     */
> +    void setSecurityInfo(in nsISupports securityInfo);

Oh, no please...

It MUST NOT be able to change/set the secinfo on channels via public XPCOM API AT ALL.


Best you should put the secinfo on the artificial cache entry (in parent/single model) or 'somewhere' when on the child.  Channel should grab it only when it is actually being intercepted (past the point to return to normal response serving.)

If that is too complicated (seems to me) and there is no way around except a public API, then expose this on nsIInterceptedChannel interface and take GREAT care to allow the sec info override only when the channel is being intercepted - as stated above.  Otherwise, fail the setter uncompromisingly and maybe even do some kind of a security warning yell.
Attachment #8572921 - Flags: review?(honzab.moz) → review-
(In reply to Honza Bambas (:mayhemer) from comment #23)
> Comment on attachment 8572921 [details] [diff] [review]
> Part 5: Allow the security info on HTTP channels to be overridden
> 
> Review of attachment 8572921 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/nsIHttpChannel.idl
> @@ +183,5 @@
> > +     *
> > +     * NOTE: This is only intended to be used for intercepted channels that
> > +     * will not get their own security info from the transport layer.
> > +     */
> > +    void setSecurityInfo(in nsISupports securityInfo);
> 
> Oh, no please...
> 
> It MUST NOT be able to change/set the secinfo on channels via public XPCOM
> API AT ALL.
> 
> 
> Best you should put the secinfo on the artificial cache entry (in
> parent/single model) or 'somewhere' when on the child.  Channel should grab
> it only when it is actually being intercepted (past the point to return to
> normal response serving.)

That is exactly what happens with my patches, see part 6.  We set the security info only when the channel is actually being intercepted.

> If that is too complicated (seems to me) and there is no way around except a
> public API, then expose this on nsIInterceptedChannel interface and take
> GREAT care to allow the sec info override only when the channel is being
> intercepted - as stated above.  Otherwise, fail the setter uncompromisingly
> and maybe even do some kind of a security warning yell.

What specific checks you'd want me to do to satisfy taking great care?  I'm not really sure what changes I should make.  nsIInterceptedChannel itself also deals with an nsIChannel, so we need to make it possible to override the security info on the nsIChannel somehow.
Flags: needinfo?(honzab.moz)
Attachment #8573264 - Attachment is obsolete: true
Attachment #8573264 - Flags: review?(nsm.nikhil)
Attachment #8572923 - Attachment is obsolete: true
Attachment #8572923 - Flags: review?(josh)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #24)
> (In reply to Honza Bambas (:mayhemer) from comment #23)
> > Comment on attachment 8572921 [details] [diff] [review]
> > Part 5: Allow the security info on HTTP channels to be overridden
> > 
> > Review of attachment 8572921 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: netwerk/protocol/http/nsIHttpChannel.idl
> > @@ +183,5 @@
> > > +     *
> > > +     * NOTE: This is only intended to be used for intercepted channels that
> > > +     * will not get their own security info from the transport layer.
> > > +     */
> > > +    void setSecurityInfo(in nsISupports securityInfo);
> > 
> > Oh, no please...
> > 
> > It MUST NOT be able to change/set the secinfo on channels via public XPCOM
> > API AT ALL.
> > 
> > 
> > Best you should put the secinfo on the artificial cache entry (in
> > parent/single model) or 'somewhere' when on the child.  Channel should grab
> > it only when it is actually being intercepted (past the point to return to
> > normal response serving.)
> 
> That is exactly what happens with my patches, see part 6.  We set the
> security info only when the channel is actually being intercepted.

Yes, you are doing it.  But this will be a publicly exposed API.  ANYONE can set secinfo on channels with your part 6.

> 
> > If that is too complicated (seems to me) and there is no way around except a
> > public API, then expose this on nsIInterceptedChannel interface and take
> > GREAT care to allow the sec info override only when the channel is being
> > intercepted - as stated above.  Otherwise, fail the setter uncompromisingly
> > and maybe even do some kind of a security warning yell.
> 
> What specific checks you'd want me to do to satisfy taking great care?  I'm
> not really sure what changes I should make.  nsIInterceptedChannel itself
> also deals with an nsIChannel, so we need to make it possible to override
> the security info on the nsIChannel somehow.

The channel must protect itself *from inside* from re-setting or later exposing any secinfo given from outside when not appropriate.

I think a better way would be to go via InterceptedChannelBase/* that have access to the channel implementation, not just an interface.  Would it be possible?

When this needs to be done on the http channel (parent/child) ideally expose (via GetSecurityInfo) this synthesized secinfo - stored in a new dedicated member - only when the http channel is responding with a synthesized response, i.e. when HttpChannelChild::mInterceptListener != null, nsHttpChannel::mInterceptCache == INTERCEPTED (if I'm not mistaken - jdm will know).
Make the tests unregister the SW after they're done so that their registrations do not step on each other's toes.  Also, switching the review to Nikhil as the tests are almost identical.
Attachment #8573318 - Flags: review?(nsm.nikhil)
Flags: needinfo?(honzab.moz)
Comment on attachment 8573264 [details] [diff] [review]
Part 8: Ensure that FetchEvent.respondWith works correctly on HTTPS requests with a cloned response

Review of attachment 8573264 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/test/serviceworkers/fetch/https/clonedresponse/register.html
@@ +1,5 @@
> +<!DOCTYPE html>
> +<script>
> +  navigator.serviceWorker.register("https_test.js", {scope: "."})
> +    .then(function(registration) {
> +      window.parent.postMessage({status: "registrationdone"}, "*");

This promise resolves before installation is done. This may lead to timing issues on other machines where the SW is still installing and does not handle the fetch for index.html. Do something like:

function continue(reg) {
  ok(reg.waiting || reg.active, "Either active or waiting worker should be available.");
  postMessage(...);
}

register()
.then(function(registration) {
  if (registration.installing) {
    registration.installing.onstatechange = function() {
      continue(registration)
    }
  } else {
    continue(registration)
  }
});

Yes it is awkward, but we don't have a better API at this point.

::: dom/workers/test/serviceworkers/test_https_fetch_cloned_response.html
@@ +42,5 @@
> +    SpecialPowers.pushPrefEnv({"set": [
> +      ["dom.serviceWorkers.enabled", true],
> +      ["dom.serviceWorkers.testing.enabled", true],
> +      ["dom.fetch.enabled", true],
> +      ["dom.caches.enabled", true]

FYI if the exemption bug lands before this, please add that pref here as it has been added to other tests - https://bugzilla.mozilla.org/attachment.cgi?id=8572779&action=diff
Attachment #8573264 - Attachment is obsolete: false
(In reply to Honza Bambas (:mayhemer) from comment #26)
> > > If that is too complicated (seems to me) and there is no way around except a
> > > public API, then expose this on nsIInterceptedChannel interface and take
> > > GREAT care to allow the sec info override only when the channel is being
> > > intercepted - as stated above.  Otherwise, fail the setter uncompromisingly
> > > and maybe even do some kind of a security warning yell.
> > 
> > What specific checks you'd want me to do to satisfy taking great care?  I'm
> > not really sure what changes I should make.  nsIInterceptedChannel itself
> > also deals with an nsIChannel, so we need to make it possible to override
> > the security info on the nsIChannel somehow.
> 
> The channel must protect itself *from inside* from re-setting or later
> exposing any secinfo given from outside when not appropriate.
> 
> I think a better way would be to go via InterceptedChannelBase/* that have
> access to the channel implementation, not just an interface.  Would it be
> possible?

Hmm, yes, that may be possible...

> When this needs to be done on the http channel (parent/child) ideally expose
> (via GetSecurityInfo) this synthesized secinfo - stored in a new dedicated
> member - only when the http channel is responding with a synthesized
> response, i.e. when HttpChannelChild::mInterceptListener != null,
> nsHttpChannel::mInterceptCache == INTERCEPTED (if I'm not mistaken - jdm
> will know).

OK.  Let me see what I can do.  I'm not sure if it's worth storing this sec info in a new member though, because then we'd need to modify everywhere that it's used and remember to keep doing this forever.  But it should be possible to make sure that this only happens for intercepted channels one way or another.
Comment on attachment 8573319 [details] [diff] [review]
Part 8: Ensure that FetchEvent.respondWith works correctly on HTTPS requests with a cloned response

Review of attachment 8573319 [details] [diff] [review]:
-----------------------------------------------------------------

Prior review applies. unregister is fine.
Attachment #8573319 - Flags: review?(nsm.nikhil) → review+
Comment on attachment 8573318 [details] [diff] [review]
Part 7: Add automated tests for using FetchEvent.respondWith on an HTTPS request

Review of attachment 8573318 [details] [diff] [review]:
-----------------------------------------------------------------

Please add same pref and complicated registration procedure as in the other test. r=me with that.
Attachment #8573318 - Flags: review?(nsm.nikhil) → review+
Attachment #8572918 - Flags: review?(bkelly) → review+
Comment on attachment 8572919 [details] [diff] [review]
Part 3: Wipe out the cache directory when detecting a change in the DB schema

Review of attachment 8572919 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks good.  I think we should just remove the DB file directly instead of using SQL queries to drop tables.  Its going to be faster and less error prone.

::: dom/cache/DBSchema.cpp
@@ +63,5 @@
> +    // We have detected an older version of the database.  Delete the entire
> +    // DB for now.  This will be replaced with a proper upgrade path in the
> +    // future.
> +    rv = aConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +          "DROP INDEX IF EXISTS response_headers_name_index;"));

I think it would be preferable to close the database, remove it completely, and then let the database get re-created.

We would probably want to run VACUUM immediately after manually deleting all these tables to cleanup pages, anyway.  That essentially rebuilds the database.  Faster just to wipe the file and start from scratch.  Also, removing the database file directly avoids missing new tables here, etc.

I recommend adding a WipeAndReopenConnection() on DBAction that could be called here.

::: dom/cache/FileUtils.cpp
@@ +60,5 @@
> +  rv = aBodyDir->Append(NS_LITERAL_STRING("morgue"));
> +  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> +
> +  rv = aBodyDir->Remove(/* recursive = */ true);
> +  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }

Can you add a comment here or at the declaration point that BodyDeleteDir() is only expected to work at initialization time?  If there is any activity going on for the Manager and its Context, then its likely a file will be held open and this remove will fail on Windows.

The failure due to external file handles is also a risk.  But in theory we will just try again the next time the user opens the origin.  So I guess thats ok.
Attachment #8572919 - Flags: review?(bkelly) → review+
Comment on attachment 8572920 [details] [diff] [review]
Part 4: Store the response's security info in the cache database

Review of attachment 8572920 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.  I think there is just a small copy-paste issue with the row declaration in the schema.

Thanks!

::: dom/cache/DBSchema.cpp
@@ +132,5 @@
>          "response_status INTEGER NOT NULL, "
>          "response_status_text TEXT NOT NULL, "
>          "response_headers_guard INTEGER NOT NULL, "
>          "response_body_id TEXT NULL, "
> +        "response_security_info TEXT NULL, "

It seems we always set this value and don't check for null when reading.  This should probably be NOT NULL.

@@ +1026,5 @@
>  
>    rv = BindId(state, 13, aResponseBodyId);
>    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>  
> +  rv = state->BindUTF8StringParameter(14, aResponse.securityInfo());

I asked Ehsan not to churn this file too much.  I'll be addressing stuff like this in a later bug.
Attachment #8572920 - Flags: review?(bkelly) → review+
Also, do you have a rough idea how large the serialized security info is?
Status: NEW → ASSIGNED
Comment on attachment 8572919 [details] [diff] [review]
Part 3: Wipe out the cache directory when detecting a change in the DB schema

Review of attachment 8572919 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/cache/DBSchema.cpp
@@ +63,5 @@
> +    // We have detected an older version of the database.  Delete the entire
> +    // DB for now.  This will be replaced with a proper upgrade path in the
> +    // future.
> +    rv = aConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +          "DROP INDEX IF EXISTS response_headers_name_index;"));

Even better, I think you could just move the wipe logic to DBAction itself.  You can do it inline in the same place we detect database corruption.  Just create a kMaxWipeSchemaVersion or something and wipe anything less than or equal to it.

Even once we start doing graceful upgrades, we'll want to wipe these ancient schema versions.
(In reply to Ben Kelly [:bkelly] from comment #35)
> Also, do you have a rough idea how large the serialized security info is?

Can be even be few kilobytes (2 and more) when the server sends a longer chain of certificates.
Comment on attachment 8572920 [details] [diff] [review]
Part 4: Store the response's security info in the cache database

Review of attachment 8572920 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/cache/DBSchema.cpp
@@ +132,5 @@
>          "response_status INTEGER NOT NULL, "
>          "response_status_text TEXT NOT NULL, "
>          "response_headers_guard INTEGER NOT NULL, "
>          "response_body_id TEXT NULL, "
> +        "response_security_info TEXT NULL, "

You should probably also store this as a BLOB to avoid text encoding on the serialized bytes.  So:

  response_security_info BLOB NOT NULL

And then use BindBlobByIndex() and GetBlob().

I do worry about storing such a large amount of data in an sqlite column, but we can move it to a file later if necessary.
(In reply to Ben Kelly [:bkelly] from comment #35)
> Also, do you have a rough idea how large the serialized security info is?

Usually a thousand something bytes.
Blocks: 1110446
Comment on attachment 8573251 [details] [diff] [review]
Part 6: Set the correct security info on intercepted channels when using the respondWith API

Review of attachment 8573251 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/ServiceWorkerEvents.cpp
@@ +188,5 @@
>      return;
>    }
>  
> +  nsRefPtr<InternalResponse> ir = response->GetInternalResponse();
> +  if (!ir) {

NS_WARN_IF

@@ +195,5 @@
> +  nsCOMPtr<nsISupports> infoObj;
> +  rv = NS_DeserializeObject(ir->GetSecurityInfo(), getter_AddRefs(infoObj));
> +  if (NS_SUCCEEDED(rv)) {
> +    nsCOMPtr<nsIChannel> channel;
> +    rv = mInterceptedChannel->GetChannel(getter_AddRefs(channel));

We should be doing this on the main thread, I think. I'd prefer an interface on nsIInterceptedChannel that deals with this, too, rather than interacting with channels directly.
Attachment #8573251 - Flags: review?(josh) → review-
Attachment #8572919 - Attachment is obsolete: true
Attachment #8573416 - Flags: review?(bkelly)
Comment on attachment 8573416 [details] [diff] [review]
Part 3: Wipe out the cache directory when detecting a change in the DB schema

Review of attachment 8573416 [details] [diff] [review]:
-----------------------------------------------------------------

One minor logic issue, but otherwise looks good.  Thanks for fix the file deletion TODO in the DB corruption case too!

::: dom/cache/DBAction.cpp
@@ +142,5 @@
> +  // Check the schema to make sure it is not too old.
> +  int32_t schemaVersion = 0;
> +  rv = (*aConnOut)->GetSchemaVersion(&schemaVersion);
> +  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> +  if (schemaVersion < DBSchema::kMaxWipeSchemaVersion) {

The default schema verison is zero.  So this will always delete and recreate new databases.  You probably want something like:

  schemaVersion > 0 && schemaVersion < DBSchema::kMaxWipeSchemaVersion
Attachment #8573416 - Flags: review?(bkelly) → review+
Actually, make that:

  schemaVersion > 0 && schemaVersion <= DBSchema::kMaxWipeSchemaVersion

I think you want <= on the max version comparison as well.
Attachment #8572920 - Attachment is obsolete: true
Attachment #8573430 - Flags: review?(bkelly)
(In reply to Ben Kelly [:bkelly] from comment #43)
> Actually, make that:
> 
>   schemaVersion > 0 && schemaVersion <= DBSchema::kMaxWipeSchemaVersion
> 
> I think you want <= on the max version comparison as well.

Not the way that the condition is coded.  See the comment in DBSchema.h.
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #45)
> Not the way that the condition is coded.  See the comment in DBSchema.h.

If thats the case, then shouldn't the value be 1 instead of 0 in DBSchema.cpp?  I guess for now you could just set it to kLatestSchemaVersion explicitly until we start upgrading.
Comment on attachment 8573430 [details] [diff] [review]
Part 4: Store the response's security info in the cache database

Review of attachment 8573430 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/cache/DBSchema.cpp
@@ +20,5 @@
>  namespace cache {
>  
>  
> +const int32_t DBSchema::kMaxWipeSchemaVersion = 1;
> +const int32_t DBSchema::kLatestSchemaVersion = 2;

Per your comment in the DBSchema.h, kMaxWipeSchemaVersion should = 2.  Since we wipe anything less than its value.  I think these two constants should be equal until we start upgrading schemas.

@@ +96,5 @@
>          "response_status INTEGER NOT NULL, "
>          "response_status_text TEXT NOT NULL, "
>          "response_headers_guard INTEGER NOT NULL, "
>          "response_body_id TEXT NULL, "
> +        "response_security_info BLOB NOT NULL, "

So it turns out sqlite treats zero length BLOB values as NULL.  So this does need to be NULL.  I'm sorry for suggesting otherwise here.

@@ +1133,5 @@
> +  uint32_t dataLength = 0;
> +  rv = state->GetBlob(6, &dataLength, &data);
> +  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> +  aSavedResponseOut->mValue.securityInfo().Adopt(
> +    reinterpret_cast<char*>(const_cast<uint8_t*>(data)), dataLength);

Why do you need this const_cast?

Also note, if GetBlob() returns dataLength of zero, then data is nullptr here.  Thats safe, right?
Attachment #8573430 - Flags: review?(bkelly) → review+
Comment on attachment 8573416 [details] [diff] [review]
Part 3: Wipe out the cache directory when detecting a change in the DB schema

Review of attachment 8573416 [details] [diff] [review]:
-----------------------------------------------------------------

Testing revealed a couple more issues with this patch.

::: dom/cache/DBAction.cpp
@@ +146,5 @@
> +  if (schemaVersion < DBSchema::kMaxWipeSchemaVersion) {
> +    rv = WipeDatabase(dbFile, aDBDir);
> +    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> +
> +    rv = ss->OpenDatabaseWithFileURL(dbFileUrl, aConnOut);

As it happens, the corruption code is broken.  So your use of the same pattern doesn't work either.

We need to close the previous connection before opening a new one.  This won't work here, though, because I'm using aConnOut which is just a **.  We need to create an

  nsCOMPtr<mozIStorageConnection> conn;

And use it everywhere.  You can then .forget(aConnOut) at the end of the method.

To be safe, I also think we should clear conn before wiping the database file.

I verified locally that this works.

::: dom/cache/FileUtils.cpp
@@ +57,5 @@
> +  nsresult rv = aBaseDir->Clone(getter_AddRefs(aBodyDir));
> +  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> +
> +  rv = aBodyDir->Append(NS_LITERAL_STRING("morgue"));
> +  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }

Its possible to have a database file, but no body dir.  So before checking for failure you should do this here:

  if (rv == NS_ERROR_FILE_NOT_FOUND ||
      rv == NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) {
    rv = NS_OK;
  }

Yes, you need both error codes as different platforms give different errors. :-\
(In reply to Ben Kelly [:bkelly] from comment #47)
> Comment on attachment 8573430 [details] [diff] [review]
> Part 4: Store the response's security info in the cache database
> 
> Review of attachment 8573430 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/cache/DBSchema.cpp
> @@ +20,5 @@
> >  namespace cache {
> >  
> >  
> > +const int32_t DBSchema::kMaxWipeSchemaVersion = 1;
> > +const int32_t DBSchema::kLatestSchemaVersion = 2;
> 
> Per your comment in the DBSchema.h, kMaxWipeSchemaVersion should = 2.  Since
> we wipe anything less than its value.  I think these two constants should be
> equal until we start upgrading schemas.

Sorry, I was all confused.  I'll fix things.

> @@ +96,5 @@
> >          "response_status INTEGER NOT NULL, "
> >          "response_status_text TEXT NOT NULL, "
> >          "response_headers_guard INTEGER NOT NULL, "
> >          "response_body_id TEXT NULL, "
> > +        "response_security_info BLOB NOT NULL, "
> 
> So it turns out sqlite treats zero length BLOB values as NULL.  So this does
> need to be NULL.  I'm sorry for suggesting otherwise here.

NP.

> @@ +1133,5 @@
> > +  uint32_t dataLength = 0;
> > +  rv = state->GetBlob(6, &dataLength, &data);
> > +  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> > +  aSavedResponseOut->mValue.securityInfo().Adopt(
> > +    reinterpret_cast<char*>(const_cast<uint8_t*>(data)), dataLength);
> 
> Why do you need this const_cast?

My mistake.

> Also note, if GetBlob() returns dataLength of zero, then data is nullptr
> here.  Thats safe, right?

Yes, nsTSubstring_CharT::Adopt handles that correctly.
Attached patch fixes.patch (obsolete) — Splinter Review
This is what I changed locally to fix the mochitests.
Attachment #8573416 - Attachment is obsolete: true
Attachment #8573499 - Flags: review?(bkelly)
Attachment #8573430 - Attachment is obsolete: true
Attachment #8573500 - Flags: review?(bkelly)
Attachment #8572921 - Attachment is obsolete: true
Attachment #8573501 - Flags: review?(honzab.moz)
Attachment #8573489 - Attachment is obsolete: true
Attachment #8573503 - Flags: review?(josh) → review+
Comment on attachment 8573499 [details] [diff] [review]
Part 3: Wipe out the cache directory when detecting a change in the DB schema

Review of attachment 8573499 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working through all the issues on this one!  It will make changing Cache a lot easier.

::: dom/cache/DBAction.cpp
@@ +140,5 @@
>  
> +    rv = ss->OpenDatabaseWithFileURL(dbFileUrl, getter_AddRefs(conn));
> +  }
> +  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> +  MOZ_ASSERT(conn);

nit: nsCOMPtr asserts in its operator->() already, so we don't need this MOZ_ASSERT any more.  Sorry, I know I had this in the local fixes patch I posted.
Attachment #8573499 - Flags: review?(bkelly) → review+
Comment on attachment 8573500 [details] [diff] [review]
Part 4: Store the response's security info in the cache database

Review of attachment 8573500 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: dom/cache/DBSchema.cpp
@@ +1133,5 @@
> +  uint32_t dataLength = 0;
> +  rv = state->GetBlob(6, &dataLength, &data);
> +  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> +  aSavedResponseOut->mValue.securityInfo().Adopt(
> +    reinterpret_cast<char*>(const_cast<uint8_t*>(data)), dataLength);

nit: const_cast is still in there.
Attachment #8573500 - Flags: review?(bkelly) → review+
Honza: ping?
Flags: needinfo?(honzab.moz)
Michal, would you feel comfortable reviewing attachment 8573501 [details] [diff] [review]?  I'd like to get this landed sooner rather than later.  Thanks!
Flags: needinfo?(michal.novotny)
Comment on attachment 8573501 [details] [diff] [review]
Part 5: Allow the security info on intercepted HTTP channels to be overridden

Review of attachment 8573501 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, but maybe it would be better to convert the assertions in HttpBaseChannel::OverrideSecurityInfo() to if conditions to do the checks in release build too.
Attachment #8573501 - Flags: feedback+
Flags: needinfo?(michal.novotny)
Blocks: 1142269
Attachment #8573501 - Attachment is obsolete: true
Attachment #8573501 - Flags: review?(honzab.moz)
Attachment #8577332 - Flags: review?(michal.novotny)
Attachment #8577332 - Flags: review?(michal.novotny) → review+
Flags: needinfo?(honzab.moz)
Depends on: 1146169
Depends on: 1173378
You need to log in before you can comment on or make changes to this bug.