[dmihalcik-virtru]

Stacked on #1034. This PR's base is DSPX-4703-validated-manifest-type.
Review #1031 and #1034 first.

The bug

Raised by CodeRabbit on #1031. The finding is real; the diagnosis and the
proposed fix were both pointed the wrong way, so this lands separately rather
than as a patch to that PR.

TDF spec 4.2.2 differs from the current spec in encoding only. Where a
modern manifest stores base64(mac), a 4.2.2 manifest stores
base64(hex(mac)). The MAC itself covers the same bytes either way — the Go
SDK's calculateSignature is one if isLegacyTDF wrapped around the encoding,
with identical input on both sides of it:

func calculateSignature(data []byte, secret []byte, alg IntegrityAlgorithm, isLegacyTDF bool) (string, error) {
	if alg == HS256 {
		hmac := ocrypto.CalculateSHA256Hmac(secret, data)
		if isLegacyTDF {
			return hex.EncodeToString(hmac), nil
		}
		return string(hmac), nil
	}
	...

segmentIntegrityVersion422 disagreed. Before HMACing a segment it ran the
ciphertext through a UTF-8 decode and re-encoded the result:

const content = buffToString(new Uint8Array(payloadBinary.asArrayBuffer()), 'utf-8');
const sig = await cryptoService.hmac(new TextEncoder().encode(content), unwrappedKey);

That decode replaces every byte sequence that is not valid UTF-8 with U+FFFD,
and re-encoding writes back three bytes (EF BF BD) where one used to be.
AES-GCM output is indistinguishable from random, so nearly every segment was
mangled and the writer signed a digest that nothing could reproduce — not Go,
not Java, and not this SDK's own decryptChunk, which HMACs the ciphertext as
it stands. A 4.2.2 file this SDK wrote with HS256 segments could not be
opened by this SDK.

GMAC hid it for the whole life of the 4.2.2 path: that branch reads the AEAD
tag back out of the trailing bytes and never touches the decode, and GMAC is
the default segment algorithm. The broken combination was simply never the one
anyone ran.

This is not a regression from #1030 or #1031. Pre-#1030 decryptChunk called
getSignature — raw HMAC, same hex-then-base64 encoding — while the writer
called getSignatureVersion422. #1030 renamed the pair and #1031 split it four
ways; the asymmetry is older than both and untouched by either.

The fix

The reader was right and the writer was wrong, so the writer moves. CodeRabbit
proposed the opposite — teach decryptChunk to call the 422 variant — which
would have been symmetric and still wrong: it makes us self-consistent, and
alone. Go and Java would keep producing legacy HS256 files we reject, and we
would keep producing ones they reject.

rootIntegrityVersion422 carried the same decode. It was inert there, not
wrong: a legacy aggregate hash is a run of hex digits, so the round trip is the
identity. It goes too, so the two legacy writers cannot drift apart again
without someone noticing.

No compatibility cost. The files the old writer produced were already
unreadable everywhere, this SDK included, so nothing that worked stops working.

How to test

cd lib && npm run build && npx mocha 'dist/web/tests/mocha/legacy-422-integrity.spec.js'

lib/tests/mocha/legacy-422-integrity.spec.ts is new — 5 cases:

• round trips (2) — a 4.2.2 file under GMAC and under HS256 segments.
• encoding pins (3) — recompute each segment hash and the root signature
  from the DEK and the payload bytes read back out of the zip, and compare
  against the manifest. These pin the encoding to the cross-SDK definition
  rather than to whatever the writer happens to emit, which is the point: a
  round-trip assertion alone would be satisfied by the symmetric-but-wrong fix.

Baseline

Against this branch's base, 2 of the 5 fail — the HS256 round trip
(IntegrityError: Failed integrity check on segment hash) and the HS256
segment hash comparison. The three GMAC and root cases stay green, which is
the same reason nobody caught this: they were never affected.

With the fix the whole suite is green: 432 passing / 6 pending under mocha, 432
under karma, 253 under web-test-runner, and tsc --noEmit and npm run lint
clean.

Risk

Write path, legacy spec version only. Changes the bytes we emit for
tdfSpecVersion: '4.2.2' + segmentIntegrityAlgorithm: 'HS256', a combination
that produced unreadable files before this. Every other combination — including
the 4.2.2 default, GMAC — is byte-for-byte unchanged, as the encoding pins
above assert.

Interop note

Worth an xtest case: 4.2.2 + HS256 segments, written by each SDK and read by
each of the others. Nothing exercises that cell of the matrix today, which is
how this survived.

DSPX-4703

[coderabbitai]

Warning

Review limit reached

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 16512bd5-85dc-4818-b507-19372dc56b94

📥 Commits

Reviewing files that changed from the base of the PR and between f03ddbc and bae2d96.

📒 Files selected for processing (2)

• lib/tdf3/src/tdf.ts
• lib/tests/mocha/legacy-422-integrity.spec.ts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[github-actions]

X-Test Results

coverage-web-app
opentdf-sdk-lib
coverage-web-app-browser
✅ js@v0.4.0-v0.9.0
✅ js@v0.4.0-main
✅ js@pull-1037-main
coverage-lib
✅ js@main-v0.9.0
coverage-roundtrip-browser
✅ go@main-main
opentdf-ctl
✅ java@main-main
✅ js@main-main
✅ js@pull-1037-v0.9.0
✅ go@main-v0.9.0
✅ java@main-v0.9.0

[dmihalcik-virtru]

I'm pretty sure this is not needed since this is supporting an de-launched experimental feature