Adding space_helmet to contrib, take 2. #765
Conversation
|
Lots of errors in the build. |
|
fixed breaking doc tests. |
|
Docstrings are still incorrectly stylized. They should be word-wrapped at 80 columns, readable without actually rendering to markdown (this especially applies to tables), and a space should be between the last docstring delimiter (a |
contrib/lib/Cargo.toml
Outdated
| @@ -1,7 +1,7 @@ | |||
| [package] | |||
| name = "rocket_contrib" | |||
| version = "0.4.0-dev" | |||
| authors = ["Sergio Benitez <sb@sergio.bz>"] | |||
| authors = ["Sergio Benitez <sb@sergio.bz>", "Tal Garfinkel <talg@cs.stanford.edu>"] | |||
There was a problem hiding this comment.
A new author shouldn't be added since this is reflected by crates.io.
contrib/lib/Cargo.toml
Outdated
| json = ["serde", "serde_json"] | ||
| msgpack = ["serde", "rmp-serde"] | ||
| tera_templates = ["tera", "templates"] | ||
| handlebars_templates = ["handlebars", "templates"] | ||
| static_files = [] | ||
| # Internal use only. | ||
| templates = ["serde", "serde_json", "glob"] | ||
| space_helmet = [] |
There was a problem hiding this comment.
This shouldn't be under "Internal use only."
contrib/lib/Cargo.toml
Outdated
| json = ["serde", "serde_json"] | ||
| msgpack = ["serde", "rmp-serde"] | ||
| tera_templates = ["tera", "templates"] | ||
| handlebars_templates = ["handlebars", "templates"] | ||
| static_files = [] | ||
| # Internal use only. | ||
| templates = ["serde", "serde_json", "glob"] | ||
| space_helmet = [] |
There was a problem hiding this comment.
I'm leaning towards calling this feature security_headers. I worry that space_helmet will make this undiscoverable.
contrib/lib/src/lib.rs
Outdated
| @@ -1,6 +1,7 @@ | |||
| #![feature(crate_visibility_modifier)] | |||
| #![feature(never_type)] | |||
| #![feature(doc_cfg)] | |||
| #![feature(extern_prelude)] | |||
There was a problem hiding this comment.
Why do we need this feature? We shouldn't.
| @@ -89,6 +91,9 @@ pub use uuid::{Uuid, UuidParseError}; | |||
| #[cfg(feature = "static_files")] | |||
| pub mod static_files; | |||
|
|
|||
| #[cfg(feature = "space_helmet")] | |||
| pub mod space_helmet; | |||
There was a problem hiding this comment.
Same with the module names; perhaps security_headers is the right way to go.
| let policy_string: Cow<'static, str> = match policy { | ||
| HSTSPolicy::Enable(max_age) => format!("max-age={}", max_age).into(), | ||
| HSTSPolicy::IncludeSubDomains(max_age) => { | ||
| format!("max-age={} ; includeSubDomains", max_age).into() |
There was a problem hiding this comment.
Is the space before ; required? If not, kill it.
|
|
||
| impl<'a, 'b> From<&'a HSTSPolicy> for Header<'b> { | ||
| fn from(policy: &HSTSPolicy) -> Header<'b> { | ||
| let policy_string: Cow<'static, str> = match policy { |
There was a problem hiding this comment.
Same here with the .into() and Cow.
contrib/lib/tests/space_helmet.rs
Outdated
| macro_rules! check_header { | ||
| ($response:ident, $header_name:expr, $header_param:expr) => { | ||
| match $response.headers().get_one($header_name) { | ||
| Some(str) => { |
There was a problem hiding this comment.
Don't name the variable str, since that's a type name. I like using string. Also, there's no need for the { } block here.
| check_header!(response, "X-Frame-Options", "SAMEORIGIN"); | ||
| check_header!(response, "X-Content-Type-Options", "nosniff"); | ||
| } | ||
| #[test] |
| @@ -0,0 +1,32 @@ | |||
| #![feature(decl_macro)] | |||
There was a problem hiding this comment.
This should be merged with the next.
|
Generally, if you require the |
|
Please see the build failure. Also, please rebase on master. |
|
Merged in c5167f1. Thanks so much. :) |
No description provided.