Skip to content

Add support for expiring oauth tokens - #549

Open
stephanmiehe wants to merge 4 commits into
ludeeus:mainfrom
stephanmiehe:oauth-token-refresh
Open

stephanmiehe wants to merge 4 commits into
ludeeus:mainfrom
stephanmiehe:oauth-token-refresh

Conversation

@stephanmiehe

Copy link
Copy Markdown

Description

This PR adds support for expiring oauth tokens. Using expiring tokens ensures that should a token ever leak, the exposure is limited by the lifetime of the oauth token. There is an influx in security incidents going on around the world because threat actors are finding oauth tokens and weaponizing them.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (which adds functionality)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements

Checklist

  • Tests pass locally (scripts/test)
  • Code has been formatted (scripts/lint)
  • Tests added for new functionality (if applicable)

Copilot AI balanced review requested due to automatic review settings November 12, 2026 11:45

Copilot AI 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.

🟡 Changes recommended

The refresh request ignores configured timeout, header, and client-name settings that registration and activation correctly honor.

1 open issue
What changed in this PR

This PR adds support for expiring OAuth credentials to the device authentication flow, allowing callers to refresh tokens and update existing API clients.

Changes:

  • Add refresh-token handling, expiration timestamps, and credential validation.
  • Allow access-token replacement without recreating API namespaces.
  • Add tests for refresh responses, error handling, cancellation, and header preservation.
File Description
tests/​device/​test_refresh.py Tests token refresh, expiration, failures, and token replacement.
tests/​common.py Adds async context-manager support to mock responses.
aiogithubapi/​models/​login_oauth.py Validates credentials and calculates expiration timestamps.
aiogithubapi/​github.py Exposes token replacement on the public API client.
aiogithubapi/​device.py Adds token refresh and captures activation request timing.
aiogithubapi/​client.py Updates the token used by existing namespaces.

Review effort: Balanced

Comment thread aiogithubapi/device.py Resolved
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings November 12, 2026 12:17

Copilot AI 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.

🔵 Needs a closer look

Token changes can leave an old Authorization credential active, and refresh requests with automatic HTTP-error raising can misclassify authentication and rate-limit failures as connection errors.

No open issues 🎉

2 previously missed issues

In code that hasn't changed since last review

Medium severity Token updates leave the configured Authorization header in effect

aiogithubapi/​client.py:87

If the client was constructed with headers={'Authorization': 'token old'}, both set_token('new') and set_token(None) leave that credential active. GitHubBaseRequestDataModel.headers applies the configured headers after the token-derived header, so subsequent requests still authenticate with the old token. Remove or update the base Authorization override here while preserving the other configured headers.

Medium severity Disable automatic status raising to classify refresh responses

aiogithubapi/​device.py:241

With an injected aiohttp.ClientSession(raise_for_status=True), this request inherits automatic HTTP-error raising. A 400/401 rejected refresh token or a 429 response then raises ClientResponseError before this block is entered and is caught as GitHubConnectionException, bypassing the authentication and rate-limit classification below. Explicitly pass raise_for_status=False so this method can inspect and classify those responses.

1 resolved since last review

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings November 12, 2026 12:55

Copilot AI 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.

🔵 Needs a closer look

A successful response with an explicitly null refresh token is accepted, risking loss of the stored refresh token and preventing future access-token renewal.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Medium severity Reject null refresh tokens in successful OAuth responses

aiogithubapi/​device.py:261

A successful response containing "refresh_token": null bypasses this fallback and is accepted by GitHubLoginOauthModel. A caller persisting the returned credentials would overwrite its refresh token with None and lose the ability to renew the access token. Reject an explicitly null refresh token as a malformed response before constructing the model.

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings November 13, 2026 00:42

Copilot AI 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.

🟢 Approval recommended

No active review issues were identified for the OAuth credential refresh and token-replacement changes.

No open issues 🎉

Review effort: Balanced

This branch has not been deployed

No deployments
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.

2 participants