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

Adding space_helmet to contrib, take 2. #765

Closed
wants to merge 3 commits into from
Closed

Conversation

talg
Copy link
Contributor

@talg talg commented Sep 5, 2018

No description provided.

@SergioBenitez
Copy link
Member

Lots of errors in the build.

@talg
Copy link
Contributor Author

talg commented Sep 6, 2018

fixed breaking doc tests.

@SergioBenitez
Copy link
Member

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 ! or a /) and the first character.

@@ -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>"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A new author shouldn't be added since this is reflected by crates.io.

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 = []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be under "Internal use only."

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 = []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm leaning towards calling this feature security_headers. I worry that space_helmet will make this undiscoverable.

@@ -1,6 +1,7 @@
#![feature(crate_visibility_modifier)]
#![feature(never_type)]
#![feature(doc_cfg)]
#![feature(extern_prelude)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here with the .into() and Cow.

macro_rules! check_header {
($response:ident, $header_name:expr, $header_param:expr) => {
match $response.headers().get_one($header_name) {
Some(str) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty line above here.

@@ -0,0 +1,32 @@
#![feature(decl_macro)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be merged with the next.

@luciusmagn
Copy link

Generally, if you require the extern_prelude feature, which is only a bundle of several QoL changes regarding module paths and visibility modifiers (see rust-lang/rfcs#2126), it usually means that you forgot to add extern crate for one of your dependencies or have otherwise violated the current rules regarding imports and paths, so I suggest checking that, finding the issue and getting rid of the feature.

@SergioBenitez
Copy link
Member

SergioBenitez commented Sep 29, 2018

Please see the build failure. Also, please rebase on master.

@SergioBenitez
Copy link
Member

Merged in c5167f1. Thanks so much. :)

@SergioBenitez SergioBenitez added the pr: merged This pull request was merged manually. label Nov 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: merged This pull request was merged manually.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants