Skip to content
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

[FRPAL-5609] Add support for redirects from API endpoints #46

Merged
merged 4 commits into from
Jul 22, 2024

Conversation

arctouch-alejandrovarela
Copy link
Contributor

@arctouch-alejandrovarela arctouch-alejandrovarela commented Jul 15, 2024

FRPAL-5609 Add support for redirects from API endpoints

[FRPAL-5609] Add support for redirects from API endpoints
Copy link

github-actions bot commented Jul 15, 2024

Test Results

  3 files   3 suites   3s ⏱️
 34 tests 33 ✅ 1 💤 0 ❌
102 runs  99 ✅ 3 💤 0 ❌

Results for commit 5556d2d.

♻️ This comment has been updated with latest results.

Copy link
Member

@mediabounds mediabounds left a comment

Choose a reason for hiding this comment

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

There is a subtle, hard-to-reproduce bug with this approach.

If a user has an expired access token, but a valid refresh token, they will get a 403 Forbidden result code (but with the same URL); the code in lines 189-198 will get a new access token and retry the request.

This request will get redirected, but then result in ForbiddenRedirectException getting thrown to the caller and not handled in Float.Core.

Copy link
Member

@mediabounds mediabounds left a comment

Choose a reason for hiding this comment

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

Can you look into the HttpClientHandler.AllowAutoRedirect property?

It seems like maybe we can set this value to false and then we would receive the original redirected response (and be able to check the status of code 301/302 as opposed to assuming an unsuccessful response should be retried).

This may also be able to be done directly in the implementing application as opposed to Float.Core (the implementing application is what defines the HttpClientHandler).

@arctouch-alejandrovarela
Copy link
Contributor Author

@mediabounds After setting the value to false now the 301/302 is returned, now Im validating if a redirect status code is returned and also that the host of both the original request and the redirect is the same to avoid sending the request to a unknown location

Float.Core/Net/RequestClient.cs Outdated Show resolved Hide resolved
@arctouch-alejandrovarela arctouch-alejandrovarela merged commit 85f2141 into main Jul 22, 2024
2 checks passed
@arctouch-alejandrovarela arctouch-alejandrovarela deleted the FRPAL-5609-add-support-for-redirect branch July 22, 2024 13:50
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