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

Support preload. #81

Merged
merged 3 commits into from Oct 16, 2014

Conversation

Projects
None yet
5 participants
@analytically
Contributor

analytically commented Oct 16, 2014

No description provided.

@analytically

This comment has been minimized.

Show comment
Hide comment
@kristerkari

This comment has been minimized.

Show comment
Hide comment
@kristerkari

kristerkari Oct 16, 2014

I'd like to see this feature 👍

kristerkari commented Oct 16, 2014

I'd like to see this feature 👍

@kevin-buttercoin

This comment has been minimized.

Show comment
Hide comment
@kevin-buttercoin

kevin-buttercoin commented Oct 16, 2014

👍

@evilpacket

This comment has been minimized.

Show comment
Hide comment
@evilpacket

evilpacket Oct 16, 2014

@EvanHahn I'll let you pull the trigger as I haven't been in the codebase for so long :) Certainly is a good feature.

evilpacket commented Oct 16, 2014

@EvanHahn I'll let you pull the trigger as I haven't been in the codebase for so long :) Certainly is a good feature.

@EvanHahn

This comment has been minimized.

Show comment
Hide comment
@EvanHahn

EvanHahn Oct 16, 2014

Member

Super busy today, but I'll take a look as soon as I can!

Member

EvanHahn commented Oct 16, 2014

Super busy today, but I'll take a look as soon as I can!

@EvanHahn

This comment has been minimized.

Show comment
Hide comment
@EvanHahn

EvanHahn Oct 16, 2014

Member

Looks good, but needs "unhappy path" stuff, in case users mess up. The HSTS preload submission page has some requirements we should check:

  • maxage must be greater than 10886400 seconds
  • includeSubdomains must be present

We either need to throw errors on bad input:

if (options.preload) {
  if (maxAgeMS < 10886400000) {
    throw new Error('HSTS max-age must be at least 18 weeks if using preload');
  } else if (!options.includeSubdomains) {
    throw new Error('HSTS must include subdomains if using preload');
  }
  header += '; preload';
}

Or turn bad input into good input:

if (options.includeSubdomains || options.preload) {
  header += '; includeSubdomains';
}
if (options.preload) {
  maxAgeMS = Math.min(maxAgeMS, 10886400000);
  header += '; preload';
}

We should also add tests for the unhappy path.

Other than that, looks great!

Member

EvanHahn commented Oct 16, 2014

Looks good, but needs "unhappy path" stuff, in case users mess up. The HSTS preload submission page has some requirements we should check:

  • maxage must be greater than 10886400 seconds
  • includeSubdomains must be present

We either need to throw errors on bad input:

if (options.preload) {
  if (maxAgeMS < 10886400000) {
    throw new Error('HSTS max-age must be at least 18 weeks if using preload');
  } else if (!options.includeSubdomains) {
    throw new Error('HSTS must include subdomains if using preload');
  }
  header += '; preload';
}

Or turn bad input into good input:

if (options.includeSubdomains || options.preload) {
  header += '; includeSubdomains';
}
if (options.preload) {
  maxAgeMS = Math.min(maxAgeMS, 10886400000);
  header += '; preload';
}

We should also add tests for the unhappy path.

Other than that, looks great!

@analytically

This comment has been minimized.

Show comment
Hide comment
@analytically

analytically Oct 16, 2014

Contributor

I can see your point, but these are only the requirements from the Google Chrome team. Other teams (eg. Mozilla) might come up with their own requirements. So I'd rather not include any of these checks, and just allow it to fail by manual review.

Contributor

analytically commented Oct 16, 2014

I can see your point, but these are only the requirements from the Google Chrome team. Other teams (eg. Mozilla) might come up with their own requirements. So I'd rather not include any of these checks, and just allow it to fail by manual review.

@EvanHahn

This comment has been minimized.

Show comment
Hide comment
@EvanHahn

EvanHahn Oct 16, 2014

Member

Good point. I'll merge this in, make a few small tweaks to the README, and then push out 0.4.2! Feel free to add yourself as a contributor in package.json.

Member

EvanHahn commented Oct 16, 2014

Good point. I'll merge this in, make a few small tweaks to the README, and then push out 0.4.2! Feel free to add yourself as a contributor in package.json.

EvanHahn added a commit that referenced this pull request Oct 16, 2014

@EvanHahn EvanHahn merged commit 9bcae9a into helmetjs:master Oct 16, 2014

@EvanHahn EvanHahn referenced this pull request Oct 16, 2014

Closed

Support preload in hsts #80

@EvanHahn

This comment has been minimized.

Show comment
Hide comment
@EvanHahn

EvanHahn Aug 5, 2016

Member

@analytically I'm making a new website for Helmet.js and I want to credit everyone who's contributed. Do you have a name and/or website you'd like me to use to credit you?

Member

EvanHahn commented Aug 5, 2016

@analytically I'm making a new website for Helmet.js and I want to credit everyone who's contributed. Do you have a name and/or website you'd like me to use to credit you?

@analytically

This comment has been minimized.

Show comment
Hide comment
@analytically

analytically Aug 5, 2016

Contributor

Just Mathias Bogaert - https://github.com/analytically, thanks!

Contributor

analytically commented Aug 5, 2016

Just Mathias Bogaert - https://github.com/analytically, thanks!

@EvanHahn EvanHahn referenced this pull request Aug 5, 2016

Closed

Add list of contributors #1

@EvanHahn

This comment has been minimized.

Show comment
Hide comment
@EvanHahn

EvanHahn Aug 5, 2016

Member

Added you to the list! Stay tuned for the website.

Member

EvanHahn commented Aug 5, 2016

Added you to the list! Stay tuned for the website.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment