Add improved security headers to FastAPI responses #1355
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Tynan DeBold <thdebold@gmail.com>
Signed-off-by: Tynan DeBold <thdebold@gmail.com>
Signed-off-by: Tynan DeBold <thdebold@gmail.com>
…ore/add-secure-response-headers-to-fastapi-app Signed-off-by: Tynan DeBold <thdebold@gmail.com>
Signed-off-by: Tynan DeBold <thdebold@gmail.com>
rashidakanchwala
approved these changes
May 12, 2023
There was a problem hiding this comment.
I don't have enough knowledge abt security headers but the code looks fine, and I get those extra stuff in the response headers. thanks @tynandebold :)
antonymilne
approved these changes
May 15, 2023
There was a problem hiding this comment.
I also have minimal understanding of this, but looks good as far as I understand it.
Just one thing: should this also be applied to create_api_app_from_file? That is what runs if you do kedro-viz --load-file. If yes, then better to put this in _create_base_api_app instead.
Signed-off-by: Tynan DeBold <thdebold@gmail.com>
5 tasks
rashidakanchwala
added a commit
that referenced
this pull request
Dec 5, 2023
In response to issue #1348, which required the addition of security headers to demo.kedro.org, we implemented a solution in PR #1355. This solution involved adding security headers to the FastAPI application which results in all instances of kedro-viz, whether hosted or local, having these security headers. Having the security headers introduced a limitation where kedro-viz could not be used as an IFrame, affecting functionalities like %run_viz that embed kedro-viz in an iframe. To address this, the current ticket introduces a conditional approach. We will add security headers only if the environment variable ADD_SECURITY_HEADER is set to true. This modification will be implemented in the Dockerfile when creating the docker image for the demo project. This image will then be uploaded to an EC2 instance and deployed using Lightsail.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Resolves #1348.
Development notes
I've added and am using the secure package to add the necessary security headers infosec requested, those headers being:
Strict-Transport-SecurityX-Frame-OptionsQA notes
You need to run the app locally or with Gitpod and view the main server response.
This is before the change:
This is after:
Checklist
RELEASE.mdfile