ring-clojure / ring-defaults

A library to provide sensible Ring middleware defaults
MIT License
345 stars 32 forks source link

Disable the X-XSS-Protection header in defaults #42

Closed terop closed 2 years ago

terop commented 2 years ago

Disable the XSS Auditor in older browsers by default. The X-XSS-Protection header has been deprecated by modern browsers due to security issues it introduces on the client.

See: https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html#x-xss-protection-header

Fixes #35.

atomist[bot] commented 2 years ago

Commit messages of this repository should follow the seven rules of a great Git commit message, as mentioned in the project's contributing guidelines. It looks like there's a few issues with the commit messages in this pull request:

terop commented 2 years ago

I resurrected the change from PR #36 with the requested commit message change. Because I cannot updated the existing the PR I created a new one instead.

weavejester commented 2 years ago

Thanks for the update. However, now that I take another look at the code of this PR, there are parts that confuse me. If the X-XSS-Protection header is deprecated, why is it still set to be added to the response? In addition, the link in the commit is now broken.

Let's instead just remove the option from site-defaults, and add a deprecation notice to the README. Let's reference this URL for the reason for the change in the commit message: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-XSS-Protection

atomist[bot] commented 2 years ago

Commit messages of this repository should follow the seven rules of a great Git commit message, as mentioned in the project's contributing guidelines. It looks like there's a few issues with the commit messages in this pull request:

terop commented 2 years ago

I tried to do the changes you suggested but I am not sure if they are correct and sufficient.

weavejester commented 2 years ago

The changes look good, and were exactly what I was thinking of. I'm not sure what the original PR's change to lines 80-82 was about. What happens if we revert those alterations?

atomist[bot] commented 2 years ago

Commit messages of this repository should follow the seven rules of a great Git commit message, as mentioned in the project's contributing guidelines. It looks like there's a few issues with the commit messages in this pull request:

terop commented 2 years ago

Code changed as suggested, unit tests pass at least.

terop commented 2 years ago

@weavejester could you kindly check the latest changes?

atomist[bot] commented 2 years ago

Commit messages of this repository should follow the seven rules of a great Git commit message, as mentioned in the project's contributing guidelines. It looks like there's a few issues with the commit messages in this pull request:

terop commented 2 years ago

@weavejester could you again kindly take a look at the latest changes?

atomist[bot] commented 2 years ago

Commit messages of this repository should follow the seven rules of a great Git commit message, as mentioned in the project's contributing guidelines. It looks like there's a few issues with the commit messages in this pull request:

weavejester commented 2 years ago

Perfect! Thank you.

terop commented 2 years ago

@weavejester would it be possible to soon have a new release of ring-defaults with this change?

weavejester commented 2 years ago

Done. Apologies for the delay.

terop commented 2 years ago

No problem, many thanks for the release.