-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Docs/security in gatsby #22395
Docs/security in gatsby #22395
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a really good start. Thanks for getting this together.
Should I write about GDPR rules in this doc?
I don't think GDPR makes sense for this doc, but could be another doc if the team things it's a good idea.
In Third-party Scripts section I thought about recommending Ackee, which is a open-source self-hosted analytics alternative. Should I?
Again, doesn't seem relevant to security. maybe a privacy/GDPR doc.
Is CSRF section relevant even when the solutions are backend only?
It's still possible for ffolks to add client side things to their gatsby sites, so yes, it's good!
Finally, I think there should probably be a section on key security. for things like APIs. If you need a place to start my blog post on security focused on that.
docs/docs/security-in-gatsby.md
Outdated
|
||
In other hand, there could be some fields in your application that you will need to render the inner HTML tags, such as a content field in a blog or a comment in a comments section, that are built in rich-text editors. | ||
|
||
That's when you expose your application to XSS attacks, since the only way to render these HTML tags in React is by using the `dangerouslySetInnerHTML` prop: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't entirely true. there are several other packages that do this. html-to-react and html-react-parser are the two I've used. I think these both sanitize on the client side, but not on SSR (but don't quote me on that...). They offer a lot of additional features too but those are not inherently related to security. I'd include this information some how so folks no it's an option...I'll leave it to you exactly how this should be done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. There are alternatives to dangerouslySetInnerHTML
but they don't sanitize HTML. see remarkablemark/html-react-parser#94 and aknuds1/html-to-react#79.
I reformulate the phrase to mention the html-to-react package, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd at least mention them. I'm fairly sure html-react-parser doesn't do script tags on the client side (I ran into this on a project). one could also use the custom parsing functionality in these to sanitize html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned html-react-parser! Please check again and see that if it is good.
Configure the created cookie in a way that it could be only sent by the same domain. [Most moderns browsers support it with a backward compatibility](https://caniuse.com/#feat=same-site-cookie-attribute). | ||
|
||
### Anti-CSRF Tokens | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe needs an example. not totally understanding how/why this works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, totally. I add a more clear example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is still outstanding?
docs/docs/security-in-gatsby.md
Outdated
|
||
## Third-party Scripts | ||
|
||
Some third-party scripts like Google Tag Manager gives the possibility to add arbitrary JavaScript code on top of them. Be sure to control the access of these services. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definitely needs some explanation. How do I control access, to the third-party scripts, or to GTM?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely. I expanded with 2 docs links to Google
docs/docs/security-in-gatsby.md
Outdated
|
||
## Check Your Dependencies | ||
|
||
In your Gatsby project, you are going to have a lot of dependencies (which has their own dependencies as well) in your `node_modules/`. Therefore, it is important to check if any of them has a security issue. NPM make this possible since v6 by executing the `npm audit` in the command line. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add the yarn equivalent. and may be worth noting the yarn v1 and v2 version if they differ.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yarn v2 does not support audit
yet. I changed the writing to talk about yarn as well.
docs/docs/security-in-gatsby.md
Outdated
Content Security Policy is a security layer added in web applications to detect and prevent attacks, e.g. the XSS attack mentioned above. | ||
|
||
To add it to your Gatsby website, add the [plugin](https://www.gatsbyjs.org/packages/gatsby-plugin-csp/) to your `gatsby-config.js` with the desired configuration. Note that | ||
currently there is a [incompatibility issue](https://github.com/gatsbyjs/gatsby/issues/10890) with [gatsby-image](https://www.gatsbyjs.org/packages/gatsby-image) and other plugins. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expound on "other plugins"...I understand it's unreasonable to list every incompatible plugin, but please define why or what makes a plugin incompatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing! I wrote the reason after that, which is more detailed in the issue.
Co-Authored-By: Alex Moon <[email protected]>
Co-Authored-By: Alex Moon <[email protected]>
Co-Authored-By: Alex Moon <[email protected]>
docs/docs/security-in-gatsby.md
Outdated
|
||
## Cross-Site Request Forgery (CSRF) | ||
|
||
A web application that use cookies could be attacked by the CSRF exploit, which deceives the browser to execute actions by the user's name without notice. By default, the browser "trusts" all the activity made validating the user's identity and therefore sending the associated cookies in every request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be re written it's pretty confusing. https://en.wikipedia.org/wiki/Cross-site_request_forgery has some good explanations you can quote from. If you do choose to fix what you have I think it would still be helpful to link out to a more complete resource like Wikipedia for those interested in diving deeper than the quick overviews we're providing here (this applies to XSS as well).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In these past few days, I'm exploring more in the topic in CS253 - Web Security classes from Stanford. I'll rewrite this section, making it simple, doing some quotes and linking these resources.
docs/docs/security-in-gatsby.md
Outdated
</form> | ||
``` | ||
|
||
**How can it be prevented?** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You "copy the site" example above could be fixed by simply scoping your cookies to a certain domain. Make sure you info is accurate (I'm no expert here either) and complete (aka. solve every problem you present, and do so correctly). Again, I think since security is such a deep and nuanced topic it'll be to your benefit to keep these short and simple then link out to more complete resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on further thought...Based on how this info is presented Tokens seem to be "THE" solution. I'm guessing they are just one, of many solutions. Present it as such, "On way CSRF attacks can be mitigated..., .... To read more on this topic check out....", then link out to more in depth resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sry I just realized you do have the Same-site cookies example...Fix you headings please The "How Can it be prevented" should probably be a ### header and then each example be #### header. Optionally, Instead of presenting several problems then several solutions, Give one problem, and its solution, then another and its solution...etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, sorry about that! I think that is good we are learning things while doing this. I'm gonna rewrite this section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to be sorry. you're doing great work, just trying to give helpful feedback. thanks and keep it up. I've already learned a thing or two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are!
docs/docs/security-in-gatsby.md
Outdated
} | ||
``` | ||
|
||
Note: Whether you need to authenticate someone in your application, Gatsby has an [Authentication Tutorial](/tutorial/authentication-tutorial) which helps doing this job in a secure way. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This covers build variables well but not client side keys. Not sure if the note is intended to cover it cause the language is a bit confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this note does not cover this part. What do you think it should be here regarding client-side keys? encryption? known methods, e.g. JWT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this doc should be a starting point. so defining the issue and maybe an example of how to solve then (again) links out to more thorough resources (like Gatsby's docs on keys and env variables).
Co-Authored-By: Alex Moon <[email protected]>
Co-Authored-By: Alex Moon <[email protected]>
Co-Authored-By: Alex Moon <[email protected]>
Co-Authored-By: Alex Moon <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work here! Really great start. I left some suggestions surrounding formatting and wording.
Co-Authored-By: LB <[email protected]>
Co-Authored-By: LB <[email protected]>
Co-Authored-By: LB <[email protected]>
Co-Authored-By: LB <[email protected]>
Co-Authored-By: LB <[email protected]>
Co-Authored-By: LB <[email protected]>
@luizcieslak I know you're incorporating some different feedback here so please let me know when you're ready for another review! Thanks again for the amazing work. |
…cs/security-in-gatsby
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is coming along really well! Some small edits and a couple bigger questions. But I'd say we should be able to get this in shortly.
Configure the created cookie in a way that it could be only sent by the same domain. [Most moderns browsers support it with a backward compatibility](https://caniuse.com/#feat=same-site-cookie-attribute). | ||
|
||
### Anti-CSRF Tokens | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is still outstanding?
Co-Authored-By: LB <[email protected]>
Co-Authored-By: LB <[email protected]>
Thank you so much for your work on this! I think we're good to merge this. 🥳 |
I thank you @laurieontech and @moonmeister for the patience and review! I learned a lot 🤗 |
Hi! I had some question while doing this writing and I'd like a feedback:
Third-party Scripts
section I thought about recommending Ackee, which is a open-source self-hosted analytics alternative. Should I?Also, I did not read Alex Moon's blog post entirely yet to not become biased. If there is some topics that you see important to have here please let me know.
Description
A documentation page for Security in Gatsby :)
Documentation
It is a documentation itself.
Related Issues
closes #13305