Revert "Get rid of the redundant multiuser module (#691)" #806

vinokurig · 2025-05-09T12:55:02Z

This reverts commit 147df19.

What does this PR do?

revert the #691 pull request, see eclipse-che/che#22710

Screenshot/screencast of this PR

What issues does this PR fix or reference?

eclipse-che/che#22710

How to test this PR?

PR Checklist

As the author of this Pull Request I made sure that:

Release Notes

Reviewers

Reviewers, please comment how you tested the PR when approving it.

...er/src/main/java/org/eclipse/che/multiuser/keycloak/server/KeycloakConfigurationService.java

+  @Path("/oidcCallbackIde.html")
+  @Produces("text/html")
+  public String ideCallback() throws IOException {
+    return getKeycloakResource("oidcCallbackIde.html");


To fix the issue, the content of the file read by getKeycloakResource should be sanitized or escaped before being returned in the HTTP response. Since the content is served as HTML, it is appropriate to use an HTML escaping library to ensure that any potentially malicious content is neutralized. The StringEscapeUtils.escapeHtml4 method from Apache Commons Text is a reliable choice for this purpose.

The fix involves:

Adding the Apache Commons Text library to the project dependencies (if not already present).

Escaping the content returned by getKeycloakResource in the ideCallback method before returning it.

...er/src/main/java/org/eclipse/che/multiuser/keycloak/server/KeycloakConfigurationService.java

+  @Path("/oidcCallbackDashboard.html")
+  @Produces("text/html")
+  public String dashboardCallback() throws IOException {
+    return getKeycloakResource("oidcCallbackDashboard.html");


To fix the issue, the content of the file should be sanitized or encoded before being returned in the HTTP response. Since the file is served as HTML, we can use an HTML encoding library to ensure that any potentially malicious content is safely escaped. This will prevent the execution of any embedded scripts or malicious HTML.

The best way to implement this fix is to use a well-known library like Apache Commons Text's StringEscapeUtils to encode the content as HTML. This ensures that any special characters in the file content are properly escaped.

...ak-server/src/main/java/org/eclipse/che/multiuser/keycloak/server/KeycloakServiceClient.java

+      }
+      url = ub.build().toString();
+    }
+    final HttpURLConnection conn = (HttpURLConnection) new URL(url).openConnection();


To fix the SSRF vulnerability, we need to validate the oauthProvider parameter against a predefined list of allowed values. This ensures that only trusted and expected values are used to construct the URL. The validation should occur as early as possible, ideally in the getIdentityProviderToken method of KeycloakServiceClient.java.

Steps to implement the fix:

Define a list of allowed oauthProvider values (e.g., in a configuration file or as a constant in the class).

Validate the oauthProvider parameter against this list before using it to construct the URL.

If the validation fails, throw an appropriate exception (e.g., BadRequestException).

...ak-server/src/main/java/org/eclipse/che/multiuser/keycloak/server/KeycloakServiceClient.java

+          // Unspecific errors always returned from Keycloak as 502 + HTML error page.
+          // So try to handle that case separately
+          if (responseCode == 502) {
+            Matcher matcher = errorPageMessagePattern.matcher(read);


...server/src/main/java/org/eclipse/che/multiuser/keycloak/server/oauth2/DelegatedOAuthAPI.java

+    }
+    String accountLinkUrl =
+        keycloakServiceClient.getAccountLinkingURL(jwtToken, oauthProvider, redirectAfterLogin);
+    return Response.temporaryRedirect(URI.create(accountLinkUrl)).build();


To fix the issue, we need to validate the accountLinkUrl before using it in the redirection. This can be achieved by ensuring that the URL is either relative or points to a trusted domain. Alternatively, we can maintain a whitelist of valid redirect URLs and check the constructed URL against this list.

The best approach here is to validate the accountLinkUrl to ensure it points to a trusted domain. If the URL is invalid or does not match the trusted domain, we should redirect the user to a safe fallback page (e.g., an error page).

...c/main/java/org/eclipse/che/multiuser/keycloak/token/provider/contoller/TokenController.java

+      validator.validate(keycloakToken);
+      token = tokenProvider.obtainGitHubToken(keycloakToken);
+    } catch (KeycloakException e) {
+      return Response.status(Response.Status.BAD_REQUEST).entity(e.getMessage()).build();


...c/main/java/org/eclipse/che/multiuser/keycloak/token/provider/contoller/TokenController.java

+    } catch (KeycloakException e) {
+      return Response.status(Response.Status.BAD_REQUEST).entity(e.getMessage()).build();
+    }
+    return Response.ok(token).build();


To fix the issue, the token value should be properly sanitized or encoded before being included in the HTTP response. Since the Response.ok() method is used to send the token back to the client, we should ensure that the token is safe for inclusion in the response. The best approach is to use contextual output encoding, such as JSON encoding, to ensure that the token is treated as a plain string and not executable code.

In this case, we will JSON-encode the token value before including it in the response. This ensures that any special characters in the token are escaped, preventing them from being interpreted as executable code.

...c/main/java/org/eclipse/che/multiuser/keycloak/token/provider/contoller/TokenController.java

+      validator.validate(keycloakToken);
+      token = tokenProvider.obtainOsoToken(keycloakToken);
+    } catch (KeycloakException e) {
+      return Response.status(Response.Status.BAD_REQUEST).entity(e.getMessage()).build();


...g/eclipse/che/multiuser/machine/authentication/server/signature/SignatureKeyManagerTest.java

+  @BeforeMethod
+  public void createEntities() throws Exception {
+    kpg = KeyPairGenerator.getInstance(ALGORITHM);
+    kpg.initialize(KEY_SIZE);


To fix the issue, the KEY_SIZE constant should be updated to use a secure key size of at least 2048 bits for RSA encryption. This ensures compliance with modern cryptographic standards and mitigates the risk of brute force attacks. The change will involve updating the value of KEY_SIZE and ensuring that all references to it use the updated value.

.../org/eclipse/che/multiuser/permission/workspace/server/spi/jpa/MultiuserJpaWorkspaceDao.java

+              .createQuery(findByWorkerQuery, WorkspaceImpl.class)
+              .setParameter("userId", userId)
+              .setMaxResults(maxItems)
+              .setFirstResult((int) skipCount)


This reverts commit 147df19.

openshift-ci · 2025-05-09T13:17:01Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: svor, tolusha, vinokurig

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vinokurig requested review from SDawley, ibuziuk and tolusha as code owners May 9, 2025 12:55

github-advanced-security bot found potential problems May 9, 2025

View reviewed changes

svor approved these changes May 9, 2025

View reviewed changes

openshift-ci bot assigned svor May 9, 2025

openshift-ci bot added the lgtm label May 9, 2025

Revert "Get rid of the redundant multiuser module (#691)"

d151b67

This reverts commit 147df19.

vinokurig force-pushed the revertCleanUp branch from 0c190fb to d151b67 Compare May 9, 2025 13:13

openshift-ci bot removed the lgtm label May 9, 2025

tolusha approved these changes May 9, 2025

View reviewed changes

openshift-ci bot assigned tolusha May 9, 2025

openshift-ci bot added the lgtm label May 9, 2025

vinokurig merged commit 744041e into main May 9, 2025
27 of 28 checks passed

vinokurig deleted the revertCleanUp branch May 9, 2025 15:02

vinokurig mentioned this pull request Jun 13, 2025

Che-Server pod crash is detected in the dogfooding instance eclipse-che/che#23436

Closed

@@ -24,2 +24,3 @@
             import java.util.Map;
+            import org.apache.commons.text.StringEscapeUtils;
             import javax.inject.Inject;
@@ -88,3 +89,3 @@
               public String ideCallback() throws IOException {
-                return getKeycloakResource("oidcCallbackIde.html");
+                return org.apache.commons.text.StringEscapeUtils.escapeHtml4(getKeycloakResource("oidcCallbackIde.html"));
               }

@@ -29,2 +29,7 @@
                     <dependency>
+                        <groupId>org.apache.commons</groupId>
+                        <artifactId>commons-text</artifactId>
+                        <version>1.13.1</version>
+                    </dependency>
+                    <dependency>
                         <groupId>com.auth0</groupId>

Package	Version	Security advisories
org.apache.commons:commons-text (maven)	1.13.1	None

@@ -22,2 +22,3 @@
             import java.net.URL;
+            import org.apache.commons.text.StringEscapeUtils;
             import java.net.URLConnection;
@@ -95,3 +96,3 @@
               public String dashboardCallback() throws IOException {
-                return getKeycloakResource("oidcCallbackDashboard.html");
+                return org.apache.commons.text.StringEscapeUtils.escapeHtml4(getKeycloakResource("oidcCallbackDashboard.html"));
               }

@@ -29,2 +29,7 @@
                     <dependency>
+                        <groupId>org.apache.commons</groupId>
+                        <artifactId>commons-text</artifactId>
+                        <version>1.13.1</version>
+                    </dependency>
+                    <dependency>
                         <groupId>com.auth0</groupId>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Revert "Get rid of the redundant multiuser module (#691)" #806

Revert "Get rid of the redundant multiuser module (#691)" #806

Uh oh!

vinokurig commented May 9, 2025

Uh oh!

Check warning

Copilot Autofix

Check warning

Copilot Autofix

Check failure

Copilot Autofix

Check failure

Check warning

Copilot Autofix

Check warning

Copilot Autofix

Check warning

Copilot Autofix

Check warning

Copilot Autofix

Check failure

Copilot Autofix

Check failure

openshift-ci bot commented May 9, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

@@ -139,2 +139,5 @@
                       ServerException, UnauthorizedException {
+                if (!isValidOAuthProvider(oauthProvider)) {
+                  throw new BadRequestException("Invalid OAuth provider: " + oauthProvider);
+                }
                 String url =
@@ -158,2 +161,8 @@
               }
+              private boolean isValidOAuthProvider(String oauthProvider) {
+                // Define a list of allowed OAuth providers
+                List<String> allowedProviders = Arrays.asList("google", "github", "facebook");
+                return allowedProviders.contains(oauthProvider);
+              }

@@ -65,3 +65,16 @@
                     keycloakServiceClient.getAccountLinkingURL(jwtToken, oauthProvider, redirectAfterLogin);
-                return Response.temporaryRedirect(URI.create(accountLinkUrl)).build();
+                try {
+                    URI accountLinkUri = URI.create(accountLinkUrl);
+                    // Validate that the URL is either relative or points to a trusted domain
+                    if (!accountLinkUri.isAbsolute() || "trusted-domain.com".equals(accountLinkUri.getHost())) {
+                        return Response.temporaryRedirect(accountLinkUri).build();
+                    } else {
+                        // Redirect to a safe fallback page if validation fails
+                        return Response.temporaryRedirect(URI.create("/error.html")).build();
+                    }
+                } catch (IllegalArgumentException e) {
+                    // Handle invalid URI syntax
+                    return Response.temporaryRedirect(URI.create("/error.html")).build();
+                }
               }

@@ -89,3 +89,3 @@
                 }
-                return Response.ok(token).build();
+                return Response.ok("{\"token\":\"" + token.replace("\"", "\\\"") + "\"}").build();
               }

@@ -49,3 +49,3 @@
-              private static final int KEY_SIZE = 512;
+              private static final int KEY_SIZE = 2048;
               private static final String ALGORITHM = "RSA";

Revert "Get rid of the redundant multiuser module (#691)" #806

Revert "Get rid of the redundant multiuser module (#691)" #806

Uh oh!

Conversation

vinokurig commented May 9, 2025

What does this PR do?

Screenshot/screencast of this PR

What issues does this PR fix or reference?

How to test this PR?

PR Checklist

Release Notes

Reviewers

Uh oh!

Check warning

Uh oh!

Copilot Autofix

Check warning

Uh oh!

Copilot Autofix

Check failure

Uh oh!

Copilot Autofix

Check failure

Uh oh!

Uh oh!

Check warning

Uh oh!

Uh oh!

Copilot Autofix

Check warning

Uh oh!

Copilot Autofix

Check warning

Uh oh!

Copilot Autofix

Check warning

Uh oh!

Copilot Autofix

Check failure

Uh oh!

Copilot Autofix

Check failure

Uh oh!

openshift-ci bot commented May 9, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants