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

Open
opened 2026-08-05 08:08:04 -04:00 by saavagebueno · 0 comments
Owner

Original Pull Request: https://github.com/netbirdio/netbird/pull/7013

State: open
Merged: No


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.

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.
**Original Pull Request:** https://github.com/netbirdio/netbird/pull/7013 **State:** open **Merged:** No --- ## 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](https://datatracker.ietf.org/doc/html/rfc8628#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](https://datatracker.ietf.org/doc/html/rfc8628#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 <!-- branch-stack --> ### Checklist - [x] Is it a bug fix - [ ] Is a typo/documentation fix - [ ] Is a feature enhancement - [ ] It is a refactor - [x] 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](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTING.md#discuss-changes-with-the-netbird-team-first). > By submitting this pull request, you confirm that you have read and agree to the terms of the [Contributor License Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [ ] I added/updated documentation for this change - [x] 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 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## 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. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
saavagebueno added the pull-request label 2026-08-05 08:08:04 -04:00
Sign in to join this conversation.
No Label pull-request
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: DYNR/netbird#29439