diff --git a/api/client.go b/api/client.go index 58d203475..86d8d6f02 100644 --- a/api/client.go +++ b/api/client.go @@ -320,7 +320,7 @@ func (a *ClientAPI) ElevateClient(ctx *gin.Context) { return } - elevatedUntil := time.Now().Add(time.Duration(params.DurationSeconds) * time.Second) + elevatedUntil := model.ElevationTime(time.Now(), params.DurationSeconds) if err := a.DB.UpdateClientElevatedUntil(client.ID, &elevatedUntil); err != nil { ctx.AbortWithError(500, err) return diff --git a/api/client_test.go b/api/client_test.go index b498c1d08..503a1f152 100644 --- a/api/client_test.go +++ b/api/client_test.go @@ -358,6 +358,47 @@ func (s *ClientSuite) Test_ElevateClient_expectBadRequestOnMissingDuration() { assert.Nil(s.T(), client.ElevatedUntil) } +func (s *ClientSuite) Test_ElevateClient_cancelSuccess() { + s.db.User(5).Client(8) + + test.WithUser(s.ctx, 5) + elevatedUntil := time.Now().Add(1 * time.Hour) + err := s.db.UpdateClientElevatedUntil(8, &elevatedUntil) + assert.NoError(s.T(), err) + client, err := s.db.GetClientByID(8) + assert.NoError(s.T(), err) + assert.Equal(s.T(), elevatedUntil.Unix(), client.ElevatedUntil.Unix()) + + s.withElevateRequest(8, -1) + + s.a.ElevateClient(s.ctx) + + assert.Equal(s.T(), 204, s.ctx.Writer.Status()) + client, err = s.db.GetClientByID(8) + assert.NoError(s.T(), err) + assert.Equal(s.T(), &time.Time{}, client.ElevatedUntil) +} + +func (s *ClientSuite) Test_ElevateClient_clampsHugeDuration() { + s.db.User(5).Client(8) + + test.WithUser(s.ctx, 5) + // A value that would overflow time.Duration must be clamped to the ceiling + // (not rejected, and not wrapped to a past timestamp). + s.ctx.AddParam("id", "8") + s.withElevateBody(`{"durationSeconds":9223372036854775807}`) + + before := time.Now() + s.a.ElevateClient(s.ctx) + + assert.Equal(s.T(), 204, s.ctx.Writer.Status()) + client, err := s.db.GetClientByID(8) + assert.NoError(s.T(), err) + assert.NotNil(s.T(), client.ElevatedUntil) + // Must be in the future (overflow would have produced a past time). + assert.True(s.T(), client.ElevatedUntil.After(before), "elevatedUntil should be in the future, got %v", client.ElevatedUntil) +} + func (s *ClientSuite) withFormData(formData string) { s.ctx.Request = httptest.NewRequest("POST", "/token", strings.NewReader(formData)) s.ctx.Request.Header.Set("Content-Type", "application/x-www-form-urlencoded") diff --git a/api/oidc.go b/api/oidc.go index 2e27d2a20..11f02e0f2 100644 --- a/api/oidc.go +++ b/api/oidc.go @@ -264,7 +264,7 @@ func (a *OIDCAPI) handleElevationCallback(w http.ResponseWriter, elevate *pendin http.Error(w, "client not found", http.StatusNotFound) return } - elevatedUntil := time.Now().Add(time.Duration(elevate.DurationSeconds) * time.Second) + elevatedUntil := model.ElevationTime(time.Now(), elevate.DurationSeconds) if err := a.DB.UpdateClientElevatedUntil(client.ID, &elevatedUntil); err != nil { http.Error(w, fmt.Sprintf("failed to elevate session: %v", err), http.StatusInternalServerError) return diff --git a/model/elevate.go b/model/elevate.go index f0bd577c7..4887de9b9 100644 --- a/model/elevate.go +++ b/model/elevate.go @@ -14,3 +14,21 @@ type ElevateRequest struct { } var DefaultElevationDuration = time.Hour + +// maxElevation 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 maxElevation = 100 * 365 * 24 * time.Hour + +// ElevationTime calculates the time at which an elevation will expire. +func ElevationTime(base time.Time, deltaSeconds int) time.Time { + if deltaSeconds > int(maxElevation/time.Second) { + deltaSeconds = int(maxElevation / time.Second) + } + if deltaSeconds < 0 { + return time.Time{} + } + return base.Add(time.Duration(deltaSeconds) * time.Second) +} diff --git a/model/elevate_test.go b/model/elevate_test.go new file mode 100644 index 000000000..ebb17146e --- /dev/null +++ b/model/elevate_test.go @@ -0,0 +1,52 @@ +package model + +import ( + "math" + "testing" + "time" +) + +func TestElevationTime(t *testing.T) { + // Use runtime variables so arithmetic doesn't happen as a constant + // expression and overflow Go's constant checker on either arch. + maxInt := math.MaxInt + minInt := math.MinInt + maxElevationSeconds := int64(maxElevation / time.Second) + + // On 32-bit math.MaxInt (~2.1B) is below maxElevationSeconds (~3.15B), + // so no clamp triggers and the expected output is the requested + // duration; on 64-bit math.MaxInt is far above, so the expected output + // is the cap. + var expectedForHuge time.Duration + if int64(maxInt) < maxElevationSeconds { + expectedForHuge = time.Duration(maxInt) * time.Second + } else { + expectedForHuge = maxElevation + } + + baseTime := time.Now() + + cases := []struct { + name string + seconds int + want time.Time + }{ + {"normal", 900, baseTime.Add(900 * time.Second)}, + {"zero stays zero", 0, baseTime}, + // Negative values collapse to a one-second past duration so the + // client can cancel an elevation (UI sends -1); they must NOT grant + // a fresh positive elevation. + {"negative value cancels", -1, time.Time{}}, + {"large negative cancels", minInt, time.Time{}}, + // A huge positive request is clamped on 64-bit; on 32-bit no clamp + // is reachable since math.MaxInt < maxElevationSeconds there. + {"huge clamped to max", maxInt, baseTime.Add(expectedForHuge)}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := ElevationTime(baseTime, c.seconds); got != c.want { + t.Fatalf("ElevationDuration(%d) = %v, want %v", c.seconds, got, c.want) + } + }) + } +}