Possible bug: pages can update Astro.locals about astro HOT 10 OPEN

I take security seriously, and it's a soft subject, and I can feel it's the same for, I think. Astro project take it seriously too, in fact we have a CVE filing system in place that you can use.

Because of that, I need to understand exactly what are the security implications you're talking about, maybe backed by a reproduction. Happy to raise the issue to a p5.

from astro.

The danger I see is the page script and any astro components you may import. They all have access to the Astro global; thus, any of them could modify the locals when they get parsed.
For the page, I might even allow it to change the locals through a dedicated method, like Astro.locals.set(name, value), but once the page starts to render, none of the components used should be able to modify data there.
One edge case is the Layouts and any Page imports, which should all behave as pages and not components, even though the template interface does expose them as components.

from astro.

@adaliszk because of the design of Astro pages and layouts having to be imported into a page like a regular component instead of being some abstraction on top, it currently seems to be quite easy to reason about updating Astro.locals from the page at least. The only place after middleware where you would have non-updated locals would be any leading code above in the same page file as where you are doing the update. I would also be in favour of not recommending or even disabling updates from within components as that will be the point where you can't reason about execution order anymore.

from astro.

Added reproduction: https://stackblitz.com/edit/github-mxldb7?file=src%2Fmiddleware.ts

from astro.

I think we might have allowed this on purpose, cc @ematipico to confirm. If middleware can mutate variables then I don't think we're gaining that much from banning pages/endpoints from doing so. The downside still exists. And making each middleware call cause a new object allocation would be expensive for little gain. In my mind, Astro.locals is a shared space and everyone needs to play well with each other.

from astro.

It makes no sense not to freeze the object right away. With the current behavior, a random component can change locals mid-rendering, potentially breaking or hijacking the rest of the page. I am not worried about allocating memory or spending CPU time; I am just concerned about breaking the expectations of a deterministic code and the potential attack surface of a supply-chain attack.

from astro.

@adaliszk this is true for every library you install, every integration you install, every adapter and your/others code. If you're worried, you can inspect your dependencies and decide to fork or send a PR.

This isn't a valid argument in open source.

Other than that, I don't think freezing the whole object is a viable solution because users might still need to write in it, especially during rewrites and 404/500 routes. If you're worried about your locals, you can freeze them. Astro freezes the assignment to locals, so third party code can't wipe them.

from astro.

Very well. I'm sorry that I raised the security aspect of it. There is no desire to block the vulnerability until someone posts a horror story of the exact attack that stole confidential information or perhaps even money.
While there is always a risk whenever you install a dependency, a framework should not make it easy to execute an attack.

/unsubscribe

from astro.

There's nothing special about Astro.locals vs. any other object that's available inside of components. Astro.request is itself mutable, just as are most JavaScript objects.

There are measures we can and probably should take to help though. We could probably make locals.__proto__ non-writable, that way if someone assigns user-input onto it, it prevents prototype pollution from occurring.

Would love more specific recommendations outside of "every object must be immutable" which is not reasonable and not something any other framework does to my knowledge.

from astro.

Thanks for all the comments. So back to the original question that caused me to explore this:

• Given that sometimes you want to update something in locals based on a post request that is handled in a page;
• Given that the frontmatter code on the page level seems to be ran before any other component code;
• Would you say its safe to update Astro.locals from the page and have the changes be reflected in all components?

Or is this a case of: "you can but you shouldn't"

from astro.