Skip to content

Conversation

axelsrz
Copy link
Member

@axelsrz axelsrz commented Sep 13, 2025

This pull request introduces a deduplication mechanism for OAuth token exchange requests to prevent replay attacks and duplicate processing. The main changes include tracking processed token exchange IDs, handling duplicate requests gracefully, and periodically clearing the registry to avoid unbounded growth. Comprehensive tests have been added to verify these behaviors.

OAuth Token Exchange Deduplication

  • Added a public registry (token_exchange_id_registry) to track processed token exchange IDs within the OAuthFlow class, preventing duplicate token exchanges. The registry is cleared lazily based on a configurable interval to avoid memory growth. [1] [2]
  • Updated the token exchange flow to check for duplicate IDs and return a specific error tag (FlowErrorTag.DUPLICATE_EXCHANGE) if a replay is detected. [1] [2] [3] [4]

Testing Improvements

  • Added new tests to ensure replayed token exchange requests are ignored and that the registry clears after the configured interval, allowing previously processed IDs to be accepted again.

Minor Code Quality Improvements

  • Minor formatting and import cleanups in test utility files (utils.py) and test classes. [1] [2] [3]

Copy link
Contributor

@rodrigobr-msft rodrigobr-msft left a comment

Choose a reason for hiding this comment

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

Hey, I think there was a miscommunication, as I've been working on this feature for a couple of days already: https://github.com/microsoft/Agents-for-python/tree/users/robrandao/oauth-improvements

All I need is to add tests for the above branch.

Anyways, the proposed changes I think would still run into race conditions, as multiple OAuthFlow objects could be instantiated from the same state and be completely unaware of each other before the continue_flow is called. Synchronization across instances of the app (across VMs/on the network) would be difficult to achieve, but within the same AgentApplication instance, I think it would be easiest if we placed the synchronization mechanism at the Authorization class level. I can add some of the changes you have here like the renaming of some of the instance variables in my current branch if you would like.

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