Comments (14)
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.
It is safe in that the frontmatter code does always run before those components, so you are not going to run into race conditions. I assume you probably are doing this to avoid needing to pass props through several layers of component props. It should work and not break to do this. Whether that's a good pattern or not, I guess that's a matter of opinion. I like the explicitness of prop passing, but you are free to do it another way.
Going to close as I don't think there's any bugs to fix from this issue. Thanks for the discussion though!
from astro.
Thanks for taking some time on this issue @matthewp!
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.
Just pointing out a little detail since that is where the Devil lies:
It is safe in that the frontmatter code does always run before those components, so you are not going to run into race conditions.
It is safe to do that in the front matter of the page. If you change locals from the front matter of a component, it may race with changes from the front matter of sibling components.
Race here in the JS sense. A logical race, not a memory race.
from astro.
Yes, don't attempt to set variables that siblings need. Only direct children
from astro.
Related Issues (20)
- Query parameters lost and function inconsistently
- Some tests are broken on main branch HOT 1
- base url not working as expected during dev HOT 3
- Cookies are not persistent when set on a page inside 'account' folder HOT 7
- Unable to generate RSS feed with directory format consistently across DEV and PROD environments
- Astro is not building to default folder according to docs and running it reports broken module reference. HOT 1
- `hast` element property names converted to `camelCase` when running in MDX mode
- Slow Rendering Performance (Astro is more than 10 times slower than Solid) HOT 3
- Refuse to apply style because its MIME type ('') is not a supported stylesheet MIME type, and strict MIME checking is enabled. HOT 4
- Special characters excluded from content collection slug
- Optimalization: ViewTransition CSS should be after after `<body>` HOT 17
- Rewrite in middleware incorrectly returns a 404 response HOT 7
- Markdown Pages - Links on new tab and additional attribute support HOT 1
- `__dirname is not defined in ES module scope` because of mdx integration HOT 2
- Missing .env variables are incorrectly resolved in SSR mode HOT 2
- Astro Actions: POST Requests with Rewriting Throw 'The Body Has Already Been Consumed' Error HOT 5
- Minifiy HTML when div in ahref
- Nested links being rendered as siblings HOT 2
- There is a bug in the rendering order of Astro components
- Build broken in Astro 4.11.6 HOT 6
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from astro.