Skip to content

fix: handle AMF3 JSON values and invalid decode input#2528

Open
skyswordw wants to merge 3 commits into
gchq:masterfrom
skyswordw:fix-amf3-json-value-encoding
Open

fix: handle AMF3 JSON values and invalid decode input#2528
skyswordw wants to merge 3 commits into
gchq:masterfrom
skyswordw:fix-amf3-json-value-encoding

Conversation

@skyswordw

@skyswordw skyswordw commented Jun 6, 2026

Copy link
Copy Markdown

Summary

  • encode AMF3 JSON values in CyberChef for booleans, null, arrays, strings, numbers, and plain objects instead of routing those cases through the broken @astronautlabs/amf Value.any() path
  • keep AMF0 encoding on the existing library path
  • convert empty, invalid, or truncated AMF decode input into clear CyberChef OperationError messages instead of surfacing low-level parser errors
  • keep AMF encode and decode coverage in the existing AMF.mjs operation test file

Fixes #2490.
Fixes #2491.
Fixes #2492.

Validation

  • npx grunt configTests
  • focused AMF operation tests (11 passing)
  • npm run lint
  • NODE_OPTIONS=--openssl-legacy-provider npm test (245 Node API tests and 1971 operation tests passing)
  • git diff --check

Notes: npm ci completed under Node v24.16.0 earlier in this branch. It reported the repository's existing deprecation/audit warnings, including 36 vulnerabilities; no dependency changes were made.

Development disclosure

I developed this change collaboratively with OpenAI Codex, with local review and validation before submission.

@skyswordw skyswordw changed the title fix: encode AMF3 JSON boolean, null, and arrays fix: handle AMF3 JSON values and invalid decode input Jun 6, 2026
Comment thread tests/operations/tests/AMF.mjs
@skyswordw skyswordw force-pushed the fix-amf3-json-value-encoding branch from 1bb6546 to 378eb0c Compare June 6, 2026 12:38
@skyswordw

Copy link
Copy Markdown
Author

Thanks, I went with option 2 here. The AMF decode tests have been folded into AMF.mjs, AMFDecode.mjs has been removed from the operation test imports, and I added a small AMF3 encode/decode round-trip smoke test.

Validated locally with Node v24.16.0:

  • focused AMF operation tests (11 passing)
  • npm run lint
  • NODE_OPTIONS=--openssl-legacy-provider npm test (245 Node API tests and 1971 operation tests passing)
  • git diff --check

@GCHQDeveloper581

Copy link
Copy Markdown
Contributor

Just checking - this PR is showing as "Draft"/"Not Ready" which indicates that it is not yet ready for merge. If you are still working on it that's absolutely fine, but if it's actually ready for formal review then could you change the state to "Ready for Review".

@skyswordw

Copy link
Copy Markdown
Author

Thanks for checking. The follow-up change is in and the review thread is resolved, so I will move this to Ready for Review now.

@skyswordw skyswordw marked this pull request as ready for review June 7, 2026 03:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants