Add NapiClient.disableVerify() and use NAPI tokio runtime#205
Add NapiClient.disableVerify() and use NAPI tokio runtime#205
Conversation
- Add disableVerify() factory method to skip TLS certificate verification, for testing with self-signed certificates - Use napi::within_runtime_if_available for sync methods that need tokio context (quinn binds UDP sockets to the current runtime) - Add SessionOptions.serverCertificateDisableVerify to the JS polyfill - Rename ts/ to src/ in @moq/web-transport - Fix build script to use --manifest-path and --output-dir Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughPackage metadata and build targets were moved from 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
js/web-transport/src/session.ts (1)
50-50: Update constructor signature to exposeSessionOptionsin the public API.The constructor overload uses
WebTransportOptions, but the implementation acceptsSessionOptions. TypeScript consumers won't seeserverCertificateDisableVerifyin IDE autocomplete.♻️ Proposed fix
// Construct from URL (client-side polyfill) - constructor(url: string | URL, options?: WebTransportOptions); + constructor(url: string | URL, options?: SessionOptions);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/web-transport/src/session.ts` at line 50, The constructor overload currently declares (constructor(url: string | URL, options?: WebTransportOptions)) but the implementation expects SessionOptions, so update the public constructor signature to accept SessionOptions (e.g., constructor(url: string | URL, options?: SessionOptions)) so consumers see serverCertificateDisableVerify in autocomplete; ensure the exported type SessionOptions is imported/visible where the constructor is declared and remove or reconcile any leftover WebTransportOptions overloads to match the implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@js/web-transport/src/session.ts`:
- Line 50: The constructor overload currently declares (constructor(url: string
| URL, options?: WebTransportOptions)) but the implementation expects
SessionOptions, so update the public constructor signature to accept
SessionOptions (e.g., constructor(url: string | URL, options?: SessionOptions))
so consumers see serverCertificateDisableVerify in autocomplete; ensure the
exported type SessionOptions is imported/visible where the constructor is
declared and remove or reconcile any leftover WebTransportOptions overloads to
match the implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bcb53745-b223-49b2-840a-8df7eb9ce1ed
📒 Files selected for processing (6)
js/web-transport/package.jsonjs/web-transport/src/datagrams.tsjs/web-transport/src/index.tsjs/web-transport/src/session.tsjs/web-transport/tsconfig.jsonrs/web-transport-node/src/lib.rs
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/web-transport/src/session.ts`:
- Around line 79-81: Reject configurations that enable
serverCertificateDisableVerify while also providing serverCertificateHashes: in
session.ts, before calling NapiClient.disableVerify() or processing hashes,
check options.serverCertificateDisableVerify and whether
hashes/serverCertificateHashes (the variable used in the diff) is non-empty; if
both are set, throw or return an explicit configuration error (e.g., throw new
Error or reject the promise) explaining that disableVerify and certificate
pinning cannot be used together so the caller must choose one, otherwise proceed
with the existing logic (either disableVerify via NapiClient.disableVerify() or
pinning via the hashes path).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb6face8-c201-48fe-a080-ebab02d274bf
📒 Files selected for processing (1)
js/web-transport/src/session.ts
- Add wasm-bindgen-cli to flake.nix so WASM tests work in CI - Reject using serverCertificateDisableVerify and serverCertificateHashes together - Move url dep to dev-dependencies in web-transport-quiche (cargo-shear fix) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The ErrorResponse type is dictated by tungstenite's callback signature and cannot be reduced in size. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
NapiClient.disableVerify()factory method that skips TLS certificate verification, for testing with self-signed certsnapi::within_runtime_if_availablefor sync methods that need a tokio context (quinn binds UDP sockets to the current runtime)SessionOptions.serverCertificateDisableVerifyto the JS polyfillts/→src/in@moq/web-transport--manifest-path+--output-dirTest plan
bun run buildcompiles successfullyhttps://cdn.moq.dev/anon— setup-only and 5/6 interop tests pass (announce-subscribe is a pre-existing timing issue)serverCertificateDisableVerify: true🤖 Generated with Claude Code