-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
prevent URLs in attributes being escaped #9820
prevent URLs in attributes being escaped #9820
Conversation
🦋 Changeset detectedLatest commit: 3cfc4c5 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@@ -104,6 +104,11 @@ Make sure to use the static attribute syntax (\`${key}={value}\`) instead of the | |||
return markHTMLString(` class="${toAttributeString(value, shouldEscape)}"`); | |||
} | |||
|
|||
// prevent URLs in `content` attributes being escaped for multiple params to work | |||
if (key === 'content' && typeof value === 'string' && URL.canParse(value)) { |
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 familiar with this part of the codebase, but can you for hrefs attributes as well?
Do we need to escape the |
Good question! Escaping I'm not sure that we should hard-code the |
content
attributes being escaped
Not sure either - it's now just a general URL check (with any &) for all attributes if that works. |
Thanks! It seems like they encode |
I also think that using HTML entities is better. |
This PR is sitting for a while without too much activity. cc @alexnguyennz and @natemoo-re |
Thanks for the ping @ematipico! I'm okay with this solution if everyone else is. We can follow-up with switching to |
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.
Just blocking until we resolve the URL.canParse
discrepancy.
Nothing for you to do @alexnguyennz, the maintainers just need to decide when we're updating our Node version.
@@ -104,6 +104,11 @@ Make sure to use the static attribute syntax (\`${key}={value}\`) instead of the | |||
return markHTMLString(` class="${toAttributeString(value, shouldEscape)}"`); | |||
} | |||
|
|||
// Prevents URLs in attributes from being escaped in static builds | |||
if (typeof value === 'string' && value.includes('&') && URL.canParse(value)) { |
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 noticed that URL.canParse
was added in Node v18.17.0, but our minimum required version is still v18.14.1.
We should either implement a fallback for URL.canParse
(try/catch
) or wait on merging this until we bump our engines to v.18.17.0 (possibly in the next minor cc @Princesseuh)
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.
URL.canParse
uses the same internal method as the URL constructor
, so a try-catch should be sufficient to unblock.
Co-authored-by: Florian Lefebvre <[email protected]>
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.
Looks great to me! It must have taken effort to find the relevant code path, thanks for the clean fix!
Using patch-package to apply this PR until it is merged & released withastro/astro#9820
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.
Thanks for getting this over the line, @lilnasy!
Changes
Issue
Adds check for server rendered URLs attributes with any
&
so they're not escaped. For edge cases where multiple params are needed e.g. for an API that generates OG images.Before:
<meta name="og:image" content="https://example.com/api/og?title=hello&description=somedescription">
After:
<meta name="og:image" content="https://example.com/api/og?title=hello&description=somedescription">
I think this change is safe - let me know if there's another way this should be done.
Testing
pnpm run test
And manually with
minimal
demo in dev and build with:Docs
N/A