Skip to content

[client] Fix device flow polling interval defaults and slow_down handling - #7013

Open
Optic00 wants to merge 3 commits into
netbirdio:mainfrom
Optic00:codex/fix-device-flow-polling
Open

[client] Fix device flow polling interval defaults and slow_down handling#7013
Optic00 wants to merge 3 commits into
netbirdio:mainfrom
Optic00:codex/fix-device-flow-polling

Conversation

@Optic00

@Optic00 Optic00 commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Describe your changes

The device authorization flow mishandled the polling interval in two RFC-relevant ways and also lacked bounds checks for values that cannot be represented safely as time.Duration.

  1. interval is optional in the device authorization response (RFC 8628, section 3.2). If an IdP omitted it, the field decoded to 0, and WaitToken passed 0 into time.NewTicker, which panics with non-positive interval for NewTicker. Login against such an IdP crashed instead of polling.
  2. On a slow_down error the interval was increased by 3 seconds. RFC 8628, section 3.5 requires the polling interval to be increased by 5 seconds for this and all subsequent requests. Each later slow_down response applies the same rule again, so repeated responses increase the interval cumulatively.
  3. Very large positive intervals could overflow when converted to time.Duration, and repeated slow_down increases could overflow during addition.

Changes:

  • initialDeviceFlowPollingInterval uses the RFC's 5-second default when interval is omitted and decoded as zero. Because AuthFlowInfo.Interval is an int, an explicit zero is indistinguishable from omission; non-positive values also fall back to 5 seconds as defensive hardening.
  • deviceFlowPollingIntervalFromSeconds converts positive values safely and saturates values above the largest whole-second duration representable by time.Duration.
  • slowDownDeviceFlowPollingInterval adds 5 seconds for each slow_down response when representable and saturates at the maximum positive time.Duration.

These helpers are unexported; production polling reaches them only through WaitToken. The change touches client/internal/auth/device_flow.go and its test file only. No public API, gRPC protocol, CLI or service flag, JSON payload, persistence format, or PKCE behavior is affected.

The tests are deterministic and cover omitted and provided intervals, non-positive values, maximum integer inputs, exact overflow boundaries, cumulative slow_down, saturation behavior, canceled contexts, and ticker safety. They require no real sleeps, fake clocks, or new dependencies.

Note on the last checklist item: this is a behavior fix, so the client's runtime polling behavior does change. The omitted-interval default and the five-second slow_down increase align the client with RFC 8628; handling explicit non-positive and unrepresentably large values is additional defensive hardening. I left that box unchecked rather than claim no behavior change. Happy to discuss if you want it handled differently.

Issue ticket number and link

N/A

Stack

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)
  • This change does not modify the public API, gRPC protocols, functionality behavior, CLI / service flags, or introduce a new feature, OR I have discussed it with the NetBird team beforehand (link the issue / Slack thread in the description). See CONTRIBUTING.md.

By submitting this pull request, you confirm that you have read and agree to the terms of the Contributor License Agreement.

Documentation

Select exactly one:

  • I added/updated documentation for this change
  • Documentation is not needed for this change (explain why): this corrects internal polling behavior to the existing RFC 8628 contract. No user-facing configuration, API, or flag changes.

Docs PR URL (required if "docs added" is checked)

Paste the PR link from https://github.com/netbirdio/docs here:

N/A

Summary by CodeRabbit

  • Bug Fixes
    • Improved device authorization polling when providers omit, provide invalid values, or specify extremely large intervals.
    • Applied the RFC's 5-second default and saturated unrepresentable intervals to prevent duration overflow and invalid ticker durations.
    • Increased polling delays consistently after slow_down responses, including boundary cases.
    • Ensured canceled authorization requests return promptly without issuing a token.
    • Improved reliability across a wider range of provider polling behaviors.

@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The device authorization flow now normalizes polling intervals, defaults invalid values to five seconds, clamps oversized values, and adds five seconds for slow_down responses. Tests cover cancellation, overflow boundaries, saturation, and ticker safety.

Changes

Device Flow Polling Interval Update

Layer / File(s) Summary
Polling interval normalization and WaitToken wiring
client/internal/auth/device_flow.go
Adds default, increment, and maximum interval constants. Safely converts provider intervals, defaults non-positive values to five seconds, clamps oversized values, and applies five-second slow_down increments.
Interval boundary and cancellation coverage
client/internal/auth/device_flow_test.go
Tests cancellation, default and positive intervals, duration clamping, maximum integer inputs, slowdown saturation, and safe ticker construction.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is detailed and complete, but it lacks the required issue or approved discussion link for this behavior change. Add the required issue ticket or approved NetBird team discussion link, and update the final checklist item to reference that approval.
✅ Passed checks (4 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the client device-flow polling interval fixes, including defaults and slow_down handling.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@Optic00
Optic00 marked this pull request as ready for review July 31, 2026 13:27

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
client/internal/auth/device_flow_test.go (1)

339-341: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Test cumulative slow_down behavior.

This assertion verifies one five-second increment only. It does not verify that a second slow_down uses the updated interval.

Apply the helper twice and expect 2s → 7s → 12s, or add a WaitToken test with two consecutive slow_down responses.

Proposed test extension
 func TestSlowDownDeviceFlowPollingInterval(t *testing.T) {
-	require.Equal(t, 7*time.Second, slowDownDeviceFlowPollingInterval(2*time.Second))
+	interval := slowDownDeviceFlowPollingInterval(2 * time.Second)
+	require.Equal(t, 7*time.Second, interval)
+	require.Equal(t, 12*time.Second, slowDownDeviceFlowPollingInterval(interval))
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/auth/device_flow_test.go` around lines 339 - 341, Extend
TestSlowDownDeviceFlowPollingInterval to apply slowDownDeviceFlowPollingInterval
cumulatively: start at 2 seconds, assert the first call returns 7 seconds, then
pass that result into a second call and assert it returns 12 seconds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@client/internal/auth/device_flow.go`:
- Around line 251-262: Bound device-flow polling durations in
initialDeviceFlowPollingInterval and slowDownDeviceFlowPollingInterval so
external interval values and repeated slow_down increments cannot overflow
time.Duration or produce non-positive durations before ticker creation/reset.
Validate and return an error, or clamp both results to a safe maximum duration
accepted by the surrounding polling flow, while preserving the default behavior
for non-positive initial values.

---

Nitpick comments:
In `@client/internal/auth/device_flow_test.go`:
- Around line 339-341: Extend TestSlowDownDeviceFlowPollingInterval to apply
slowDownDeviceFlowPollingInterval cumulatively: start at 2 seconds, assert the
first call returns 7 seconds, then pass that result into a second call and
assert it returns 12 seconds.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 45aa81cd-fc70-438f-ba52-782ea59363aa

📥 Commits

Reviewing files that changed from the base of the PR and between 234abd7 and 0e026a2.

📒 Files selected for processing (2)
  • client/internal/auth/device_flow.go
  • client/internal/auth/device_flow_test.go

Comment thread client/internal/auth/device_flow.go
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant