Skip to content

Cognito logout fails with "Bad authority" behind reverse proxy#1888

Open
Respirayson wants to merge 1 commit into
kafbat:mainfrom
Respirayson:fix/rfcuriparser-negative-port
Open

Cognito logout fails with "Bad authority" behind reverse proxy#1888
Respirayson wants to merge 1 commit into
kafbat:mainfrom
Respirayson:fix/rfcuriparser-negative-port

Conversation

@Respirayson

@Respirayson Respirayson commented Jun 21, 2026

Copy link
Copy Markdown

Issue: #1878

  • Breaking change? (if so, please describe the impact and migration path for existing application instances)

What changes did you make? (Give an overview)

What

Fixes a 500 error on logout (InvalidUrlException: Bad authority) when kafbat-ui runs behind a reverse proxy with SSL termination (e.g. AWS ALB) and is configured with Cognito OAuth2 auth.

Why

CognitoLogoutSuccessHandler previously rebuilt the base redirect URL via UrlUtils.buildFullRequestUrl(scheme, host, port, ...), manually passing the request's port. When no explicit port is present on the incoming request (common behind a proxy), URI.getPort() returns -1, and the old helper always appended :port regardless which produces https://host:-1/. Spring Framework 6.2's RfcUriParser strictly rejects this as an invalid authority.

Fix

Replaced the manual URL reconstruction with UriComponentsBuilder.fromUri(requestUri), which copies the URI's fields directly and correctly omits the port segment entirely when it's -1, instead of serializing it as :-1.

Testing

Added CognitoLogoutSuccessHandlerTest covering:

  • A request with no explicit port (simulating behind a reverse proxy): verifies no port is appended to the logout_uri.
  • A request with an explicit port: verifies the port is still correctly preserved.

Is there anything you'd like reviewers to focus on?

How Has This Been Tested? (put an "x" (case-sensitive!) next to an item)

  • No need to
  • Manually (please, describe, if necessary)
  • Unit checks
  • Integration checks
  • Covered by existing automation

Checklist (put an "x" (case-sensitive!) next to all the items, otherwise the build will fail)

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (e.g. ENVIRONMENT VARIABLES)
  • My changes generate no new warnings (e.g. Sonar is happy)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged

Check out Contributing and Code of Conduct

A picture of a cute animal (not mandatory but encouraged)
image

Summary by CodeRabbit

  • Bug Fixes

    • Improved Cognito logout redirect handling to properly support proxy configurations.
  • Tests

    • Added test coverage for logout URL construction in proxy and explicit port scenarios.

@Respirayson Respirayson requested a review from a team as a code owner June 21, 2026 14:43
@kapybro kapybro Bot added status/triage/manual Manual triage in progress and removed status/triage/manual Manual triage in progress labels Jun 21, 2026
@kapybro

kapybro Bot commented Jun 21, 2026

Copy link
Copy Markdown

AI Summary

The issue describes a Cognito logout failure with a "Bad authority" error when the application runs behind a reverse proxy with SSL termination. The problem arises because the old URL reconstruction logic incorrectly appended a port (:-1) when none was specified, which Spring Framework 6.2's RfcUriParser now rejects. The fix replaces the manual URL construction with UriComponentsBuilder, which correctly omits the port when it's -1, ensuring valid logout URIs. Unit tests were added to verify the fix works for both requests with and without explicit ports.

@kapybro kapybro Bot changed the title fix: Cognito logout fails with "Bad authority" behind reverse proxy Cognito logout fails with "Bad authority" behind reverse proxy Jun 21, 2026
@kapybro kapybro Bot added area/auth App authentication related issues impact/changelog A PR with changes which should be addressed in the changelog explicitly impact/documentation A PR with changes which should be addressed in the documentation scope/backend Related to backend changes type/bug Something isn't working labels Jun 21, 2026
@coderabbitai

coderabbitai Bot commented Jun 21, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f2b8a96d-9df0-4124-a93a-a8dcc781dff0

📥 Commits

Reviewing files that changed from the base of the PR and between e78f6a6 and 77d4832.

📒 Files selected for processing (2)
  • api/src/main/java/io/kafbat/ui/config/auth/logout/CognitoLogoutSuccessHandler.java
  • api/src/test/java/io/kafbat/ui/config/auth/logout/CognitoLogoutSuccessHandlerTest.java

📝 Walkthrough

Walkthrough

CognitoLogoutSuccessHandler now builds baseUrl via UriComponentsBuilder.fromUri(requestUri) instead of UrlUtils.buildFullRequestUrl(...), removing the related import. A new test class covers both a proxy/implicit-port scenario and an explicit-port scenario, asserting the correct 302 redirect and Location header.

Changes

Cognito Logout URL Fix

Layer / File(s) Summary
baseUrl derivation fix and tests
api/src/main/java/.../logout/CognitoLogoutSuccessHandler.java, api/src/test/java/.../logout/CognitoLogoutSuccessHandlerTest.java
Removes UrlUtils import and replaces UrlUtils.buildFullRequestUrl(...) + fromUriString(fullUrl) with UriComponentsBuilder.fromUri(requestUri). Adds 70-line test class with two cases: proxy host with implicit port and localhost with explicit port 8080, both asserting 302 FOUND and a correctly formed Cognito logout_uri.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related issues

Poem

🐇 Hopping past the proxy wall,
No "Bad authority" shall fall,
fromUri leaps where UrlUtils once tread,
The Location header points straight ahead,
Two tests now guard the redirect's way —
This bunny's logout works today! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 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 (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and accurately describes the main issue being fixed: Cognito logout failing with a 'Bad authority' error when behind a reverse proxy.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hi Respirayson! 👋

Welcome, and thank you for opening your first PR in the repo!

Please wait for triaging by our maintainers.

Please take a look at our contributing guide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth App authentication related issues impact/changelog A PR with changes which should be addressed in the changelog explicitly impact/documentation A PR with changes which should be addressed in the documentation scope/backend Related to backend changes type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant