Skip to content

Conversation

@LiamRiddell
Copy link
Contributor

@LiamRiddell LiamRiddell commented Jul 3, 2021

Modified the conf/nginx/st2.conf to disable the X-XSS-Protection functionality in order to align with the OWASP standards. As most browser vendors have deprecated or removed this feature due to the fact it can introduce additional security issues. Please view the following sources:

OWASP:
image

Mozilla Developer Network Web Docs:
image

References:
OWASP - https://owasp.org/www-project-secure-headers/#x-xss-protection
MDN - https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-XSS-Protection

@pull-request-size pull-request-size bot added the size/XS PR that changes 0-9 lines. Quick fix/merge. label Jul 3, 2021
@CLAassistant
Copy link

CLAassistant commented Jul 3, 2021

CLA assistant check
All committers have signed the CLA.

@LiamRiddell LiamRiddell changed the title Changed X-XSS-Protection to follow standards due to deprecation. Changed X-XSS-Protection to follow OWASP standards due to deprecation. Jul 3, 2021
@Kami Kami added the security label Jul 4, 2021
@Kami
Copy link
Member

Kami commented Jul 4, 2021

Seems reasonable to me although it would also be good to check the "legacy browsers" list and ensure it's indeed very old version with very little or no usage in real world.

@LiamRiddell
Copy link
Contributor Author

Agreed, I believe we could potentially use Can I Use? maintained service to get an idea of browser list.
https://caniuse.com/mdn-http_headers_x-xss-protection

@cognifloyd
Copy link
Member

Do we need to replace X-XSS-Protection with CSP? https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP

@cognifloyd
Copy link
Member

@LiamRiddell Would you please add a changelog entry and replace X-XSS-Protection with Content-Security-Policy?

@cognifloyd cognifloyd added status:needs changes status:needs cla A manual tag to note PRs that are otherwise ready, but the CLA bot says the CLA has not been signed. labels Apr 30, 2022
@LiamRiddell
Copy link
Contributor Author

@cognifloyd Hi Jacob, apologies. You want me to add a new changelog entry in the "CHANGELOG.rst" file in the root directory of the project? Best, Liam.

@cognifloyd
Copy link
Member

Correct

@LiamRiddell
Copy link
Contributor Author

Correct

Do you want me to add it under the "in development" change log?

@cognifloyd
Copy link
Member

Yes. Then when we cut a release, that section gets renamed to the version number and we add a new in development section.

@LiamRiddell
Copy link
Contributor Author

Yes. Then when we cut a release, that section gets renamed to the version number and we add a new in development section.

Perfect, please review the changes. Sorry again for the delay!

@LiamRiddell
Copy link
Contributor Author

LiamRiddell commented Jul 19, 2022

Do we need to replace X-XSS-Protection with CSP? https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP

Yes, if possible that is standard practice to mitigate against XSS. However, in some apps it may not be a simple change as the blast radius of blocking resources based on allow-list could cause issues across the app.

@cognifloyd cognifloyd removed the status:needs cla A manual tag to note PRs that are otherwise ready, but the CLA bot says the CLA has not been signed. label Jul 19, 2022
@cognifloyd cognifloyd added this to the 3.8.0 milestone Jul 19, 2022
@cognifloyd cognifloyd requested a review from a team July 19, 2022 17:10
Copy link
Member

@rush-skills rush-skills left a comment

Choose a reason for hiding this comment

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

👍🏼

@cognifloyd
Copy link
Member

cognifloyd commented Jul 19, 2022

I'm not sure why CircleCI isn't running this PR. I'm going to close and reopen to trigger it (hopefully)

@cognifloyd cognifloyd closed this Jul 19, 2022
@cognifloyd cognifloyd reopened this Jul 19, 2022
@cognifloyd cognifloyd merged commit 4dbc4f6 into StackStorm:master Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

nginx security size/XS PR that changes 0-9 lines. Quick fix/merge.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants