Overview
SubscriptionService::create (src/services/subscription.rs:48-101) only validates that interval_seconds is positive — there is no lower bound at all:
if req.interval_seconds <= 0 {
return Err(AppError::Validation("interval_seconds must be positive".into()));
}
The database migration matches this — interval_seconds BIGINT NOT NULL CHECK (interval_seconds > 0) (migrations/006_add_subscriptions.sql:24) — so nothing anywhere in the stack stops a client from creating a subscription with interval_seconds = 1.
The keeper background loop (main.rs:134-168) polls for due work every keeper_poll_interval_secs (default 60, configurable, floored at 5 via .max(5) at main.rs:138). SubscriptionService::run_due_executions selects every active subscription whose next_execution_at <= NOW() (subscription.rs:203-214) and, on a successful execution, reschedules next_execution_at = Utc::now() + ChronoDuration::seconds(sub.interval_seconds) (subscription.rs:263-264). With interval_seconds = 1, a subscription becomes "due" again essentially immediately after each execution — meaning it will be picked up and executed on every single keeper pass for as long as it keeps succeeding, i.e. once every keeper_poll_interval_secs (as often as every 5 seconds, if an operator has tuned the poll interval down), indefinitely, with no cap.
Since execute_subscription moves real funds against a pre-granted on-chain allowance (per this file's own extensive doc comments: "the keeper only pays the transaction fee; whether the call actually succeeds ... is entirely gated by the on-chain authorization the payer already granted"), a subscription created (accidentally via a client bug, or deliberately by a malicious/compromised payer account, or by a griefer targeting a recipient they don't like by spamming tiny payments to them) with an unreasonably small interval will have the keeper draining that pre-authorized allowance far faster than any sane recurring-payment product should allow, and hammering the Stellar network/Soroban RPC with one signed submission every few seconds per such subscription, for as long as it remains active and funded.
This is a plain missing-bound validation gap, but a consequential one given what actually consumes the field: unlike a display-only or cosmetic parameter, interval_seconds directly controls the rate of real on-chain fund movement by an autonomous background process.
Requirements
- Add a minimum bound on
interval_seconds at creation time — e.g. a MIN_SUBSCRIPTION_INTERVAL_SECS constant (a reasonable floor might be measured in minutes/hours depending on the intended product use case for "subscriptions," but even a conservative floor like 60s materially closes the worst-case drain rate) — enforced in SubscriptionService::create, mirroring how MAX_CONSECUTIVE_FAILURES/KEEPER_BATCH_LIMIT are already defined as named constants in this same file.
- Add the equivalent
CHECK constraint at the database level (CHECK (interval_seconds >= <min>)) so the floor holds even against a future code path that bypasses the service layer.
- Consider whether an upper bound is also worth enforcing (extremely large but finite values are less operationally dangerous than tiny ones, but are handled separately — see the companion issue on
interval_seconds near i64::MAX causing a chrono::Duration panic, which this issue's fix does not itself resolve).
Acceptance Criteria
Additional Notes
Edge cases
- The floor should be validated against
first_execution_at/rescheduling too, not just the initial creation value — since interval_seconds is immutable after creation in the current API (there's no update endpoint, only cancel), a creation-time check is sufficient today, but worth a note if an "update subscription" endpoint is ever added.
Testing strategy
- A straightforward unit test on
SubscriptionService::create analogous to the existing positive/negative-amount validation tests already present in sibling services (e.g. services/payment_request.rs's test module).
Cross-references
- Distinct from the companion issue about
interval_seconds near i64::MAX panicking chrono::Duration::seconds() — that's an upper-bound crash/DoS bug; this is a lower-bound business-logic/fund-safety gap. Both point at the same field but are different bug classes requiring different fixes (one needs a floor, the other needs a ceiling well below the panic threshold).
Overview
SubscriptionService::create(src/services/subscription.rs:48-101) only validates thatinterval_secondsis positive — there is no lower bound at all:The database migration matches this —
interval_seconds BIGINT NOT NULL CHECK (interval_seconds > 0)(migrations/006_add_subscriptions.sql:24) — so nothing anywhere in the stack stops a client from creating a subscription withinterval_seconds = 1.The keeper background loop (
main.rs:134-168) polls for due work everykeeper_poll_interval_secs(default 60, configurable, floored at 5 via.max(5)atmain.rs:138).SubscriptionService::run_due_executionsselects everyactivesubscription whosenext_execution_at <= NOW()(subscription.rs:203-214) and, on a successful execution, reschedulesnext_execution_at = Utc::now() + ChronoDuration::seconds(sub.interval_seconds)(subscription.rs:263-264). Withinterval_seconds = 1, a subscription becomes "due" again essentially immediately after each execution — meaning it will be picked up and executed on every single keeper pass for as long as it keeps succeeding, i.e. once everykeeper_poll_interval_secs(as often as every 5 seconds, if an operator has tuned the poll interval down), indefinitely, with no cap.Since
execute_subscriptionmoves real funds against a pre-granted on-chain allowance (per this file's own extensive doc comments: "the keeper only pays the transaction fee; whether the call actually succeeds ... is entirely gated by the on-chain authorization the payer already granted"), a subscription created (accidentally via a client bug, or deliberately by a malicious/compromised payer account, or by a griefer targeting a recipient they don't like by spamming tiny payments to them) with an unreasonably small interval will have the keeper draining that pre-authorized allowance far faster than any sane recurring-payment product should allow, and hammering the Stellar network/Soroban RPC with one signed submission every few seconds per such subscription, for as long as it remainsactiveand funded.This is a plain missing-bound validation gap, but a consequential one given what actually consumes the field: unlike a display-only or cosmetic parameter,
interval_secondsdirectly controls the rate of real on-chain fund movement by an autonomous background process.Requirements
interval_secondsat creation time — e.g. aMIN_SUBSCRIPTION_INTERVAL_SECSconstant (a reasonable floor might be measured in minutes/hours depending on the intended product use case for "subscriptions," but even a conservative floor like 60s materially closes the worst-case drain rate) — enforced inSubscriptionService::create, mirroring howMAX_CONSECUTIVE_FAILURES/KEEPER_BATCH_LIMITare already defined as named constants in this same file.CHECKconstraint at the database level (CHECK (interval_seconds >= <min>)) so the floor holds even against a future code path that bypasses the service layer.interval_secondsneari64::MAXcausing achrono::Durationpanic, which this issue's fix does not itself resolve).Acceptance Criteria
SubscriptionService::createrejectsinterval_secondsbelow the configured minimum withAppError::Validation.CHECKconstraint to thesubscriptionstable.interval_seconds = 1(or any value below the chosen floor) is rejected at creation, and that a value at/above the floor succeeds.docs/API.md) so it's an intentional product decision, not an arbitrary magic number.Additional Notes
Edge cases
first_execution_at/rescheduling too, not just the initial creation value — sinceinterval_secondsis immutable after creation in the current API (there's no update endpoint, only cancel), a creation-time check is sufficient today, but worth a note if an "update subscription" endpoint is ever added.Testing strategy
SubscriptionService::createanalogous to the existing positive/negative-amount validation tests already present in sibling services (e.g.services/payment_request.rs's test module).Cross-references
interval_secondsneari64::MAXpanickingchrono::Duration::seconds()— that's an upper-bound crash/DoS bug; this is a lower-bound business-logic/fund-safety gap. Both point at the same field but are different bug classes requiring different fixes (one needs a floor, the other needs a ceiling well below the panic threshold).