From 1ef9e299b96a32b6a33fe7910b60c4c63ecb873a Mon Sep 17 00:00:00 2001 From: piyush295 Date: Mon, 28 Sep 2026 14:01:39 +0530 Subject: [PATCH 1/5] Bound client elevation duration to prevent overflow and permanent elevation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ElevateRequest.DurationSeconds was only validated as non-zero and both elevation paths multiply it straight into a time.Duration: - api/client.go POST /client/:id/elevate - api/oidc.go the OIDC elevation callback This allowed two problems (reproduced on v3.1.1): 1. Elevation could be made effectively permanent — a single request could set elevatedUntil a century into the future. 2. Very large values silently overflowed: time.Duration(DurationSeconds) * time.Second wraps past ~292 years, yielding a past elevatedUntil so the elevation was silently dropped while the request still returned 204. Add a max bound (30 days) via the binding tag on both ElevateRequest and the OIDC pendingElevation struct, plus a min of 1, so out-of-range durations are rejected with 400 instead of being silently mishandled. A MaxElevationDurationSeconds constant documents the ceiling. Adds regression tests for the above-max and overflow cases. Closes #1051 --- api/client_test.go | 32 ++++++++++++++++++++++++++++++++ api/oidc.go | 2 +- model/elevate.go | 9 ++++++++- 3 files changed, 41 insertions(+), 2 deletions(-) diff --git a/api/client_test.go b/api/client_test.go index b498c1d08..fbccf2ba8 100644 --- a/api/client_test.go +++ b/api/client_test.go @@ -358,6 +358,38 @@ func (s *ClientSuite) Test_ElevateClient_expectBadRequestOnMissingDuration() { assert.Nil(s.T(), client.ElevatedUntil) } +func (s *ClientSuite) Test_ElevateClient_expectBadRequestOnDurationAboveMax() { + s.db.User(5).Client(8) + + test.WithUser(s.ctx, 5) + // One second past the 30-day ceiling must be rejected. + s.withElevateRequest(8, model.MaxElevationDurationSeconds+1) + + s.a.ElevateClient(s.ctx) + + assert.Equal(s.T(), 400, s.recorder.Code) + client, err := s.db.GetClientByID(8) + assert.NoError(s.T(), err) + assert.Nil(s.T(), client.ElevatedUntil) +} + +func (s *ClientSuite) Test_ElevateClient_expectBadRequestOnOverflowDuration() { + s.db.User(5).Client(8) + + test.WithUser(s.ctx, 5) + // A value that would overflow time.Duration (wrapping to a past + // timestamp) must be rejected rather than silently accepted. + s.ctx.AddParam("id", "8") + s.withElevateBody(`{"durationSeconds":9223372036854775807}`) + + s.a.ElevateClient(s.ctx) + + assert.Equal(s.T(), 400, s.recorder.Code) + client, err := s.db.GetClientByID(8) + assert.NoError(s.T(), err) + assert.Nil(s.T(), 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..5a0a2efd3 100644 --- a/api/oidc.go +++ b/api/oidc.go @@ -86,7 +86,7 @@ type pendingOIDCSession struct { type pendingElevation struct { ClientID uint `form:"id" binding:"required"` - DurationSeconds int `form:"durationSeconds" binding:"required"` + DurationSeconds int `form:"durationSeconds" binding:"required,min=1,max=2592000"` } // OIDCAPI provides handlers for OIDC authentication. diff --git a/model/elevate.go b/model/elevate.go index f0bd577c7..1a7efbcbc 100644 --- a/model/elevate.go +++ b/model/elevate.go @@ -10,7 +10,14 @@ type ElevateRequest struct { // // required: true // example: 900 - DurationSeconds int `form:"durationSeconds" query:"durationSeconds" json:"durationSeconds" binding:"required"` + DurationSeconds int `form:"durationSeconds" query:"durationSeconds" json:"durationSeconds" binding:"required,min=1,max=2592000"` } var DefaultElevationDuration = time.Hour + +// MaxElevationDurationSeconds is the upper bound (30 days) accepted for a +// requested elevation duration. It bounds ElevateRequest.DurationSeconds so a +// request can neither make elevation effectively permanent nor overflow +// time.Duration (which wraps past ~292 years, silently yielding a past +// elevatedUntil timestamp). Requests above this are rejected with 400. +const MaxElevationDurationSeconds = 2592000 From f2bbea76b850d7c833f716180a7a916adb90a926 Mon Sep 17 00:00:00 2001 From: piyush295 Date: Mon, 28 Sep 2026 17:10:45 +0530 Subject: [PATCH 2/5] address review: clamp elevation duration instead of rejecting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per maintainer feedback (@eternal-flame-AD): a hard max is not a meaningful security control (an elevated attacker can already change the password / create admins, and could re-extend before expiry), and an effectively-infinite elevation is an intentionally supported relief valve. So instead of rejecting large durations with 400, clamp them: add model.ElevationDuration(seconds) which converts to a time.Duration and caps at ~100 years — large enough to remain a practical 'infinite', but safely below the point where time.Duration overflows (int64 ns wrap past ~292 years) and would yield a past elevatedUntil. Both elevation paths now use it; the min/max binding tags are removed. Tests: model.TestElevationDuration (normal, zero/negative default, huge clamp, ceiling) and Test_ElevateClient_clampsHugeDuration (MaxInt64 still elevates with a future timestamp). Signed-off-by: piyush295 --- api/client.go | 2 +- api/client_test.go | 28 ++++++++-------------------- api/oidc.go | 4 ++-- model/elevate.go | 32 +++++++++++++++++++++++++------- model/elevate_test.go | 32 ++++++++++++++++++++++++++++++++ 5 files changed, 68 insertions(+), 30 deletions(-) create mode 100644 model/elevate_test.go diff --git a/api/client.go b/api/client.go index 58d203475..79366f56f 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 := time.Now().Add(model.ElevationDuration(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 fbccf2ba8..fc3b287b2 100644 --- a/api/client_test.go +++ b/api/client_test.go @@ -358,36 +358,24 @@ func (s *ClientSuite) Test_ElevateClient_expectBadRequestOnMissingDuration() { assert.Nil(s.T(), client.ElevatedUntil) } -func (s *ClientSuite) Test_ElevateClient_expectBadRequestOnDurationAboveMax() { +func (s *ClientSuite) Test_ElevateClient_clampsHugeDuration() { s.db.User(5).Client(8) test.WithUser(s.ctx, 5) - // One second past the 30-day ceiling must be rejected. - s.withElevateRequest(8, model.MaxElevationDurationSeconds+1) - - s.a.ElevateClient(s.ctx) - - assert.Equal(s.T(), 400, s.recorder.Code) - client, err := s.db.GetClientByID(8) - assert.NoError(s.T(), err) - assert.Nil(s.T(), client.ElevatedUntil) -} - -func (s *ClientSuite) Test_ElevateClient_expectBadRequestOnOverflowDuration() { - s.db.User(5).Client(8) - - test.WithUser(s.ctx, 5) - // A value that would overflow time.Duration (wrapping to a past - // timestamp) must be rejected rather than silently accepted. + // 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(), 400, s.recorder.Code) + assert.Equal(s.T(), 204, s.ctx.Writer.Status()) client, err := s.db.GetClientByID(8) assert.NoError(s.T(), err) - assert.Nil(s.T(), client.ElevatedUntil) + 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) { diff --git a/api/oidc.go b/api/oidc.go index 5a0a2efd3..4acc43d08 100644 --- a/api/oidc.go +++ b/api/oidc.go @@ -86,7 +86,7 @@ type pendingOIDCSession struct { type pendingElevation struct { ClientID uint `form:"id" binding:"required"` - DurationSeconds int `form:"durationSeconds" binding:"required,min=1,max=2592000"` + DurationSeconds int `form:"durationSeconds" binding:"required"` } // OIDCAPI provides handlers for OIDC authentication. @@ -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 := time.Now().Add(model.ElevationDuration(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 1a7efbcbc..ff97f220f 100644 --- a/model/elevate.go +++ b/model/elevate.go @@ -10,14 +10,32 @@ type ElevateRequest struct { // // required: true // example: 900 - DurationSeconds int `form:"durationSeconds" query:"durationSeconds" json:"durationSeconds" binding:"required,min=1,max=2592000"` + DurationSeconds int `form:"durationSeconds" query:"durationSeconds" json:"durationSeconds" binding:"required"` } var DefaultElevationDuration = time.Hour -// MaxElevationDurationSeconds is the upper bound (30 days) accepted for a -// requested elevation duration. It bounds ElevateRequest.DurationSeconds so a -// request can neither make elevation effectively permanent nor overflow -// time.Duration (which wraps past ~292 years, silently yielding a past -// elevatedUntil timestamp). Requests above this are rejected with 400. -const MaxElevationDurationSeconds = 2592000 +// 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 + +// 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. +func ElevationDuration(seconds int) time.Duration { + if seconds <= 0 { + return DefaultElevationDuration + } + 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 +} diff --git a/model/elevate_test.go b/model/elevate_test.go new file mode 100644 index 000000000..9c872f59a --- /dev/null +++ b/model/elevate_test.go @@ -0,0 +1,32 @@ +package model + +import ( + "testing" + "time" +) + +func TestElevationDuration(t *testing.T) { + cases := []struct { + name string + seconds int + 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}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + got := ElevationDuration(c.seconds) + if 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) + } + }) + } +} From c2da15e45a510d2c2ad16d9cc6a54179dcaaa37d Mon Sep 17 00:00:00 2001 From: piyush295 Date: Mon, 5 Oct 2026 07:09:41 +0530 Subject: [PATCH 3/5] 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. --- model/elevate.go | 34 +++++++++++++++------------------- model/elevate_test.go | 20 ++++++++++---------- 2 files changed, 25 insertions(+), 29 deletions(-) diff --git a/model/elevate.go b/model/elevate.go index ff97f220f..d78370cba 100644 --- a/model/elevate.go +++ b/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 } diff --git a/model/elevate_test.go b/model/elevate_test.go index 9c872f59a..5aa6abb07 100644 --- a/model/elevate_test.go +++ b/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) - } }) } } From 78afe8ef8b6b865ac2a31a64720fe61f7d8f1911 Mon Sep 17 00:00:00 2001 From: RAKSHANEX TECHNOLOGIES <126451870+piyush295@users.noreply.github.com> Date: Fri, 16 Oct 2026 00:32:34 +0530 Subject: [PATCH 4/5] Fix 32-bit overflow + collapse negatives to -1s per review - Define cap as time.Duration (maxElevation = 100y) so the untyped int constant doesn't overflow int on 386/arm/v7 (where the Makefile also ships binaries). Compare on int64 so the clamp works independent of int width. - Negative requests now collapse to a one-second past duration (per @eternal-flame-AD) so cancel still works but a wild negative value can't produce a bogus -100-year ElevatedUntil. - Test table uses math.MaxInt / math.MinInt and runtime variables so it compiles under GOARCH=386 and GOARCH=arm/v7 (where literal int64 values and constant-folded Duration arithmetic overflow). --- model/elevate.go | 24 +++++++++++++----------- model/elevate_test.go | 34 ++++++++++++++++++++++++++-------- 2 files changed, 39 insertions(+), 19 deletions(-) diff --git a/model/elevate.go b/model/elevate.go index d78370cba..56c364e2b 100644 --- a/model/elevate.go +++ b/model/elevate.go @@ -15,23 +15,25 @@ type ElevateRequest struct { var DefaultElevationDuration = time.Hour -// maxElevationSeconds caps a requested elevation duration (~100 years). A +// 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 maxElevationSeconds = 100 * 365 * 24 * 60 * 60 +const maxElevation = 100 * 365 * 24 * time.Hour // ElevationDuration converts a requested duration in seconds to a -// 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). +// time.Duration, clamping to a safe range before multiplying so the result +// cannot overflow. Negative requests collapse to a one-second past duration 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 > maxElevationSeconds { - seconds = maxElevationSeconds - } else if seconds < -maxElevationSeconds { - seconds = -maxElevationSeconds + s := int64(seconds) + if s > int64(maxElevation/time.Second) { + return maxElevation } - return time.Duration(seconds) * time.Second + if s < 0 { + return -time.Second + } + return time.Duration(s) * time.Second } diff --git a/model/elevate_test.go b/model/elevate_test.go index 5aa6abb07..035df393b 100644 --- a/model/elevate_test.go +++ b/model/elevate_test.go @@ -1,11 +1,29 @@ package model import ( + "math" "testing" "time" ) func TestElevationDuration(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 + } + cases := []struct { name string seconds int @@ -13,14 +31,14 @@ func TestElevationDuration(t *testing.T) { }{ {"normal", 900, 900 * time.Second}, {"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}, + // 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. + {"cancel -1 preserved as past", -1, -time.Second}, + {"large negative preserved as past", minInt, -time.Second}, + // 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, expectedForHuge}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { From 91d73bc073b0d938b4e00df11559917202b66f0b Mon Sep 17 00:00:00 2001 From: Yumechi Date: Thu, 22 Oct 2026 15:19:46 +0800 Subject: [PATCH 5/5] refactor: put elevation duration math inside ElevationTime() --- api/client.go | 2 +- api/client_test.go | 21 +++++++++++++++++++++ api/oidc.go | 2 +- model/elevate.go | 19 +++++++------------ model/elevate_test.go | 18 ++++++++++-------- 5 files changed, 40 insertions(+), 22 deletions(-) diff --git a/api/client.go b/api/client.go index 79366f56f..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(model.ElevationDuration(params.DurationSeconds)) + 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 fc3b287b2..503a1f152 100644 --- a/api/client_test.go +++ b/api/client_test.go @@ -358,6 +358,27 @@ 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) diff --git a/api/oidc.go b/api/oidc.go index 4acc43d08..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(model.ElevationDuration(elevate.DurationSeconds)) + 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 56c364e2b..4887de9b9 100644 --- a/model/elevate.go +++ b/model/elevate.go @@ -22,18 +22,13 @@ var DefaultElevationDuration = time.Hour // (~292 years) and would otherwise wrap to a bogus (even past) timestamp. const maxElevation = 100 * 365 * 24 * time.Hour -// ElevationDuration converts a requested duration in seconds to a -// time.Duration, clamping to a safe range before multiplying so the result -// cannot overflow. Negative requests collapse to a one-second past duration 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 { - s := int64(seconds) - if s > int64(maxElevation/time.Second) { - return maxElevation +// 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 s < 0 { - return -time.Second + if deltaSeconds < 0 { + return time.Time{} } - return time.Duration(s) * time.Second + return base.Add(time.Duration(deltaSeconds) * time.Second) } diff --git a/model/elevate_test.go b/model/elevate_test.go index 035df393b..ebb17146e 100644 --- a/model/elevate_test.go +++ b/model/elevate_test.go @@ -6,7 +6,7 @@ import ( "time" ) -func TestElevationDuration(t *testing.T) { +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 @@ -24,25 +24,27 @@ func TestElevationDuration(t *testing.T) { expectedForHuge = maxElevation } + baseTime := time.Now() + cases := []struct { name string seconds int - want time.Duration + want time.Time }{ - {"normal", 900, 900 * time.Second}, - {"zero stays zero", 0, 0}, + {"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. - {"cancel -1 preserved as past", -1, -time.Second}, - {"large negative preserved as past", minInt, -time.Second}, + {"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, expectedForHuge}, + {"huge clamped to max", maxInt, baseTime.Add(expectedForHuge)}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { - if got := ElevationDuration(c.seconds); got != c.want { + if got := ElevationTime(baseTime, c.seconds); got != c.want { t.Fatalf("ElevationDuration(%d) = %v, want %v", c.seconds, got, c.want) } })