feat(playground): compact runtime execution results#2128
feat(playground): compact runtime execution results#2128
Conversation
Deploying midscene with
|
| Latest commit: |
de15877
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4506a939.midscene.pages.dev |
| Branch Preview URL: | https://feat-playground-compact-exec.midscene.pages.dev |
![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: 46d695f693
ℹ️ 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".
| const liveExecutionDump = this.agent?.dump?.executions?.[0]; | ||
| if (liveExecutionDump) { | ||
| return liveExecutionDump; |
There was a problem hiding this comment.
readLiveExecutionDump now returns this.agent.dump.executions[0] directly, which keeps the raw ExecutionTask objects from TaskRunner.dump() (including fields like executor functions and other non-serializable runtime data). In local mode, usePlaygroundExecution persists results via the default IndexedDB provider, and IndexedDBStorageProvider.saveResult writes the whole dump object through dbManager.put; structured cloning fails on function-valued fields (DataCloneError), so execution history is no longer persisted for local runs. Returning a serialized copy (as before via dumpDataString parsing) avoids this regression.
Useful? React with 👍 / 👎.
quanru
force-pushed
the
feat/playground-compact-execution-results
branch
from
8c51574 to
de15877
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| artifact?.absolutePath ?? | ||
| join(this.screenshotsDir, `${ref.id}.${ref.format}`); | ||
|
|
||
| const buffer = readFileSync(absolutePath); |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
In general, to fix this kind of issue you must validate or constrain user-controlled data before using it to build a filesystem path. For a case like this, where the server should only ever access images that are part of the current run’s screenshot artifacts, the safest approach is: (1) ensure all file reads go through IDs that are resolved against a known in-memory artifact registry, and (2) when falling back to constructing a path, only allow simple, sanitized filenames (no directory separators) and ensure the final absolute path stays under a known screenshots root.
Concretely, the critical path is in RuntimeArtifactStore.resolveImage in packages/core/src/dump/runtime-artifact-store.ts, where absolutePath is set based on the (tainted) ref.id and ref.format. The best minimally invasive fix is to: (a) validate that ref looks like a legitimate screenshot reference using the existing isScreenshotArtifactRef function, (b) if there is an artifact entry for ref.id, use its absolutePath but also ensure that path is under this.screenshotsDir, and (c) if there is no artifact entry, sanitize ref.id so that it cannot contain path separators, build a candidate path under this.screenshotsDir, resolve it to an absolute path with resolve, and verify that the resolved path starts with the canonical screenshots directory path. If any of these checks fail, throw an error instead of reading the file.
To implement this, we only need to modify resolveImage in packages/core/src/dump/runtime-artifact-store.ts and add an import of resolve from node:path in that file. We will reuse the existing isScreenshotArtifactRef export from the same module to validate the ref. The rest of the system’s behavior stays the same: callers still pass a ScreenshotArtifactRef, and successful calls still return a base64-encoded data URL. The new checks simply prevent directory traversal or arbitrary file access via malicious id or artifact.absolutePath values.
| @@ -6,7 +6,7 @@ | ||
| rmSync, | ||
| writeFileSync, | ||
| } from 'node:fs'; | ||
| import { join } from 'node:path'; | ||
| import { join, resolve } from 'node:path'; | ||
| import { getMidsceneRunSubDir } from '@midscene/shared/common'; | ||
| import { uuid } from '@midscene/shared/utils'; | ||
| import { ScreenshotItem } from '../screenshot-item'; | ||
| @@ -206,11 +206,33 @@ | ||
| } | ||
|
|
||
| async resolveImage(ref: ScreenshotArtifactRef): Promise<string> { | ||
| // Basic shape validation to avoid obviously malformed refs | ||
| if (!isScreenshotArtifactRef(ref)) { | ||
| throw new Error('Invalid screenshot artifact reference'); | ||
| } | ||
|
|
||
| // Resolve screenshots directory to an absolute, normalized path | ||
| const screenshotsRoot = resolve(this.screenshotsDir); | ||
|
|
||
| let candidatePath: string | undefined; | ||
| const artifact = this.artifacts.get(ref.id); | ||
| const absolutePath = | ||
| artifact?.absolutePath ?? | ||
| join(this.screenshotsDir, `${ref.id}.${ref.format}`); | ||
|
|
||
| if (artifact?.absolutePath) { | ||
| candidatePath = artifact.absolutePath; | ||
| } else { | ||
| // Disallow path separators in the ID to prevent directory traversal | ||
| const safeId = ref.id.replace(/[\\/]/g, ''); | ||
| candidatePath = join(screenshotsRoot, `${safeId}.${ref.format}`); | ||
| } | ||
|
|
||
| const absolutePath = resolve(candidatePath); | ||
|
|
||
| // Ensure the resolved path is contained within the screenshots root | ||
| if (!absolutePath.startsWith(screenshotsRoot + '/') && | ||
| absolutePath !== screenshotsRoot) { | ||
| throw new Error('Screenshot path is outside of the allowed directory'); | ||
| } | ||
|
|
||
| const buffer = readFileSync(absolutePath); | ||
| return `data:image/${ref.format};base64,${buffer.toString('base64')}`; | ||
| } |
| return; | ||
| } | ||
|
|
||
| state.event = payload.event; |
Check warning
Code scanning / CodeQL
Prototype-polluting assignment Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
In general, to fix prototype-polluting assignments you must ensure that untrusted strings are not used directly as property names on plain objects that inherit from Object.prototype. This can be done in two main ways: (1) use a data structure that is not affected by prototype keys (like Map or an object created via Object.create(null)), or (2) validate/restrict keys to disallow dangerous values like "__proto__", "constructor", and "prototype".
For this specific code, the problematic part is using requestId (from req.body) as an index into this.taskExecutionState[requestId] and then treating the result as a mutable state object. The minimal, behavior-preserving fix is to ensure that this.taskExecutionState is a prototype-less object, so that even if requestId is "__proto__" it will not resolve to Object.prototype. We can do this by initializing this.taskExecutionState with Object.create(null) inside the same file. Since we are allowed to edit only shown snippets in packages/playground/src/server.ts, we insert an initialization of taskExecutionState as a field on the containing class (or object) using Object.create(null) where its declaration is present in this file. If the declaration is not visible in the snippet, we can still safely add an initialization in the closest visible location where this is the class instance (for example, in the constructor), but we must stay within the given file.
Concretely:
- Locate where
taskExecutionStateis defined/initialized inpackages/playground/src/server.ts(most likely as a field of the server class). - Replace its initialization from
{}(or an uninitialized field) toObject.create(null). - If
taskExecutionStateis currently not initialized at all in the file, add an explicit initialization, e.g.this.taskExecutionState = Object.create(null);in the constructor or as a class field initializer. - No additional imports are needed because
Object.createis built-in.
This change preserves existing semantics (it is still a plain “dictionary” of task states) but removes the Object.prototype from its prototype chain, eliminating prototype pollution via requestId.
| @@ -26,6 +26,15 @@ | ||
|
|
||
| const defaultPort = PLAYGROUND_SERVER_PORT; | ||
|
|
||
| /** | ||
| * Task execution state storage. | ||
| * Use a prototype-less object to avoid prototype pollution when indexing with untrusted keys. | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore - initialized here if not already declared as a class field | ||
| (ServerImpl as unknown as { prototype: { taskExecutionState?: Record<string, unknown> } }).prototype.taskExecutionState ??= | ||
| Object.create(null); | ||
|
|
||
| // Static path for playground files | ||
| const __filename = fileURLToPath(import.meta.url); | ||
| const __dirname = dirname(__filename); |
| } | ||
|
|
||
| state.event = payload.event; | ||
| state.snapshot = payload.getSnapshot(); |
Check warning
Code scanning / CodeQL
Prototype-polluting assignment Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
In general, to fix prototype-polluting assignments you must avoid using untrusted strings as property names on plain objects that inherit from Object.prototype. You can either (1) switch to a safe associative structure like Map or a prototype-less object, or (2) validate or mangle untrusted keys so they cannot be __proto__, constructor, prototype, or otherwise collide with built-in properties.
For this specific code, the minimal, behavior-preserving fix is to validate requestId before using it as a key into this.taskExecutionState and to reject dangerous values. The vulnerable access is at line 474: this.taskExecutionState[requestId] = { ... }, and the subsequent use at line 482: const state = this.taskExecutionState[requestId];. We can add a short guard right after destructuring the body, or immediately before the first use of requestId as an index, that checks for forbidden property names. If the requestId is missing or equals __proto__, prototype, or constructor, we return an HTTP 400 error. This keeps the overall functionality (one task per requestId) intact while eliminating the possibility of writing to Object.prototype.
Concretely:
- In
packages/playground/src/server.ts, inside the/executehandler, after destructuringrequestIdfromreq.bodyand before line 471–474 wherethis.currentTaskIdandthis.taskExecutionState[requestId]are set, add a validation block that:- If
requestIdis not provided, returns a 400 error (if that’s considered required; if it’s optional, just skip the polluted keys check when it’s falsy). - If
requestIdis one of__proto__,prototype, orconstructor, returns a 400 error.
- If
- This requires no new imports or type changes and does not alter behavior for valid IDs.
| @@ -422,6 +422,16 @@ | ||
| }); | ||
| } | ||
|
|
||
| if ( | ||
| requestId === '__proto__' || | ||
| requestId === 'constructor' || | ||
| requestId === 'prototype' | ||
| ) { | ||
| return res.status(400).json({ | ||
| error: 'invalid requestId', | ||
| }); | ||
| } | ||
|
|
||
| // Recreate agent only when AI config has changed (via /config API) | ||
| if (this.agentFactory && this._configDirty) { | ||
| this._configDirty = false; |
| state.event = payload.event; | ||
| state.snapshot = payload.getSnapshot(); | ||
| if (payload.event.type === 'execution_updated') { | ||
| state.executionDump = payload.event |
Check warning
Code scanning / CodeQL
Prototype-polluting assignment Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
In general, to fix prototype-polluting assignments you must ensure untrusted strings are not used directly as property names on plain objects that inherit from Object.prototype. You can do this by: (1) validating and rejecting dangerous keys ("__proto__", "prototype", "constructor"), (2) prefixing keys so they cannot collide with special properties, or (3) using safe structures like Map or Object.create(null).
For this specific case, the minimal, non-breaking fix is to validate requestId before it is used as an index into this.taskExecutionState and before any callbacks that rely on it are registered. Right after destructuring requestId from req.body and before checking this.currentTaskId, we can add a guard that rejects a malformed or dangerous requestId (including __proto__, prototype, constructor). If the requestId is missing or invalid, we should respond with a 400 error and not continue, preventing it from ever being used as a key in this.taskExecutionState or for other internal lookups. This requires no new imports and does not alter existing behavior for valid IDs; it only adds rejection of clearly invalid values that could lead to prototype pollution.
Concretely:
- In
packages/playground/src/server.ts, within the/executehandler, after extractingrequestIdfromreq.body, add a block that:- If
requestIdis defined but not a non-empty string, rejects with 400. - If
requestIdequals"__proto__","prototype", or"constructor", rejects with 400.
- If
- This ensures that the subsequent usage at lines 473–482 cannot receive a prototype-polluting key and makes the CodeQL alert go away.
| @@ -422,6 +422,27 @@ | ||
| }); | ||
| } | ||
|
|
||
| // Validate requestId to prevent prototype pollution via special property names | ||
| if ( | ||
| requestId !== undefined && | ||
| requestId !== null && | ||
| typeof requestId !== 'string' | ||
| ) { | ||
| return res.status(400).json({ | ||
| error: 'requestId must be a string when provided', | ||
| }); | ||
| } | ||
|
|
||
| if ( | ||
| requestId === '__proto__' || | ||
| requestId === 'prototype' || | ||
| requestId === 'constructor' | ||
| ) { | ||
| return res.status(400).json({ | ||
| error: 'invalid requestId', | ||
| }); | ||
| } | ||
|
|
||
| // Recreate agent only when AI config has changed (via /config API) | ||
| if (this.agentFactory && this._configDirty) { | ||
| this._configDirty = false; |
1 participant
Summary
Validation