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

Bugfix: CORS config on error handling #555

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

hadisfr
Copy link
Contributor

@hadisfr hadisfr commented Sep 21, 2024

  • Breaking change?

What changes did you make? (Give an overview)

diff --cc api/src/main/java/io/kafbat/ui/config/CorsGlobalConfiguration.java
--- a/api/src/main/java/io/kafbat/ui/config/CorsGlobalConfiguration.java
+++ b/api/src/main/java/io/kafbat/ui/config/CorsGlobalConfiguration.java
@@@ -22,7 -22,7 +22,8 @@@ public class CorsGlobalConfiguration 
  
        final ServerHttpResponse response = ctx.getResponse();
        final HttpHeaders headers = response.getHeaders();
--      headers.add("Access-Control-Allow-Origin", "*");
++      headers.add("Access-Control-Allow-Origin", request.getHeaders().getOrigin());
++      headers.add("Access-Control-Allow-Credentials", "true");
        headers.add("Access-Control-Allow-Methods", "GET, PUT, POST, DELETE, OPTIONS");
        headers.add("Access-Control-Max-Age", "3600");
        headers.add("Access-Control-Allow-Headers", "Content-Type");
  • Furthermore, if you call an API which causes any Error (happy path example: calling writable APIs on readonly mode clusters), the error handler (GlobalErrorWebExceptionHandler) baypasses CORS configs in CorsGlobalConfiguration, causing "Something went wrong: An error occurred" instead of "405 Method Not Allowed: This cluster is in read-only mode." in frontend. It seems that we should manually fill response headers in error handler, too.

I splitted changes into multiple commits to make tracking chnages easier.

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

Spring web CORS configurations

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 -> automatic testing spring configurations is too hard!
  • 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

@hadisfr hadisfr requested a review from a team as a code owner September 21, 2024 12:37
@kapybro kapybro bot added status/triage Issues pending maintainers triage status/triage/manual Manual triage in progress status/triage/completed Automatic triage completed and removed status/triage Issues pending maintainers triage labels Sep 21, 2024
hadisfr added a commit to hadisfr/kafka-ui that referenced this pull request Sep 21, 2024
hadisfr added a commit to hadisfr/kafka-ui that referenced this pull request Sep 21, 2024
hadisfr added a commit to hadisfr/kafka-ui that referenced this pull request Sep 21, 2024
* add Access-Control-Allow-Credentials=true
* use real request origin instead of '*' to fill Access-Control-Allow-Origin, due to high security standards of modern browsers
@hadisfr hadisfr force-pushed the bugfix/cors-config-on-error-handling branch from c01ed86 to 615e043 Compare September 21, 2024 12:38
@hadisfr hadisfr changed the title Bugfix CORS config on error handling Bugfix: CORS config on error handling Sep 21, 2024
@Haarolean
Copy link
Member

Hi, which issues are you experiencing besides seeing an error in your dev console? What are we trying to fix here exactly?

@hadisfr
Copy link
Contributor Author

hadisfr commented Sep 24, 2024

Hi, which issues are you experiencing besides seeing an error in your dev console? What are we trying to fix here exactly?

Hi!

Without the first change I described (Access-Control-Allow-Credentials header and explicitly filling Access-Control-Allow-Origin) only a white screen was visible in react dev mode (port 3000), providing features like hot auto-reload triggered by save. Because spring server takes the port 8080, I could not run react dev mode on the same port (without using perhaps something like nginx).

After fixing that, the second change (in GlobalErrorWebExceptionHandler) was needed to show the right messages in the red warning box at the bottom-right part of the page, not just in dev console. I can provide some screenshots tomorrow if needed.

By the way, all the changes in this PR aim to facilitate running the product in dev mode and will not provide any improvement in the production mode.

@Haarolean
Copy link
Member

only a white screen

That's weird, as this works for me and the other folks, given you're not running the backend with SSL enabled.

I don't mind these changes for dev purposes, but I previously considered allowing the end user to configure cors for production via properties rather than just disabling it completely.

* add Access-Control-Allow-Credentials=true
* use real request origin instead of '*' to fill Access-Control-Allow-Origin, due to high security standards of modern browsers
@hadisfr hadisfr force-pushed the bugfix/cors-config-on-error-handling branch from 615e043 to aebf6b2 Compare September 28, 2024 15:27
@hadisfr
Copy link
Contributor Author

hadisfr commented Sep 28, 2024

That's weird, as this works for me and the other folks, given you're not running the backend with SSL enabled.

I don't mind these changes for dev purposes, but I previously considered allowing the end user to configure cors for production via properties rather than just disabling it completely.

This is my procedure which causes the problem on main branch:
I run the front-end in dev mode via npm run dev after the following change:

diff --git a/frontend/src/lib/constants.ts b/frontend/src/lib/constants.ts
index 3e3925ec..33f49c7a 100644
--- a/frontend/src/lib/constants.ts
+++ b/frontend/src/lib/constants.ts
@@ -7,7 +7,7 @@ declare global {
 }
 
 export const BASE_PARAMS: ConfigurationParameters = {
-  basePath: window.basePath || '',
+  basePath: window.basePath || 'http://localhost:8080',
   credentials: 'include',
   headers: {
     'Content-Type': 'application/json',

I see only a white screen in my Firefox 130.0.1 on Ubuntu 20.04.6 LTS. I get the same result using Chrome. The network inspector claims CORS problems:
image

Maybe I should run the React dev mode via another way or something? Or maybe I should do something else other than modifying constants.ts?

P.S. I rebased the branch with main.

@Haarolean Haarolean added the hacktoberfest-accepted PRs accepted towards hacktoberfest goal and will be counted as approved label Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted PRs accepted towards hacktoberfest goal and will be counted as approved status/triage/completed Automatic triage completed status/triage/manual Manual triage in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants