chore(xtest): cross-SDK test for duplicate KAO on same KAS+split (DSPX-3379)#555
Open
dmihalcik-virtru wants to merge 2 commits into
Open
chore(xtest): cross-SDK test for duplicate KAO on same KAS+split (DSPX-3379)#555dmihalcik-virtru wants to merge 2 commits into
dmihalcik-virtru wants to merge 2 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds multikao support detection for SDK feature checks and a tamper test helper that duplicates the first KAO entry in a TDF manifest, then verifies decryption still succeeds. ChangesDuplicate KAO Tamper Test
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant TestHarness
participant SDK
participant JSCLI
TestHarness->>SDK: Check multikao support
alt Go or Java SDK
SDK-->>TestHarness: Return supported
else JavaScript SDK
SDK->>JSCLI: Run supports multikao
JSCLI->>JSCLI: Parse SDK version
JSCLI-->>SDK: Return version gate result
end
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request adds a test case test_tdf_with_duplicate_kao_same_kas and a helper function duplicate_first_kao in xtest/test_tdfs.py to ensure that a TDF file remains decryptable when the manifest contains duplicate Key Access Objects (KAOs) sharing the same split ID and KAS URI. There are no review comments, so I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
xtest/test_tdfs.py (1)
876-879: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider asserting
wrappedKeyequality to strengthen the "byte-identical" duplicate verification.The test verifies
urlandsidmatch between the original and duplicated KAO, but notwrappedKey— the field that actually matters for split unwrapping. The PR summary and docstring both claim a "byte-identical copy," so assertingwrappedKeyequality would make that claim explicit and catch any future regression in the copy logic that might alter the wrapped key.♻️ Suggested addition
assert kaos[0].url == kaos[-1].url assert kaos[0].sid == kaos[-1].sid + assert kaos[0].wrappedKey == kaos[-1].wrappedKey🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xtest/test_tdfs.py` around lines 876 - 879, The duplicate KAO check in the tdfs manifest test only verifies url and sid, so it does not fully prove the copy is byte-identical. Update the assertion block around tdfs.manifest(b_file).encryptionInformation.keyAccess to also compare wrappedKey between kaos[0] and kaos[-1], alongside the existing url and sid checks, so the test explicitly guards the split-unwrapping key material.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@xtest/test_tdfs.py`:
- Around line 876-879: The duplicate KAO check in the tdfs manifest test only
verifies url and sid, so it does not fully prove the copy is byte-identical.
Update the assertion block around
tdfs.manifest(b_file).encryptionInformation.keyAccess to also compare wrappedKey
between kaos[0] and kaos[-1], alongside the existing url and sid checks, so the
test explicitly guards the split-unwrapping key material.
dmihalcik-virtru
added a commit
to opentdf/web-sdk
that referenced
this pull request
Jul 9, 2026
splitLookupTableFactory now returns split id -> KeyAccessObject[] (a disjunction of alternatives) instead of a url-keyed map, and no longer throws when a KAS repeats within a split. unwrapKey feeds each alternative to anyPool with a unique key so duplicate KAS entries are each tried until one succeeds. Matches go-sdk/java-sdk behavior; validated by xtest test_tdf_with_duplicate_kao_same_kas (opentdf/tests#555). Adds unit tests for duplicate + same-KAS-different-KID, an in-process decrypt-path test, and fills spec/DSPX-3379.md.
dmihalcik-virtru
force-pushed
the
DSPX-3379-multisplit-allowed
branch
from
July 9, 2026 19:08
2e4565f to
2cdec32
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
xtest/sdk/js/cli.sh (1)
99-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the
jqfilter into a variable.SonarCloud flags
'.["@opentdf/sdk"]'as repeated 5 times across thesupportsblock. A local variable would reduce duplication and make future version checks cleaner. This is optional and can be deferred since it touches lines outside this PR's scope.♻️ Optional refactor
if [ "$1" == "supports" ]; then if ! cd "$SCRIPT_DIR"; then echo "failed: [cd $SCRIPT_DIR]" exit 1 fi + SDK_VERSION_FILTER='.["`@opentdf/sdk`"]' case "$2" in # ... multikao) # Tolerate a KAO array where the same KAS wraps the same split more than # once (DSPX-3379). Shipped in web-sdk >= 0.20.0. set -o pipefail - npx $CTL --version | jq -re '.["`@opentdf/sdk`"]' | awk -F. '{ if ($1 > 0 || ($1 == 0 && $2 >= 20)) exit 0; else exit 1; }' + npx $CTL --version | jq -re "$SDK_VERSION_FILTER" | awk -F. '{ if ($1 > 0 || ($1 == 0 && $2 >= 20)) exit 0; else exit 1; }' exit $? ;;The same
SDK_VERSION_FILTERvariable could then replace the literal in theecwrap,key_management,obligations, andattribute_traversalbranches.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xtest/sdk/js/cli.sh` around lines 99 - 105, Extract the repeated jq filter used in the supports block into a local variable, and update the multikao branch to use it through the existing cli.sh flow. Keep the change centered on the version-check logic around npx, jq, and awk so the same SDK version filter can be reused consistently in the other branches mentioned by SonarCloud (ecwrap, key_management, obligations, and attribute_traversal).Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@xtest/sdk/js/cli.sh`:
- Around line 99-105: Extract the repeated jq filter used in the supports block
into a local variable, and update the multikao branch to use it through the
existing cli.sh flow. Keep the change centered on the version-check logic around
npx, jq, and awk so the same SDK version filter can be reused consistently in
the other branches mentioned by SonarCloud (ecwrap, key_management, obligations,
and attribute_traversal).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f4f60caf-f898-4549-95ad-8090781a02f4
📒 Files selected for processing (3)
xtest/sdk/js/cli.shxtest/tdfs.pyxtest/test_tdfs.py
🚧 Files skipped from review as they are similar to previous changes (1)
- xtest/test_tdfs.py
dmihalcik-virtru
force-pushed
the
DSPX-3379-multisplit-allowed
branch
from
July 10, 2026 14:15
2cdec32 to
b8939fa
Compare
dmihalcik-virtru
changed the title
dmihalcik-virtru
added a commit
to opentdf/web-sdk
that referenced
this pull request
Jul 10, 2026
splitLookupTableFactory now returns split id -> KeyAccessObject[] (a disjunction of alternatives) instead of a url-keyed map, and no longer throws when a KAS repeats within a split. unwrapKey feeds each alternative to anyPool with a unique key so duplicate KAS entries are each tried until one succeeds. Matches go-sdk/java-sdk behavior; validated by xtest test_tdf_with_duplicate_kao_same_kas (opentdf/tests#555). Adds unit tests for duplicate + same-KAS-different-KID, an in-process decrypt-path test, and fills spec/DSPX-3379.md. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
added a commit
to opentdf/web-sdk
that referenced
this pull request
Jul 10, 2026
splitLookupTableFactory now returns split id -> KeyAccessObject[] (a disjunction of alternatives) instead of a url-keyed map, and no longer throws when a KAS repeats within a split. unwrapKey feeds each alternative to anyPool with a unique key so duplicate KAS entries are each tried until one succeeds. Matches go-sdk/java-sdk behavior; validated by xtest test_tdf_with_duplicate_kao_same_kas (opentdf/tests#555). Adds unit tests for duplicate + same-KAS-different-KID, an in-process decrypt-path test, and fills spec/DSPX-3379.md. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
…X-3379) Adds test_tdf_with_duplicate_kao_same_kas + duplicate_first_kao manifest mutation. Gated behind a new 'multikao' feature so SDK versions without the fix are skipped instead of failing the workflow: go/java always support it; web-sdk (js) reports support at >= 0.20.0 (where DSPX-3379 ships). Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
force-pushed
the
DSPX-3379-multisplit-allowed
branch
from
July 13, 2026 17:56
b8939fa to
6e4cf33
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
xtest/sdk/js/cli.sh (1)
103-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the SDK version jq filter constant.
This adds another occurrence of the literal
.["@opentdf/sdk"], which is already duplicated across the version checks. Define the filter once and reuse it to reduce maintenance drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xtest/sdk/js/cli.sh` at line 103, Define a shared variable for the jq SDK version filter used by the version checks, then update the command in the version check to reference that variable instead of repeating the literal .["`@opentdf/sdk`"].Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@xtest/sdk/js/cli.sh`:
- Line 103: Define a shared variable for the jq SDK version filter used by the
version checks, then update the command in the version check to reference that
variable instead of repeating the literal .["`@opentdf/sdk`"].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6dae0e98-b758-486c-9103-a480fcab9244
📒 Files selected for processing (3)
xtest/sdk/js/cli.shxtest/tdfs.pyxtest/test_tdfs.py
🚧 Files skipped from review as they are similar to previous changes (2)
- xtest/tdfs.py
- xtest/test_tdfs.py
dmihalcik-virtru
added a commit
to opentdf/web-sdk
that referenced
this pull request
Jul 16, 2026
splitLookupTableFactory now returns split id -> KeyAccessObject[] (a disjunction of alternatives) instead of a url-keyed map, and no longer throws when a KAS repeats within a split. unwrapKey feeds each alternative to anyPool with a unique key so duplicate KAS entries are each tried until one succeeds. Matches go-sdk/java-sdk behavior; validated by xtest test_tdf_with_duplicate_kao_same_kas (opentdf/tests#555). Adds unit tests for duplicate + same-KAS-different-KID, an in-process decrypt-path test, and fills spec/DSPX-3379.md. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
added a commit
to opentdf/web-sdk
that referenced
this pull request
Jul 16, 2026
splitLookupTableFactory now returns split id -> KeyAccessObject[] (a disjunction of alternatives) instead of a url-keyed map, and no longer throws when a KAS repeats within a split. unwrapKey feeds each alternative to anyPool with a unique key so duplicate KAS entries are each tried until one succeeds. Matches go-sdk/java-sdk behavior; validated by xtest test_tdf_with_duplicate_kao_same_kas (opentdf/tests#555). Adds unit tests for duplicate + same-KAS-different-KID, an in-process decrypt-path test, and fills spec/DSPX-3379.md. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
added a commit
to opentdf/web-sdk
that referenced
this pull request
Jul 16, 2026
splitLookupTableFactory now returns split id -> KeyAccessObject[] (a disjunction of alternatives) instead of a url-keyed map, and no longer throws when a KAS repeats within a split. unwrapKey feeds each alternative to anyPool with a unique key so duplicate KAS entries are each tried until one succeeds. Matches go-sdk/java-sdk behavior; validated by xtest test_tdf_with_duplicate_kao_same_kas (opentdf/tests#555). Adds unit tests for duplicate + same-KAS-different-KID, an in-process decrypt-path test, and fills spec/DSPX-3379.md. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
added a commit
to opentdf/web-sdk
that referenced
this pull request
Jul 16, 2026
splitLookupTableFactory now returns split id -> KeyAccessObject[] (a disjunction of alternatives) instead of a url-keyed map, and no longer throws when a KAS repeats within a split. unwrapKey feeds each alternative to anyPool with a unique key so duplicate KAS entries are each tried until one succeeds. Matches go-sdk/java-sdk behavior; validated by xtest test_tdf_with_duplicate_kao_same_kas (opentdf/tests#555). Adds unit tests for duplicate + same-KAS-different-KID, an in-process decrypt-path test, and fills spec/DSPX-3379.md. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
added a commit
to opentdf/web-sdk
that referenced
this pull request
Jul 16, 2026
splitLookupTableFactory now returns split id -> KeyAccessObject[] (a disjunction of alternatives) instead of a url-keyed map, and no longer throws when a KAS repeats within a split. unwrapKey feeds each alternative to anyPool with a unique key so duplicate KAS entries are each tried until one succeeds. Matches go-sdk/java-sdk behavior; validated by xtest test_tdf_with_duplicate_kao_same_kas (opentdf/tests#555). Adds unit tests for duplicate + same-KAS-different-KID, an in-process decrypt-path test, and fills spec/DSPX-3379.md. --------- Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds a cross-SDK xtest,
test_tdf_with_duplicate_kao_same_kas, asserting that aTDF whose
keyAccessarray contains two elements with the same split id andthe same KAS URI still decrypts on every SDK.
Uses the existing
update_manifesthelper to append a byte-identical copy of thefirst KAO (new change function
duplicate_first_kao), producing a valid,decryptable duplicate — either copy unwraps the split.
Why (DSPX-3379)
The same KAS wrapping the same split is legitimate — an authoring mistake (same
key used twice) or an intentional multi-key-on-one-KAS setup. Expected behavior:
the file decrypts (or fails with permission-denied) exactly as if the copies
lived on distinct URIs.
This is the executable spec for the fix:
keep reconstructing) → expected pass.
InvalidFileError: Multiple keys detected for Key Access Server [...]insplitLookupTableFactory→ expected failuntil the companion web-sdk fix lands, after which this goes green for
js.Test status
Merging this test first intentionally red-lights
jsdecrypt to lock in theexpected behavior; the opentdf/web-sdk fix (branch
DSPX-3379-multisplit-allowed)makes it pass.
Local validation
pytest --collect-onlydiscovers the new test against a running platform(v0.9.0). A full cross-SDK roundtrip run requires the SDK CLI wrappers
(
otdf-sdk-mgr checkout --all+make), which were not built in thisenvironment.
Summary by CodeRabbit