Skip to content

fix(security): harden auth code flow and narrow error handling#1

Open
appleboy wants to merge 4 commits intomainfrom
worktree-sdk-python
Open

fix(security): harden auth code flow and narrow error handling#1
appleboy wants to merge 4 commits intomainfrom
worktree-sdk-python

Conversation

@appleboy
Copy link
Member

@appleboy appleboy commented Mar 23, 2026

Summary

  • URL injection fix: Use urllib.parse.urlencode for authorization URL construction instead of f-string concatenation to prevent parameter injection
  • Callback timeout: Add 300s timeout on OAuth callback event.wait() to prevent indefinite blocking when the browser callback never arrives
  • Thread-safe callback handler: Replace class-attribute shared state with a per-flow factory function to avoid race conditions
  • Narrow exception handling: Replace broad except Exception with (NotFoundError, AuthFlowError) in authenticate() / async_authenticate() so real errors are no longer silently swallowed
  • RFC 7009 compliance: Include client_id and client_secret in token revocation requests per spec recommendation
  • Remove broken require_scope: FastAPI Depends() with no argument would instantiate an empty TokenInfo dataclass instead of receiving validated token data; BearerAuth(required_scopes=[...]) already handles scope validation
  • Efficiency: async_authenticate() no longer creates an unnecessary sync OAuthClient + connection pool just to check the credential store
  • Consistency: Add missing from future import annotations in exceptions.py

Test plan

  • make lint passes
  • make typecheck passes (mypy strict)
  • make test passes (65/65 tests)
  • Manual test: auth code flow with browser callback
  • Manual test: device code flow (sync + async)

Generated with Claude Code

@appleboy @claude
fix(security): harden auth code flow and narrow error handling
8b74376 
- Use urllib.parse.urlencode for authorization URL to prevent parameter injection
- Add 300s timeout on auth code callback to prevent indefinite blocking
- Replace class-attribute shared state with per-flow factory for thread safety
- Narrow except Exception to specific types in authenticate entry points
- Include client_id and client_secret in token revocation per RFC 7009
- Remove broken require_scope FastAPI dependency with empty Depends()
- Avoid creating unnecessary sync OAuthClient in async_authenticate
- Add missing from __future__ import annotations in exceptions module

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 23, 2026 13:45
Copilot AI reviewed Mar 23, 2026
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens the OAuth authentication flows and error handling in AuthGate’s Python SDK, focusing on safer URL construction, preventing indefinite waits during browser callbacks, improving thread safety, and tightening which errors are swallowed vs surfaced.

Changes:

  • Add client authentication fields to RFC 7009 token revocation requests (sync + async clients).
  • Refactor auth code callback handling to be per-flow (thread-safe), build authorization URLs via urlencode, and add a 300s callback timeout.
  • Narrow exception handling in authenticate() / async_authenticate() and simplify async credential-store checking to avoid creating a sync client.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/authgate/oauth/client.py Include client_id (+ optional client_secret) in token revocation form body.
src/authgate/oauth/async_client.py Same revocation hardening for async client.
src/authgate/middleware/fastapi.py Remove require_scope dependency helper and unused FastAPI imports.
src/authgate/exceptions.py Add from __future__ import annotations for consistency.
src/authgate/authflow/authcode.py Use urlencode for auth URL query, add callback timeout, and make callback handler per-flow.
src/authgate/init.py Narrow exception handling and change async token reuse/persist logic to avoid sync client creation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@appleboy @claude
fix(oauth): catch OAuthError during refresh and add async token refresh
10e1aaf 
- Include OAuthError in authenticate() exception handling so expired
  refresh tokens fall back to interactive flow instead of propagating
- Add refresh token logic to async_authenticate() to avoid unnecessary
  device-flow prompts when a stored refresh token is available

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@appleboy appleboy requested a review from Copilot March 23, 2026 13:52
Copilot AI reviewed Mar 23, 2026
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@appleboy @claude
fix(oauth): narrow OAuthError handling and publicize token converters
a2351a9 
- Only swallow invalid_grant/invalid_token OAuthError during token
  refresh, re-raise unexpected errors like server_error
- Rename _credstore_to_oauth/_oauth_to_credstore to public API
  (credstore_to_oauth/oauth_to_credstore) and export from authflow
- Apply same selective OAuthError handling in async_authenticate()

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@appleboy appleboy requested a review from Copilot March 23, 2026 14:00
Copilot AI reviewed Mar 23, 2026
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@appleboy @claude
fix(authflow): close server socket and avoid blocking event loop
9ee2590 
- Call server.server_close() after shutdown() in auth code flow to
  release the listening socket/port promptly
- Wrap sync credential store operations in asyncio.to_thread() in
  async_authenticate() to avoid blocking the event loop

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@appleboy appleboy requested a review from Copilot March 23, 2026 14:10
Copilot AI reviewed Mar 23, 2026
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@appleboy