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.
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
feat: Enable CSP with nonce for Helix 5 #773
base: main
Are you sure you want to change the base?
feat: Enable CSP with nonce for Helix 5 #773
Changes from 1 commit
de852c9
2489e05
fdb2aeb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if we really should rewrite the provided HTML. and if so, with unified which might alter the html. maybe using a very simple text based parser that only understands minimal HTML and repaces the
nonce
tokens would less intrusive.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used unified because that what was used in processing the
head.html
, which allowed me to keep the same codebase for processing both the document based and the static html and I like how it works with selectors.Indeed, it does alter the resulting HTML, it makes it canonical (e.g. from the point of view of spaces, indentation,
<SCRIPT>
-><script>
, etc.).Would you have any in mind that I could try out? Intuitively, I would expect that any parser when serialising back to canonicalise, since otherwise it would be very hard to store all the nuances of the original HTML.
If we want to keep the customer's HTML untouched except for the nonce generation, we could try a regex approach.
I usually avoid regexes for this kind of processing, because I'm not good at them, they become hard to maintain troubleshoot and I've seen Kodiak complain about ReDos, which means they could be slow for certain files.
Did a quick try with
I can't speak for all customers, but personally, as a developer, I think I would like that if I drop some messy HTML in github that I copied from somewhere I get a clean one when looking through the delivery service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tripodsan WDYT of the latest approach from fdb2aeb?
I found a nice parser from
parse5
which allows working with structured information for taking decisions, while still making it possible to make the final transformations directly on the raw html string, so we preserve (almost?) everything that the customer has (indentation, spacing, casing, etc.)Additionally, if I understand correctly, the
parse5
ecosystem sits at the lower level ofrehype
(not sure if this specific class that I used), which hopefully means that there should be no compatibility issues when running this JS in cloudflare.Here is an example where it preserves everything and doesn't add anything extra: https://github.com/adobe/helix-html-pipeline/pull/773/files#diff-aced0b80c69cd5d57663548aa6ff82a7ab4f3ce37078a65eecc97ecc0c2cf4ba
Can now be tried with the following
hlx-pipeline-version
in helix 4.