Skip to content

fix(cryptography): verify public key matches private key in BasicCredential::from_raw#3801

Open
Sertug17 wants to merge 1 commit into
xmtp:mainfrom
Sertug17:fix/3799-fromraw-pubkey-verify
Open

fix(cryptography): verify public key matches private key in BasicCredential::from_raw#3801
Sertug17 wants to merge 1 commit into
xmtp:mainfrom
Sertug17:fix/3799-fromraw-pubkey-verify

Conversation

@Sertug17

@Sertug17 Sertug17 commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Closes #3799

Add a verification step after constructing the SigningKey to ensure
the caller-supplied public key corresponds to the private key.
Previously, (private_key_A, public_key_B) pairs were accepted,
enabling signature forgery where an attacker could sign with their own
key but claim a victim's public key.

Note

Verify public key matches private key in BasicCredential::from_raw

  • Adds validation in BasicCredential::from_raw to reject mismatched private/public key pairs, confirmed by a new test in basic_credential.rs.
  • Updates browser SDK tests to replace fixed sleep() delays with polling via waitFor, reducing timing-related flakiness in device sync and preferences tests.
  • Extends the waitFor helper in helpers.ts to support async predicates by awaiting the condition on each poll iteration.

Macroscope summarized a6d4cd3.

@Sertug17 Sertug17 requested a review from a team as a code owner June 29, 2026 18:35
@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.69%. Comparing base (7a0f50f) to head (a6d4cd3).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3801      +/-   ##
==========================================
+ Coverage   84.54%   84.69%   +0.15%     
==========================================
  Files         410      409       -1     
  Lines       60716    60902     +186     
==========================================
+ Hits        51331    51583     +252     
+ Misses       9385     9319      -66     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Sertug17 Sertug17 force-pushed the fix/3799-fromraw-pubkey-verify branch from 65a8b49 to 6194ca0 Compare July 3, 2026 09:48
…tests (xmtp#3786)

Resolves xmtp#3784

## Problem

Two browser-sdk (WASM) tests fail intermittently in CI
(`test-browser-sdk` shard 1). Both share one root cause: a fixed delay
is used as a proxy for *"the async, network-driven operation has
completed"*, which is racy under CI timing variability.

- **`DeviceSync.test.ts` → "should sync device archive …"**: after
`alix.sendSyncArchive("123")`, the test did a single
`syncAllDeviceSyncGroups()` pass and immediately called
`alix2.processSyncArchive("123")`. When the archive payload hadn't
propagated to `alix2`, it threw `DeviceSyncError::MissingPayload ("Could
not find payload with pin Some(\"123\")")`.
- **`Preferences.test.ts` → "should stream preferences"**: expected 4
updates (2 `ConsentUpdate` + 2 `HmacKeyUpdate` from two new
installations) but ended the stream after a fixed `setTimeout(…, 100)`.
A late update left only 3 collected (`expected 3 to be 4`).

## Fix

- **DeviceSync**: retry `processSyncArchive("123")` — re-syncing the
device-sync groups on each attempt — until the pinned payload is found,
bounded by a 30s timeout.
- **Preferences**: collect stream updates concurrently and keep the
stream open, re-syncing the streaming client, until all 4 expected
updates are observed (bounded by 30s), then close it deterministically.
- Extend the shared `waitFor` test helper to accept async
(`Promise<boolean>`) conditions in addition to sync ones. Backward
compatible — `await` on a synchronous boolean returns it unchanged.

All existing assertions (counts, types, entities, states, message
contents) are unchanged. This is a **test-only** change; production
WASM/SDK code is untouched.

## Validation

- `yarn lint` (eslint) passes on the three changed files.
- The new control-flow patterns (extended `waitFor`, retry loop,
concurrent collector) were typechecked in isolation. The only
outstanding workspace `tsc` errors are the pre-existing unbuilt
`@xmtp/wasm-bindings` portal, unrelated to this change.
- Behavioral validation is the CI `test-browser-sdk` job (builds WASM
bindings, runs Playwright/Chromium against a local XMTP node).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

<!-- Macroscope's pull request summary starts here -->
<!-- Macroscope will only edit the content between these invisible
markers, and the markers themselves will not be visible in the GitHub
rendered markdown. -->
<!-- If you delete either of the start / end markers from your PR's
description, Macroscope will append its summary at the bottom of the
description. -->
> [!NOTE]
> ### De-flake DeviceSync archive and Preferences stream tests by
replacing fixed sleeps with polling
> - Replaces fixed `sleep` calls in
[`DeviceSync.test.ts`](https://github.com/xmtp/libxmtp/pull/3786/files#diff-3b766774cb49132df20a7115e21a02c1f13baa791d2575124b09ed9e57b96fd6)
with a `waitFor` loop that retries `processSyncArchive` until it
succeeds (up to 30s, 1s interval).
> - Replaces fixed sleeps and `setTimeout`-based stream termination in
[`Preferences.test.ts`](https://github.com/xmtp/libxmtp/pull/3786/files#diff-dfca7a9e688244959eead5c9545b25d98bd0b8535025e8244ba16410fa5566d3)
with a `waitFor` loop that polls until 4 preference updates are observed
before ending the stream.
> - Extends `waitFor` in
[`helpers.ts`](https://github.com/xmtp/libxmtp/pull/3786/files#diff-991c5f12cc0b3b6bd46e844ea6e682f635b513eff525462e05425c9cd33660ee)
to accept async conditions (returning `Promise<boolean>`).
>
> <!-- Macroscope's review summary starts here -->
>
> <sup><a href="https://app.macroscope.com">Macroscope</a> summarized
b9f195b.</sup>
> <!-- Macroscope's review summary ends here -->
>
<!-- macroscope-ui-refresh -->
<!-- Macroscope's pull request summary ends here -->

Co-authored-by: insipx <insipx@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
@Sertug17 Sertug17 force-pushed the fix/3799-fromraw-pubkey-verify branch from 6194ca0 to a6d4cd3 Compare July 3, 2026 10:08
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.

bug(cryptography): BasicCredential::from_raw does not verify public key matches private key

2 participants