Skip to content

fix(proxy): restore header forwarding through redirects when byte-metering transport is active#6282

Merged
hassan254-prog merged 3 commits into
masterfrom
wari/fix-proxy-forward-headers
May 29, 2026
Merged

fix(proxy): restore header forwarding through redirects when byte-metering transport is active#6282
hassan254-prog merged 3 commits into
masterfrom
wari/fix-proxy-forward-headers

Conversation

@hassan254-prog

Copy link
Copy Markdown
Contributor

Describe the problem and your solution

  • Fix redirect header forwarding when using custom axios transport. Wire the user-provided beforeRedirect callback into the metering transport so forwaded headers can be restored correctly across redirects, this is needed for providers like dayforce.

@hassan254-prog hassan254-prog self-assigned this May 28, 2026

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues found across 3 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

@hassan254-prog hassan254-prog requested a review from a team May 28, 2026 18:56
@hassan254-prog

hassan254-prog commented May 28, 2026

Copy link
Copy Markdown
Contributor Author

I’m tempted to remove the metric, and set forward headers to false for all providers except Dayforce with this pr, now that we have a clearer picture of the redirects, and dayforce is the only provider that has had issues since this has been live for the past 10 days. @TBonnin, @ErickRDev wdyt?

Redirects Total %
dayforce 362,079 82.1%
confluence 41,750 9.5%
github-app 13,084 3.0%
sharepoint-online 6,915 1.6%
salesforce 5,756 1.3%
avoma 5,370 1.2%
unauthenticated 1,296 0.3%
crowdstrike 1,005 0.2%
microsoft-tenant-specific 805 0.2%
all others (38 providers) 2,921 0.7%

Comment thread packages/shared/lib/services/proxy/byte-metering-transport.ts Outdated
Comment thread packages/shared/lib/services/proxy/byte-metering-transport.unit.test.ts Outdated
@ErickRDev

Copy link
Copy Markdown
Contributor

I’m tempted to remove the metric, and set forward headers to false for all providers except Dayforce with this pr, now that we have a clearer picture of the redirects, and dayforce is the only provider that has had issues since this has been live for the past 10 days. @TBonnin, @ErickRDev wdyt?

Redirects Total %
dayforce 362,079 82.1%
confluence 41,750 9.5%
github-app 13,084 3.0%
sharepoint-online 6,915 1.6%
salesforce 5,756 1.3%
avoma 5,370 1.2%
unauthenticated 1,296 0.3%
crowdstrike 1,005 0.2%
microsoft-tenant-specific 805 0.2%
all others (38 providers) 2,921 0.7%

Hmm, I guess it comes down to what the correct thing (product-wise) is: should we or should we not forward headers on redirects? If so, I'd say keep it.

@hassan254-prog

Copy link
Copy Markdown
Contributor Author

I’m tempted to remove the metric, and set forward headers to false for all providers except Dayforce with this pr, now that we have a clearer picture of the redirects, and dayforce is the only provider that has had issues since this has been live for the past 10 days. @TBonnin, @ErickRDev wdyt?
Redirects Total %
dayforce 362,079 82.1%
confluence 41,750 9.5%
github-app 13,084 3.0%
sharepoint-online 6,915 1.6%
salesforce 5,756 1.3%
avoma 5,370 1.2%
unauthenticated 1,296 0.3%
crowdstrike 1,005 0.2%
microsoft-tenant-specific 805 0.2%
all others (38 providers) 2,921 0.7%

Hmm, I guess it comes down to what the correct thing (product-wise) is: should we or should we not forward headers on redirects? If so, I'd say keep it.

This was originally added for Dayforce only. Since its not good to forward headers with redirects we added a metric to count the number of providers that need to use this so that when we flip the switch we can only define them in the providers config as forward_headers_on_redirect, in this case dayforce is the only one that needs headers for redirect, based on the numbers and also what has failed so far, so its safe it remove the metric as we default it to back to false.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

2 issues found across 3 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/server/lib/controllers/proxy/allProxy.ts">

<violation number="1" location="packages/server/lib/controllers/proxy/allProxy.ts:214">
P1: This drops the previous default for redirect header forwarding. When callers omit the header, existing proxy requests now fall back to `false`, so auth/custom headers stop being restored after redirects unless the provider config explicitly opts in.</violation>
</file>

Tip: Review your code locally with the cubic CLI to iterate faster.

Fix all with cubic | Re-trigger cubic

Comment thread packages/shared/lib/services/proxy/utils.ts
Comment thread packages/server/lib/controllers/proxy/allProxy.ts
@hassan254-prog hassan254-prog added this pull request to the merge queue May 29, 2026
Merged via the queue into master with commit 34e7171 May 29, 2026
28 checks passed
@hassan254-prog hassan254-prog deleted the wari/fix-proxy-forward-headers branch May 29, 2026 07:58
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