-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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: add origin check for CSRF protection #10678
Conversation
🦋 Changeset detectedLatest commit: 51b6dd9 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 |
3d56339
to
bd7cd9f
Compare
89edeb2
to
6e55a59
Compare
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.
Only one comment below, but others seem simple enough! I feel like it shouldn't need the experimental flag since it's rather straight-forward, but works for me anyways.
I don't think we need the experimental flag for the |
Yes that's the plan. The cookie/token won't be as straightforward as this one, so I felt that it was better to have the experimental flag, so we could change the shape of the configuration as we please because it's part of the experimentation phase. |
Also we have to have experimental flag if the RFC is not merged, and that one is not, yet. |
f5bfa5b
to
d052811
Compare
I haven't reviewed the docs here yet, but just popping in to say that I think @ematipico 's choice of where to put docs here are perfect. The changeset will have the example, then config-reference will have both the I don't think this calls for an entire new page yet, but I will add it to my task list (I'm reworking our sidebar/content structure this quarter) to keep an eye out for "security related things" so we can eventually have a page. Right now, the entry in config will be our documenting, if everyone's OK with that! |
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.
The only thing I have not yet reviewed here is the changeset, @ematipico !
First, I want to double check: ONLY csrfProtection: true
is experimental, but security
is not?
It's not clear to me which of these things is doing what, nor how they work together. Do I need to enable the experimental flag AND the security
config? Can I just enable security and not the experimental flag? What extra feature am I getting from the experimental flag in that case?
So, all the individual words seem OK, but I really don't get a good handle on how these pieces fit together. THAT's what the changeset should ideally make clear!
packages/astro/src/@types/astro.ts
Outdated
* @version 4.6.0 | ||
* @description | ||
* | ||
* It enables some security measures to prevent CSRF attacks: https://owasp.org/www-community/attacks/csrf |
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.
* It enables some security measures to prevent CSRF attacks: https://owasp.org/www-community/attacks/csrf | |
* It enables some security measures to prevent CSRF attacks: https://owasp.org/www-community/attacks/csrf |
I think we'll need to provide some more detail here. Not heavy implementation details, but like a sentence or so on HOW are you doing this or WHAT MEASURES are you taking, in lay speak. (cookies, tokens, other random words Sarah knows?) This doesn't exactly inspire trust. 😅
Ideally, think about what people will find on web pages as advice of "things they should do" in their project to protect against this, and be able to say, "Astro enables security measures recommended to protect against CSRF attacks, like [thing that people will recognize as stuff they're supposed to want/need/do] -- if this is at all possible!
@sarah11918 Thank you for the feedback. Based on your thoughts, I reworked the configuration and now the experimental flags is like this:
Since we are still debating if it makes sense to have a top-level |
@ematipico This change looks great! I was even wondering whether it might be possible to just enable |
Just left a few tiny nit suggestions, but otherwise, Docs is happy with this one! Great job @ematipico! |
Co-authored-by: Sarah Rainsberger <[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.
Approving for the green check!
This will break form submissions when using A cookie based implementation would at least work with everything which has a sense of session. So not curl but at least the other stuff I listed. |
Hey @septatrix, thank you for your feedback, although we would prefer to channel the discussions in the relative RFC: withastro/roadmap#879 A cookie solution has been pondered, but we are still discussing if it's worth the effort considering how Astro works. |
* feat: add origin check for CSRF protection * add tests * chore: documentation * changeset and grammar * chore: add casing check * split function * better naming * make the whole object experimental * remove unused type * update changeset * manually apply Sarah's suggestions * Apply suggestions from code review Co-authored-by: Sarah Rainsberger <[email protected]> --------- Co-authored-by: Sarah Rainsberger <[email protected]>
* Update sharp to 0.33 to fix issue with Alpine Linux It was impossible for me to use the Astro image service in an Alpine Linux docker container. Even though I would install sharp `0.33.3` in my app, pnpm would download version `0.32.6`, which doesn't work in Alpine Linux container. Currently, I have to override the downloaded version in my package.json. For more details see: - lovell/sharp#4054 - https://discord.com/channels/830184174198718474/1224861729792458803/1224861729792458803 This PR updates the version of the `sharp` dependency to `^0.33` and thus makes Astro compatible with Alpine Linux. * update lock file * add changelog * Update packages/astro/package.json * Update pnpm-lock.yaml * ci: update check-merge.yml action (#10690) * test(@astrojs/node) listen for server setup errors in test-utils (#10692) * Add disableremoteplayback attribute to VideoHTMLAttributes interface (#10693) * Add disableremoteplayback attribute to VideoHTMLAttributes interface * Move disableremoteplayback from VideoHTMLAttributes to MediaHTMLAttributes * Create olive-camels-greet.md * Update packages/astro/astro-jsx.d.ts --------- Co-authored-by: Erika <[email protected]> * test(@astrojs/node) wait for server listening in trailing-slash tests (#10694) * test(@astrojs/node) wait for server listening in trailing-slash tests * fix missing waitServerListen * fix import statement --------- Co-authored-by: Emanuele Stoppa <[email protected]> * chore: add `test:citgm` command that would run tests without caching and `build:ci:no-cache` to skip caching for build as well (#10696) * chore: add test:citgm command that would run tests without caching * adding build ci with no cache * chore: fix build:ci:no-cache test:citgm (#10698) * Improve dev toolbar notification contrast on hover (#10657) * fix(#10399, PLT-1786): improve notification contrast on hover * chore: add changeset * fix test:citgm --force flag not passed correctly (#10706) * Skip prerender chunk in static output (#10695) * Remove unused config in Vue JSX integration (#10687) * Refactor MDX transformJSX handling (#10688) * Fix vue-jsx change (#10716) * Fix script inline with directRenderScript (#10686) * Provide better messaging when renaming a table (#10600) * Provide better messaging when renaming a table * Update based on review * [ci] format * [ci] update lockfile (#10718) Co-authored-by: matthewp <[email protected]> * Increase log severity when a page's `getStaticPaths` fails (#10707) * [ci] release (#10680) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * fix: use just pnpm to run the build and tests in citgm (#10727) * fix(devtool): do not trigger interaction check for `div` and `span` (#10719) * fix(devtool): do not trigger interaction check for `div` and `span` * add test * add tests * Rephrase changeset * remove log * add reference link * Update .changeset/swift-coats-teach.md Co-authored-by: Florian Lefebvre <[email protected]> --------- Co-authored-by: Florian Lefebvre <[email protected]> * fix: regression for astro attributes escaping (#10728) * Fix db seeding when srcDir is root (#10720) * ci: add ref when checking out the repository (#10733) * fix(dev): break implicit rerouting loop (#10737) * fix(dev): infinite implicit rerouting * test adapter * changeset * [ci] format * [ci] release (#10729) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * fix: Fixed errorOverlay theme toggle bug. (#10661) * fix: save `localStorage.astroErrorOverlayTheme` when detected dark mode * add changeset * Fix theme toggle in ErrorOverlay * update changeset * [ci] format * feat: add origin check for CSRF protection (#10678) * feat: add origin check for CSRF protection * add tests * chore: documentation * changeset and grammar * chore: add casing check * split function * better naming * make the whole object experimental * remove unused type * update changeset * manually apply Sarah's suggestions * Apply suggestions from code review Co-authored-by: Sarah Rainsberger <[email protected]> --------- Co-authored-by: Sarah Rainsberger <[email protected]> * feat: upgrade the minimum Node.js maintainance LTS version (#10689) * feat: upgrade the minimum Node.js maintainance LTS version * chore: update minimum npm version * chore: revert npm change * chore: address Erika's feedback * chore: apply further suggestions * Update .changeset/empty-rules-type.md Co-authored-by: Sarah Rainsberger <[email protected]> --------- Co-authored-by: Sarah Rainsberger <[email protected]> * [ci] format * Accept common cookie attributes when deleting a cookie (#10671) * Accept common cookie attributes when deleting a cookie * Fix AstroCookieSetOptions IDE annotations * Use AstroCookieSetOptions to construct AstroCookieDeleteOptions * Update .changeset/shaggy-cats-film.md Co-authored-by: Florian Lefebvre <[email protected]> --------- Co-authored-by: Florian Lefebvre <[email protected]> * feat(i18n): manual routing (#10193) * feat(i18n): manual routing * one more function * different typing * tests * fix merge * throw error for missing middleware * rename function * fix conflicts * lock file update * fix options, error thrown and added tests * rebase * add tests * docs * lock file black magic * increase timeout? * fix regression * merge conflict * add changeset * chore: apply suggestions * apply suggestion * Update .changeset/little-hornets-give.md Co-authored-by: Erika <[email protected]> * chore: address feedback * fix regression of last commit * update name * add comments * fix regression * remove unused code * Apply suggestions from code review Co-authored-by: Sarah Rainsberger <[email protected]> * chore: update reference * Update packages/astro/src/@types/astro.ts Co-authored-by: Sarah Rainsberger <[email protected]> * chore: improve types * fix regression in tests * apply Sarah's suggestion --------- Co-authored-by: Erika <[email protected]> Co-authored-by: Sarah Rainsberger <[email protected]> * [ci] format * feat(toolbar): allow the user to change the placement (#10591) * feat(toolbar): add `placement` to settings * feat(toolbar): update `settings.placement` with `<select>` * feat(toolbar): adjust position based on `settings.placement` * test(toolbar): add a test case for `settings.placement` * refactor(toolbar): extract select element from settings app * feat(toolbar): allow select element to have colors * test(toolbar): fix failed test case * refactor(toolbar): add `placement` property to window element * refactor(toolbar): notify apps when placement changes * test(toolbar): fix failed test case * refactor(toolbar): extract `synchronizePlacementOnUpdate` function * chore: add changeset * chore: update changeset Co-authored-by: Sarah Rainsberger <[email protected]> --------- Co-authored-by: Erika <[email protected]> Co-authored-by: Sarah Rainsberger <[email protected]> * Adds dataLanguage property to the replacement <pre> element. (#10538) * Update highlight.ts * Create cold-snakes-train.md * Update Code.astro Solution for use-case described in withastro/roadmap#276 (withastro/roadmap#276) * roll-back initial fix * new fix * update changeset * Update packages/markdown/remark/src/rehype-prism.ts * Update .changeset/cold-snakes-train.md Co-authored-by: Sarah Rainsberger <[email protected]> * Update .changeset/cold-snakes-train.md Co-authored-by: Sarah Rainsberger <[email protected]> * Update .changeset/cold-snakes-train.md Co-authored-by: Sarah Rainsberger <[email protected]> * Update .changeset/cold-snakes-train.md Co-authored-by: Sarah Rainsberger <[email protected]> --------- Co-authored-by: Bjorn Lu <[email protected]> Co-authored-by: Matthew Phillips <[email protected]> Co-authored-by: Sarah Rainsberger <[email protected]> * [ci] format * fix some i18n config docs (#10746) * fix some i18n config docs * Move manual option to last position --------- Co-authored-by: = <[email protected]> * [ci] format * fix(rendering): remove render instructions from slot expressions (#10747) * [ci] format * Update a11y-no-noninteractive-tabindex rule for dev tool bar (#10750) * fix: a11y-no-noninteractive-tabindex * add changeset * Update utils.ts: Optimize and simplify code (#10749) Update utils.ts: Optimize and simplify code * fix(cli): call path.replace only if it is a function (#10745) * fix(cli): call `path.replace` only if it is a function * add changeset * fix: rewrite Node.js changeset (#10753) * fix: rewrite Node.js changeset * Update .changeset/empty-rules-type.md Co-authored-by: Sarah Rainsberger <[email protected]> --------- Co-authored-by: Sarah Rainsberger <[email protected]> * [ci] release (#10739) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * fix(i18n): fallback SSR (#10755) * fix(i18n): fallback SSR * Update .changeset/old-pugs-jog.md Co-authored-by: Florian Lefebvre <[email protected]> --------- Co-authored-by: Florian Lefebvre <[email protected]> * [docs] config reference link fix (#10758) * Limit imports in flight for `getCollection` (#10708) * [ci] format * Add useful links to `@astrojs/db` package.json (#10764) Co-authored-by: Reuben Tier <[email protected]> * Performance improvement in createAstro function (#10765) * Fix typo in error message for IncorrectStrategyForI18n (#10768) * fix(vercel): Fix srcset generation not working on Vercel (#10756) * fix(vercel): Fix `srcset` generation not working on Vercel * chore: changeset * fix: remove densities and widths from the HTML attributes * nit: better changeset * nit: add formats * [ci] format * [ci] release (#10757) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * fix(add): Fixes astro add modifying baseUrl by accident (#10774) * fix(add): Fixes `astro add` modifying `baseUrl` by accident * chore: changeset * test: add test * fix: tsconfig not being a json maybe is a mistake, I don't know! * test: fix * Update packages/astro/test/fixtures/tsconfig-handling/baseUrl/tsconfig.json Co-authored-by: Nate Moore <[email protected]> --------- Co-authored-by: Nate Moore <[email protected]> * [ci] format * Remove MDX processor on buildEnd (#10770) * lint: upgrade eslint to version 9 (#10730) * [ci] format * fix: picture fallback check (#10783) * Fix @types/cookie dependency (#10776) Co-authored-by: Florian Lefebvre <[email protected]> * Make viewTransition.finished wait for animations triggered by viewTransition.ready (#10787) * [ci] format * fix(assets): Forward headers from the original request to the internal request to the image (#10775) * [ci] format * astro/cli/install-package.ts: whichPm may return null if ran in an empty directory (#10782) * [ci] update lockfile (#10791) Co-authored-by: matthewp <[email protected]> * Improve sitemap generate performance (#10795) * Disable streaming for SSG (#10796) * fix: use assetsDir in creating vite config (#10732) Co-authored-by: Emanuele Stoppa <[email protected]> * fix: MDX cannot find relative image path without leading ./ (#10754) Co-authored-by: Oliver Speir <[email protected]> * [ci] release (#10777) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * db: export 'alias' from drizzle-orm/sqlite-core (#10789) * db: export 'alias' from drizzle-orm/sqlite-core * chore: changeset * fix: changeset target --------- Co-authored-by: Ben Holmes <[email protected]> * chore: use Biome to format JS files (#10788) * chore: configuration * chore: update main commands * chore: revert formatting package.json * chore: rebase * [ci] format * Fixes an issue with persisted non-text input fields that have the focus during view transition navigation. (#10799) * Fixes an issue with persisted non-text input fields that have the focus during view transition navigation. * better check * [ci] format * test: fix regression upon import sorting (#10802) * fix(sitemap): Trailing slashes on root url (#10772) * add tests that reveal issue * fix trailing slash root page issue * add changeset * [ci] format * optimization(runtime): create smaller objects for each Astro global (#10773) * optimization(runtime): create smaller objects for each Astro global * add changeset * Make slots lazy --------- Co-authored-by: bluwy <[email protected]> * add/cli: update list of integrations (#10811) * Invalidate CC cache manifest when lockfile or config changes (#10763) * Invalidate CC cache manifest when lockfile or config changes * Close the handle and increment manifest version * debug info * Provide a reason for cache busting * Handle compile metadata missing * Try it this way * Copy over cached assets as well * Only restore chunks when cache is valid * Better handle invalid caches * Explain when there is no content manifest * Add tests * debugging * Remove debugging * Update packages/astro/src/core/build/plugins/plugin-content.ts Co-authored-by: Bjorn Lu <[email protected]> * Update packages/astro/src/core/build/plugins/plugin-content.ts Co-authored-by: Bjorn Lu <[email protected]> * Review comments * Add chunks path constant --------- Co-authored-by: Bjorn Lu <[email protected]> * [ci] format * [ci] release (#10798) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * fixed the path to checkout existing blog posts on the home page of the blog starter template (#10814) Currently the 4th point on home page says: "Check out the included blog posts in src/pages/blog/". It the path here should be "src/content/blog/". * Update packages/astro/package.json * Update pnpm-lock.yaml * update sharp version in examples/starlog and update pnpm-lock.yaml --------- Co-authored-by: Erika <[email protected]> Co-authored-by: Emanuele Stoppa <[email protected]> Co-authored-by: Raz Luvaton <[email protected]> Co-authored-by: apetta <[email protected]> Co-authored-by: Nate Moore <[email protected]> Co-authored-by: Bjorn Lu <[email protected]> Co-authored-by: Matthew Phillips <[email protected]> Co-authored-by: Matthew Phillips <[email protected]> Co-authored-by: Houston (Bot) <[email protected]> Co-authored-by: horo <[email protected]> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Florian Lefebvre <[email protected]> Co-authored-by: Arsh <[email protected]> Co-authored-by: Arsh <[email protected]> Co-authored-by: liruifengv <[email protected]> Co-authored-by: liruifengv <[email protected]> Co-authored-by: Sarah Rainsberger <[email protected]> Co-authored-by: Farzad <[email protected]> Co-authored-by: Ming-jun Lu <[email protected]> Co-authored-by: 604qgc <[email protected]> Co-authored-by: Matthew Phillips <[email protected]> Co-authored-by: = <[email protected]> Co-authored-by: Sarah Rainsberger <[email protected]> Co-authored-by: Oliver Speir <[email protected]> Co-authored-by: aswind7 <[email protected]> Co-authored-by: horo <[email protected]> Co-authored-by: Chris Swithinbank <[email protected]> Co-authored-by: Reuben Tier <[email protected]> Co-authored-by: Leander Gilles <[email protected]> Co-authored-by: Erika <[email protected]> Co-authored-by: Jason <[email protected]> Co-authored-by: Juraj Kapsz <[email protected]> Co-authored-by: Martin Trapp <[email protected]> Co-authored-by: Meghan Denny <[email protected]> Co-authored-by: Rishi Raj Jain <[email protected]> Co-authored-by: Nick Dubelman <[email protected]> Co-authored-by: Ben Holmes <[email protected]> Co-authored-by: Robin Gisler <[email protected]> Co-authored-by: Avinash Reddy <[email protected]> Co-authored-by: Damanjeet Singh <[email protected]>
Changes
This PR adds the origin header check for the CSRF protection.
Closes PLT-1872
Here's the RFC for more details: withastro/roadmap#879
This is a feature that is supposed to work only for on-demand pages. It should not work during the build. To enable the feature, I had considered some generic way to change the
internalMiddleware
that is present in theBasePipeline
class, but after having a chat with @lilnasy, we decided that it felt a bit premature and decided to go for a more custom way.The
manifest
inside the pipeline contains the middleware, and if the feature is enabled, theApp
class (the one that all adapters use) will modify the middleware to add the origin check middleware function.The logic of the origin check is heavily inspired from SvelteKit, and adapted to match Astro.
Testing
I added new test cases
Docs
I documented new options. I am not sure about the creation of a new page. We can still create a new page later on, considering that the RFC is still open.