Allow only normalized URLs in requests #43765

ahus1 · 2025-10-28T11:07:55Z

Closes #43763

mabartos

@ahus1 Nice work! Just added 2 comments for improvement.

quarkus/deployment/src/main/java/org/keycloak/quarkus/deployment/KeycloakProcessor.java

...ntime/src/main/java/org/keycloak/quarkus/runtime/services/RejectNonNormalizedPathFilter.java

mabartos

Last comments, otherwise LGTM.

mabartos · 2025-10-29T13:21:11Z

quarkus/config-api/src/main/java/org/keycloak/config/HttpOptions.java

+            .description("If the server should accept paths that are not normalized according to RFC3986 or that contain a double slash ('//'). While accepting those requests might be relevant for legacy applications, it is recommended to disable it to allow for more concise URL filtering.")
+            .buildTime(true)
+            .deprecated()


Suggested change

.description("If the server should accept paths that are not normalized according to RFC3986 or that contain a double slash ('//'). While accepting those requests might be relevant for legacy applications, it is recommended to disable it to allow for more concise URL filtering.")

.buildTime(true)

.deprecated()

.description("If the server should accept paths that are not normalized according to RFC3986 or that contain a double slash ('//'). While accepting those requests might be relevant for legacy applications, it is recommended to disable it to allow for more concise URL filtering.")

.deprecated()

It might not be necessary to have it as a build-time option.

I'm adding the filter to a build time option ... so I'd assume it is build time? Or do you suggest to always add the filter, and then parameterize it?

@ahus1 These filters are actually added during the static init phase (Record(ExecutionTime.STATIC_INIT)) and not created during the augmentation.

It's always better to have an option configurable at runtime if it's only some marker of behavior checked in runtime anyway.

If you remove the buildTime(true) here and add the @Consume(ConfigBuildItem.class) as described in the additional comments, it should be fine :)

I gave it a try, and then the test did no longer pass. I reverted the change.

quarkus/deployment/src/main/java/org/keycloak/quarkus/deployment/KeycloakProcessor.java

...ntime/src/main/java/org/keycloak/quarkus/runtime/services/RejectNonNormalizedPathFilter.java

quarkus/deployment/src/main/java/org/keycloak/quarkus/deployment/KeycloakProcessor.java

mabartos · 2025-10-29T14:48:55Z

quarkus/config-api/src/main/java/org/keycloak/config/HttpOptions.java

+            .description("If the server should accept paths that are not normalized according to RFC3986 or that contain a double slash ('//'). While accepting those requests might be relevant for legacy applications, it is recommended to disable it to allow for more concise URL filtering.")
+            .buildTime(true)
+            .deprecated()


@ahus1 These filters are actually added during the static init phase (Record(ExecutionTime.STATIC_INIT)) and not created during the augmentation.

It's always better to have an option configurable at runtime if it's only some marker of behavior checked in runtime anyway.

If you remove the buildTime(true) here and add the @Consume(ConfigBuildItem.class) as described in the additional comments, it should be fine :)

vmuzikar

@ahus1 Thank you for the PR.

I'm not sure if I'm doing something wrong (probably yes as the tests pass) but if I do something as simple as curl -v http://localhost:8080/realms/xxx/../master/, I get 200 response instead of 400.

vmuzikar · 2025-10-29T16:26:25Z

quarkus/config-api/src/main/java/org/keycloak/config/HttpOptions.java

                    "Specify a list of comma-separated values defined in milliseconds. Example with buckets from 5ms to 10s: 5,10,25,50,250,500,1000,2500,5000,10000")
            .build();
+
+    public static final Option<Boolean> HTTP_ACCEPT_NON_NORMALIZED_PATHS = new OptionBuilder<>("http-accept-non-normalized-paths", Boolean.class)


Do we really want to make it configurable? It's a breaking change but to fix a security issue. What would be a real and valid use case for the non-normalized paths?

Sorry if this has been already discussed somewhere.

What would be a real and valid use case for the non-normalized paths?

I would say adding .. to the path is not a valid use case, maybe someone used it as a workaround for some issue in a machine-to-machine communication.

The most common thing I could think of is a double // in the URL, where someone might have concatenated URLs, and they end up with two //. We would break those setups. And to back out of such a setup, there is this option to revert it. And it is deprecated, so we can remove it again soon - like in 27.0

I'd say if someone uses // it's probably an oversight and a misconfiguration. So having it configurable is more like a convenience feature. But it's not a blocker for me, let's keep it there.

shawkins · 2025-10-29T16:50:05Z

@ahus1 Thank you for the PR.

I'm not sure if I'm doing something wrong (probably yes as the tests pass) but if I do something as simple as curl -v http://localhost:8080/realms/xxx/../master/, I get 200 response instead of 400.

curl needs --path-as-is otherwise it will do the normalization.

vmuzikar · 2025-10-29T16:54:01Z

curl needs --path-as-is otherwise it will do the normalization.

Thanks, sorry for the noise. What confused me is that even browser normalized the path (did not get 400).

vmuzikar

LGTM, thank you for the fix.

@mabartos Are you ok with the changes as is?

mabartos · 2025-10-30T11:26:34Z

@ahus1 We should ensure the build chain execution order to prevent any issues + would be good to have the property runtime.

Sent a PR to your fork: ahus1#434

Closes keycloak#43763 Co-authored-by: Martin Bartoš <[email protected]> Signed-off-by: Martin Bartoš <[email protected]> Signed-off-by: Alexander Schwartz <[email protected]>

mabartos

@ahus1 LGTM, thanks!

ahus1 · 2025-10-30T11:38:39Z

@mabartos - thank you for the PR. I merge it to this branch, rebased it, and fixed the help test. Let's wait for the build to be green.

pohjanoksajukka · 2025-11-04T13:19:26Z

@ahus1 @mabartos, is this the fix for CVE-2025-10939?

ahus1 · 2025-11-04T15:19:39Z

is this the fix for CVE-2025-10939?

Yes.

Closes keycloak#43763 Signed-off-by: Martin Bartoš <[email protected]> Signed-off-by: Alexander Schwartz <[email protected]> Co-authored-by: Martin Bartoš <[email protected]>

ahus1 self-assigned this Oct 28, 2025

ahus1 force-pushed the is-43763-only-allow-normalized-urls branch from a96522d to 6ffc54f Compare October 28, 2025 13:42

keycloak-github-bot bot added the flaky-test label Oct 28, 2025

keycloak-github-bot bot mentioned this pull request Oct 28, 2025

Flaky test: org.keycloak.testsuite.federation.ldap.LDAPProvidersIntegrationTest#updateLDAPUsernameTest #43754

Closed

ahus1 force-pushed the is-43763-only-allow-normalized-urls branch from 6ffc54f to fb72910 Compare October 28, 2025 15:06

ahus1 marked this pull request as ready for review October 28, 2025 16:33

ahus1 requested review from a team as code owners October 28, 2025 16:33

keycloak-github-bot bot added the team/cloud-native label Oct 28, 2025

mabartos reviewed Oct 29, 2025

View reviewed changes

quarkus/deployment/src/main/java/org/keycloak/quarkus/deployment/KeycloakProcessor.java Outdated Show resolved Hide resolved

...ntime/src/main/java/org/keycloak/quarkus/runtime/services/RejectNonNormalizedPathFilter.java Show resolved Hide resolved

mabartos reviewed Oct 29, 2025

View reviewed changes

ahus1 force-pushed the is-43763-only-allow-normalized-urls branch from 57a9e46 to ed95fc3 Compare October 29, 2025 15:23

vmuzikar reviewed Oct 29, 2025

View reviewed changes

ahus1 force-pushed the is-43763-only-allow-normalized-urls branch from ed95fc3 to b9d2ae0 Compare October 29, 2025 20:41

vmuzikar previously approved these changes Oct 30, 2025

View reviewed changes

ahus1 dismissed vmuzikar’s stale review via 0bf4554 October 30, 2025 11:32

Allow only normalized paths in requests

9274d24

Closes keycloak#43763 Co-authored-by: Martin Bartoš <[email protected]> Signed-off-by: Martin Bartoš <[email protected]> Signed-off-by: Alexander Schwartz <[email protected]>

ahus1 force-pushed the is-43763-only-allow-normalized-urls branch from f25b740 to 9274d24 Compare October 30, 2025 11:37

ahus1 requested a review from mabartos October 30, 2025 11:37

mabartos approved these changes Oct 30, 2025

View reviewed changes

vmuzikar approved these changes Oct 30, 2025

View reviewed changes

vmuzikar merged commit 0f01444 into keycloak:main Oct 30, 2025
83 checks passed

bahaa mentioned this pull request Nov 9, 2025

Allow only normalized URLs in requests caused a regression in view authz permission details in Admin Consol #44056

Closed

2 tasks

This was referenced Nov 17, 2025

Admin Client creates malformed paths for requests #44269

Closed

Tests are failing due to malformed URLs keycloak/keycloak-js#217

Closed

ahus1 mentioned this pull request Nov 17, 2025

Keycloak throws an error "Missing Normalization" when attaching policies to permissions #44267

Open

2 tasks

Allow only normalized URLs in requests #43765

Allow only normalized URLs in requests #43765

Uh oh!

Conversation

ahus1 commented Oct 28, 2025

Uh oh!

mabartos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mabartos left a comment

Choose a reason for hiding this comment

Uh oh!

mabartos Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahus1 Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

mabartos Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahus1 Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mabartos Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vmuzikar left a comment

Choose a reason for hiding this comment

Uh oh!

vmuzikar Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

ahus1 Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

vmuzikar Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

shawkins commented Oct 29, 2025

Uh oh!

vmuzikar commented Oct 29, 2025

Uh oh!

vmuzikar left a comment

Choose a reason for hiding this comment

Uh oh!

mabartos commented Oct 30, 2025

Uh oh!

mabartos left a comment

Choose a reason for hiding this comment

Uh oh!

ahus1 commented Oct 30, 2025

Uh oh!

Uh oh!

pohjanoksajukka commented Nov 4, 2025

Uh oh!

ahus1 commented Nov 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mabartos Oct 29, 2025 •

edited

Loading

mabartos Oct 29, 2025 •

edited

Loading

mabartos Oct 29, 2025 •

edited

Loading