Jonathan Mayer reports: HSTS flags might not be set for subordinate loads (non-document page elements). Here are steps for reproducing: 1) Navigate to http://www.eff.org The page includes the element <img src="https://www.eff.org/files/https.png" width="1" height="1" border="0">. In its response to the image request, EFF's HTTPS server includes the HSTS header Strict-Transport-Security: max-age=2628000. 2) Re-navigate to http://www.eff.org There's no redirect to https://www.eff.org This same usage results in a redirect with Chrome's HSTS implementation.

That's... odd. There's nothing that should be different about images in ProcessSTSHeader. And if I follow the steps to reproduce in comment 0, I get redirected to https://www.eff.org, in a Jan 18 nightly.

I'm able to confirm the same (non-redirecting) behavior as Jonathan in 4.0b9 on Mac OS X 10.6.6, but I am too confused. Using the web console, I could confirm that the image is loaded over HTTPS and served with a valid "Strict-transport-security" HTTP header in the response. Perhaps there's a caching issue at play here somewhere? I'll dive into a debug/logging build and see if I can reproduce.

Just to make sure, is comment 2 with a clean profile? Comment 1 was on 10.6.6 with a clean nightly build and a clean profile.

bz: good call. Forgot to start fresh. Comment 2 was with an old profile; with a clean profile HSTS seems to work as desired. I still want to look into it to see if something in the permissions DB when awry during a beta upgrade.

I was able to reproduce this effect when in permanent private browsing mode (never remember history). This could be a side-effect that bug 557598 would fix by supporting HSTS better in private mode. Following up with the reporter.

Able to reliably reproduce after trashing the innards of a profile. Weird.

Here's what I've found. Say you're in private browsing mode and you've never visited example.com. Say example.com sends an hsts header without includeSubdomains. The sts service adds a temporary permission for example.com (in the private browsing table). Since includeSubdomains is not set, it deletes the subdomain permission. However, since there is nothing in the permission manager about example.com, this takes the code path that deletes the entry in the private browsing table. The fix is to set the appropriate flags in RemovePermission based on what permission you're modifying.

David: would this issue still exist if we landed your patch for bug 760307 (hsts preload list)?

Comment on attachment 638532 [details] [diff] [review] patch Review of attachment 638532 [details] [diff] [review]: ----------------------------------------------------------------- I'm not a peer so I'm not supposed to r+, but this looks fine to me. Redirecting review to a peer.

The hsts preload list patch does actually fix this - should I roll the test for this one into the test for that one? Or just add it as a separate test? (Or should we continue moving forward with this as a separate patch?)

If you think we'll land the HSTS preload in the next few weeks, we can merge the two bits of work (tests and all). Otherwise, we should land this first with its own tests.

Personally, I'd start with a test anyway. There would definitely be a review request from me to add a test anyway.

mayhemer: the patch flagged r? for you already has a test, so this patch is ready for review. I think David was asking if he should keep the test for this bug separate from the ones in the other bug or if he should combine them all (if we decide to combine the two bits of work).

(In reply to Sid Stamm [:geekboy] from comment #13) > mayhemer: the patch flagged r? for you already has a test, so this patch is > ready for review. Ah, I had to take a look at the patch first :) Now I can see it.

Comment on attachment 638532 [details] [diff] [review] patch Review of attachment 638532 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab Since author is not a PSM module peer/owner this needs a secondary review.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=9e658d0da601 Also, I had to change the test slightly to properly clean up state, so if Brian or Honza could sign off that that's okay, that'd be great.

(In reply to David Keeler from comment #10) > The hsts preload list patch does actually fix this - should I roll the test > for this one into the test for that one? Or just add it as a separate test? > (Or should we continue moving forward with this as a separate patch?) Sorry about the delay here. Now I am confused about what needs to be done and what needs to be reviewed. Do we still need this patch, or just the test?

We just need the test now.

Comment on attachment 673922 [details] [diff] [review] test Review of attachment 673922 [details] [diff] [review]: ----------------------------------------------------------------- Please land this on -aurora and -beta too. You can use a=testonly to avoid bothering release drivers for approval.

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/35e15835ddc9 I'll make sure it makes it to central before uplifting.

Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/f748ad6a38fc Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/417cbc243c6c