From cf4fcd53bc3f9e29e27b2be58cdc0f3148692878 Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Sun, 4 Oct 2026 21:47:41 -0700 Subject: [PATCH 1/2] fix: reject overflowing backup and PostgreSQL durations --- internal/app/backup_schema.go | 3 ++ internal/app/duration_limits_test.go | 67 ++++++++++++++++++++++++++++ internal/app/runtime.go | 22 ++++++--- 3 files changed, 85 insertions(+), 7 deletions(-) create mode 100644 internal/app/duration_limits_test.go diff --git a/internal/app/backup_schema.go b/internal/app/backup_schema.go index 19ef86d3..c4d7a352 100644 --- a/internal/app/backup_schema.go +++ b/internal/app/backup_schema.go @@ -214,6 +214,9 @@ func PositiveDuration(value string) (time.Duration, error) { if err != nil || days <= 0 { return 0, fmt.Errorf("%q is not positive", value) } + if days > maxDurationDays { + return 0, fmt.Errorf("%q exceeds the maximum representable duration", value) + } return time.Duration(days) * 24 * time.Hour, nil } d, err := time.ParseDuration(value) diff --git a/internal/app/duration_limits_test.go b/internal/app/duration_limits_test.go new file mode 100644 index 00000000..e0edfbf7 --- /dev/null +++ b/internal/app/duration_limits_test.go @@ -0,0 +1,67 @@ +package app + +import ( + "strings" + "testing" + "time" +) + +func TestPositiveDurationRejectsOverflow(t *testing.T) { + for _, value := range []string{"106752d", "213504d", "2147483647d", "9223372036854775807d"} { + if got, err := PositiveDuration(value); err == nil || got != 0 { + t.Errorf("PositiveDuration(%q) = %v, %v; want refusal", value, got, err) + } + } + for value, want := range map[string]time.Duration{ + "1d": 24 * time.Hour, + "106751d": 106751 * 24 * time.Hour, + "15m": 15 * time.Minute, + } { + if got, err := PositiveDuration(value); err != nil || got != want { + t.Errorf("PositiveDuration(%q) = %v, %v; want %v", value, got, err, want) + } + } +} + +func TestPostgresDurationsRejectOverflowInEveryUnit(t *testing.T) { + for _, value := range []string{ + "9223372037", "9223372037s", "9223372036855ms", + "153722868min", "2562048h", "106752d", "213504d", + "9223372036854775807s", "9223372036854775808ms", + } { + if got, ok := ParsePostgresDuration(value); ok || got != 0 { + t.Errorf("ParsePostgresDuration(%q) = %v, %v; want refusal", value, got, ok) + } + } + for value, want := range map[string]time.Duration{ + "9223372036": 9223372036 * time.Second, + "9223372036s": 9223372036 * time.Second, + "9223372036854ms": 9223372036854 * time.Millisecond, + "153722867min": 153722867 * time.Minute, + "2562047h": 2562047 * time.Hour, + "106751d": 106751 * 24 * time.Hour, + " 1 MIN ": time.Minute, + "0ms": 0, + } { + if got, ok := ParsePostgresDuration(value); !ok || got != want { + t.Errorf("ParsePostgresDuration(%q) = %v, %v; want %v", value, got, ok, want) + } + } +} + +func TestBackupPolicyRejectsOverflowBeforeItCanBecomeAShortWindow(t *testing.T) { + for name, tc := range map[string]struct { + policy string + code string + }{ + "data loss": {" maxDataLoss: 213504d\n", "project_invalid"}, + "retention": {" maxDataLoss: 15m\n retention: {window: 213504d}\n", "backup_retention_unsupported"}, + "drill age": {" maxDataLoss: 15m\n drill: {maxAge: 213504d}\n", "project_invalid"}, + } { + t.Run(name, func(t *testing.T) { + project := strings.Replace(validBackupProject, " maxDataLoss: 15m\n", tc.policy, 1) + _, err := loadFixtureBytes([]byte(project), "ob.yml") + assertAppErrorCode(t, err, tc.code) + }) + } +} diff --git a/internal/app/runtime.go b/internal/app/runtime.go index 9d3246af..35dc21dd 100644 --- a/internal/app/runtime.go +++ b/internal/app/runtime.go @@ -441,24 +441,32 @@ func ParsePostgresDuration(value string) (time.Duration, bool) { if digits == 0 { return 0, false } - count, err := strconv.Atoi(trimmed[:digits]) + count, err := strconv.ParseInt(trimmed[:digits], 10, 64) if err != nil || count < 0 { return 0, false } unit := strings.ToLower(strings.TrimSpace(trimmed[digits:])) + var scale time.Duration switch unit { case "", "s": - return time.Duration(count) * time.Second, true + scale = time.Second case "ms": - return time.Duration(count) * time.Millisecond, true + scale = time.Millisecond case "min": - return time.Duration(count) * time.Minute, true + scale = time.Minute case "h": - return time.Duration(count) * time.Hour, true + scale = time.Hour case "d": - return time.Duration(count) * 24 * time.Hour, true + scale = 24 * time.Hour + default: + return 0, false + } + // Check before multiplying: overflow can wrap to a plausible positive + // duration and make an unsafe server setting satisfy a backup objective. + if count > int64((1<<63-1)/scale) { + return 0, false } - return 0, false + return time.Duration(count) * scale, true } // maxDurationDays is the largest whole-day count that fits in int64 From 0dca12fe75297dcf7c143247b626c0da61560c78 Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Sun, 4 Oct 2026 22:06:44 -0700 Subject: [PATCH 2/2] fix: report unusable archiving durations in backup health checks --- internal/engine/backup_archiving_test.go | 66 ++++++++++++++++++++++++ internal/engine/backup_postgres.go | 18 ++++++- 2 files changed, 82 insertions(+), 2 deletions(-) create mode 100644 internal/engine/backup_archiving_test.go diff --git a/internal/engine/backup_archiving_test.go b/internal/engine/backup_archiving_test.go new file mode 100644 index 00000000..f9900d7f --- /dev/null +++ b/internal/engine/backup_archiving_test.go @@ -0,0 +1,66 @@ +package engine + +import ( + "io" + "strings" + "testing" + + "github.com/labstack/onebox/internal/app" + "github.com/labstack/onebox/internal/transport" +) + +func TestArchivingIssuesDoNotTreatUnusableTimeoutsAsWithinPolicy(t *testing.T) { + for _, tc := range []struct { + timeout string + issue string + policy string + }{ + {timeout: "900s"}, + {timeout: "15min"}, + {timeout: "14min"}, + {timeout: "16min", issue: "closes a write-ahead log segment"}, + {timeout: "106752d", issue: "cannot determine"}, + {timeout: "213504d", issue: "cannot determine"}, + {timeout: "9223372037s", issue: "cannot determine"}, + {timeout: "9223372036855ms", issue: "cannot determine"}, + {timeout: "153722868min", issue: "cannot determine"}, + {timeout: "2562048h", issue: "cannot determine"}, + {timeout: "soon", issue: "cannot determine"}, + {timeout: "0", issue: "disabled"}, + {timeout: "900s", issue: "cannot determine", policy: "213504d"}, + {timeout: "900s", issue: "cannot determine", policy: "0"}, + {timeout: "900s", issue: "cannot determine", policy: "soon"}, + } { + t.Run(tc.timeout+"/"+tc.policy, func(t *testing.T) { + policy := tc.policy + if policy == "" { + policy = "15m" + } + fake := &transport.Fake{Dynamic: func(command string) (transport.Result, bool) { + if strings.Contains(command, "show archive_timeout;") { + return transport.Result{Stdout: "on\n" + app.WalgBinary + " wal-push %p\n" + tc.timeout + "\n"}, true + } + return transport.Result{}, false + }} + spec := &app.Spec{ + Name: "shop", + Services: map[string]app.Service{ + "database": {Driver: "postgres", Version: "18", Backup: &app.BackupPolicy{Target: "offsite", MaxDataLoss: policy}}, + }, + BackupTargets: map[string]app.BackupTarget{"offsite": {}}, + } + e := New(&app.Resolved{Spec: spec, Env: "production"}, nil, fake, Options{Out: io.Discard}) + issues, err := e.archivingIssues(t.Context(), "database") + if err != nil { + t.Fatal(err) + } + if tc.issue == "" { + if len(issues) != 0 { + t.Fatalf("valid timeout raised issues: %v", issues) + } + } else if len(issues) != 1 || !strings.Contains(issues[0], tc.issue) { + t.Fatalf("timeout %q silently accepted or misreported: %v", tc.timeout, issues) + } + }) + } +} diff --git a/internal/engine/backup_postgres.go b/internal/engine/backup_postgres.go index ae014221..efb467a4 100644 --- a/internal/engine/backup_postgres.go +++ b/internal/engine/backup_postgres.go @@ -556,8 +556,22 @@ func (e *Engine) archivingIssues(ctx context.Context, service string) ([]string, issues = append(issues, fmt.Sprintf( "the server's archive_command is not the one Onebox installed, so where the write-ahead log goes is not what this project describes; re-run `ob backup enable %s`", service)) } - if declared, ok := app.ParseDuration(projection.Policy.MaxDataLoss); ok { - if observed, parsed := app.ParsePostgresDuration(timeout); parsed && observed > declared { + declared, policyErr := app.PositiveDuration(projection.Policy.MaxDataLoss) + if policyErr != nil { + issues = append(issues, fmt.Sprintf( + "cannot determine whether archiving satisfies the maximum data loss policy %q: %v", + projection.Policy.MaxDataLoss, policyErr)) + } else { + observed, parsed := app.ParsePostgresDuration(timeout) + switch { + case !parsed: + issues = append(issues, fmt.Sprintf( + "cannot determine whether archive_timeout %q satisfies the maximum data loss policy %s; the server value is invalid or exceeds the maximum representable duration", + timeout, projection.Policy.MaxDataLoss)) + case observed == 0: + issues = append(issues, fmt.Sprintf( + "the server has archive_timeout disabled, so an idle database can lose more than the policy permits; re-run `ob backup enable %s`", service)) + case observed > declared: issues = append(issues, fmt.Sprintf( "the server closes a write-ahead log segment every %s, but the policy tolerates losing at most %s; an idle database can lose more than the policy permits", timeout, projection.Policy.MaxDataLoss))