Repository navigation
Conversation
5d84f22 to
ae4a9b5
Compare
|
@cdk-bot review |
cdk-bot
left a comment
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- Idle timeout ignores inbound Ping/Pong frames, force-closing every passive subscriber after ws_idle_timeout_secs (default 120s) (high) - see inline comment
Additional locations included in summary:- crates/cdk-axum/src/ws/mod.rs:170
- crates/cdk-axum/src/ws/mod.rs:238
- crates/cdk-axum/src/ws/limits.rs:47
- Unauthenticated WebSocket upgrades can permanently occupy the whole global connection budget, denying NUT-17 to every client (medium) - see inline comment
Additional locations included in summary:- crates/cdk-axum/src/ws/mod.rs:237
Unanchored locations included in summary: - crates/cdk-axum/src/router_handlers.rs:136
- crates/cdk-axum/src/ws/mod.rs:237
- Reduced default caps (10 subscriptions, 100 filters) plus new -32000 ServerBusy rejections are fatal to the CDK wallet transport, causing a whole-stream reconnect crash-loop (medium) - see inline comment
Additional locations included in summary:- crates/cdk-axum/src/ws/subscribe.rs:27
- crates/cdk-axum/src/ws/subscribe.rs:58
- Off-by-one in validate_limits_config: the minimum accepted ws_max_request_burst_units cannot actually fit one maximum-size subscription (low) - see inline comment
Additional locations included in summary:- crates/cdk-axum/src/ws/subscribe.rs:58
- crates/cdk-axum/src/ws/mod.rs:238
- Subscribe-time backfill silently stops verifying mint-quote payment state after pubsub_max_quote_checks_per_backfill quotes (low) - see inline comment
Additional locations included in summary:- crates/cdk-common/src/pub_sub/limits.rs:31
ae4a9b5 to
f7be4c1
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2533 +/- ##
==========================================
+ Coverage 80.06% 80.31% +0.25%
==========================================
Files 387 391 +4
Lines 107098 108562 +1464
==========================================
+ Hits 85744 87192 +1448
- Misses 21354 21370 +16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@cdk-bot review |
cdk-bot
left a comment
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- Default ws_max_connections_per_ip=2 becomes a mint-wide 2-connection WebSocket cap behind the standard reverse-proxy/NAT topology, silently applied on upgrade and trivially exhaustible (medium) - Availability of the NUT-17 WebSocket notification channel only: behind a reverse proxy (the standard HTTPS topology, since the mint listener has no TLS) or shared NAT, all clients share a 2-connection budget that any client can hold indefinitely; affected wallets silently lose real-time notifications while REST endpoints (which re-check state) remain available. No fund-loss path.
Additional locations included in summary:- crates/cdk-mintd/src/config.rs:1182
- crates/cdk-mintd/src/config.rs:1285
- crates/cdk-mintd/example.config.toml:386
Unanchored locations included in summary: - crates/cdk-axum/src/router_handlers.rs:140
- WsLimits invariants are validated only in cdk-mintd; cdk-axum accepts incoherent limits and can silently wedge or disable the WebSocket endpoint (low) - see inline comment
Unanchored locations included in summary:- crates/cdk-axum/src/lib.rs:102
8c75338 to
8882357
Compare
|
@cdk-bot review |
thesimplekid
left a comment
thesimplekid
left a comment
There was a problem hiding this comment.
I think we should drop per-IP limiting from this PR. The reverse proxy is a better place to enforce that policy, while CDK should retain the connection, subscription, and backfill resource limits. We can add application-level IP limiting in a follow-up if we decide we need it.
| let backfill = spawn(async move { | ||
| // Backfill reads the database and the payment backend once per topic, | ||
| // so a burst of subscriptions has to queue rather than fan out. | ||
| let _permit = match backfill_slots.acquire_owned().await { |
There was a problem hiding this comment.
Could we add deadlines to the cancellable database reads and payment-backend status checks performed while this permit is held? With the defaults, 32 stalled checks can occupy every slot and prevent unrelated proof-state backfills from running. The quote-check count bounds calls, not their duration. We should preserve payment commits already underway and explicitly fail subscriptions whose backfill times out. Could we test that a queued subscription proceeds after a stalled check reaches its deadline?
There was a problem hiding this comment.
The deadline now releases the backfill slot, but a timeout only logs an error and leaves the subscription open. A client waiting for an already-settled quote may never receive another event. Could we explicitly fail/close the subscription or provide bounded retry, and test that the affected client can recover?
54bfaf5 to
402868f
Compare
|
@cdk-bot review |
cdk-bot
left a comment
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- New PubsubLimits lack the validation this PR adds for WsLimits: 0-permit max_concurrent_backfills silently strands all backfills (skip branch unreachable), and mintd accepts an undocumented pubsub_max_quote_checks_per_backfill = 0 (low) - Low. Embedders passing degenerate values through the new PubsubLimits APIs (MintBuilder::with_pubsub_limits / Pubsub::with_limits) get subscriptions that never receive initial state with no diagnostic at all: max_concurrent_backfills: 0 parks every backfill task forever on a 0-permit semaphore whose only skip/log branch is unreachable, leaking the parked tasks; backfill_timeout: ZERO fails every backfill instantly (that path at least logs). A mintd operator setting pubsub_max_quote_checks_per_backfill = 0 — accepted while 0 is rejected for every sibling key in the same section — gets backfills that serve stale quote states from storage without consulting the payment backend (a just-paid invoice backfills as Pending), warned once per backfill. No state corruption; subscriptions stay live for subsequent events in every case, and mintd does validate the two wedging keys.
Additional locations included in summary:- crates/cdk-common/src/pub_sub/limits.rs:6
- crates/cdk-common/src/pub_sub/pubsub.rs:184
- crates/cdk-common/src/pub_sub/pubsub.rs:67
- crates/cdk/src/mint/subscription.rs:42
- crates/cdk/src/mint/builder.rs:332
402868f to
4ebe42e
Compare
|
@cdk-bot review |
thesimplekid
left a comment
thesimplekid
left a comment
There was a problem hiding this comment.
Please address the remaining backfill recovery and cancellation gaps before merging, complete validation for direct pub/sub construction, and move the migration notes to v0.19. I reopened the existing timeout thread with the remaining recovery requirement: #2533 (comment)
Validation: all 19 cdk-common pub/sub tests and 34 cdk-axum WebSocket tests passed. The full integration suite was not run. Please add regression coverage for timeout recovery, disconnect during payment recording, and invalid limits through the public constructors.
d94043c to
a666941
Compare
|
@cdk-bot review |
cdk-bot
left a comment
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- v0.19 migration guide omits this PR's Rust API breaking changes (and one new mintd startup rejection) (low) - Downstream users upgrading the library crates (cdk, cdk-common, cdk-axum) to v0.19 hit compile breaks in four public APIs with no documented migration path; the migration guide covers only the operator-facing TOML keys.
Additional locations included in summary:- crates/cdk/src/mint/payment_backend.rs:225
- crates/cdk-common/src/pub_sub/types.rs:62
Unanchored locations included in summary: - crates/cdk/src/mint/mod.rs:247
- crates/cdk-axum/src/lib.rs:102
a666941 to
d70276f
Compare
|
@cdk-bot review |
cdk-bot
left a comment
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- Melt pending-settlement wait maps the new PubSubError::TooManyTopics to a generic Error::Internal (medium) - see inline comment
Unanchored locations included in summary:- crates/cdk/src/mint/melt/mod.rs:990
- crates/cdk-common/src/pub_sub/pubsub.rs:166
981a114 to
3e7ec69
Compare
|
@cdk-bot review |
cdk-bot
left a comment
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- WsLimits::validate() floor for max_message_bytes cannot admit a legitimate max-size subscribe frame (low) - Low: only operators who lower ws_max_message_bytes below the size of a max-size subscribe frame (while leaving the 512 KB-inconsistent filter defaults) are affected; startup validation passes without naming the key and oversized subscribe frames are refused at the transport for the life of the process. Default configurations are unaffected. Primary actionable defect is the overclaiming MIN_MAX_MESSAGE_BYTES doc comment and the inconsistency with validate()'s stated invariant.
Additional locations included in summary:- crates/cdk-axum/src/ws/limits.rs:142
3e7ec69 to
a79722a
Compare
|
@cdk-bot review |
2b205e2 to
6b6c138
Compare
Give each of those a ceiling the operator can tune, refuse an over-quota upgrade before it happens so the client gets a plain HTTP error rather than a socket closed a moment later, and let a subscription own its backfill so dropping it stops the database and payment backend work. A ceiling bounds concurrency, not rate, so churning subscriptions still stayed under every cap while repeatedly taking the topic lock that event delivery also needs. Charge each connection a budget, per filter on subscribe, refuse what it cannot cover and close a connection that keeps flooding. Claim the connection slot before verifying auth, since auth reads the database and may reach the signatory, and parse each frame once instead of building a throwaway value so a refused frame costs almost nothing.
6b6c138 to
5154f98
Compare
|
@cdk-bot review |
cdk-bot
left a comment
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- Unbounded
pubsub_backfill_timeout_secspanics the backfill task viaInstant + Durationoverflow (low) - see inline comment
Additional locations included in summary:- crates/cdk-common/src/pub_sub/limits.rs:84
| topics: Vec<Self::Topic>, | ||
| reply_to: Subscriber<Self>, | ||
| ) -> Result<(), PubSubError> { | ||
| let deadline = Instant::now() + self.limits.backfill_timeout; |
There was a problem hiding this comment.
What the PR does
MintPubSubSpec::fetch_events gains a backfill deadline: let deadline = Instant::now() + self.limits.backfill_timeout; (crates/cdk/src/mint/subscription.rs:296). The timeout is operator-configurable via pubsub_backfill_timeout_secs (u64 seconds, mapped with Duration::from_secs) and validated by the new PubsubLimits::validate.
The bug
PubsubLimits::validate (crates/cdk-common/src/pub_sub/limits.rs:84-88) rejects backfill_timeout.is_zero() but places no upper bound on it. Instant + Duration panics on overflow (std::time::Instant's Add impl, which tokio::time::Instant delegates to), so a configured value above ~584 years (trivially expressible as a u64, e.g. a fat-fingered 18446744073709551615) panics at line 296 instead of being rejected at startup.
Impact
The panic runs inside the backfill task spawned in Pubsub::subscribe_with (crates/cdk-common/src/pub_sub/pubsub.rs). That task's JoinHandle is kept in ActiveSubscription and is only ever abort()ed — never awaited — so the panic produces no error and no log. The semaphore permit is released on unwind and the subscription stays live, but it silently never receives its initial state, and this happens for every subscription until the config is corrected. Bounded (requires an absurd config value, no process crash, no state corruption), hence low severity — but it is exactly the class of "unservable limit" the PR's validation layer was built to catch loudly, and this one slips through silently.
Suggested fix
- In
PubsubLimits::validate, reject absurdly largebackfill_timeoutvalues (any sane ceiling works), socdk-mintdnames the key at startup like the other rules; and/or - Replace
Instant::now() + self.limits.backfill_timeoutwithInstant::now().checked_add(self.limits.backfill_timeout)and fall back to no effective deadline on overflow.
The same unbounded-u64-seconds pattern exists for ws_idle_timeout_secs / ws_ping_interval_secs feeding tokio::time::timeout/interval in cdk-axum; worth a matching audit.
3 participants
Description
Fixes #2532
Give each of those a ceiling the operator can tune, refuse an over-quota upgrade before it happens so the client gets a plain HTTP error rather than a socket closed a moment later, and let a subscription own its backfill so dropping it stops the database and payment backend work.
A ceiling bounds concurrency, not rate, so churning subscriptions still stayed under every cap while repeatedly taking the topic lock that event delivery also needs. Charge each connection a budget, per filter on subscribe, refuse what it cannot cover and close a connection that keeps flooding.
Claim the connection slot before verifying auth, since auth reads the database and may reach the signatory, and parse each frame once instead of building a throwaway value so a refused frame costs almost nothing.
Notes to the reviewers
Suggested CHANGELOG Updates
CHANGED
ADDED
REMOVED
FIXED
Checklist
just quick-checkbefore committingcrates/cdk-ffi)