[Fix/205 csp storage] fix(core): include storage origin in CSP and add unit tests#226
[Fix/205 csp storage] fix(core): include storage origin in CSP and add unit tests#226WebDevByCam wants to merge 6 commits intoemdash-cms:mainfrom
Conversation
🦋 Changeset detectedLatest commit: e5e7761 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
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 |
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
Pull request overview
This PR fixes S3/R2-style direct uploads being blocked by the admin/API Content Security Policy by adding the configured storage origin to the CSP, while extracting CSP generation into a standalone utility and adding unit coverage.
Changes:
- Extract CSP generation into
buildEmDashCsp()and add storage-origin support forconnect-srcandimg-src. - Update admin/API middleware CSP header setting to pass the storage endpoint origin.
- Add Vitest unit tests and a patch changeset for the fix.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/core/tests/unit/middleware-auth.test.ts | Adds unit tests validating storage origin inclusion and invalid URL handling. |
| packages/core/src/astro/middleware/csp.ts | Introduces a standalone CSP builder that can include marketplace + storage origins. |
| packages/core/src/astro/middleware/auth.ts | Replaces inline CSP builder with shared helper and injects storage endpoint into CSP. |
| .changeset/fix-csp-storage.md | Declares a patch release entry for the CSP fix + tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| emdash: EmDashHandlers; | ||
| emdashManifest: EmDashManifest; |
There was a problem hiding this comment.
App.Locals now declares emdash and emdashManifest as required, but the core runtime middleware only attaches these fields on certain routes / conditions (see packages/core/src/astro/middleware.ts where locals.emdash is set inside the runtime init path, and many requests return early without setting it). Making them required makes the global Astro locals typing inaccurate for non-EmDash routes and can force downstream consumers/middlewares to assume these fields always exist. Consider reverting these to optional (or introducing a narrower locals type for the auth middleware context only).
| emdash: EmDashHandlers; | |
| emdashManifest: EmDashManifest; | |
| emdash?: EmDashHandlers; | |
| emdashManifest?: EmDashManifest; |
There was a problem hiding this comment.
you are wrong, you missed that locals.d.ts is the public contract for consumers of the package
typescript requires all interface augmentations to agree on optionality
|
all tests pass and 3 line of |
|
Thanks to Nick Gray for sponsoring my time on this review |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Matt Kane <m@mk.gg>
| "base-uri 'self'", | ||
| ].join("; "); | ||
| } | ||
| export { buildEmDashCsp }; |
There was a problem hiding this comment.
Instead of re-exporting from here, can any other imports be changed to import directly from csp.js
What does this PR do?
Updates the Content Security Policy (CSP) for admin and API routes to include the configured storage origin.
Closes #205
Type of change
Checklist
pnpm typecheckpassespnpm --silent lint:json | jq '.diagnostics | length'returns 0pnpm testpasses (or targeted tests for my change)pnpm formathas been run