Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Obey Strict-Transport-Security header #6722

Merged
merged 2 commits into from Aug 2, 2015
Merged

Conversation

@samfoo
Copy link
Contributor

samfoo commented Jul 24, 2015

Resolves #6703.

Done:

  • When STS headers received, add the host to the HSTS list

Todo:

  • Persist the in-memory list so that it's reloaded on the next browser boot
  • Add tests to http_loader::load - it's pretty well completely untested right now, but it's a bit gnarly to untangle and without mocking, it's hard to deal with the dependency on making a real network request. Writing a mock request object should be doable for testing, but there's a lot going on in the function right now.

Review on Reviewable

@@ -14,7 +14,16 @@ use std::collections::HashSet;
use file_loader;
use flate2::read::{DeflateDecoder, GzDecoder};
use hyper::client::Request;
use hyper::header::{AcceptEncoding, Accept, ContentLength, ContentType, Host, Location, qitem, Quality, QualityItem};
use hyper::header::StrictTransportSecurity;

This comment has been minimized.

@jdm

jdm Jul 24, 2015

Member

I'd rather use the form:

use hyper::header::{Some, Headers, Here};
use hyper::header::HeaderThatDoesNotFitOnPreviousLine;
@pcwalton
Copy link
Contributor

pcwalton commented Jul 25, 2015

I'd like to suggest that the resource task e10s lands first, as this affects messages in tasks that have pending e10s PRs.

@jdm
Copy link
Member

jdm commented Jul 27, 2015

-S-awaiting-review


Reviewed 3 of 3 files at r1, 1 of 1 files at r2.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@jdm jdm removed the S-awaiting-review label Jul 27, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Jul 28, 2015

The latest upstream changes (presumably #6774) made this pull request unmergeable. Please resolve the merge conflicts.

@metajack
Copy link
Contributor

metajack commented Jul 29, 2015

Waiting on #6586.

@jdm
Copy link
Member

jdm commented Aug 1, 2015

We're all good now! This can merge as soon as it's rebased :)

@samfoo samfoo force-pushed the samfoo:sts-headers branch from baa1d48 to 0d94ee9 Aug 2, 2015
@jdm
Copy link
Member

jdm commented Aug 2, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Aug 2, 2015

📌 Commit 0d94ee9 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Aug 2, 2015

Testing commit 0d94ee9 with merge f1c26c5...

bors-servo pushed a commit that referenced this pull request Aug 2, 2015
Obey Strict-Transport-Security header

Resolves #6703.

Done:

* [x] When STS headers received, add the host to the HSTS list

Todo:

* [ ] Persist the in-memory list so that it's reloaded on the next browser boot
* [ ] Add tests to `http_loader::load` - it's pretty well completely untested right now, but it's a bit gnarly to untangle and without mocking, it's hard to deal with the dependency on making a real network request. Writing a mock request object should be doable for testing, but there's a lot going on in the function right now.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6722)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 2, 2015

💔 Test failed - linux3

@jdm
Copy link
Member

jdm commented Aug 2, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Aug 2, 2015

Previous build results for android, gonk, linux1, linux2, mac1, mac2, mac3 are reusable. Rebuilding only linux3...

@bors-servo
Copy link
Contributor

bors-servo commented Aug 2, 2015

☀️ Test successful - android, gonk, linux1, linux2, linux3, mac1, mac2, mac3

@bors-servo bors-servo merged commit 0d94ee9 into servo:master Aug 2, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.