Skip to content

fix(security): registry frontmatter parse failure erases publisher attribution — surface as typed rejection (#756 Cat 1) - #837

Merged
isadeks merged 7 commits into
aws-samples:mainfrom
ClintEastman02:fix/791-registry-parse-failure-attribution
Sep 1, 2026
Merged

fix(security): registry frontmatter parse failure erases publisher attribution — surface as typed rejection (#756 Cat 1)#837
isadeks merged 7 commits into
aws-samples:mainfrom
ClintEastman02:fix/791-registry-parse-failure-attribution

Conversation

@ClintEastman02

Copy link
Copy Markdown
Contributor

What & why

Closes #791 (P1, security — #756 Category 1).

A SKILL.md frontmatter block that fails to YAML-parse was silently collapsed to {} in both the TS adapter (parseSkillFrontmatter) and its Python mirror (_extract_runtime). Collapsing erases the publisher (attribution) and runtime, so a malformed record becomes indistinguishable from a legitimately empty one — letting attacker-influenced input drop an audit-critical trust field. It also disarms the duplicate-key frontmatter-injection defense (#664 / #246 B1/B2), which relies on the YAML error surfacing rather than being swallowed.

Change

Both parse sites now raise a typed RegistryRecordMalformedError (new, mirrored across cdk/.../registry/types.ts and agent/src/registry/client.py) instead of returning {}. Both inline nosemgrep: *-silent-success-masking suppressions are removed.

Read-path behavior:

Path Behavior
resolve Fail closed — raises RegistryResolutionError with the new MALFORMED reason (distinct from REMOVED). A malformed winning version is rejected, not silently downgraded to a lower valid one.
getRecord Fail closed — rethrows the parse failure for the targeted record.
listRecords (TS) Skip-with-warning — one corrupt SKILL.md can't break enumerating a whole namespace. Browse tolerates; trust paths reject.

TS carries malformed records as an envelope-identity marker (RecordEntry) so list/resolve/getRecord act on kind/namespace/name/version/status without trusting the erased payload. TS↔Python parity preserved (MALFORMED reason token added to both).

Scope

Tightly scoped to the two registry frontmatter sites. The third suppression (orchestration-store.ts parsePreScreenedAttachments) is fail-safe screening (drops attachments, never passes unscreened content) and is deferred to #792.

Testing

  • cdk: 20/20 in agent-registry-client.test.ts (added 4: resolve→MALFORMED, reject-malformed-winner-no-downgrade, getRecord throws, listRecords skips). Full mise //cdk:test suite green.
  • agent: 11/11 in test_registry_agent_registry_client.py (added malformed-raises-not-empty + malformed-winner-rejected).
  • mise //cdk:eslint, agent ruff check + ruff format --check clean.
  • Masking scan: both frontmatter suppressions removed with no new findings (the CI ratchet checks only new findings; this PR removes 2 and adds 0).

cdk:synth fails only locally on ec2:DescribeAvailabilityZones (my Isengard role lacks the permission for AgentCore AZ resolution) — unrelated to this change; CI synths with proper credentials.

…jection (aws-samples#756 Cat 1)

A SKILL.md frontmatter block that fails to YAML-parse was silently collapsed
to `{}` in both the TS adapter (`parseSkillFrontmatter`) and its Python mirror
(`_extract_runtime`). Collapsing erases the publisher (attribution) and runtime,
making a malformed record indistinguishable from a legitimately empty one and
letting attacker-influenced input drop an audit-critical trust field (aws-samples#791). It
also disarms the duplicate-key frontmatter-injection defense (aws-samples#664/aws-samples#246 B1/B2),
which relies on the YAML error surfacing rather than being swallowed.

Both parse sites now raise a typed `RegistryRecordMalformedError` (new, mirrored
across `types.ts` and `registry/client.py`) instead of returning `{}`, and the
two inline `nosemgrep: *-silent-success-masking` suppressions are removed.

Read-path behavior:
- resolve / getRecord: fail closed. resolve raises `RegistryResolutionError`
  with the new `MALFORMED` reason (distinct from `REMOVED`); a malformed winning
  version is rejected rather than silently downgraded to a lower valid one.
- listRecords (TS): skip-with-warning, so one corrupt SKILL.md can't break
  enumerating a whole namespace (browse tolerates; trust paths reject).

TS carries malformed records as an envelope-identity marker (`RecordEntry`) so
list/resolve/getRecord can act on kind/namespace/name/version/status without
trusting the erased payload. TS<->Python parity preserved.

Scope: the two registry frontmatter sites only. The third suppression
(`orchestration-store.ts parsePreScreenedAttachments`) is fail-safe screening
(drops attachments, never passes unscreened content) and is deferred to aws-samples#792.

Closes aws-samples#791
@ClintEastman02
ClintEastman02 requested review from a team as code owners August 31, 2026 20:54
@codecov-commenter

codecov-commenter commented Aug 31, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 98.98477% with 4 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@4da0a1f). Learn more about missing BASE report.

Files with missing lines Patch % Lines
.../handlers/shared/registry/agent-registry-client.ts 98.56% 4 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #837   +/-   ##
=======================================
  Coverage        ?   92.57%           
=======================================
  Files           ?      321           
  Lines           ?    91123           
  Branches        ?    10142           
=======================================
  Hits            ?    84356           
  Misses          ?     6767           
  Partials        ?        0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

t added 3 commits September 1, 2026 11:14
…p show masking corruption (aws-samples#791 review)

Addresses the three blocking findings from the PR aws-samples#837 review:

- B1: the read-side comments claimed a duplicate frontmatter key is a YAML
  error that keeps the injection defense live. Verified false: `yaml.load(...,
  {json:true})` / PyYAML `safe_load` take last-wins without raising. Reworded to
  attribute the injection defense to the write side (buildSkillMd's YAML dumper)
  and frame the read-side rejection as not-masking-corruption. (TS only; the
  Python comment was already accurate.)
- B2: MALFORMED is a public REGISTRY_RESOLUTION_FAILED reason via
  registry-resolve; documented it in REGISTRY.md §7.2 + §8 and regenerated the
  Starlight mirror.
- B3: registry-show derived existence from listRecords, which now skips
  malformed records, so an asset whose only versions were corrupt returned 404
  as if absent. Added a listBrowseEntries port method that retains malformed
  records as envelope-only markers; show now surfaces the corrupt version
  flagged (malformed: true, publisher/created_at null) instead of hiding it.
…t YAML frontmatter (aws-samples#837)

The aws-samples#791 fail-closed guarantee (browse-tolerance, show-flagging, resolve-reject,
cross-language classification) previously held only for SKILL.md frontmatter YAML
corruption. A valid-YAML record whose x-abca-runtime value was undecodable
base64/JSON, or a CUSTOM/MCP body that was not valid JSON, threw a raw
SyntaxError that:
  - aborted listRecords/listBrowseEntries for the whole namespace (500),
  - escaped resolve as an unclassified error instead of RegistryResolutionError,
  - and, on the Python side, was classified REMOVED rather than MALFORMED —
    so the two languages disagreed on why.

TS: parseSkillRuntime and extractPayload (CUSTOM + MCP) now box parse/decode
failures as RegistryRecordMalformedError, so loadRecordById carries them as
markers and the read paths skip/reject them precisely. Broaden the error's
`reason` to a 3-way MalformedReason discriminator
(FRONTMATTER | RUNTIME | DESCRIPTOR).

Python: _extract_runtime boxes the CUSTOM/MCP/runtime json/base64 decodes as
RegistryRecordMalformedError → resolve classifies MALFORMED (removing the now-dead
ValueError→REMOVED branch); get_record now fails closed on a malformed target,
matching TS getRecord.

Also fix the false registry-show.ts comment (created_at comes from the envelope,
not the erased payload) and the stale Python injection-defense comment.

Tests: TS + Python coverage for corrupt runtime base64/JSON, corrupt CUSTOM body,
corrupt MCP body (resolve MALFORMED, listRecords tolerance, getRecord fail-close).

@isadeks isadeks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — #837 fix(security): registry frontmatter parse failure erases publisher attribution

Status check first: CI is 4/4 green and the branch is MERGEABLE. The only thing gating merge is REVIEW_REQUIRED — no reviews submitted yet. Nothing is broken in the pipeline.

Verdict: request changes. The design is right and the resolve() fail-closed guarantee is genuinely complete — the malformed-winner check correctly precedes the empty-runtime check, so a corrupt winner cannot be misreported as REMOVED or silently downgraded. But the PR ships one unboxed error class that reproduces exactly the namespace-wide breakage it claims to eliminate, opens a new information-disclosure channel on the 422 path, and leaves the sharpest instance of #791's attribution erasure unfixed. None of these are hard to fix; the shape of the change is sound.

Vision alignment

Fits clearly. Strengthens tenet 4 (Fail closed on risk) and tenet 7 (Observable, attributable, replayable — "operators and reviewers must answer: what happened, why…") by making publisher non-erasable rather than silently undefined. Separating MALFORMED from REMOVED is precisely the "reject rather than silently proceed" posture of §4. No tenet is traded, so no ADR is needed. Worth noting tenet 7's review guidance — "favor structured events, spans, or version stamps over ad-hoc logging" — cuts against the logger.warn-only skip path (below).

Blocking

1. parseDescriptorJson doesn't validate object-ness — one poisoned record DoSes an entire (kind, namespace), and it needs no approval rights

cdk/src/handlers/shared/registry/agent-registry-client.ts:254-264 returns JSON.parse(data) typed as Record<string, unknown>. JSON.parse("null") succeeds, so the box never fires and the next line dereferences null — :620 (body.runtime, CUSTOM) and :641 (body._meta, MCP). loadRecordById:592-600 converts only RegistryRecordMalformedError into a marker and rethrows everything else, so the TypeError escapes loadEntries and takes down:

Path Result
registry-list.ts:48 500 for the whole namespace listing
registry-show.ts:55 500 — less visible than before this PR
resolve() 500, not 422 MALFORMED
orchestrator.ts resolveRegistryAssets task admission fails for every task pinning a ref in that namespace
publish() immutability pre-check at :303 → same throw → opaque 500

Two amplifiers make this the most serious finding:

  • loadEntries:439-462 applies no status filter. Every matching record is parsed before resolve narrows to APPROVED/DEPRECATED (:475-478), so a DRAFT record is enough — only agent-registry:CreateRegistryRecord is required, no approver, no RegistryPublisher group.
  • The record name is writer-chosen and the namespace is derived from it via decodeName, so any namespace can be targeted, including one the writer doesn't own.

This directly contradicts the comment at :398-400 ("one corrupt SKILL.md can't break listing an entire namespace") and the new test listRecords tolerates a corrupt CUSTOM/MCP/runtime record — which only seeds 'not json{' / '{bad json', both SyntaxErrors. "null" is valid JSON and walks straight through the new boxing.

Fix — validate inside the boxing function:

const parsed: unknown = JSON.parse(data);
if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) {
  throw new RegistryRecordMalformedError('MALFORMED_DESCRIPTOR', `${what} is not a JSON object`);
}
return parsed as Record<string, unknown>;

Python is worse. agent/src/registry/agent_registry_client.py:47-55 is annotated -> Any, and every non-object shape (null, [1,2], 123, "s") raises an unboxed AttributeError at :90/:135, escaping resolve's except RegistryRecordMalformedError at :207. It also needs the dict guard on _meta that TS gained at :642-644 and Python did not — {"_meta": "x"} is a parity regression introduced by this PR. (Currently latent, since AgentRegistryClient has no callers outside agent/tests/ — the agent consumes pre-resolved assets via registry/loader.py. Worth fixing now, while the parity claim is being made in the docstrings.)

2. The new 422 body leaks raw descriptor bytes, bypassing redactRuntimeForResponse

cdk/src/handlers/registry-resolve.ts:121 returns `${err.reason}: ${err.message}` verbatim, and agent-registry-client.ts:498 interpolates the parser message into it. Per REGISTRY.md §9, resolve/list/show are available to any authenticated caller with no group gate.

js-yaml 4.3.1 embeds a snippet of the actual document in .message, and Node's JSON.parse embeds a ~20-char window around the error position — so a truncated MCP server.json can echo a fragment of _meta["dev.abca.runtime"].headers.Authorization, or a token in a url query string, into the response body. That is exactly what the allowlist at registry-resolve.ts:43-81 exists to prevent (per its own comment). Pre-PR this content was unreachable because the parse collapsed to {}this PR introduces the disclosure.

Fix: put coordinates + the MalformedReason discriminator in the client-facing message, keep the parser text log-only.

throw new RegistryResolutionError('MALFORMED', refStr,
  `resolved ${k}/${ns}/${n}@${v} has a malformed descriptor (${winner.error.reason})`);
logger.error('malformed descriptor', { recordId, reason, error: winner.error.message });

Same treatment for agent_registry_client.py:213-218.

3. Attribution is still silently erased when publisher is present but wrong-typed — and the asset stays loadable

agent-registry-client.ts:650 (MCP), :625 (CUSTOM), :219 (SKILL) all coerce via typeof v === 'string' ? v : undefined. Flip _meta["dev.abca.publisher"] from "sub-abc" to ["sub-abc"] or 123: the runtime is untouched, so the record stays APPROVED, resolves normally, loads into the agent — while show reports publisher: null with no malformed flag, indistinguishable from a legitimately publisher-less record. That is #791's stated vulnerability verbatim, and it's worse than the parse-failure path because nothing fails closed.

Same class, lower impact: frontmatter that is valid YAML but not a mapping (---\n- a\n- b\n---, ---\nscalar\n---) still collapses to {} at :211-213 and agent_registry_client.py:115-116, and surfaces through listRecords/getRecord/show as an ordinary healthy record. resolve does fail closed here (REMOVED), so no trust bypass — but the audit hole and the misclassification remain.

Fix: treat present but wrong-shaped as MALFORMED; absent stays absent. Note the new docblock at :189-197 describes this branch as "a record legitimately without frontmatter" — that only covers if (!m) return {} at :200, not the non-mapping fall-through, so the comment currently launders the gap.

One semgrep note: these residuals are return {} on a type check, not in a catch, so the masking rule is structurally blind to them. Both suppressions named in #791 are genuinely removed (verified — the only one left in the registry path is the pre-existing ResourceNotFoundException → null at :577, which is legitimately the port's absent-record contract). But "no suppressions left" is not "no masking left."

4. No test asserts publisher preservation or erasure — the property the PR is named after

Across all three changed test files, publisher appears only in a pre-existing happy-path assertion (agent-registry-client.test.ts:274), a mock echo (registry-handlers.test.ts:360), and comments. Every new test asserts a reason string or an exception class.

Two consequences: there is no read-path positive control, so if parseSkillPublisher or PUBLISHER_FM_KEY regresses, attribution silently drops from every healthy record with the suite green; and the malformed-path claim ("publisher would be erased") is never demonstrated.

The missing fixture class — valid JSON/YAML that parses to the wrong shape — is also the direct reason blocking items 1 and 3 shipped. Suggest parametrizing ['null','123','[1,2]','"str"'] × {CUSTOM, MCP} plus non-mapping frontmatter, on both sides.

5. publish regresses an accurate 409 into an opaque 500

agent-registry-client.ts:303 calls getRecord for the immutability check outside the try that begins at :330. getRecord:393 now throws RegistryRecordMalformedError, and that type is mapped in zero handlers — so registry-publish.ts:93-108 falls through to the catch-all. Republishing over coordinates holding a corrupt record was 409 REGISTRY_VERSION_EXISTS; it is now 500 INTERNAL_ERROR / "Failed to publish record."

Fail-closed still holds (nothing is overwritten), so this is availability/UX rather than integrity. But the one operator action that could plausibly repair a corrupt record now returns an error mentioning neither the conflict, the corruption, nor the recordId — and a planted malformed record permanently blocks those coordinates. Suggest catching it at :303 and treating it as "exists" (409 with a corruption hint), or mapping it explicitly in the handler with reason + recordId.

Non-blocking

  • registry list still silently downgrades. groupLatest (registry-list.ts:58-78) consumes listRecords, which skips malformed records — so if 1.1.0 is corrupt and 1.0.0 is fine, list reports latest_version: 1.0.0, APPROVED while resolve @^1.0.0 fails MALFORMED. The stale-downgrade hazard this PR closed on resolve and show is live on list. Either surface a malformed count per asset or have groupLatest consume listBrowseEntries.
  • No metric or alarm on the skip path (:405-412). The log line itself is well-formed (record_id, coords, error message) — the gap is around it: no EMF metric, no alarm, and no response-level signal (list returns 200 with the record simply absent). The repo has both an EMF builder (approval-metrics.ts) and an alerts topic with addAlarmActions (operational-alerts.ts:166). A RegistryMalformedRecordSkipped count dimensioned on kind/namespace, alarmed at >0, would be a small addition.
  • MalformedReason has three construction sites and zero consumers. Nothing branches on it, no test asserts any arm (all three could be swapped with the suite green), and it never reaches the one human-facing surface — show/CLI print a generic MALFORMED (descriptor unparseable). Its stated purpose ("tell corruption apart without re-inspecting the cause") is unrealized. Either plumb it to RegistryBrowseEntry and the wire summary, or inline the text.
  • isMalformed needs a cast (:107) because RegistryRecord has no malformed member, so RecordEntry isn't structurally a discriminated union — the guard will accept any object. Tagging the healthy arm { malformed: false; record } removes the cast, enables exhaustiveness checking, and collapses registry-show.ts:56's (e.malformed ? e.name : e.record.name) ternary. That ternary exists only because the internal and public unions nest on opposite arms (MalformedRecordEntry.coords nested + healthy flat, vs. RegistryBrowseEntry malformed flat + healthy nested).
  • created_at is discarded although it's recoverable. MalformedRecordEntry carries only coords, so registry-show.ts:64 nulls it — but createdAt comes from the envelope (raw.createdAt, :607), a sibling of the fields this PR describes at :84-85 as staying readable when the payload is corrupt. Adding readonly createdAt?: string to RecordCoords gives an operator triaging a corrupt record the one field that says when. (See also the doc issue below.)
  • Duplicate-version tie-break is nondeterministic and inconsistent across languages. TS candidates.find (:490) keeps the first entry at the winning version; Python's by_version dict (:191) keeps the last. Reachable because the immutability check is itself eventually consistent (per the comment at :383-384). Prefer "reject if any candidate at the winning version is malformed."
  • No parity corpus entry. contracts/registry-resolution/ covers ref grammar and semver ranking; MALFORMED classification rests entirely on hand-mirrored tests — which is exactly how the _meta guard asymmetry in item 1 drifted. A small descriptor-classification corpus would lock it the way ranking is locked.
  • CLI overloads the CREATED column (cli/src/commands/registry.ts:149) with a 35-char string, breaking fixed-width alignment and conflating "when" with "what's wrong". A trailing marker column or a status suffix reads better; --output json is unaffected.
  • Smaller: super(message, { cause }) rather than the readonly cause?: unknown parameter property (types.ts:184-192, same in RegistryPublishIncompleteError); _ = self._extract_runtime(raw) or a named _assert_descriptor_parseable for the discard-for-side-effect call at agent_registry_client.py:178; the two resolve tests that call client.resolve() twice; MalformedReason's per-arm comments are position-coupled between = and the first | (per-member JSDoc would bind them and give hover support).

Documentation

Correct where updated: §7.2 and §8 gained MALFORMED in both docs/design/REGISTRY.md and the Starlight mirror, with matching wording — and CI's "Fail build on mutation" passing confirms the mirror is genuinely in sync. cli/src/types.ts correctly mirrors the cdk/src/handlers/shared/types.ts change per AGENTS.md.

Gaps:

  • §7.4's show-response schema is stale in both mirrors — still versions: [{ version, status, created_at, publisher }], missing the malformed field this PR adds to a public API contract. Edit docs/design/REGISTRY.md:136, then mise //docs:sync.
  • created_at is documented as unrecoverable when it isn't. cdk/src/handlers/shared/types.ts:91-93 and cli/src/types.ts:69-71 say publisher and created_at are "unrecoverable (erased)". Only publisher is — all three extractPayload branches derive it from the payload, while createdAt is envelope data. registry-show.ts:51-54 says so honestly ("nulled by convention"), which directly contradicts the type docblock. Pick the honest wording for both. That comment also lists three envelope fields where RecordCoords carries five.
  • Three more claims don't hold. The Python docstring says cause is "intentionally omitted" — but all three raise sites use raise ... from exc, so it travels on __cause__. _decode_descriptor_json's docstring credits itself with the runtime body it never handles (that's a separate inline try/except at :91-98). MalformedRecordEntry:94-95 still says "(SKILL.md frontmatter)" when the marker now covers all three MalformedReason arms — same narrowness at :399.
  • The Python docstrings overstate the fix. publisher appears in agent/src/registry/ only inside comments — the Python client never extracts it. On that side only runtime is at stake, so framing the change there as preventing attribution erasure is inaccurate.
  • Minor rot: (PR #837 review) in three test comments is self-referential and unresolvable after merge; and #664/#246 mixes a PR number with an issue number (per ADR-022, #664 is the PR) — buildSkillMd:174 already uses the clearer (#246 review B1/B2).

Scope / governance. #791 enumerates three sites; this PR fixes two and defers orchestration-store.ts parsePreScreenedAttachments to #792 — which is labeled P2/infra-cdk with no security label and no approved label, scoped as "Cat 2 / 13 best-effort lookup sites". That moves a Category-1 content-screening site into a P2 non-security bucket. Either keep #791 open with that site tracked at P1/security, or add security to #792. Also: this PR carries no labels while the issue carries security/P1/registry.

Tests & CI

4/4 green, and the single build (agentcore) check is full coverage rather than partial — the matrix is compute_type: [agentcore] by design, and the job runs mise run build (agent quality + cdk test/synth + cli + docs sync + mutation check).

Bootstrap synth-coverage: not applicable — no cdk/src/constructs/ or cdk/src/stacks/ files touched, so no new CloudFormation resource types. The new tests correctly avoid CDK synth entirely (no new App(), no Template.fromStack()), so the cdk/AGENTS.md bundling and beforeAll-caching guidance doesn't apply.

Test shape is good: corruption is seeded through the same FakeClient the rest of the suite uses, so the tests exercise the real loadEntries → loadRecordById → extractPayload chain rather than poking parse helpers, and the seedMalformed* fixture factories with a status parameter are a clean DAMP factoring. resolve rejects a malformed winner rather than downgrading to a lower valid version is the highest-value test in the PR — it pins a real security property that a naive "skip malformed" implementation would violate.

But coverage is shaped around one input class: text that fails to parse. Untested: everything that parses to the wrong shape (item 4), all three MalformedReason arms, the logger.warn operator-visibility guarantee (deletable without failing anything), registry-show with mixed healthy + malformed versions of one asset, the inverse resolve direction (a malformed lower version must not block a valid higher winner), and — on Python — the malformed-winner-no-downgrade case that is TS's best test.

Review agents run

All six in-scope agents ran; none omitted.

Agent Outcome
code-reviewer parseDescriptorJson hole, Python _meta parity gap, publish 409→500, stale §7.4
silent-failure-hunter Confirmed the hole empirically; traced escape to registry-resolve/orchestrator; found the list downgrade
type-design-analyzer Confirmed the hole; union asymmetry, isMalformed cast, created_at provenance
comment-analyzer Four false claims; verified js-yaml/PyYAML duplicate-key behavior empirically
pr-test-analyzer Publisher-assertion gap; missing wrong-shape fixture class; TS↔Py parity matrix
/security-review (scoped) DRAFT/no-status-filter amplifier; 422 disclosure; publisher-wrong-typed residual

Four agents reached the parseDescriptorJson finding independently, two by executing the trigger inputs.

Human heuristics

  • Proportionality — pass. The internal/public entry split is justified: the private marker carries recordId and a live Error that shouldn't cross the port boundary. No speculative abstraction; the three read surfaces genuinely need three behaviors.
  • Coherence — concern. Two representations of one concept nest on opposite arms, producing the workaround ternary at registry-show.ts:56; and two comments assert contradictory things about the same field (types.ts:91-93 vs registry-show.ts:51-54).
  • Clarity — concern. parseDescriptorJson:254 declares Record<string, unknown> and does not establish it — the type lie is blocking item 1, and it misleads the compiler at :620 and :641. agent_registry_client.py:178 relies on a discarded call for its side effect.
  • Appropriateness — concern (AI001, AI005). The new tests verify against self-written 'not json{' fixtures rather than the real space of valid-JSON inputs, and assert what the code does (throws on syntax errors) rather than what it should (rejects any descriptor that isn't parseable into an object). That gap is the direct cause of items 1 and 3.

Credit where it's due: the reworded parseSkillFrontmatter docblock corrects a pre-existing false security claim. The old comment said "a duplicate key is a YAML error"; verified against the pinned js-yaml 4.3.1, { json: true } makes duplicates last-wins with no throw, while omitting the flag throws. The old comment asserted a property that the json: true flag actively disables — and relocating the injection defense to the write side (buildSkillMd's yaml.dump, independently confirmed to neutralize a newline-bearing description) is correct. Correcting a load-bearing false claim instead of propagating it is exactly the right instinct.

One follow-up worth a tracked issue rather than only a comment: since the threat model explicitly includes out-of-band writes, the read path does accept a duplicate-key document written outside ABCA, taking the last value. PyYAML's safe_load behaves the same way, so the caveat applies to both languages — the Python mirror comment currently omits it.

@ClintEastman02

Copy link
Copy Markdown
Contributor Author

Thanks for the review — pushed 55e01653 addressing all five blocking items. Summary:

1. Non-object descriptors no longer collapse silently. parseDescriptorJson (TS) / _decode_descriptor_json (Py) now reject a payload that parses as valid JSON but isn't an object (null, array, number, string) with MALFORMED_DESCRIPTOR instead of proceeding with a bogus shape. Same guard added on the MCP _meta block (non-object → treated as empty, not crashed).

2. Wrong-typed publisher is a typed rejection, not a dropped field. New readPublisherField helper: absent → undefined, string → value, anything else → RegistryRecordMalformedError. Applied across the CUSTOM body, MCP _meta, and SKILL frontmatter paths so a non-string publisher can't silently erase attribution.

3. 422 no longer leaks descriptor bytes. The malformed-winner rejection message now omits winner.error.message (which could echo raw descriptor content) — it reports only the MalformedReason discriminator. Added a test asserting the 422 body contains MALFORMED_DESCRIPTOR but not the secret token from the corrupt payload.

4. Malformed winner is rejected, not downgraded. resolve() now logs and throws on a malformed resolution winner rather than falling through to an empty runtime. Mirrored in the Python client.

5. Publish over a corrupt slot returns 409, not 500. The immutability pre-read wraps getRecord in try/catch: a RegistryRecordMalformedError there now surfaces as ConflictException (409) with a corruption hint, instead of an opaque 500.

Parity kept between the TS and Python clients throughout; docstrings/REGISTRY.md §7.4 + the Starlight mirror updated. tsc clean, 39 TS + the new Python registry tests pass.

Deferred (not blocking, noting for the record): list-path stale-downgrade, EMF metric/alarm on the skip path, MalformedReason wire plumbing, created_at recovery into the marker, dedup tie-break determinism, and a shared parity corpus for classification.

@isadeks isadeks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at 55e01653. All five blocking items are fixed, each with a behavior-level test. CI is green (build (agentcore) 11m52s), so the full suite backs it. Approving.

Verified

  1. Non-object descriptorsparseDescriptorJson now rejects null/array/non-object as MALFORMED_DESCRIPTOR; _decode_descriptor_json returns dict[str, Any] behind an isinstance guard; Python gained the _meta dict guard TS already had, closing the parity gap. Parametrized over null / 123 / [1,2] / "str" on both sides.
  2. 422 disclosurewinner.error.message is out of the client-facing message; the detail moved to logger.error with recordId and coords, and Python keeps the parser text on __cause__ only. The test that seeds a body containing SUPERSECRETTOKEN and asserts the message contains MALFORMED_DESCRIPTOR but not the token is exactly the right shape — it pins the property rather than the implementation.
  3. Wrong-typed publisherreadPublisherField gives one rule for all three paths (absent → undefined, string → value, otherwise → MALFORMED), and non-mapping frontmatter now throws instead of collapsing to {}.
  4. Publisher assertions — the new section carries both a read-path positive control and the wrong-typed rejections. This is the one your summary numbered differently, so flagging that I checked it specifically: it is covered.
  5. publish 409 — conflict with the reason in the hint, plus expect(fake.sent).toEqual([]) proving no create was attempted over the occupied slot. Good extra assertion.

Also picked up unprompted: the created_at docblock now separates unrecoverable (publisher) from nulled by convention (created_at); §7.4 documented in both mirrors; #246 review B1/B2; the broadened MalformedRecordEntry docblock; _ = self._extract_runtime(raw); and the Python docstring's incorrect "cause omitted" claim corrected to point at __cause__. Python parity now includes the malformed-winner-no-downgrade case that was TS-only.

Follow-ups (non-blocking, none gate this merge)

Two are consequences of the fix rather than pre-existing, so worth capturing before they get lost:

  1. A wrong-typed publisher now removes the record from registry list entirely. readPublisherField throwing routes through loadRecordById → malformed marker → listRecords skips it, warn-only. Failing closed on trust paths is right, but a record that previously appeared with publisher: null now silently vanishes from the catalog with no in-band signal. That makes the deferred list-path work (stale latest_version, no EMF metric or alarm on the skip) more pressing than when I first raised it — I'd pull it forward rather than leave it to a follow-up.
  2. {"_meta": "x"} classifies as REMOVED, not MALFORMED. Both languages agree and the Python test documents it deliberately, so this is a conscious choice — but it's the one spot that doesn't apply this PR's own "present but wrong-shape → MALFORMED" principle. Fails closed either way, so cosmetic.
  3. agent-registry-client.ts cites REGISTRY.md §10 for open read access — §9 is Access control (MVP), §10 is Grammar.
  4. The publish test asserts the exception type but not that the message carries the corruption hint.

Your deferred list (list-path downgrade, EMF metric, MalformedReason wire plumbing, created_at on the marker, tie-break determinism, shared parity corpus) is the right call for a separate PR — the tie-break and parity-corpus items in particular, since hand-mirrored tests are how the _meta asymmetry drifted in the first place.

Two tracker items outside the diff, which I'll sort out rather than leaving to you: #792 holds the deferred Category-1 screening site from #791 but carries neither security nor approved, so #791 can't close cleanly; and this PR has no labels while #791 has security/P1/registry.

Nice turnaround on this one — and the duplicate-key comment correction from the earlier commit is still the most valuable thing in the PR: it replaced a false security claim ("a duplicate key is a YAML error", which json: true actively disables) with an accurate one.

@isadeks
isadeks added this pull request to the merge queue Sep 1, 2026
Merged via the queue into aws-samples:main with commit 06b3f22 Sep 1, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(security): registry frontmatter parse failure erases publisher attribution — surface as typed rejection, don't suppress (#756 Cat 1)

3 participants