Conversation
Add checksummed JSON export/import for text agent resources, with secret redaction, binary omission, a 200-file/1MB cap that fails closed, and personal skip-on-conflict import. Wire the same actions into the Resources tab and keep workspace import refused in v1. Co-authored-by: Matt Van Horn <mvanhorn@users.noreply.github.com>
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
great idea @mvanhorn - can you take a look at the automated review comments and fix anything oyu agree with? and share any screnshots of any UI you updated (if any)? |
Invert isBinaryResourceMimeType to a text allow-list. The deny-list failed open: PDF, archive and Office uploads passed the export filter and were serialized into the JSON pack. The allow-list matches how handlers.ts already decides whether a resource is text. Redact labeled credentials through the end of the value instead of stopping at the first whitespace. A quoted secret with spaces previously kept everything after the first space in a pack advertised as redacted. Quoting is preserved so a redacted JSON or YAML resource still parses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011hbhAdQ6qFfwMgCNscPHwS
|
Thanks @steve8708. Both findings here were correct and are fixed in fee72e2. The binary filter was failing open. The redaction one was the more serious of the two, since the output was labelled redacted and was not. Replacement now consumes through the matching closing quote, handling escaped quotes and doubled One thing worth flagging, because it was not in the findings: consuming the quotes along with the value turns a redacted JSON resource into The redaction tests assert the secret substring is absent from the whole serialized pack rather than just checking Adjacent observation, not changed here: Verification: resources suites pass, 19 tests, and oxfmt is clean on the changed files. |
There was a problem hiding this comment.
Builder reviewed your changes and found 3 potential issues 🔴
Review Details
Incremental Code Review – PR #5468
Status: Two previously flagged high-severity issues have been resolved and closed (binary MIME filtering and labeled-credential redaction logic). However, a new round of code review discovered 3 additional high-severity bugs blocking export functionality and validation.
Summary
The PR implements portable resource-pack export/import with checksumming, scope isolation, redaction, and HTTP handlers. The previous high-severity findings (incomplete binary MIME detection and whitespace-stopping credential redaction) have been fixed with:
isBinaryResourceMimeType()now uses an allow-list (text/*+application/json) instead of a reject-list- Labeled-credential redaction now correctly bounds quoted and unquoted values, handling escape sequences and newlines
However, three new blocking issues have emerged:
🔴 HIGH — Export endpoint expects POST but action/client use GET → 405 on all exports
🔴 HIGH — Byte-count validation counts original content, not redacted → size check is inaccurate
🔴 HIGH — Escape-sequence index arithmetic has operator precedence bug → mishandles backslashes in credentials
🧪 Browser testing: Skipped — export endpoint is broken and cannot be tested until fixed.
| getH3App(nitroApp).use( | ||
| "/_agent-native/resources/export-pack", | ||
| defineEventHandler(async (event) => { | ||
| if (getMethod(event) !== "POST") { |
There was a problem hiding this comment.
🔴 HTTP method mismatch blocks all export requests
The handler at line 77 rejects all requests that are not POST, but the action declares http: { method: "GET" } and the client calls with method: "GET". All export-resource-pack requests fail with 405 Method Not Allowed. Change to if (getMethod(event) !== "GET") since export is read-only.
| if (redacted.redacted) { | ||
| redactions.push({ path: meta.path, reason: "secret" }); | ||
| } | ||
| byteCount += Buffer.byteLength(resource.content, "utf8"); |
There was a problem hiding this comment.
🔴 Byte count validation uses original content instead of redacted
Line 114 counts resource.content bytes, but the exported pack contains redacted.content (which is shorter after secrets are removed). Size validation is inconsistent with the actual exported size, and the pack could exceed the advertised 1MB limit. Change to Buffer.byteLength(redacted.content, "utf8").
| while (index < input.length) { | ||
| const char = input[index]; | ||
| if (char === "\\") { | ||
| index += index + 1 < input.length ? 2 : 1; |
There was a problem hiding this comment.
🔴 Operator precedence bug in escape-sequence handling
Line 163: index += index + 1 < input.length ? 2 : 1 evaluates the condition as (index + (1 < input.length)) ? 2 : 1 due to precedence, not ((index + 1) < input.length) ? 2 : 1. This breaks escape-sequence skipping in quoted credential values. Add parentheses: index += (index + 1 < input.length) ? 2 : 1.
The security guards flagged two catch-alls on this path. redactResourceContent wrapped JSON.parse in a bare catch, so any failure from dropMcpSecretFields was silently treated as "not JSON" and the row fell through to string redaction. Parsing now goes through parseJsonContent, which catches SyntaxError and rethrows anything else; genuinely non-JSON MCP rows still take the string path. resolveAppId caught everything around getAppConfig().app.id. getAppConfig does not throw, so the catch only hid real failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Du315CaKLufAYPEdEwcocq Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
|
Pushed Both findings were catch-alls on the resource-pack path.
|
There was a problem hiding this comment.
Builder reviewed your changes and found 2 potential issues 🔴
Review Details
Incremental Code Review – PR #5468
The three previously reported issues remain open and were intentionally not reposted: the export route method mismatch, original-content byte counting, and escape-sequence precedence bug. The latest changes were reviewed independently with randomized diff ordering. The resource-pack design remains directionally sound, but two new high-severity security issues are present in the raw HTTP import path and redaction matcher.
New Findings
- 🔴 HIGH — The import HTTP handler calls
importResourcePack.rundirectly without parsing the action schema. An attacker can provide an invalidtargetScopethat bypasses both the workspace rejection and organization-admin check, then write into the shared/organization owner. - 🔴 HIGH — Credential redaction does not match underscore-separated environment variable names such as
GITHUB_TOKENorSENDGRID_API_KEY, allowing those token values to be exported.
🧪 Browser testing: Will run after this review (PR touches UI code)
| return await importResourcePack.run( | ||
| { | ||
| pack: body.pack, | ||
| targetScope: body.targetScope ?? "personal", |
There was a problem hiding this comment.
🔴 Validate import target scope before bypassing the action schema
This HTTP handler calls importResourcePack.run directly with body.targetScope instead of applying importResourcePackSchema. An authenticated non-admin can send an arbitrary target string: it is neither rejected as workspace nor checked as organization, while ownerForPackTarget treats every non-personal value as the shared organization owner. Parse the body with the action schema or invoke the validated dispatcher before writing resources.
Additional Info
New finding reported by 1 of 2 incremental review agents; independently confirmed in current HEAD.
| const pattern = new RegExp( | ||
| `["']?\\b(?:${CREDENTIAL_NAME})\\b["']?\\s*[:=]\\s*`, |
There was a problem hiding this comment.
🔴 Redact underscore-separated environment credential names
The labeled-secret matcher requires a \b immediately before the credential name. Because _ is a word character, assignments such as GITHUB_TOKEN=... and SENDGRID_API_KEY=... do not match, and their values are not necessarily covered by the standalone-key pattern. These conventional environment credentials can therefore be included in exported packs despite the redaction guarantee. Treat _ as a separator or explicitly match full environment variable names, with regression tests.
Additional Info
New finding reported by 1 of 2 incremental review agents; independently confirmed in current HEAD.
3 participants
Problem
Agent resources (
AGENTS.md,LEARNINGS.md,memory/MEMORY.md, skills) live as SQL rows with list/read/write/promote. There is no export of a selected set and no import into another app or user. Memory and skills stop at the product boundary even though the docs advertise them as the customization layer you would expect from Claude Code, minus the local disk.Merged PR #4982 is a 500 on invalid agent URL import. It is not a resource pack. Content collection exports and GIF downloads are unrelated.
Solution
Add
export-resource-packandimport-resource-packon the existingresourcesaction group. Export builds a versioned JSON pack of text resources the caller can already read, with per-file and pack checksums, a 200-file / 1 MB cap, and secret/binary redaction. Import verifies the checksum and writes into personal scope by default, skip-on-conflict. Workspace import is refused. Organization import uses the same admin write ACL as editing organization files.The Resources tab gets Export pack / Import pack controls on those same actions. HTTP handlers under
/_agent-native/resources/export-packandimport-packdelegate to the actionrunfunctions (no twin store or/api/*wrapper).Rules:
env/headerstoken fields (same patterns as observability traces)too_largewith counts; never truncatesEvidence
export-audit-eventsis already the bulk-export pattern on this framework. Resource pack is that pattern on the resources group.Testing
Also ran handlers, Resources panel/hook specs, action-discovery, i18n-key-coverage,
guard:i18n-catalogs,guard:i18n-changed-copy,guard:no-silent-coercion,guard:no-default-chrome,guard:no-action-twin-routes.Demo
Docs / skill
agent-resources.mdx(and existing locales): "Take resources with you"adding-a-feature: text resources participate inexport-resource-pack; binaries and secrets do not@agent-native/core