-
Notifications
You must be signed in to change notification settings - Fork 96
fix: handle EasyAuth 302 redirect and improve auth error detection in useAgentCard #8801
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🤖 AI PR Validation ReportPR Review ResultsThank you for your submission! Here's detailed feedback on your PR title and body compliance:✅ PR Title
✅ Commit Type
✅ Risk Level
✅ What & Why
|
| Section | Status | Recommendation |
|---|---|---|
| Title | ✅ | No change needed. |
| Commit Type | ✅ | No change needed. |
| Risk Level | ✅ | Matches label; OK. |
| What & Why | ✅ | Clear and sufficient. |
| Impact of Change | Optionally call out changed files. | |
| Test Plan | ✅ | Unit tests present; ensure CI passes. |
| Contributors | Optional: add credits if applicable. | |
| Screenshots/Videos | ✅ | Not applicable. |
Final notes and actionable recommendations
Overall this PR passes the PR title/body checks and the advised risk level based on the code diff is low, which matches the submitter's assigned Low risk.
A few actionable code-related suggestions discovered from the diff that you should address before merging:
-
Remove the debug console.log: the diff contains
console.log('charlie error:', er);in the catch block. Please remove or replace with an appropriate logger call (or leave a meaningful comment) before merging. -
CI and linter: ensure the added tests and TypeScript changes pass CI and linting. (The
handleUnauthorizedhelper returnsPromise<never>and is used where anAgentCardwould be expected at the type level — TypeScript acceptsneveras a subtype, but please verify your build/tsconfig flags don't flag this pattern.) -
Manual test documentation: if you ran manual verification in an EasyAuth-enabled environment, consider checking the optional Test Plan checkbox and adding 1–2 lines describing the steps you followed (so reviewers can reproduce if needed).
-
Optional test enhancement: you already added good unit tests for network/CORS errors and 403 handling. If
onUnauthorizedmay be async in some callers, consider adding a test that validates an asynconUnauthorizedis awaited (the code usesawait config.onUnauthorized(), so that behavior is already implemented, but a test would protect regressions).
Please make the minor fixes (remove debug log, verify CI), update any small test or documentation gaps if desired, and then re-run CI. Thank you for the thorough change and the test coverage — this looks well scoped and safe to merge once the small items above are addressed.
Last updated: Mon, 09 Feb 2026 21:58:24 GMT
|
📊 Coverage check completed. See workflow run for details. |
|
📊 Coverage check completed. See workflow run for details. |
There was a problem hiding this 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 updates the iframe-app’s useAgentCard hook to better handle Azure App Service EasyAuth scenarios where unauthenticated requests can result in a 302 redirect (often surfacing as an opaque redirect / status 0 in the browser), and to funnel those cases through the existing onUnauthorized flow.
Changes:
- Adds a shared
handleUnauthorizedhelper to centralizeonUnauthorizedinvocation + error throwing. - Forces
fetchto useredirect: 'manual'and treats opaque redirects /status === 0as unauthorized. - Treats
fetchrejection (e.g., network/CORS failures) as unauthorized.
…check, update tests
|
📊 Coverage check completed. See workflow run for details. |
|
📊 Coverage check completed. See workflow run for details. |
Commit Type
Risk Level
What & Why
When Azure EasyAuth is enabled and the user's session expires, the server returns a 302 redirect to the login page instead of a 401. Because the
fetchAPI follows redirects by default and the login page is on a different origin, this results in a CORS error (opaque redirect /TypeError: Failed to fetch) rather than a meaningful HTTP status code. TheuseAgentCardhook did not handle this scenario, causing it to surface a generic network error instead of triggering theonUnauthorizedcallback.Additionally, authentication detection relied on comparing
response.statusText === 'Unauthorized', which is fragile and does not cover 403 Forbidden responses.Changes
fetch()call in a try/catch block so thatTypeError: Failed to fetch(caused by EasyAuth 302 redirects or CORS blocks) triggers theonUnauthorizedcallback and throws anUnauthorizederror.response.statusText === 'Unauthorized'check withresponse.status === 401 || response.status === 403for robust authentication failure detection.handleUnauthorizedhelper: Consolidated the duplicated unauthorized handling logic (callingonUnauthorizedcallback + throwing error) into a reusablehandleUnauthorizedasync function.Failed to fetch agent card: 500 Internal Server Error).onUnauthorizedcallback. Updated existing test mocks to includestatusproperty.Impact of Change
onUnauthorizedcallback now also fires on network/CORS errors and 403 responses.useAgentCardhook.Test Plan
Contributors
Screenshots/Videos