Skip to content

fix: return 307 redirects for envoy proxy instead of 401#782

Merged
steveiliop56 merged 2 commits intomainfrom
fix/envoy-browser-detection
Apr 10, 2026
Merged

fix: return 307 redirects for envoy proxy instead of 401#782
steveiliop56 merged 2 commits intomainfrom
fix/envoy-browser-detection

Conversation

@steveiliop56
Copy link
Copy Markdown
Member

@steveiliop56 steveiliop56 commented Apr 10, 2026

Summary by CodeRabbit

  • Bug Fixes
    • Refined authentication behavior per proxy: Nginx kept as non-browser-only; Envoy now follows browser detection so browsers receive redirects while API clients get appropriate error codes.
  • Tests
    • Expanded test coverage to validate per-proxy browser vs non-browser handling and expected redirect or error responses.

@dosubot dosubot Bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Apr 10, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 10, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: cef0225e-983c-4018-a4b6-31d3cbf27d0b

📥 Commits

Reviewing files that changed from the base of the PR and between 8137443 and 90e0936.

📒 Files selected for processing (1)
  • internal/controller/proxy_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/controller/proxy_controller_test.go

📝 Walkthrough

Walkthrough

The proxy controller's useBrowserResponse method now treats only Nginx as requiring a non-browser response; Envoy is no longer treated as an exception. Corresponding tests were updated to reflect Envoy's browser-style redirect behavior and adjusted detection/fallback assertions.

Changes

Cohort / File(s) Summary
Proxy Controller Logic
internal/controller/proxy_controller.go
Changed useBrowserResponse to return false only for Nginx proxy type; removed Envoy exception and updated inline comments.
Proxy Controller Tests
internal/controller/proxy_controller_test.go
Updated tests to distinguish Nginx vs Envoy behavior, added User-Agent: browserUserAgent in detection requests, changed Envoy fallback expectations from 401 to 307 with redirect Location checks, and renamed/adjusted related test cases.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Poem

🐰 I nibble code at break of day,
Nginx sits still, Envoy hops away,
Redirects twinkle, tests align,
A tiny patch, a tidy sign,
Hooray for hops and passing CI!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: Envoy proxy now returns 307 redirects instead of 401, which is the primary behavior modification evident in both the controller and test changes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/envoy-browser-detection

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 10, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 19.97%. Comparing base (061d28f) to head (90e0936).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #782      +/-   ##
==========================================
+ Coverage   19.82%   19.97%   +0.15%     
==========================================
  Files          50       50              
  Lines        3960     3960              
==========================================
+ Hits          785      791       +6     
+ Misses       3104     3100       -4     
+ Partials       71       69       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@steveiliop56 steveiliop56 merged commit b44dc75 into main Apr 10, 2026
8 checks passed
@Rycochet Rycochet deleted the fix/envoy-browser-detection branch April 26, 2026 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants