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

todo: improve security of HTTP headers #591

Closed
iuioiua opened this issue Sep 14, 2023 · 10 comments
Closed

todo: improve security of HTTP headers #591

iuioiua opened this issue Sep 14, 2023 · 10 comments
Assignees
Labels

Comments

@iuioiua
Copy link
Contributor

iuioiua commented Sep 14, 2023

HTTP headers have much room for improvement, in terms of security. This should be fixed by a plugin with reasonable settings. Report: https://observatory.mozilla.org/analyze/hunt.deno.land

@iuioiua iuioiua changed the title todo: improve security headers todo: improve security of HTTP headers Sep 14, 2023
@Jabolol
Copy link
Contributor

Jabolol commented Sep 16, 2023

I'd like to tackle this issue, can I get it assigned? Thanks!

@Jabolol
Copy link
Contributor

Jabolol commented Sep 16, 2023

The report was at 20/100, and F grade. I've managed to get a 80/100, that is a B+. I've encountered mayor obstacles while trying to implement the last suggestion, the Content-Security-Policy header. Implementing this would achieve a perfect score.

The problem

Content Security Policy (CSP from now on) is a security feature used in web applications to mitigate certain types of attacks, such as cross-site scripting (XSS) and data injection. Mozilla's Observatory requires the values script-src and style-src of the CSP header to not be set as unsafe-inline. Due to the nature of fresh (scripts and styles being injected and inlined at build time), there is no practical way of overcoming this limitation without modifying fresh itself.

Possible solutions

  1. Compute the sha256/sha384 hash at build step for every script tag at the fresh level itself and set it in the integrity, then set the script-src CSP value to strict-dynamic.
  2. Create a middleware handler that checks if the file is internal (through ctx.destination) and a JavaScript file, compute the sha256/sha384 hash on the fly and set it in the CSP header.

Thoughts

For the moment, an 80/100 score is good enough, I suggest to implement one of the aforementioned possible solutions after the v1 release, unless someone comes up with a better solution. The second suggestion is slow for big files, and that would take a considerable toll on lighthouse score, making it inviable.

Here's the Observatory report for my PR (#592): https://observatory.mozilla.org/analyze/saaskit-staging.deno.dev

@iuioiua iuioiua added the v1 label Sep 17, 2023
@marvinhagemeister
Copy link
Collaborator

@Jabolol The CSP issue will be fixed in Fresh itself. Already did so for inline scripts (see denoland/fresh#1772), and inline styles are next on my agenda.

iuioiua added a commit that referenced this issue Sep 22, 2023
Work in progress. See #591 for the relevant discussion.

---------

Co-authored-by: Asher Gomez <ashersaupingomez@gmail.com>
@iuioiua
Copy link
Contributor Author

iuioiua commented Sep 22, 2023

@Jabolol, I've upgraded Fresh to canary (as of writing this comment) in #603, which includes these CSP improvements.

@Jabolol
Copy link
Contributor

Jabolol commented Sep 22, 2023

@Jabolol, I've upgraded Fresh to canary (as of writing this comment) in #603, which includes these CSP improvements.

Cool! I'll create a PR with the CSP header implementation ASAP.

@Jabolol
Copy link
Contributor

Jabolol commented Sep 27, 2023

For the moment I'm a little bit reticent because the only way to implement the CSP header the fresh way is by adding the following code to every route.

export const config: RouteConfig = {
  csp: true,
};

Per se, there is no way to add this in a plugin as of yet. How should we go forward?

@iuioiua
Copy link
Contributor Author

iuioiua commented Sep 27, 2023

I don't understand. Can you elaborate?

@Jabolol
Copy link
Contributor

Jabolol commented Sep 27, 2023

So since we are using plugins mainly for this extra functionality, what makes sense is to enable CSP from the plugin itself.

Nonetheless, this is not feasible as of yet. To enable CSP we have to add the snippet I sent before on every route. I was asking if this should be done as stated or if we should look for another approach.

@iuioiua
Copy link
Contributor Author

iuioiua commented Sep 27, 2023

Oh, yeah, I always thought CSP functionality would come from the plugin we previously created anyway 👍🏾

@iuioiua
Copy link
Contributor Author

iuioiua commented Oct 3, 2023

Oh, my misunderstanding... Looking through Fresh's documentation and codebase, it seems that adding the route config to each route is the only current means of enabling CSP. Perhaps we should add the ability to allow CSP at the middleware level to Fresh itself. Either way, thinking about it further, the current security headers are sufficient for our use case, and CSP may be overkill. I'd be open to it in the future, but for now, it's good enough. Thanks for your help, @Jabolol!

@iuioiua iuioiua closed this as completed Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants