Skip to content

fix(trust-record-verify): reject non-object inputs in parentRecordHash, verifyDelegationLink, and bundleDigest (#616) - #622

Open
drk-ram wants to merge 1 commit into
agentrust-io:mainfrom
drk-ram:drk-ram-fix-ts-non-object-inputs-616
Open

drk-ram wants to merge 1 commit into
agentrust-io:mainfrom
drk-ram:drk-ram-fix-ts-non-object-inputs-616

Conversation

@drk-ram

@drk-ram drk-ram commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

What this changes

Closes #616. Current source head: 916cec5e37bc0bdb2d4b5c3e5002e3a4c36ff060.

parentRecordHash and verifyDelegationLink reject non-object record/parent inputs with record_not_object. bundleDigest rejects non-plain bundle objects with invalid_argument. checkRevocationBundle checks the bundle shape before schema/digest processing and reports unverified_for_revocation with cause bundle_malformed, rather than allowing a digest exception to escape. parentRecordHash validates the algorithm before the record, matching Python's ordering.

The latest reviewed fixture error is already corrected on this head: the regression reads 20-malformed-statement-on-another-log.json and verifies vec.records[0], not the nonexistent filename and vec.record. No further runtime or fixture change was needed in this follow-through.

The latest review identified the two fixture mistakes above. The existing current head includes both corrections. This description replaces the older unchecked validation claims with current-head evidence.

Type of change

  • Editorial (typo, link fix, clarification: no normative effect)
  • Non-breaking spec change (new optional field, new platform profile, informative addition)
  • Breaking spec change (requires 14-day comment period and Project Lead sign-off)
  • Schema change
  • Example addition

Reference TypeScript verifier fix; no specification or schema edit.

Spec section

None. trust-record-verify/src/delegation.ts and trust-record-verify/src/revocation.ts, with unit and Python-to-TypeScript regression coverage.

Tests for a fix

trust-record-verify/tests/units.test.mjs tests parentRecordHash, verifyDelegationLink, and bundleDigest with null, numbers, strings, booleans, arrays, and dates; it also calls verifyDelegationLink({}, bad) and pins algorithm-before-record ordering with parentRecordHash(123, "md5").

checkRevocationBundle and verifyRecord report non-plain bundle objects as bundle_malformed tests class-instance and custom-prototype copies against both committed revocation vectors. The latest review confirmed that removing the plain-object guard makes the corrected regression fail through bundleDigest; the current rerun below verifies the repaired fixture, not a new mutation measurement.

Validation on this head, Node v26.13.0:

  • npm --prefix trust-record-verify ci --ignore-scripts --silent -> passed, restoring the initially missing local ajv dependency from the existing lockfile.
  • npm --prefix trust-record-verify test -> 165 passed, 0 failed, 0 skipped; validator generation and TypeScript build also passed.

The current-head hosted trust-record-verify workflow is successful. The changes-requested review and independent maintainer-approval requirement remain pending re-review. The full Python suite was not rerun as part of this follow-through.

Changelog entry

Checklist

  • DCO sign-off on all commits (git commit -s)
  • Breaking changes marked with <!-- CHANGED: #NNN: description --> in spec text
  • Backward compatibility statement included (for breaking changes)

@drk-ram
drk-ram requested review from a team, lywinged and rajnisht7 as code owners October 23, 2026 04:02
@github-actions

github-actions Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

❔ Contributor Check: UNKNOWN

Check Result
Profile MEDIUM
Credential UNKNOWN
Overall UNKNOWN

Automated check by AgenTrust Contributor Check.

@github-actions github-actions Bot added the needs-review:UNKNOWN Contributor check flagged UNKNOWN risk label Oct 23, 2026

@dinakarjs dinakarjs 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.

Reviewed exact head 3098d7a. Checked the plain-object guards at parent-record, delegation-link and bundle digest boundaries and their return/throw contracts. Local npm test (including generated validators and TypeScript build): 161 passed. Python wrapper suite tests/test_trust_record_verify_citations_and_platform.py: 34 passed. Relevant hosted checks are successful. No blocking finding.

@drk-ram
drk-ram force-pushed the drk-ram-fix-ts-non-object-inputs-616 branch from 3098d7a to 3a50396 Compare October 23, 2026 15:05
rajnisht7
rajnisht7 previously approved these changes Oct 25, 2026

@lywinged lywinged left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Request changes. Reviewed at 3a50396 on c5d49a5.

What I checked rather than read:

  • The reproduction in #616, on main at dcca4e0: parentRecordHash(123) and bundleDigest(123) resolve to the SHA-256 of the text 123, and verifyDelegationLink returns a link for parent 123. On this head all three reject: parentRecordHash and verifyDelegationLink with record_not_object, bundleDigest with invalid_argument.
  • The six inputs the description lists, through all three functions: all 18 calls reject, with the codes and messages the description gives.
  • npm ci && npm test in trust-record-verify: 162 of 162 tests pass on Node 20.20.0 and 22.22.0, and the test workflows, Node 24 included, are green at this head.
  • The differential job on Node 22: 1646 cases, 0 unexpected. Its TypeScript verdicts are byte-identical to main's, 134 chain digests included.
  • CI's commands, run on 3.11 and 3.12: ruff, check_dashes.py and mypy are clean, and pytest gives 4787 passed, 48 skipped. The LC_ALL=C, -OO and replay steps exit 0.
  • Removing the parentRecordHash guard, or the bundleDigest guard, fails one Node test and one Python test.

The one thing.

checkRevocationBundle catches only canonicalization_failed from bundleDigest, so the new invalid_argument escapes it. A class instance or an Object.create({}) object carrying a valid bundle's members now throws instead of being reported. With the bundle and context of examples/revocation-bundle/01-fresh-well-inside-both-bounds.json, checkRevocationBundle(Object.assign(new (class Bundle {})(), bundle), options) returns unverified_for_revocation with cause bundle_malformed on main. This head throws invalid_argument. Passed to verifyRecord as revocationBundle, the same bundle now makes it throw where main resolves.

123 as the bundle still gets bundle_malformed: what throws now is an object that is not a plain one. The doc comment of checkRevocationBundle says "Every other defect in the bundle is an outcome, not an exception", #616 expects a bundle that is not a JSON object to be reported as bundle_malformed at step 1, and by this pull request's own bundleDigest guard these are not JSON objects, and Python's verify_record reports a non-dict mapping as bundle_malformed at /. This, right after the shape step, does all three:

  if (!isPlainObject(bundle)) {
    return unverified("bundle_malformed", { path: "/", error: "the bundle is not a plain JSON object" });
  }

With it, and a test that passes class-instance and Object.create({}) copies of the bundles of vectors 01 and 20, npm test passes 163 of 163, and the test fails on this head. Catching invalid_argument around bundleDigest instead gives back exactly what main does, which reports the copy of vector 20 at statements/0/log_id, after step 2; the check above also meets #616's step 1 and agrees with Python.

Things I would not hold the merge for:

  • No test needs the || !isPlainObject(parent) clause. Without it, all 162 Node tests and the whole Python suite still pass, because a non-object parent reaches the guard in parentRecordHash anyway. Its only effect is order: verifyDelegationLink({}, 123) gives record_not_object here and delegation_absent without it, and refusing the parent first is what #588's verify_delegation_link does. Adding await rejects(verifyDelegationLink({}, bad), "record_not_object") to the new loop pins it.
  • The description and commit message say the delegation guards match Python agentrust_trace.delegation, which is #588's module and not on main yet. Against #588's head they part in one place: parent_record_hash(123, "md5") there refuses the algorithm first, where parentRecordHash(123, "md5") here refuses the record. The bundleDigest half holds: bundle_digest raises ValueError for None, 123, True, False, "record", [] and [1, 2], and gives the same digest for the vector 01 bundle.

Generated by Claude Code

@drk-ram

drk-ram commented Oct 29, 2026

Copy link
Copy Markdown
Contributor Author

Thank you @lywinged! Addressed all points in c94f4f9 (rebased onto origin/main at e9ab302):

  1. checkRevocationBundle non-plain object guard at Step 1:
    • Added if (!isPlainObject(bundle)) { return unverified("bundle_malformed", { path: "/", error: "the bundle is not a plain JSON object" }); } at the start of Step 1 in trust-record-verify/src/revocation.ts, and added checkRevocationBundle and verifyRecord report non-plain bundle objects as bundle_malformed in trust-record-verify/tests/units.test.mjs testing class-instance and Object.create({}) copies of vectors 01 and 20 through both checkRevocationBundle and verifyRecord.
  2. Pinned !isPlainObject(parent) in verifyDelegationLink and aligned parentRecordHash algorithm check order:
    • Added await rejects(verifyDelegationLink({}, bad), "record_not_object") to the non-object loop in trust-record-verify/tests/units.test.mjs.
    • Checked algorithm before isPlainObject(record) in parentRecordHash so parentRecordHash(123, "md5") rejects with delegation_algorithm_unsupported, matching Python agentrust_trace.delegation.parent_record_hash(123, "md5") on main.

@drk-ram
drk-ram force-pushed the drk-ram-fix-ts-non-object-inputs-616 branch from 3a50396 to c94f4f9 Compare October 29, 2026 15:07

@lywinged lywinged left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Request changes. Reviewed at c94f4f9 on e9ab302.

The three changes the first round asked for are in: the plain-object guard at the top of checkRevocationBundle, ahead of its schema check; the verifyDelegationLink({}, bad) case; and parentRecordHash checking the algorithm before the record. The new test for the first is the problem. npm test here, on Node 22, passes 164 of 165: checkRevocationBundle and verifyRecord report non-plain bundle objects as bundle_malformed reads 20-statement-for-another-log-rejected.json, and the vector in examples/revocation-bundle is 20-malformed-statement-on-another-log.json. With the name corrected it still fails, at verifyRecord(vec.record, ...): the vectors carry records, an array, so vec.record is undefined and verifyRecord throws record_not_object. With vec.records[0] as well, all 165 pass, and the test then pins the guard: without it, or with a typeof bundle === "object" guard in its place that the class instance and the Object.create({}) copy get past, the test fails where bundleDigest throws invalid_argument through checkRevocationBundle, which is the first round's symptom. The checklist line ticked when the pull request opened, that npm --prefix trust-record-verify test passes, does not hold for this head, and the trust-record-verify workflow runs that command on Node 20, 22 and 24.

What I checked rather than read:

  • CI's commands, run here on 3.11 and 3.12: ruff, the dash check and mypy clean; the suite at 5221 passed and 98 skipped on both; the replay experiment, the two LC_ALL=C PYTHONUTF8=0 runs, the PYTHONOPTIMIZE=2 command and the two --help calls pass.
  • tests/test_trust_record_verify_citations_and_platform.py, 35 tests, fails one when the parentRecordHash guard or the bundleDigest guard is removed, and passes with the new shape guard removed, the parent clause removed or the algorithm order reversed, so the Python side pins the first round's two guards and none of this round's three changes.
  • Removing || !isPlainObject(parent) from verifyDelegationLink fails the verifyDelegationLink({}, bad) case with delegation_absent, and moving the record check in parentRecordHash back ahead of the algorithm check fails the parentRecordHash(123, "md5") case. On main, with #588 merged, Python's parent_record_hash(123, "md5") refuses the algorithm first, as parentRecordHash does here.
  • tools/review_bar.py reports the one changed Python test failing on behaviour at the merge base.

Generated by Claude Code

…h, verifyDelegationLink, and bundleDigest (agentrust-io#616)

Require isPlainObject(record) in parentRecordHash (after algorithm
validation, matching Python agentrust_trace.delegation.parent_record_hash)
and isPlainObject(parent) in verifyDelegationLink, failing with
record_not_object; require isPlainObject(bundle) in bundleDigest
(failing with invalid_argument) and in checkRevocationBundle (reporting
bundle_malformed at path "/" for non-plain objects such as class
instances or Object.create({})); and add Node and Python-to-TypeScript
tests.

Closes agentrust-io#616

Signed-off-by: Raghuvar Karthik Durga <177662415+drk-ram@users.noreply.github.com>
@drk-ram
drk-ram force-pushed the drk-ram-fix-ts-non-object-inputs-616 branch from c94f4f9 to 916cec5 Compare October 30, 2026 14:47
@drk-ram

drk-ram commented Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

@lywinged Rebased onto origin/main (b938790) in 916cec5, corrected the vector filename (20-malformed-statement-on-another-log.json) and record access (vec.records[0]) in trust-record-verify/tests/units.test.mjs (npm --prefix trust-record-verify test: 165 pass, 0 fail), and expanded tests/test_trust_record_verify_citations_and_platform.py so the Python suite also pins parentRecordHash(123, 'md5'), verifyDelegationLink({}, 123), and non-plain checkRevocationBundle.

Copy link
Copy Markdown
Contributor

@drk-ram The two-line fixture correction is available for adoption on top of current c94f4f9: dinakarjs#7 (commit 97d3ebd). It uses the committed vector filename and vec.records[0]. On Node 24.22.1, npm test changes from 164 passed/1 failed to 165 passed/0 skipped; removing only the checkRevocationBundle plain-object guard makes the repaired test fail, and restoring it returns all 165 to passing. Runtime source is unchanged. The Node 20/22 and hosted matrices remain for normal review. Prepared with OpenAI Codex; DCO signed, original history preserved.

@drk-ram

drk-ram commented Nov 8, 2026

Copy link
Copy Markdown
Contributor Author

The current head 916cec5 already contains both fixture corrections: the committed vector filename and vec.records[0]. I reran npm --prefix trust-record-verify test: 165 passed, 0 failed, 0 skipped. I updated the description to match this head and the current template. The hosted verifier workflow is green; please re-review the remaining changes request.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-review:UNKNOWN Contributor check flagged UNKNOWN risk

Projects

None yet

Development

Successfully merging this pull request may close these issues.

trust-record-verify: reject non-object inputs in parentRecordHash, verifyDelegationLink, and bundleDigest

4 participants