fix(subscription): reject interval_seconds that overflow chrono Duration (#51) - #65
Open
OluRemiFour wants to merge 1 commit into
Open
Conversation
…ion (StellarSend#51) Duration::seconds panics when a value near i64::MAX overflows chrono's internal millisecond representation, turning a POST /api/subscriptions into a server panic and, if such a value ever reached the DB, silently killing the background keeper loop from within its spawned task. Bound interval_seconds at validation time with Duration::try_seconds, returning AppError::Validation instead of panicking, and use the same fallible constructor inside run_due_executions to reschedule, marking a row with an unrepresentable stored interval as failed so the keeper loop keeps running for every other subscription. Adds a regression test submitting i64::MAX and other extreme values to create() and asserting a clean AppError::Validation rather than a panic.
OluRemiFour
force-pushed
the
fix/subscription-interval-overflow-panic
branch
from
August 18, 2026 10:12
6947aef to
14592e4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CreateSubscriptionRequest.interval_secondsis a plaini64validated only for being positive. A client-supplied value neari64::MAXdeserializes cleanly and is passed intochrono::Duration::seconds(), which panics once the value would overflow chrono's internal millisecond representation (i64::MAX / 1000-ish). That panic was reachable:POST /api/subscriptionswith"interval_seconds": 9223372036854775807panicked instead of returning a4xx, with nocatch_unwind/CatchPanicLayeranywhere in the middleware stack.run_due_executionsruns the identical, unguarded expression for every successful execution; if a malformed interval ever reached the DB, the panic would fire insiderun_keeper_loop's spawned task and silently take down scheduling for every subscription.Fix
SubscriptionService::create— boundinterval_secondswithchrono::Duration::try_secondsat validation time, returningAppError::Validation(→422/VALIDATION_ERROR) instead of ever panicking. No hardcoded internal threshold; the fallible constructor defines the safe range.SubscriptionService::run_due_executions— usetry_secondsto reschedule (defense in depth for stored/less-trusted values). If a stored interval can't be represented, the row is markedfailedwith a descriptivelast_errorand the loop continues, so one corrupt row can no longer kill the keeper loop for everyone else.Acceptance criteria
POST /api/subscriptionswithinterval_secondsat or neari64::MAXreturns a4xxvalidation error instead of panicking.Duration::try_secondsconstructor.i64::MAXand other extreme values toSubscriptionService::createand asserts a cleanAppError::Validation(absence of panic) — no DB needed, validation short-circuits before anysqlxcall.Tests
cargo test: 34 passed; the newcreate_rejects_interval_seconds_outside_duration_rangecovers the panic boundary.cargo check --all-targetsclean; no new clippy/rustfmt findings in this file.The
retry_at = Utc::now() + Duration::seconds(60)constant is not attacker-controlled and is intentionally left unchanged (out of scope per the issue). Both this and the companion "no minimum interval" issue touch the same field and call site; this change is deliberately additive (only rejects values chrono cannot represent) so the two fixes merge cleanly.Fixes #51