Fix async redirect handling regression from FOLLOW_REDIRECTS default#1314
Draft
cursor[bot] wants to merge 1 commit into
Draft
Fix async redirect handling regression from FOLLOW_REDIRECTS default#1314cursor[bot] wants to merge 1 commit into
cursor[bot] wants to merge 1 commit into
Conversation
Commit #1278 changed redirect following to default true unless FOLLOW_REDIRECTS is explicitly set. That broke async connectors (e.g. Slack Analytics) which rely on intercepting 3xx responses when FOLLOW_REDIRECTS is not configured. Restore the prior default: do not follow redirects in async processing unless FOLLOW_REDIRECTS is explicitly set. Add a unit test that locks in the behavior. Co-authored-by: Erik Schultink <eschultink@users.noreply.github.com>
Member
|
@aperez-worklytics thoughts? cursor keeps trying to make this change. I don't think anything OTHER than chatgpt- sends the 307 .. but yeah, question of what behavior should be in default case. |
Contributor
IMHO default should be That's what we have as default in absence of the env var: And the only case we want to have to put false is the chatgpt use case. Slack don't use the 307 in the response. I still don't understand why cursor complains about this. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug and impact
Commit #1278 (
3a6c679a) changed the defaultFOLLOW_REDIRECTSbehavior from "disabled in async mode" to "always true unless explicitly set". This breaks async connectors such as Slack Analytics that rely on intercepting 3xx responses and manually fetching from theLocationURL without forwarding source API auth headers (required for pre-signed download URLs).Concrete trigger: An async
admin.analytics.getFilerequest returns a 307 redirect to a pre-signed storage URL. WithfollowRedirects=true, the HTTP client auto-follows the redirect while still sending the Slack OAuthAuthorizationheader, causing the download to fail (403) and async data collection to break.ChatGPT Enterprise is unaffected because its connector spec explicitly sets
FOLLOW_REDIRECTS=FALSE; Slack Analytics does not.Root cause
ApiDataRequestHandlerchanged from.setFollowRedirects(!processingContext.getAsync())to.orElse(true)when resolving theFOLLOW_REDIRECTSconfig property.Fix
Restore the prior default: when
FOLLOW_REDIRECTSis unset, follow redirects in sync mode but disable them in async mode so the existing manual redirect-handling block can fetch fromLocationwithout auth headers.Added
asyncModeDisablesRedirectFollowingByDefaultunit test to lock in the behavior.Validation
All 3 tests pass.