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

Removes inline scripts and inline styles to make it compatible with the newly added security headers #4369

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

ubaskota
Copy link

@ubaskota ubaskota commented Dec 5, 2024

Due to the recently added Content Security Policy(CSP), all inline scripts are automatically blocked. In order to fix this, we had to remove inline scripts and styles, and keep them in separate files. This change performs all necessary edits to remove the inline scripts and keep them in separate files.

Copy link
Contributor

@jonathan343 jonathan343 left a comment

Choose a reason for hiding this comment

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

Hey @ubaskota, thanks for opening this PR!

This is a great start! I've left some comments mainly related to improving maintainability.

The main concern I have is with the docs/source/_templates/head_css_variables.html file. I don't understand how it works and a bit skeptical of the change.

Copy link
Contributor

Choose a reason for hiding this comment

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

This file seems to be a port of the partials/icons.html file in furo. Is there a reason why this is being added to layout.html? Furo also has a layout.html file with different contents.

</script>
{% include "partials/icons.html" %}
<svg xmlns="http://www.w3.org/2000/svg" style="display: none;">
{% include 'layout.html' %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the discussion in the related comment above, we'll need to update this to {% include "partials/icons.html" %}

{% include "partials/icons.html" %}
<svg xmlns="http://www.w3.org/2000/svg" style="display: none;">
{% include 'layout.html' %}
<svg xmlns="http://www.w3.org/2000/svg" class="hidden-svg">
Copy link
Contributor

Choose a reason for hiding this comment

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

We should choose a more generic name than hidden-svg, maybe instead use hidden. If we ever decide to apply this to another element that isn't an SVG, this class wouldn't make sense.

@@ -103,3 +103,7 @@ h3.admonition-title::before {
mask-image: var(--icon-admonition-default);
mask-repeat: no-repeat;
}
/* Apply SVG from inline page.html */
.hidden-svg {
Copy link
Contributor

Choose a reason for hiding this comment

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

See comment below about renaming this to something more generic.

if (typeof AWSCShortbread !== 'undefined') {
const shortbread = AWSCShortbread({
domain: ".amazonaws.com"
// domain: ".cloudfront.net"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit - remove this comment


document.body.dataset.theme = localStorage.getItem("theme") || "auto";

if (typeof AWSCShortbread !== 'undefined') {
Copy link
Contributor

Choose a reason for hiding this comment

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

In what cases would this condition fail?

@@ -0,0 +1,61 @@
{# Adapted from Just the Docs #}
<svg xmlns="http://www.w3.org/2000/svg" class="hidden-svg">
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to class="hidden" if you decide to change the class name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain how the changes to this file work, it doesn't make sense to me? Where is the declare_css_variables called?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we should add this file to a new partials directory to mimic the structure of furo (as previously mentioned, icons.html will also be added to this directory. The name should also be changed to _head_css_variables.html (currently missing an underscore).

Copy link
Contributor

Choose a reason for hiding this comment

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

The only reason I see for porting over base.html from furo is to add extra content to the <head> element.
We can prevent having to port over this entire file and instead add the following to page.html.

{%- block extrahead %}
<script src="https://prod.assets.shortbread.aws.dev/shortbread.js"></script>
<link href="https://prod.assets.shortbread.aws.dev/shortbread.css" rel="stylesheet">
<script src="https://a0.awsstatic.com/s_code/js/3.0/awshome_s_code.js"></script>
{% endblock %}

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

Successfully merging this pull request may close these issues.

2 participants