Bound client elevation duration to prevent overflow and permanent elevation

[piyush295]

Fixes #1051.

Problem

ElevateRequest.DurationSeconds was only validated as non-zero (binding:"required"), 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 with durationSeconds: 3153600000 set elevatedUntil a century into the future.
2. Large values silently overflowed — time.Duration(DurationSeconds) * time.Second wraps for anything past ~292 years, so durationSeconds: 9223372036854775807 produced a past elevatedUntil. The request still returned 204, but the elevation was silently dropped.

Fix

Add a max bound (30 days) plus min=1 via the binding tag on both ElevateRequest.DurationSeconds and the OIDC pendingElevation.DurationSeconds, so out-of-range durations are rejected with 400 instead of being silently mishandled. A MaxElevationDurationSeconds constant documents the ceiling and the reasoning.

30 days is a generous ceiling (the default login elevation is 1 hour) that still eliminates both the "permanent elevation" and the overflow cases.

Testing

• Added regression tests Test_ElevateClient_expectBadRequestOnDurationAboveMax (max+1 → 400) and Test_ElevateClient_expectBadRequestOnOverflowDuration (MaxInt64 → 400), asserting ElevatedUntil stays nil.
• go test ./api/ — all pass, including the existing elevate suite.
• gofmt clean, go vet ./model/ ./api/ clean.

[eternal-flame-AD]

Hi, we have already handled the same concern in a separate private advisory. Our response was:

Thanks for the report. I'm not sure I understand the impact, or rather how an high elevation period has a meaningful effect.

When the attacker has an elevated session, they can just change the password, create new admin users. Which is much more intrusive than having an unlimited elevated session.

If there was a limit to the duration, the attacker still can extend the elevation period before it expires, so I don't think adding it makes much difference.

So I would just clamp excessive inputs to a sane number (like 100 years) to fix the overflow and not impose an arbitrary "security" limit. When we designed this feature we also considered use cases where people might need to opt out of this timer feature, and setting a practically inifinite expiration time is a necessary relief valve.

[piyush295]

Thanks @eternal-flame-AD, that makes sense — I've reworked it accordingly.

Instead of imposing a hard max / rejecting with 400, I now clamp the duration: added model.ElevationDuration(seconds) which converts to a time.Duration and caps at ~100 years. That keeps an effectively-infinite elevation available as the relief valve you described, while staying safely below the point where time.Duration overflows (int64 ns wraps past ~292 years) and would silently produce a past elevatedUntil. So the only behavioural change is that the overflow case now elevates far into the future instead of dropping to the past — no arbitrary security limit, and no 400s.

Both elevation paths (/client/:id/elevate and the OIDC callback) use the helper; the min/max binding tags are gone.

Tests: model.TestElevationDuration (normal / zero+negative default / huge clamp / ceiling) and Test_ElevateClient_clampsHugeDuration (MaxInt64 still elevates with a future timestamp). Full go test ./api/ ./model/ passes.

[lbellows]

Thanks for reworking this. Two problems with ElevationDuration:

1. "Cancel elevation" now elevates for an hour. The UI sends durationSeconds: -1 to cancel (ui/src/client/ElevateClientDialog.tsx:22). On master that sets elevatedUntil to the past. With this PR every seconds <= 0 becomes DefaultElevationDuration, so ElevationDuration(-1) returns 1h: clicking Cancel grants a fresh hour of elevation while the UI says "Canceled client elevation". The "negative falls back to default" test case locks this in.

2. The overflow check misses wraps that land positive. It multiplies first and then checks d <= 0, but int64 wrap can come out positive:

• 18446744074 → 290ms (elevation silently expires, the original bug)
• 20023544073 → ~50 years instead of the 100-year cap

Clamping in seconds before multiplying fixes both, and also covers the large negative wrap @somaz94 raised on #1051 (-9223372037 elevating until 2319):

const maxElevationSeconds = 100 * 365 * 24 * 60 * 60

func ElevationDuration(seconds int) time.Duration {
	if seconds > maxElevationSeconds {
		seconds = maxElevationSeconds
	} else if seconds < -maxElevationSeconds {
		seconds = -maxElevationSeconds
	}
	return time.Duration(seconds) * time.Second
}

Tests worth adding: -1 (result in the past), 18446744074, -9223372037. Minor: the "just under ceiling" case actually passes exactly the ceiling.

[eternal-flame-AD]

I agree with @lbellows except negative values should be clamped to -1 or use a hardcoded result (like 0).

Also no need to make this overly complicated, this should be a small change in the function itself. You can add a unit test ensuring the hardcoded max will work, but don't overthink this.

[piyush295]

Thanks @lbellows @eternal-flame-AD — fixed both, and kept it simple:

• Clamp seconds before multiplying so the int64 multiply can't overflow — this covers both the wrap-to-positive case (20023544073) and the large-negative wrap (-9223372037).
• Preserve negatives instead of mapping seconds <= 0 to the default, so the UI's cancel (durationSeconds: -1) sets elevatedUntil in the past again instead of granting a fresh hour. Removed the "negative falls back to default" case that locked in the wrong behaviour.

ElevationDuration is now a small guard inside the function (no extra helper ceremony). Tests cover: normal, zero, cancel -1 preserved, huge clamped, overflow-positive clamped, large-negative clamped. go test ./model/ ./api/ green.

[lbellows]

Thanks, the logic looks right now. Two things left, and the first one is my fault from the snippet I posted:

1. It doesn't compile on 32-bit. maxElevationSeconds (3153600000) overflows int on 386/arm/v7, which the Makefile ships:

   model/elevate.go:32:13: cannot use maxElevationSeconds (untyped int constant 3153600000) as int value in assignment (overflows)
   

   The test table has the same issue with 9223372036854775807 in an int field; math.MaxInt / math.MinInt work on both.
2. @eternal-flame-AD asked for negatives to clamp to -1 (or 0) rather than -100 years.

Doing it in int64 covers both:

const maxElevation = 100 * 365 * 24 * time.Hour

func ElevationDuration(seconds int) time.Duration {
	s := int64(seconds)
	if s > int64(maxElevation/time.Second) {
		return maxElevation
	}
	if s < 0 {
		return -time.Second
	}
	return time.Duration(s) * time.Second
}

GOARCH=386 go vet ./model/ ./api/ is a quick check.

[piyush295]

@lbellows thanks, applied exactly that:

• maxElevation is now a time.Duration constant (100 * 365 * 24 * time.Hour) and the comparison uses int64(maxElevation/time.Second), so nothing hits an untyped-int overflow on 386/arm/v7.
• Negative seconds collapse to -time.Second (per @eternal-flame-AD), so cancel still works and a wild negative can't produce a bogus -100-year ElevatedUntil.
• Test table rewritten to use math.MaxInt / math.MinInt via runtime variables (so the constant-folded time.Duration(math.MaxInt) * time.Second doesn't overflow during build) with a per-arch expected value for the "huge" case, since on 386/arm/v7 math.MaxInt < maxElevationSeconds and clamping is unreachable there.

GOARCH=386 go vet ./model/ ./api/ and GOARCH=arm GOARM=7 go vet ./model/ ./api/ are both clean. Commit 87bde75.

[codecov]

Codecov Report

❌ Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 76.73%. Comparing base (d02796b) to head (87bde75).
⚠️ Report is 21 commits behind head on master.

Files with missing lines	Patch %	Lines
api/oidc.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1058      +/-   ##
==========================================
+ Coverage   76.09%   76.73%   +0.63%     
==========================================
  Files          67       69       +2     
  Lines        3619     3731     +112     
==========================================
+ Hits         2754     2863     +109     
- Misses        654      656       +2     
- Partials      211      212       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[piyush295]

Re the codecov note: go test -coverprofile locally shows ElevationDuration at 100% function coverage — all three branches of the clamp (overflow, negative, normal multiply) are hit by the test matrix. The 88.88% / 1-missing-line is from a surrounding file in the package where I didn't add new code; happy to add an extra assertion if you'd prefer, but functionally the fix path itself is fully covered.