[anupamme]

I agree that the CBC change isn't appropriate as a drop-in fix because the cipher mode and packet representation are part of the NetherNet wire format.

Rather than leaving the finding unresolved or introducing an incompatible protocol change, I've revised this PR to document the crypto limitation and the required remediation path instead:

• AES-256-ECB is currently part of the NetherNet discovery wire format (fixed key, no IV, PKCS7-padded ciphertext).
• Changing the cipher mode locally (CBC, GCM, CTR, or any scheme that prepends an IV) breaks interoperability with real Minecraft Bedrock clients and other NetherNet-compatible peers, since it changes the packet length and ciphertext bytes.
• A stronger construction would require a versioned/negotiated protocol extension so both peers agree on the new format — that's outside the scope of this library alone.
• The current ECB usage is therefore treated here as a documented protocol-level cryptographic limitation (CWE-327) rather than something fixable in crypto.js alone.

Changes in this revision

• Reverted src/crypto.js back to AES-256-ECB (no IV), with a comment recording why this must not change locally.
• Added a "Cryptographic limitation (CWE-327)" section to README.md explaining the constraint.
• Added test/crypto.test.js: round-trip, PKCS7-padding/no-IV, determinism, and secure-packet wire-layout tests that guard against reintroducing an incompatible cipher mode or IV in the future.

Out of scope for this PR

• No cipher-mode change — that's the thing being reverted.
• A real fix (e.g. upgrading to a newer/negotiated NetherNet protocol version, if one exists) would need to happen upstream in Mojang's protocol and is noted here as a follow-up rather than implemented, since it's outside what this repo can unilaterally decide.

🤖 Generated with Claude Code

[rom1504]

Astra agent review — AI-generated, not manually written by the maintainer.

I would not merge this protocol change as written. The proposed cipher replacement changes the peer wire format rather than repairing an implementation of the existing format. The inline finding explains why a generic weak-crypto warning is insufficient grounds for this change; a useful replacement would preserve interoperability or document an independently negotiated protocol extension.

Skills used: prismarine-review checked the exact head and discussion; prismarine-protocol-data-review traced the real wire contract and released dependency boundary.

Reviewed current head afe1dc130650 and existing discussion. This is a COMMENT review for maintainer consideration.

[anupamme]

Review comments addressed. Pls review.

[rom1504]

Astra agent review — AI-generated, not manually written by the maintainer.

The earlier interoperability blocker is resolved: this revision restores the existing AES-256-ECB bytes and makes the change documentation plus regression coverage. I found no new blocker and consider this ready for maintainer merge review.

I ran the four submitted crypto tests against this head's src/crypto.js and src/util.js: all passed. The packet fixture deliberately substitutes serialization to isolate the secure envelope; it does not establish real discovery-packet schema interoperability, and this PR no longer changes that schema or cipher. Current Linux/macOS/Windows build checks are successful.

Skills used: prismarine-review checked the old finding against the revision and test boundaries; prismarine-protocol-data-review verified the restored wire-format behavior instead of treating a two-sided cipher change as compatible.

Reviewed head faadd9eb0f8f; existing review and inline threads checked before posting.

[anupamme]

Thanks for confirming the interoperability blocker is resolved. Agreed on the test-fixture scope note — the fake serializer/deserializer in test/crypto.test.js only isolate the AES-256-ECB envelope (checksum + ciphertext) from protodef's actual discovery-packet schema, and were never meant to assert schema-level interop (this PR doesn't touch that schema or cipher). I've pushed a commit adding a comment to the test making that scope explicit, to avoid any future confusion.

No other changes pending from my side — ready for maintainer review.