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

Add security related headers for static resources #24272

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yhwang
Copy link
Member

@yhwang yhwang commented Dec 17, 2024

Description

Add the following security headers to static resources, including html, js, and css files.

  • Add X-Content-Type-Options header and assign the value to 'nosniff'
  • Add Content-Security-Policy header and proper values

Motivation and Context

fix: #24271

Impact

N/A

Test Plan

Check the response headers for the static resources

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==
Add security-related headers to the static resources serving from the Presto UI, including: ``Content-Security-Policy``,
``X-Content-Type-Options``.
And here are reference docs about these headers `Content-Security-Policy <https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP>`,
`X-Content-Type-Options <https://learn.microsoft.com/en-us/previous-versions/windows/internet-explorer/ie-developer/compatibility/gg622941(v=vs.85)>`

Add the following security headers to static resources, including
`html`, `js`, and `css` files.
- Add `X-Content-Type-Options` header and assign the value
  to 'nosniff'
- Add `Content-Security-Policy` header and proper values
  - default-src: 'self'
  - style-src: 'self' 'unsafe-inline' https://fonts.googleapis.com
  - font-src: self fonts.gstatic.com
  - frame-ancestors: self

Signed-off-by: Yihong Wang <yh.wang@ibm.com>
@yhwang yhwang requested a review from a team as a code owner December 17, 2024 19:52
@yhwang yhwang requested a review from presto-oss December 17, 2024 19:52
@prestodb-ci prestodb-ci added the from:IBM PR from IBM label Dec 17, 2024
@prestodb-ci prestodb-ci requested review from a team, ScrapCodes and aaneja and removed request for a team December 17, 2024 19:52
@yhwang
Copy link
Member Author

yhwang commented Dec 17, 2024

I also added the html5shiv and respond JavaScript files to vendors. Now, they are serving locally, which avoids the cross-domain JavaScript source.

@yhwang
Copy link
Member Author

yhwang commented Dec 17, 2024

highlight the two new headers:
Screenshot 2024-12-17 at 11 56 16 AM

<script src="https://oss.maxcdn.com/html5shiv/3.7.2/html5shiv.min.js"></script>
<script src="https://oss.maxcdn.com/respond/1.4.2/respond.min.js"></script>
<script src="vendor/html5shiv/html5shiv.min.js"></script>
<script src="vendor/respond/respond.min.js"></script>

Choose a reason for hiding this comment

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

Hi @yhwang , thanks for the work. Just wondering why did we vendor these, instead of getting from CDN?

Copy link
Member Author

Choose a reason for hiding this comment

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

good question and the answer is Cross-Domain JavaScript Source File Inclusion.
We already include most of the 3rd party libs except these two. So I directly included them as well. Besides, the script-src attribute of the CSP header has self as the value. I don't want to add oss.maxcdn.com just for these two libs.

Choose a reason for hiding this comment

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

Thanks for clarifying, it seems you are trying to follow the standard here. May be some one more expert can comment on the right path here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
from:IBM PR from IBM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing security related headers for static resources serving from the Presto UI
4 participants