Skip to content

test: cover ECDH deriveBits length boundaries - #249

Open
harrshita123 wants to merge 3 commits into
google:masterfrom
harrshita123:issue-130-ecdh-derivebits-docs
Open

harrshita123 wants to merge 3 commits into
google:masterfrom
harrshita123:issue-130-ecdh-derivebits-docs

Conversation

@harrshita123

@harrshita123 harrshita123 commented Feb 7, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #289.

This change adds explicit ECDH deriveBits boundary coverage for each supported curve:

  • exercises the exact maximum length in generated cases;
  • verifies the documented maximum succeeds; and
  • verifies the first invalid bit length is rejected.

The tested limits are 256 bits for P-256, 384 bits for P-384, and 528 bits for P-521.

Comment thread lib/src/webcrypto/webcrypto.ecdh.dart Outdated
Comment thread test/crypto_subtle_test.dart Outdated
@jonasfj

jonasfj commented Apr 24, 2026

Copy link
Copy Markdown
Member

Please rebase, I think CI mostly works on master branch now.

@harrshita123

Copy link
Copy Markdown
Contributor Author

Please rebase, I think CI mostly works on master branch now.

I have rebased the branch onto the latest master so the CI tests can run properly.

Comment thread test/crypto_subtle_test.dart Outdated
@harrshita123
harrshita123 force-pushed the issue-130-ecdh-derivebits-docs branch from 3797510 to 0c84b6e Compare June 30, 2026 18:33
@harrshita123

harrshita123 commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

I updated the branch based on the feedback:

  • Rebased the PR onto the latest master, which resolved the merge conflict.
  • Kept the ECDH deriveBits documentation change focused on the maximum length for each curve.
  • Moved the new ECDH deriveBits boundary tests out of crypto_subtle_test.dart and into the package test runner under lib/src/testing/ecdh/derive_bits.dart.
  • Simplified the tests to use shared curve cases for P-256, P-384, and P-521.
  • Left the JSDomException behavior out of this PR since it was called out as a separate issue.

I also cleaned up the branch history into a single commit and verified it locally with dart format, dart analyze, and dart test test/webcrypto_test.dart -p vm.

@HamdaanAliQuatil HamdaanAliQuatil left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

these tests are still useful even though #130 was closed by #289. the generated cases added there don’t actually hit the documented limits: maxDeriveLength is a bit count, but those calls pass 32/48/66, and the random range excludes its upper bound.

could we correct those calls here by setting both minDeriveLength and maxDeriveLength to 256/384/528 respectively?

since #130 is already closed, I’d also reframe this as a follow-up to #289, replace Fixes #130, and perhaps rename the PR to test: cover ECDH deriveBits length boundaries.

Comment thread lib/src/testing/ecdh/derive_bits.dart Outdated
@harrshita123 harrshita123 changed the title Document and test maximum deriveBits length for ECDH curves test: cover ECDH deriveBits length boundaries Jul 24, 2026
@harrshita123
harrshita123 force-pushed the issue-130-ecdh-derivebits-docs branch from ffb25b7 to 509717b Compare September 5, 2026 10:21
maxDeriveLength: 66,
minDeriveLength: 528,
maxDeriveLength: 528,
);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think these runner.generate things spit out JSON that we put into _testData.

Feel free to make a follow up PR, where we generate a bunch of these with the configurations you put in here. I've usually generated in chrome/firefox/boringssl, but just have one or two of those configurations could be useful.

Mostly those _testData entries serves as sanity tests and pins behavior.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for clarifying how runner.generate is intended to be used. I’ll keep that work separate from this PR and add a focused follow-up with generated maximum-length vectors. P-521 already has 528-bit vectors, so the follow-up will add the missing 256-bit P-256 and 384-bit P-384 BoringSSL cases.

Comment thread lib/src/testing/ecdh/derive_bits.dart Outdated
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.

3 participants