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.

Sign up for GitHub

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

Updates astro-island for safer props deserialization #3268

Closed
wants to merge 7 commits into from
Closed

Updates astro-island for safer props deserialization #3268

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b71325c
astro-island should use eval() to deserialize props
May 3, 2022
cceaa47
chore: adding changeset
May 3, 2022
1fc5f00
Use stringify instead
matthewp May 3, 2022
2919d39
Remove serialize-javascript
matthewp May 3, 2022
c7d633c
Add comment on why we use stringify
matthewp May 3, 2022
c0d18ca
Updated lock file
matthewp May 3, 2022
fa51fe6
More removing of serialize-javascript
JuanM04 May 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/astro/src/runtime/server/astro-island.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ customElements.define('astro-island', class extends HTMLElement {
const opts = JSON.parse(this.getAttribute('opts'));
setup(this, opts, async () => {
const propsStr = this.getAttribute('props');
const props = propsStr ? eval('(' + propsStr + ')') : {};
const props = propsStr ? JSON.parse(propsStr) : {};
const rendererUrl = this.getAttribute('renderer-url');
const [
{ default: Component },
Expand All @@ -32,4 +32,4 @@ customElements.define('astro-island', class extends HTMLElement {
*
* And copy/paste the result below
*/
export const islandScript = `customElements.define("astro-island",class extends HTMLElement{async connectedCallback(){const[{default:t}]=await Promise.all([import(this.getAttribute("directive-url")),import(this.getAttribute("before-hydration-url"))]);const e=JSON.parse(this.getAttribute("opts"));t(this,e,(async()=>{const t=this.getAttribute("props");const e=t?eval('('+t+')'):{};const r=this.getAttribute("renderer-url");const[{default:s},{default:i}]=await Promise.all([import(this.getAttribute("component-url")),r?import(r):()=>()=>{}]);return(t,r)=>i(t)(s,e,r)}))}});`;
export const islandScript = `customElements.define("astro-island",class extends HTMLElement{async connectedCallback(){const[{default:t}]=await Promise.all([import(this.getAttribute("directive-url")),import(this.getAttribute("before-hydration-url"))]);t(this,JSON.parse(this.getAttribute("opts")),(async()=>{const t=this.getAttribute("props"),e=t?JSON.parse(t):{},r=this.getAttribute("renderer-url"),[{default:i},{default:s}]=await Promise.all([import(this.getAttribute("component-url")),r?import(r):()=>()=>{}]);return(t,r)=>s(t)(i,e,r)}))}});`;
4 changes: 1 addition & 3 deletions packages/astro/src/runtime/server/hydration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,10 @@ import type { AstroComponentMetadata, SSRLoadedRenderer } from '../../@types/ast
import type { SSRElement, SSRResult } from '../../@types/astro';
import { hydrationSpecifier, serializeListValue } from './util.js';
import { escapeHTML } from './escape.js';
import serializeJavaScript from 'serialize-javascript';

// Serializes props passed into a component so that they can be reused during hydration.
// The value is any
export function serializeProps(value: any) {
return serializeJavaScript(value);
return JSON.stringify(value);
Copy link
Contributor

@JuanM04 JuanM04 May 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

You can use SuperJSON.
Also, this is the only place where serialize-javascript is being used. Maybe it should be completely removed

Copy link
Contributor

@matthewp matthewp May 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Way too big for this use case IMO. We have to deserialize in the browser.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm torn on this one since it removes functionality, but maybe it is the right move to go with JSON.stringify() here

functions, regexps, dates, sets or maps

This is the list of datatypes that serialize-javascript helps with. Functions are the only ones I could see being used in a component, but then maybe that really isn't an ideal use case anyway? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Removed the dep, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Way too big for this use case IMO

Well point. And what about devalue? Sorry for insisting, but not having undefined, Date and may break existing code

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries! that's definitely a better option. I still don't feel super great about putting it into a script tag but it's better than having many script tags. Let me try that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I have Tried this out. It turns the inline script from 254 bytes to 2.08kB :(

Copy link
Contributor

@JuanM04 JuanM04 May 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It turns the inline script from 254 bytes to 2.08kB :(

How weird. devalue itself shouldn't be included in the output. See XSS Mitigation

Copy link
Contributor

Choose a reason for hiding this comment

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

I just mean that devalue adds that many bytes. I don't feel comfortable adding that in a PR to fix a regression. Maybe it's a good tradeoff and maybe not, but I think we should weight the pros and cons in a separate issue. So I'm going to revert the astro-island change and go back to script tags until we have time to figure out what to do here.

}

const HydrationDirectives = ['load', 'idle', 'media', 'visible', 'only'];
Expand Down