Fix Challenge Auth replay bug and update tests#47742
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a request-replay bug in the Key Vault ChallengeAuthPolicy. Since 4.4.0 the policy stored the original request in a client-level field (self._request_copy) and never cleared it, so a later 401 challenge on an unrelated request (e.g., a polling GET to azure-asyncoperation) would swap in the stale POST body and replay it — causing duplicate POST /backup and a 409. The fix moves storage to the per-request pipeline context, scoping it to a single request and guarding against overwriting on retries.
Changes:
- Replace client-level
self._request_copywith a per-requestrequest.context[_REQUEST_COPY_KEY]stash in sync and async policies for keys and administration. - Add a guard so a previously stashed copy isn't overwritten on retry, and refresh CHANGELOGs and test recordings (assets.json).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/keyvault/azure-keyvault-keys/.../challenge_auth_policy.py | Per-request context storage for original request |
| sdk/keyvault/azure-keyvault-keys/.../async_challenge_auth_policy.py | Same fix, imports _REQUEST_COPY_KEY |
| sdk/keyvault/azure-keyvault-administration/.../challenge_auth_policy.py | Same fix for administration |
| sdk/keyvault/azure-keyvault-administration/.../async_challenge_auth_policy.py | Same fix, imports _REQUEST_COPY_KEY |
| sdk/keyvault/azure-keyvault-keys/CHANGELOG.md | Bug-fix note (grammar nit) |
| sdk/keyvault/azure-keyvault-administration/CHANGELOG.md | Bug-fix note (grammar nit) |
| both assets.json | Updated recording tags |
Notable concern: the same policy file is duplicated in azure-keyvault-secrets, azure-keyvault-certificates, and azure-keyvault-securitydomain, which still contain the buggy client-level storage and were not updated; no unit test was added for the per-request isolation despite the PR checklist.
|
|
||
| ### Bugs Fixed | ||
|
|
||
| - Fixed an replay bug in challenge authentication policy. The original request is now stored at the request level instead of the client level. |
|
|
||
| ### Bugs Fixed | ||
|
|
||
| - Fixed an replay bug in challenge authentication policy. The original request is now stored at the request level instead of the client level. |
| # Key under which the original request is stashed on the per-request pipeline context during the challenge flow. | ||
| # Storing this per-request (rather than on the policy instance) prevents the body of one request from leaking into a | ||
| # subsequent request made by the same client. | ||
| _REQUEST_COPY_KEY = "key_vault_request_copy" |
| if request.http_request.content and _REQUEST_COPY_KEY not in request.context: | ||
| request.context[_REQUEST_COPY_KEY] = request.http_request |
Description
Bug Description :
In 4.4.0, ChallengeAuthPolicy introduces a cleint-level field self._request_copy. During the initial Key Vault challenge flow, the policy stores the original request in self._request_copy and sends a modified “bodiless” request to elicit the 401 challenge. However, self._request_copy is never cleared, so when a later request (the polling GET to the azure-asyncoperation URL) receives a 401 challenge, on_challenge() sees the stale self._request_copy and swaps the current request (GET) back to the old request (POST). This results in the service receiving a duplicate POST /backup, and it correctly responds 409 because a backup job is already running.
Fix :
we have moved back to a request level storage instead of Client Level Storing to prevent this bug
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines