Prevent large amounts of data from getting into cards #3850
Conversation
Preview deployments |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3ddefa247
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let sizeLimitError = this.cardService.getSizeLimitError(input.targetRef); | ||
| if (sizeLimitError) { | ||
| return new CorrectnessResultCard({ | ||
| correct: false, | ||
| errors: [sizeLimitError.message], |
There was a problem hiding this comment.
This early return relies on cardService.getSizeLimitError() being populated, but that map is only set when validateSizeLimit runs during the async persist flow. For AI patch commands that call store.patch with doNotWaitForPersist (e.g., PatchCardInstanceCommand/PatchCodeCommand), CheckCorrectnessCommand can run before the save reaches validation, so it will report correct: true even though the write later fails with a 413. Consider validating size synchronously before returning from the patch commands or ensuring correctness checks wait for the persist attempt to complete so the size-limit error is guaranteed to be recorded.
Useful? React with 👍 / 👎.
| cardRenderTimeout: Number( | ||
| process.env.RENDER_TIMEOUT_MS ?? DEFAULT_CARD_RENDER_TIMEOUT_MS, | ||
| ), | ||
| cardSizeLimit: Number(process.env.CARD_SIZE_LIMIT ?? 64 * 1024), |
There was a problem hiding this comment.
Adjust name or add comment indicating units (bytes, I assume?)
There was a problem hiding this comment.
why do we hard code 64 *1024 twice? once here and once in runtime-common. let's just have a single place for this constant
| fromScratchIndexPriority?: number; | ||
| } | ||
|
|
||
| const DEFAULT_CARD_SIZE_LIMIT = 64 * 1024; |
There was a problem hiding this comment.
i think this is 64KB (please add units to this env var name). if so, i think that is a perfectly fine limit.
There was a problem hiding this comment.
Pull request overview
This PR introduces safeguards to prevent large payloads from being stored in cards by implementing size validation at both the host and realm levels.
Changes:
- Added a shared
validateWriteSizefunction to validate content size against a configurable limit - Implemented host-level validation in
card-serviceto fail early for client-originated requests - Implemented realm-level validation as a second line of defense for direct realm endpoint calls
- Extended the check-correctness command to retrieve and report size limit errors from AI-based patching operations
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/runtime-common/write-size-validation.ts | New shared utility function for validating content size against byte limits |
| packages/runtime-common/realm.ts | Added realm-level size validation with CardError responses and configurable size limit |
| packages/runtime-common/index.ts | Exported validateWriteSize function for use in other packages |
| packages/realm-server/tests/helpers/index.ts | Added cardSizeLimit parameter to test helper functions |
| packages/realm-server/tests/card-source-endpoints-test.ts | Added test coverage for 413 errors when source file payloads exceed size limit |
| packages/realm-server/tests/card-endpoints-test.ts | Added test coverage for 413 errors when card payloads exceed size limit |
| packages/realm-server/server.ts | Added cardSizeLimit property and passed it to realm instances |
| packages/realm-server/main.ts | Passed cardSizeLimit configuration when creating realm instances |
| packages/host/tests/integration/commands/check-correctness-test.gts | Added tests for size limit error reporting in file and card writes via commands |
| packages/host/tests/helpers/index.gts | Passed cardSizeLimit configuration when setting up test realm |
| packages/host/tests/acceptance/interact-submode-test.gts | Added test for size limit error display in interact submode edit view |
| packages/host/tests/acceptance/code-submode-test.ts | Added test for size limit error display in code submode editor |
| packages/host/config/environment.js | Added cardSizeLimit configuration with default value of 64KB |
| packages/host/app/services/environment-service.ts | Added cardSizeLimit property to environment service |
| packages/host/app/services/card-service.ts | Implemented host-level size validation with error tracking for check-correctness command |
| packages/host/app/config/environment.d.ts | Added cardSizeLimit type definition to config |
| packages/host/app/components/operator-mode/code-submode/editor-indicator.gts | Added error message display styling and logic to editor indicator |
| packages/host/app/components/operator-mode/code-submode.gts | Added writeError state tracking and onWriteError handler |
| packages/host/app/components/operator-mode/code-editor.gts | Added error handling for write operations with error message propagation |
| packages/host/app/commands/check-correctness.ts | Added size limit error retrieval from cardService for AI-based patching scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let message = | ||
| error?.message ?? | ||
| (error?.title ? `${error.title}: ${error.message ?? ''}` : undefined); |
There was a problem hiding this comment.
This code checks for error.title when status is 413, but the actual error message is displayed. However, when the error doesn't have a title (for non-413 errors), the logic falls through to constructing a message from error.title and error.message. If error.title exists but error.message doesn't, this will result in "title: " with no actual message. Consider checking if error.message exists before attempting to construct this compound message.
| let message = | |
| error?.message ?? | |
| (error?.title ? `${error.title}: ${error.message ?? ''}` : undefined); | |
| let message = error?.message ?? error?.title; |
| environmentService.cardSizeLimit = currentSize + 50; | ||
|
|
||
| let doc = JSON.parse(content); | ||
| doc.data.attributes = { | ||
| ...(doc.data.attributes ?? {}), | ||
| title: 'x'.repeat(currentSize + 200), | ||
| }; | ||
| setMonacoContent(JSON.stringify(doc, null, 2)); | ||
| await waitFor('[data-test-save-error]'); | ||
| assert | ||
| .dom('[data-test-save-error]') | ||
| .includesText('exceeds maximum allowed size (270 bytes)'); |
There was a problem hiding this comment.
The test logic may be flawed. The test sets cardSizeLimit = currentSize + 50, then creates a title with 'x'.repeat(currentSize + 200). However, the assertion checks for "exceeds maximum allowed size (270 bytes)". The number 270 doesn't match currentSize + 50. The test should either use a fixed size limit or assert against the actual calculated limit. The hardcoded "270 bytes" makes this test brittle and unclear.
| #definitionLookup: DefinitionLookup; | ||
| #copiedFromRealm: URL | undefined; | ||
| #sourceCache = new AliasCache<SourceCacheEntry>(); | ||
| #moduleCache = new AliasCache<ModuleCacheEntry>(); |
There was a problem hiding this comment.
The private field #cardSizeLimit is used to validate both card and file sizes, but the name only refers to cards. Consider renaming to something more generic like #writeSizeLimit or #maxPayloadSize to better reflect its dual purpose, or document that despite the name, it applies to both cards and files.
| #moduleCache = new AliasCache<ModuleCacheEntry>(); | |
| #moduleCache = new AliasCache<ModuleCacheEntry>(); | |
| // Used to validate the size of both cards and files written to this realm. |
| cardRenderTimeout: Number( | ||
| process.env.RENDER_TIMEOUT_MS ?? DEFAULT_CARD_RENDER_TIMEOUT_MS, | ||
| ), | ||
| cardSizeLimit: Number(process.env.CARD_SIZE_LIMIT ?? 64 * 1024), |
There was a problem hiding this comment.
The configuration variable cardSizeLimit is used to validate both card and file sizes, but the name only refers to cards. This is consistent with the naming throughout the codebase, but it may be misleading. Consider renaming to something more generic like writeSizeLimit or maxPayloadSize in a future refactor to better reflect that it applies to both cards and files.
| export function validateWriteSize( | ||
| content: string, | ||
| maxSizeBytes: number, | ||
| type: 'card' | 'file', | ||
| ): void { |
There was a problem hiding this comment.
The function lacks documentation explaining its purpose, parameters, and when/how it should be used. Consider adding JSDoc comments to document the validation logic, especially since this is a shared utility that's used in multiple contexts (host and realm).
| export function validateWriteSize( | ||
| content: string, | ||
| maxSizeBytes: number, | ||
| type: 'card' | 'file', | ||
| ): void { | ||
| const actualSize = new TextEncoder().encode(content).length; |
There was a problem hiding this comment.
A new TextEncoder instance is created on every validation call. For better performance, consider creating a single TextEncoder instance at the module level and reusing it, since TextEncoder instances are stateless and reusable.
| export function validateWriteSize( | |
| content: string, | |
| maxSizeBytes: number, | |
| type: 'card' | 'file', | |
| ): void { | |
| const actualSize = new TextEncoder().encode(content).length; | |
| const textEncoder = new TextEncoder(); | |
| export function validateWriteSize( | |
| content: string, | |
| maxSizeBytes: number, | |
| type: 'card' | 'file', | |
| ): void { | |
| const actualSize = textEncoder.encode(content).length; |
| validateWriteSize(content, maxSizeBytes, type); | ||
| } catch (e: any) { | ||
| this.sizeLimitError.set(url, e); | ||
| throw new Error(e); |
There was a problem hiding this comment.
The error is wrapped in a new Error() with the original error object passed as a string, which loses the error's properties. This should be throw e; to preserve the error object and its properties, or properly construct the Error with the message: throw new Error(e.message);
| throw new Error(e); | |
| throw e; |
Purpose
This PR introduces a guard to prevent large amounts of data from being stored in cards.
Background
Card data can currently be inserted or updated through several pathways:
Without a guard, these paths can allow large payloads to enter the system.
Implementation
To address this, I implemented
validateWriteSizefunction in both the host and the realm:Host-level guard:
Implemented to fail early for scenarios that originate in the host.
Realm-level guard:
Added as a second line of defense to ensure protection even when requests reach the realm directly.
Error Handling by Scenario
Interact submode and Code submode:
Errors are thrown directly and surfaced to the user immediately.
AI-based patching:
I extended the check correctness command to retrieve the card limit error from
cardService, so we can handle errors originating from:patch-codecommandpatch-card-instancecommandI’m open to alternative approaches here if there’s a better way to handle validation or error propagation for AI-driven updates.
Interact Submode:
Code Submode: