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

Upgrade to Helmet 4 #10

Closed
wants to merge 2 commits into from
Closed

Conversation

SethFalco
Copy link
Sponsor Member

I believe the main reasons this project is stuck using Helmet 3 specifically is because of the following breaking changes:

  1. helmet.noCache() was removed.
  2. Support for a fake powered by string was removed.

Please let me know if there are any other reasons Helmet was held back.

helmet.noCache()

This functionality isn't going away. It's just moving to a separate module, nocache. You can find it on npm and on GitHub.
- helmetjs/helmet#215

I've added the dependency nocache to the project, the syntax has just changed a little but this can still be achieved with Helmet 4.

X-Powered-By

This relates to the following issue and pull request.
Issue: freeCodeCamp/freeCodeCamp#40476
Pull Request: freeCodeCamp/freeCodeCamp#40900

helmet.hidePoweredBy removes the X-Powered-By header, which is set by default in some frameworks (like Express). Removing the header offers very limited security benefits (see this discussion) and is mostly removed to save bandwidth.
- https://helmetjs.github.io/

On the same page, it's recommended not to use app.use(helmet.hidePoweredBy()) and instead opt for app.disable("x-powered-by"), anyway.

I updated the test to only verify that the x-powered-by header isn't equal to Express, this logic is stolen from a challenge before the projects.

https://github.com/freeCodeCamp/freeCodeCamp/blob/main/curriculum/challenges/english/09-information-security/information-security-with-helmetjs/hide-potentially-dangerous-information-using-helmet.hidepoweredby.md#--hints--

@SethFalco
Copy link
Sponsor Member Author

SethFalco commented Mar 19, 2021

Converting this to a draft, I thought I had tested this properly, but seems I made a mistake.
app.use(helmet.xssFilter()); now disables XSS filtering. This results in the x-xss-protection header being set to 0.

Related:
helmetjs/helmet#230
expressjs/expressjs.com#1248

Although these protections are largely unnecessary in modern browsers when sites implement a strong Content-Security-Policy that disables the use of inline JavaScript ('unsafe-inline'), they can still provide protections for users of older web browsers that don't yet support CSP.

  • Chrome has removed their XSS Auditor
  • Firefox has not, and will not implement X-XSS-Protection
  • Edge has retired their XSS filter

This means that if you do not need to support legacy browsers, it is recommended that you use Content-Security-Policy without allowing unsafe-inline scripts instead.
- https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-XSS-Protection

I think this test should be replaced with using an appropriate Content Security Policy instead that doesn't allow unsafe-inline in JavaScript at least. I'll update this PR later.

@gikf
Copy link
Member

gikf commented Apr 7, 2021

From my understanding changing this project to work on Helmet 4, would also require updating learning parts of curriculum related to Helmet.

@SethFalco SethFalco marked this pull request as ready for review April 21, 2021 14:30
Copy link
Member

@huyenltnguyen huyenltnguyen left a comment

Choose a reason for hiding this comment

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

Hey @SethFalco, thank you for the PR!

Since this is a major change and we are still discussing on it in freeCodeCamp/freeCodeCamp#41527, I will have to temporarily block this one to avoid accidental merge.

@raisedadead
Copy link
Member

Closing as discussed in the parent PR on the main repo.

@raisedadead raisedadead closed this May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants