Repository navigation
fix(coderd): reject overflowing chat workspace TTL values - #31036
ibetitsmike wants to merge 1 commit into
Conversation
putChatWorkspaceTTL converted workspace_ttl_ms to a time.Duration before it checked the 30-day maximum, so a value whose nanosecond count overflows int64 wrapped into range: 18446744133710 ms became about one minute, passed both checks, and was stored. Check the maximum on the millisecond value before the conversion.
|
@codex review
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review
|
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
PUT /api/v2/chats/config/workspace-ttlconvertedworkspace_ttl_msto atime.Durationbefore it checked the 30-day maximum. A value whose nanosecond count overflowsint64wraps into range:18446744133710ms becomes about one minute, passes both range checks, and is stored. This PR checks the maximum on the millisecond value, before the conversion.Why it is safe: for every value that does not overflow,
req.WorkspaceTTLMillis > ttlMaximum.Milliseconds()is the same test asd > ttlMaximum, and it writes the same400message.TestChatWorkspaceTTLalready pins the boundary: exactly 30 days is accepted and 721 hours is rejected. A mutant that rejects exactly 30 days fails it.Red test:
TestChatWorkspaceTTLnow sends18446744133710ms and expects a400and an unchanged stored value. Onmainthe handler accepts the value:Review focus: the order of the checks. The maximum now runs on milliseconds before the conversion; the one-minute minimum still runs on the converted duration, which is safe once the maximum has passed.
#31018 moves this handler into a shared
putChatOperationalSettinghelper. This fix is based onmain, and whichever of the two merges second will mergemainand keep this check.Validation:
go test ./coderd -run '^TestChatWorkspaceTTL$' -count=1fails onmainas above and passes on this branch. On this branch,go test ./coderd -run '^(TestChatPersonalModelOverridesAdminSettings|TestChatComputerUseProvider|TestChatDebugLoggingSettings|TestChatWorkspaceTTL|TestChatRetentionDays.*|TestChatDebugRetentionDays|TestChatAutoArchiveDays|TestAuditedChatOperationalSettingWrite.*)$' -count=1passes (11 tests, 27 with subtests) andgo test ./enterprise/coderd -run '^TestChatOperationalSettingsAudit$' -count=1passes (17 with subtests).go vet ./coderd/is clean, and the pre-commit hook passed (make gen, formatting, golangci-lint, slim build).