address review: clamp seconds before multiplying, preserve negatives

Per @lbellows / @eternal-flame-AD feedback:
- Clamp the requested seconds to [-maxElevationSeconds, maxElevationSeconds]
  BEFORE multiplying, so an int64 multiply can never overflow (including wraps
  that land positive, e.g. 20023544073) and large negatives (e.g. -9223372037).
- Preserve negative values instead of mapping them to DefaultElevationDuration,
  so the UI's cancel (durationSeconds: -1) still sets elevatedUntil in the past
  rather than granting a fresh hour.
- Keep it simple: a small guard inside the function, no extra ceremony.

Tests updated: cancel -1 preserved, zero stays zero, overflow-positive and
large-negative both clamped.

Author: piyush295

model/elevate.go

@@ -15,27 +15,23 @@ type ElevateRequest struct {
 
 var DefaultElevationDuration = time.Hour
 
-// maxElevationDuration caps a requested elevation duration. A practically
-// "infinite" elevation is an intentionally supported relief valve, so this is
-// deliberately large (~100 years) rather than a security limit. Its only
-// purpose is to keep the value well below the point where
-// time.Duration(seconds) * time.Second overflows (int64 nanoseconds wrap past
-// ~292 years), which would otherwise yield an elevatedUntil in the past and
-// silently drop the elevation.
-const maxElevationDuration = 100 * 365 * 24 * time.Hour
+// maxElevationSeconds caps a requested elevation duration (~100 years). A
+// practically "infinite" elevation is intentionally supported, so this is not a
+// security limit; its only purpose is to keep the value well below the point
+// where time.Duration(seconds) * time.Second overflows int64 nanoseconds
+// (~292 years) and would otherwise wrap to a bogus (even past) timestamp.
+const maxElevationSeconds = 100 * 365 * 24 * 60 * 60
 
 // ElevationDuration converts a requested duration in seconds to a
-// time.Duration, clamping absurdly large or negative values to a safe range so
-// the subsequent time.Time.Add cannot overflow into a past timestamp.
+// time.Duration, clamping the seconds (before multiplying) to a safe range so
+// the subsequent multiply cannot overflow. Negative values are preserved so the
+// client can still cancel an elevation (the UI sends durationSeconds: -1, which
+// must set elevatedUntil in the past rather than grant a fresh elevation).
 func ElevationDuration(seconds int) time.Duration {
-	if seconds <= 0 {
-		return DefaultElevationDuration
+	if seconds > maxElevationSeconds {
+		seconds = maxElevationSeconds
+	} else if seconds < -maxElevationSeconds {
+		seconds = -maxElevationSeconds
 	}
-	d := time.Duration(seconds) * time.Second
-	// Detect overflow (wrap to negative) or an intentionally huge value and
-	// clamp to the ceiling.
-	if d <= 0 || d > maxElevationDuration {
-		return maxElevationDuration
-	}
-	return d
+	return time.Duration(seconds) * time.Second
 }

model/elevate_test.go

@@ -12,21 +12,21 @@ func TestElevationDuration(t *testing.T) {
 		want    time.Duration
 	}{
 		{"normal", 900, 900 * time.Second},
-		{"zero falls back to default", 0, DefaultElevationDuration},
-		{"negative falls back to default", -5, DefaultElevationDuration},
-		{"huge value is clamped", 9223372036854775807, maxElevationDuration},
-		{"just under ceiling is unchanged", 100 * 365 * 24 * 3600, maxElevationDuration},
+		{"zero stays zero", 0, 0},
+		// Negative values must be preserved so the client can cancel an
+		// elevation (UI sends -1); they must NOT become a positive duration.
+		{"cancel -1 preserved", -1, -1 * time.Second},
+		{"huge clamped to max", 9223372036854775807, maxElevationSeconds * time.Second},
+		// Values that would overflow when multiplied but land positive are
+		// clamped by clamping seconds first.
+		{"overflow-positive clamped", 20023544073, maxElevationSeconds * time.Second},
+		{"large negative clamped", -9223372037, -maxElevationSeconds * time.Second},
 	}
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
-			got := ElevationDuration(c.seconds)
-			if got != c.want {
+			if got := ElevationDuration(c.seconds); got != c.want {
 				t.Fatalf("ElevationDuration(%d) = %v, want %v", c.seconds, got, c.want)
 			}
-			// The resulting time must always be in the future (no overflow).
-			if time.Now().Add(got).Before(time.Now()) {
-				t.Fatalf("ElevationDuration(%d) produced a past time", c.seconds)
-			}
 		})
 	}
 }