-
Notifications
You must be signed in to change notification settings - Fork 252
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
Update Clear-Site-Data to use the current spec format. #334
Conversation
@@ -1,3 +1,7 @@ | |||
## 3.6.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doh..need to bump the actual version too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or do you all prefer to just suck in the changes and do the version bumping yourself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or do you all prefer to just suck in the changes and do the version bumping yourself?
If we weren't on the same team I'd probably complain, it's 🆒 😎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok..I bumped the gem version number in d2070ea.
@@ -18,7 +18,7 @@ The gem will automatically apply several headers that are related to security. | |||
- X-Permitted-Cross-Domain-Policies - [Restrict Adobe Flash Player's access to data](https://www.adobe.com/devnet/adobe-media-server/articles/cross-domain-xml-for-streaming.html) | |||
- Referrer-Policy - [Referrer Policy draft](https://w3c.github.io/webappsec-referrer-policy/) | |||
- Public Key Pinning - Pin certificate fingerprints in the browser to prevent man-in-the-middle attacks due to compromised Certificate Authorities. [Public Key Pinning Specification](https://tools.ietf.org/html/rfc7469) | |||
- Clear-Site-Data - Clearing browser data for origin. [Clear-Site-Data specification](https://www.w3.org/TR/clear-site-data/). | |||
- Clear-Site-Data - Clearing browser data for origin. [Clear-Site-Data specification](https://w3c.github.io/webappsec-clear-site-data/). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated this, since it is the URL that Mike West referenced. The current URL seems to have not been updated recently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Linking to the editor's draft is generally the right thing to do. The document on w3,org
is practically always out of date. :)
def normalize_json(json) | ||
JSON.dump(JSON.parse(json)) | ||
describe "make_header_value" do | ||
it "returns a string of quoted values that are comma separated" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whitespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doh..darn you sublime whitespace auto-detect.
else | ||
raise ClearSiteDataConfigError.new("config must be an Array of Strings or `true`") | ||
end | ||
end | ||
|
||
# Public: Transform a Clear-Site-Data config (an Array of String) into a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an Array of String
Strings
] | ||
} | ||
HERE | ||
expect(value).to eq(ClearSiteData.make_header_value(["foo", "bar"])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer literals in the eq
. It feels like you're comparing the output of the same function to itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah..debated that myself and laziness won out 😄. I'll change it.
describe "make_header_value" do | ||
it "returns a string of quoted values that are comma separated" do | ||
value = described_class.make_header_value(["foo", "bar"]) | ||
expect(value).to eq(%("foo", "bar")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
I threw this on the 3.x branch since technically master is prepping for 4.x and there have been commits dropping support for ruby versions which would require a major version bump. We still have some TODOs so we'll ship another 3.x and I'll merge these changes in to master as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙇
Released in 3.6.5: https://rubygems.org/gems/secure_headers/versions/3.6.5 |
Per request from @mikewest via twitter, this PR updates the format used in the
Clear-Site-Data
header to the current format./cc @oreoshake for review