Add security.serverIslandBodySizeLimit and shared body-reading utility#15755
Add security.serverIslandBodySizeLimit and shared body-reading utility#15755
Conversation
🦋 Changeset detectedLatest commit: 4869953 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 |
github-actions
bot
added
the
pkg: astro
Merging this PR will improve performance by 10.28%
Performance Changes
Comparing 
|
…g utility - Introduce security.serverIslandBodySizeLimit config option so server islands have their own body size limit separate from actions - Extract readBodyWithLimit into a shared utility (core/request-body.ts) used by both actions and server islands, eliminating code duplication - Update server islands endpoint to use the new config and shared utility - Update actions runtime to use the shared utility while preserving ActionError behavior
matthewp
changed the title
…Length check reader.cancel() hangs in Node.js when reading Request body streams in the SSR build context. The original actions code never called it — just throwing from inside the read loop is sufficient. Also restores the upfront Content-Length check in parseRequestBody to match the original control flow.
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
Few things needs to be addressed
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| 'astro': patch | |||
There was a problem hiding this comment.
This should be a minor, with a bigger explanation
| export class BodySizeLimitError extends Error { | ||
| limit: number; | ||
| constructor(limit: number) { | ||
| super(`Request body exceeds the configured limit of ${limit} bytes`); | ||
| this.name = 'BodySizeLimitError'; | ||
| this.limit = limit; | ||
| } | ||
| } |
There was a problem hiding this comment.
This should be an Astro error. Actually, I believe we have already an error for this, probably the one we use for Astro Actions
There was a problem hiding this comment.
AstroError is for development and is an error intended for the developer, usually about something like a misconfiguration. We don't have an AstroError for this. I actually started to do what you suggest before looking at the errors and seeing that they were all for dev.
There was a problem hiding this comment.
I see! I think it's fine for now to keep this here, however we should have a directory for runtime errors, so they don't get scattered across the codebase
| * | ||
| * @throws {BodySizeLimitError} if the body exceeds the configured limit | ||
| */ | ||
| export async function readBodyWithLimit(request: Request, limit: number): Promise<Uint8Array> { |
There was a problem hiding this comment.
I don't understand why the code of this function was changed, but with this change, we also changed the error message thrown for the actions body limit. We need to make sure we are consistent with error handling
There was a problem hiding this comment.
The actions body limit error remains the same because it gets caught and rethrown here: https://github.com/withastro/astro/pull/15755/changes#diff-81e2d08f5cbae97ab8006d21a810c86d3d6a18e3ab93d7bdc9eb5a91a03ec26cR295
|
@ematipico Updated the changeset here: 4869953 Left comments on the other two things. |
sarah11918
left a comment
sarah11918
left a comment
There was a problem hiding this comment.
Changeset and JSDoc look great, Matthew! Thanks! 🎉
4 participants
Changes
security.serverIslandBodySizeLimitconfig option (defaults to 1 MB) so server islands have their own body size limit, separate fromsecurity.actionBodySizeLimitcore/request-body.ts) withreadBodyWithLimit()andBodySizeLimitError, used by both Actions and Server IslandsContent-Lengthheader upfront for early rejection of oversized requestsreadRequestBodyWithLimitfrom the actions runtime in favor of the shared utilityTesting
Docs