Skip to content

fix(wp): remove sjcl fallback in eddsa mpcv2 and screen v1 cipher mode - #10070

Merged
pranavjain97 merged 4 commits into
masterfrom
pranavjain/wcn-3831-remove-sjcl-fallback-in-eddsampcv2-and-screen-v1-cipher-mode
Nov 6, 2026
Merged

pranavjain97 merged 4 commits into
masterfrom
pranavjain/wcn-3831-remove-sjcl-fallback-in-eddsampcv2-and-screen-v1-cipher-mode

Conversation

@pranavjain97

@pranavjain97 pranavjain97 commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Reject hostile v1 envelopes before they reach SJCL.

  • EdDSA recovery helpers (no bitgo passed): enforce the 100,000 iter cap and ccm-only mode.
  • decryptV1WithFallback: reject any explicit mode other than ccm, in browser and Node.

No signature changes. BitGo only wrote CCM with iter 10,000, so real keycards are unaffected.

Ticket: WCN-3831

SJCL reads the cipher mode from the envelope and implements the weak OCB2 mode.
BitGo only wrote CCM, so refuse any other explicit mode before SJCL runs, in both
the browser and Node paths. An absent mode still defaults to CCM.

Ticket: WCN-3831
Without a bitgo instance the recovery helpers called sjcl.decrypt directly, skipping
the 100,000 iteration cap the main decrypt path enforces. Screen iter and mode before
SJCL runs. Signatures are unchanged.

Ticket: WCN-3831

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

Tip: disable this comment in your organization's Code Review settings.

@linear-code

linear-code Bot commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

WCN-3831

@pranavjain97

Copy link
Copy Markdown
Contributor Author

@claude review

@github-actions

github-actions Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @pranavjain97's task in 22s —— View job


Review of #10070

No blocking issues. The change does what the title says, and I found no other raw sjcl.decrypt call sites in modules/*/src. The only ones left are the guarded calls in decryptV1WithFallback and the new decryptLegacyV1.

What I checked

  • assertCcmMode (sdk-api/src/encrypt.ts) runs before both the browser and node paths, so a non-ccm mode can't reach SJCL. Absent mode is still allowed, so legacy envelopes keep working. Non-JSON input and JSON that isn't an object (null, a string) fall through to the existing decoder errors without crashing.
  • decryptLegacyV1 (eddsaMPCv2.ts) applies the same iter cap (100,000) and mode check, and all three no-bitgo call sites now use it: isEddsaMpcV1SigningMaterial, getEddsaMpcV2RecoveryKeySharesFromReducedKey and getEddsaSigningMaterial.

Minor suggestions (non-blocking)

  1. Duplicated cap. LEGACY_V1_MAX_ITER is a copy of sdk-api's V1_MAX_ITER, and the two can drift apart. If sdk-core can import from sdk-api without a dependency cycle, export and reuse the constant. Otherwise the comment is a fine stopgap.
  2. Timing assertion. assert.ok(Date.now() - start < 200, ...) in the sdk-core test could be flaky on a slow CI runner. The /iter out of range/ rejection already shows the cap fired before SJCL ran, so I'd drop the timing check or loosen it a lot.
  3. Error message mismatch. decryptLegacyV1 says iter out of range, expected an integer between 1 and N, while sdk-api's isIterCapViolation matches iter: expected integer|iter out of range. They agree today, but the messages are written separately, so a later wording change could break the match.
  4. Test gap. There's no test for a non-integer iter (e.g. "10000" as a string or 1.5) in the sdk-core path. The code handles it, so a one-line case would pin it down.

I haven't run the tests here.

Move the iter cap and ccm-only mode check into one sdk-core helper used by the EdDSA
recovery fallback and sdk-api decrypt, so the two cannot drift. Drop a flaky timing
assertion and cover non-integer iter and non-object input in tests.

Ticket: WCN-3831

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

@github-actions

github-actions Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

⚠️ Unit tests are failing on Node 26.x (Current release line, non-blocking). This is not an LTS version yet, so it does not block merge, but it signals an incompatibility to fix before Node 26.x becomes LTS.

View run

SJCL strips all whitespace before decoding, so a key like "it er" is iter to SJCL but
unknown to JSON.parse, which let a hostile iter or mode past the guards. Read the envelope
with sjcl.json.decode instead, in the shared helper used by sdk-api and the EdDSA
recovery fallback.

Ticket: WCN-3831
@OttoAllmendinger
OttoAllmendinger removed their request for review October 31, 2026 10:02
@pranavjain97
pranavjain97 merged commit 446a8cf into master Nov 6, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants