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.