-
Notifications
You must be signed in to change notification settings - Fork 12
Prevent large amounts of data from getting into cards #3850
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
base: main
Are you sure you want to change the base?
Changes from all commits
b8164e4
0978e4c
b7373db
ec91480
bb414d5
a3ddefa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -64,6 +64,7 @@ interface Signature { | |||||||||
| onSetup: ( | ||||||||||
| updateCursorByName: (name: string, fieldName?: string) => void, | ||||||||||
| ) => void; | ||||||||||
| onWriteError?: (message: string | undefined) => void; | ||||||||||
| }; | ||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
@@ -350,6 +351,7 @@ export default class CodeEditor extends Component<Signature> { | |||||||||
| this.syncWithStore.perform(content); | ||||||||||
|
|
||||||||||
| await timeout(this.environmentService.autoSaveDelayMs); | ||||||||||
| this.args.onWriteError?.(undefined); | ||||||||||
| this.writeSourceCodeToFile( | ||||||||||
| this.args.file, | ||||||||||
| content, | ||||||||||
|
|
@@ -433,7 +435,11 @@ export default class CodeEditor extends Component<Signature> { | |||||||||
| private waitForSourceCodeWrite = restartableTask(async () => { | ||||||||||
| if (isReady(this.args.file)) { | ||||||||||
| this.args.onFileSave('started'); | ||||||||||
| await all([this.args.file.writing, timeout(500)]); | ||||||||||
| try { | ||||||||||
| await all([this.args.file.writing, timeout(500)]); | ||||||||||
| } catch { | ||||||||||
| // Errors are surfaced via writeError banners, so don't rethrow. | ||||||||||
| } | ||||||||||
| this.args.onFileSave('finished'); | ||||||||||
| } | ||||||||||
| }); | ||||||||||
|
|
@@ -459,10 +465,21 @@ export default class CodeEditor extends Component<Signature> { | |||||||||
|
|
||||||||||
| // flush the loader so that the preview (when card instance data is shown), | ||||||||||
| // or schema editor (when module code is shown) gets refreshed on save | ||||||||||
| return file.write(content, { | ||||||||||
| flushLoader: hasExecutableExtension(file.name), | ||||||||||
| saveType, | ||||||||||
| }); | ||||||||||
| return file | ||||||||||
| .write(content, { | ||||||||||
| flushLoader: hasExecutableExtension(file.name), | ||||||||||
| saveType, | ||||||||||
| }) | ||||||||||
| .catch((error) => { | ||||||||||
| if (error?.status === 413 && error?.title) { | ||||||||||
| this.args.onWriteError?.(error.title); | ||||||||||
| return; | ||||||||||
| } | ||||||||||
| let message = | ||||||||||
| error?.message ?? | ||||||||||
| (error?.title ? `${error.title}: ${error.message ?? ''}` : undefined); | ||||||||||
|
Comment on lines
+478
to
+480
|
||||||||||
| let message = | |
| error?.message ?? | |
| (error?.title ? `${error.title}: ${error.message ?? ''}` : undefined); | |
| let message = error?.message ?? error?.title; |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -12,9 +12,9 @@ import { | |||||
| type RealmInfo, | ||||||
| type Loader, | ||||||
| } from '@cardstack/runtime-common'; | ||||||
|
|
||||||
| import type { AtomicOperation } from '@cardstack/runtime-common/atomic-document'; | ||||||
| import { createAtomicDocument } from '@cardstack/runtime-common/atomic-document'; | ||||||
| import { validateWriteSize } from '@cardstack/runtime-common/write-size-validation'; | ||||||
|
|
||||||
| import type { | ||||||
| BaseDef, | ||||||
|
|
@@ -27,6 +27,7 @@ import type * as CardAPI from 'https://cardstack.com/base/card-api'; | |||||
|
|
||||||
| import LimitedSet from '../lib/limited-set'; | ||||||
|
|
||||||
| import type EnvironmentService from './environment-service'; | ||||||
| import type LoaderService from './loader-service'; | ||||||
| import type MessageService from './message-service'; | ||||||
| import type NetworkService from './network'; | ||||||
|
|
@@ -57,10 +58,13 @@ export default class CardService extends Service { | |||||
| @service declare private loaderService: LoaderService; | ||||||
| @service declare private messageService: MessageService; | ||||||
| @service declare private network: NetworkService; | ||||||
| @service declare private environmentService: EnvironmentService; | ||||||
| @service declare private realm: Realm; | ||||||
| @service declare private reset: ResetService; | ||||||
|
|
||||||
| private subscriber: CardSaveSubscriber | undefined; | ||||||
| // This error will be used by check-correctness command to report size limit errors | ||||||
| private sizeLimitError = new Map<string, Error>(); | ||||||
| // For tracking requests during the duration of this service. Used for being able to tell when to ignore an incremental indexing realm event. | ||||||
| // We want to ignore it when it is a result of our own request so that we don't reload the card and overwrite any unsaved changes made during auto save request and realm event. | ||||||
| declare private loaderToCardAPILoadingCache: WeakMap< | ||||||
|
|
@@ -103,6 +107,10 @@ export default class CardService extends Service { | |||||
| this.subscriber = undefined; | ||||||
| } | ||||||
|
|
||||||
| getSizeLimitError(url: string): Error | undefined { | ||||||
| return this.sizeLimitError.get(url); | ||||||
| } | ||||||
|
|
||||||
| async fetchJSON( | ||||||
| url: string | URL, | ||||||
| args?: CardServiceRequestInit, | ||||||
|
|
@@ -140,6 +148,20 @@ export default class CardService extends Service { | |||||
| requestInit.method = 'POST'; | ||||||
| requestHeaders.set('X-HTTP-Method-Override', 'QUERY'); | ||||||
| } | ||||||
| let urlString = url instanceof URL ? url.href : url; | ||||||
| let method = requestInit.method?.toUpperCase?.(); | ||||||
| if ( | ||||||
| !isReadOperation && | ||||||
| (method === 'POST' || method === 'PATCH') && | ||||||
| requestInit.body | ||||||
| ) { | ||||||
| let jsonString = | ||||||
| typeof requestInit.body === 'string' | ||||||
| ? requestInit.body | ||||||
| : JSON.stringify(requestInit.body, null, 2); | ||||||
| this.validateSizeLimit(urlString, jsonString, 'card'); | ||||||
| } | ||||||
|
|
||||||
| let response = await this.network.authedFetch(url, requestInit); | ||||||
| if (!response.ok) { | ||||||
| let responseText = await response.text(); | ||||||
|
|
@@ -191,6 +213,7 @@ export default class CardService extends Service { | |||||
| options?: SaveSourceOptions, | ||||||
| ) { | ||||||
| try { | ||||||
| this.validateSizeLimit(url.href, content, 'file'); | ||||||
| let clientRequestId = options?.clientRequestId ?? `${type}:${uuidv4()}`; | ||||||
| this.clientRequestIds.add(clientRequestId); | ||||||
|
|
||||||
|
|
@@ -294,6 +317,17 @@ export default class CardService extends Service { | |||||
| } | ||||||
|
|
||||||
| async executeAtomicOperations(operations: AtomicOperation[], realmURL: URL) { | ||||||
| for (let operation of operations) { | ||||||
| if (operation.data?.type === 'source') { | ||||||
| let content = operation.data.attributes?.content; | ||||||
| if (typeof content === 'string') { | ||||||
| this.validateSizeLimit(operation.href, content, 'file'); | ||||||
| } | ||||||
| } else if (operation.data?.type === 'card') { | ||||||
| let jsonString = JSON.stringify(operation.data, null, 2); | ||||||
| this.validateSizeLimit(operation.href, jsonString, 'card'); | ||||||
| } | ||||||
| } | ||||||
| let doc = createAtomicDocument(operations); | ||||||
| let response = await this.network.authedFetch(`${realmURL.href}_atomic`, { | ||||||
| method: 'POST', | ||||||
|
|
@@ -304,6 +338,21 @@ export default class CardService extends Service { | |||||
| }); | ||||||
| return response.json(); | ||||||
| } | ||||||
|
|
||||||
| private validateSizeLimit( | ||||||
| url: string, | ||||||
| content: string, | ||||||
| type: 'card' | 'file', | ||||||
| ) { | ||||||
| let maxSizeBytes = this.environmentService.cardSizeLimit; | ||||||
| try { | ||||||
| this.sizeLimitError.delete(url); | ||||||
| validateWriteSize(content, maxSizeBytes, type); | ||||||
| } catch (e: any) { | ||||||
| this.sizeLimitError.set(url, e); | ||||||
| throw new Error(e); | ||||||
|
||||||
| throw new Error(e); | |
| throw e; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,7 @@ module.exports = function (environment) { | |
| cardRenderTimeout: Number( | ||
| process.env.RENDER_TIMEOUT_MS ?? DEFAULT_CARD_RENDER_TIMEOUT_MS, | ||
| ), | ||
| cardSizeLimit: Number(process.env.CARD_SIZE_LIMIT ?? 64 * 1024), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adjust name or add comment indicating units (bytes, I assume?)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we hard code
|
||
| iconsURL: process.env.ICONS_URL || 'https://boxel-icons.boxel.ai', | ||
| publishedRealmBoxelSpaceDomain: | ||
| process.env.PUBLISHED_REALM_BOXEL_SPACE_DOMAIN || 'localhost:4201', | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1965,6 +1965,38 @@ module('Acceptance | code submode tests', function (_hooks) { | |
| ); | ||
| }); | ||
|
|
||
| test('code editor shows size limit error when json save exceeds limit', async function (assert) { | ||
| let environmentService = getService('environment-service') as any; | ||
| let originalMaxSize = environmentService.cardSizeLimit; | ||
|
|
||
| try { | ||
| await visitOperatorMode({ | ||
| submode: 'code', | ||
| codePath: `${testRealmURL}person-entry.json`, | ||
| }); | ||
|
|
||
| await waitFor('[data-test-editor]'); | ||
|
|
||
| let content = getMonacoContent(); | ||
| let encoder = new TextEncoder(); | ||
| let currentSize = encoder.encode(content).length; | ||
| 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)'); | ||
|
Comment on lines
+1983
to
+1994
|
||
| } finally { | ||
| environmentService.cardSizeLimit = originalMaxSize; | ||
| } | ||
| }); | ||
|
|
||
| test('card preview live updates when index changes', async function (assert) { | ||
| await visitOperatorMode({ | ||
| stacks: [ | ||
|
|
||
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.
This early return relies on
cardService.getSizeLimitError()being populated, but that map is only set whenvalidateSizeLimitruns during the async persist flow. For AI patch commands that callstore.patchwithdoNotWaitForPersist(e.g., PatchCardInstanceCommand/PatchCodeCommand),CheckCorrectnessCommandcan run before the save reaches validation, so it will reportcorrect: trueeven 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 👍 / 👎.