#2116 Escaping unsafe pattern values of `Regex` constructor derived from URL query parameter values containing special `Regex` chars #2150

int0x81 · 2024-09-13T12:45:44Z

Fixes #2116

This PR introduces unit tests as well as a fix for issue: Regex exception when route template placeholder contains invalid character ) #2116

Bug

When the downstream URL was created dynamically by a path like api/{path} and the URL contained malicious characters, the request might result in a 500 response from the server. This happened because malicious characters were used for dynamic Regex creation.

Solution

All parameters that are used for template replacements are now Regex-escaped before they are used in the Regex constructor.

raman-m · 2024-09-13T13:38:51Z

Hello Finn!
Welcome to the world of Ocelot! 🐅
Thank you for creating the PR!

Could we please scan the codebase and other Regex constructors for potentially similar issues?
This search query will help you...

src/Ocelot/DownstreamUrlCreator/Middleware/DownstreamUrlCreatorMiddleware.cs

test/Ocelot.UnitTests/DownstreamUrlCreator/DownstreamUrlCreatorMiddlewareTests.cs

raman-m

Please move the test to https://github.com/ThreeMammals/Ocelot/blob/develop/test/Ocelot.AcceptanceTests/Routing/RoutingTests.cs

test/Ocelot.AcceptanceTests/PathToDownstreamUrlTests.cs

ggnaegi

Hello, sound for me! Thanks a lot @int0x81!

raman-m · 2024-09-19T15:04:19Z

test/Ocelot.UnitTests/DownstreamUrlCreator/DownstreamUrlCreatorMiddlewareTests.cs

+    [InlineData("api/debug()")] // no query
+    [InlineData("api/debug%28%29")] // debug()


TODO Special chars in values of query strings

Hello @ggnaegi,
I would like to delay my approval until we have made a decision on the additional test case:

Suggested change

[InlineData("api/debug()")] // no query

[InlineData("api/debug%28%29")] // debug()

[InlineData("api/debug()")] // no query

[InlineData("api/debug%28%29")] // encoded debug()

[InlineData("api/debug()?special=(\\,*,+,?,|,{,},[,],(,),^,$,.,#, ,)&2=(2)&3=[3]&4={4}")] // with query

The third test has revealed issues within the middleware logic, indicating that this test case is likely to fail. The MergeQueryStringsWithoutDuplicateValues method struggles to handle URLs containing query strings with special characters, such as Regex patterns and reserved URL specification characters. This issue became apparent in release 20.0.0 during the refactoring of the MergeQueryStringsWithoutDuplicateValues method, which aimed to integrate existing and new logic (for example OData filters in query parameters, with bug fixes, with new feature for query string placeholders). Regrettably, the method fails to process special characters in parameter values.

Would you recommend addressing the design bottleneck within this bug fix, or would creating a new TODO task be a better approach to ensure the current release proceeds without delay?

If you don't mind I will submit a small code-review commit... → 0c150e5

@raman-m the whole thing is a mess, @int0x81 who is new to ocelot said straight away that the implementation could be optimized. For now, I would suggest merging the current changes, refactorings are out of scope in my opinion. @int0x81 would be happy to create a follow up issue and PR.

raman-m

It looks good to me!
I've marked the message as the follow-up TODO task.
@ggnaegi, because you're Assignee and if we're done, please press the magic Squash button 🪄

ggnaegi · 2024-09-20T11:59:57Z

Squashed and Merged it is!

@raman-m

…Blue Olympic Balumbes release * #2084 Apply default config file paths in `GetMergedOcelotJson` when providing the `folder` argument of `AddOcelot` (#2120) * Adding unit test first * Fixing default global config file not being found in folder * Adding PR trait to test * Backing out whitespace changes * Code review by @raman-m * Create Configuration feature folder and move test classes * Adjust namespace and review what we have * Acceptance tests for #2084 user scenario --------- Co-authored-by: Raman Maksimchuk <[email protected]> * Bump Steeltoe.Discovery.Eureka from 3.2.5 to 3.2.8 in /src/Ocelot.Provider.Eureka (#2122) * Bump Steeltoe.Discovery.Eureka in /src/Ocelot.Provider.Eureka Bumps [Steeltoe.Discovery.Eureka](https://github.com/SteeltoeOSS/Steeltoe) from 3.2.5 to 3.2.8. - [Release notes](https://github.com/SteeltoeOSS/Steeltoe/releases) - [Changelog](https://github.com/SteeltoeOSS/Steeltoe/blob/main/Steeltoe.Release.ruleset) - [Commits](SteeltoeOSS/Steeltoe@3.2.5...3.2.8) --- updated-dependencies: - dependency-name: Steeltoe.Discovery.Eureka dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> * Bump Steeltoe.Discovery.ClientCore from 3.2.5 to 3.2.8 --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Raman Maksimchuk <[email protected]> * #2110 Review load balancing and independent fetching the list of services in `Kube` provider (#2111) * Move the creation of the services list from the class field to the method, to prevent modification list from different threads * Early return after data checking * Add unit test for concurrent get list of services * Add logging for invalid service configuration error in RoundRobin load balancer * Code review by @raman-m * Workaround for mistakes made during acceptance testing of load balancing versus service discovery, where tests designed for parallel requests were mistakenly executed sequentially. This resulted in load balancers being loaded by sequential `HttpClient` calls, which was a significant oversight. * Let's DRY StickySessionsTests * Add acceptance tests, but... RoundRobin is not actually RoundRobin 😁 -> 😆 * Independent static indexing iterators per route via service names * Stabilize `CookieStickySessions` load balancer. Review tests after refactoring of `RoundRobin` load balancer * Refactor Lease operation for load balancing. Review LeastConnection load balancer * Leasing mechanism in Round Robin load balancer * Acceptance tests, final version * Apply Retry pattern for K8s endpoint integration * Fix IDE warnings and messages * Follow suggestions and fix issues from code review by @ggnaegi * Bump KubeClient from 2.4.10 to 2.5.8 * Fix warnings * Final version of `Retry` pattern --------- Co-authored-by: Raman Maksimchuk <[email protected]> * Downgrade the Warning to Information on missing `Content-Length` header in `MultiplexingMiddleware` (#2146) * fix: downgrade the warning to information on missing content-length header * chore: add route name to logs * test: fixing multiplexing middleware tests * Code review by @raman-m --------- Co-authored-by: Paul Roy <[email protected]> Co-authored-by: Raman Maksimchuk <[email protected]> * Correct the broken link to the GraphQL sample's `README.md` (#2149) Signed-off-by: Emmanuel Ferdman <[email protected]> Co-authored-by: Raman Maksimchuk <[email protected]> * #2116 Escaping unsafe pattern values of `Regex` constructor derived from URL query parameter values containing special `Regex` chars (#2150) * regex escape handling for url templates * refactored regex method to lamda version * Quick code review by @raman-m * added acceptance test for url regex bug * moved acceptance test to routing tests * Convert to theory: define 2 test cases --------- Co-authored-by: Raman Maksimchuk <[email protected]> * #2119 Review load balancing (2nd round) and redesign `DefaultConsulServiceBuilder` with `ConsulProviderFactory` refactoring to make it thread safe and friendly (#2151) * Review tests * History of Service Discovery testing: add traits * LoadBalancer traits * #2119 Steps to Reproduce * Reuse service handlers of `ConcurrentSteps` * Reuse service counters of `ConcurrentSteps` * Add LoadBalancer namespace and move classes * Move `Lease` * Move `LeaseEventArgs` * Analyze load balancers aka `ILoadBalancerAnalyzer` interface objects * Prefer using named local methods as delegates over anonymous methods for awesome call stack, ensuring the delegate's typed result matches the typed balancer's creator. Additionally, employ an IServiceProvider workaround. * Review load balancing. Assert service & leasing counters as concurrent step. Final version of acceptance test. * Fixed naming violation for asynchronous methods: `Lease` -> `LeaseAsync` * Fix ugly reflection issue of dymanic detection in favor of static type property * Propagate the `ConsulRegistryConfiguration` object through `HttpContext` in the scoped version of the default service builder, utilizing the injected `IHttpContextAccessor` object. Update `ConsulProviderFactory`. Update docs. Update tests. * Add tests from clean experiment * Final review of the tests * Review `IHttpContextAccessor` logic. Convert anonymous delegates to named ones in placeholders processing * Tried to enhance more, but failed --------- Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: Emmanuel Ferdman <[email protected]> Co-authored-by: Ben Bartholomew <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Roman <[email protected]> Co-authored-by: Paul Roy <[email protected]> Co-authored-by: Paul Roy <[email protected]> Co-authored-by: Emmanuel Ferdman <[email protected]> Co-authored-by: Finn <[email protected]>

int0x81 added 2 commits September 12, 2024 16:32

regex escape handling for url templates

f2d773a

refactored regex method to lamda version

549aa56

raman-m assigned int0x81 and ggnaegi Sep 13, 2024

raman-m changed the title ~~Bugfix for Issue 2116~~ #2116 Escaping unsafe pattern values of Regex constructor derived from URL query parameter values containing unsafe regexp chars Sep 13, 2024

raman-m added bug Identified as a potential bug Routing Ocelot feature: Routing labels Sep 13, 2024

raman-m added this to the v23.3 Hotfixes milestone Sep 13, 2024

raman-m requested review from raman-m and ggnaegi September 13, 2024 13:26

Merge branch 'develop' into bugfix/2116_regex_handling

43c652b

raman-m reviewed Sep 15, 2024

View reviewed changes

src/Ocelot/DownstreamUrlCreator/Middleware/DownstreamUrlCreatorMiddleware.cs Outdated Show resolved Hide resolved

test/Ocelot.UnitTests/DownstreamUrlCreator/DownstreamUrlCreatorMiddlewareTests.cs Outdated Show resolved Hide resolved

raman-m and others added 2 commits September 15, 2024 17:01

Quick code review by @raman-m

32f16ec

added acceptance test for url regex bug

6dc998a

raman-m requested changes Sep 17, 2024

View reviewed changes

test/Ocelot.AcceptanceTests/PathToDownstreamUrlTests.cs Outdated Show resolved Hide resolved

test/Ocelot.AcceptanceTests/PathToDownstreamUrlTests.cs Outdated Show resolved Hide resolved

moved acceptance test to routing tests

4848e11

ggnaegi approved these changes Sep 18, 2024

View reviewed changes

raman-m reviewed Sep 19, 2024

View reviewed changes

Convert to theory: define 2 test cases

0c150e5

raman-m approved these changes Sep 19, 2024

View reviewed changes

raman-m mentioned this pull request Sep 19, 2024

TODO and other follow-up tasks #2141

Open

ggnaegi merged commit 58d87c9 into ThreeMammals:develop Sep 20, 2024
1 check passed

raman-m mentioned this pull request Sep 21, 2024

Regex exception when route template placeholder contains invalid character ) #2116

Closed

int0x81 deleted the bugfix/2116_regex_handling branch September 21, 2024 11:53

raman-m mentioned this pull request Oct 3, 2024

Release 23.3.4 | v23.3 Hotfixes | Blue Olympic Balumbes release #2162

Merged

raman-m mentioned this pull request Nov 8, 2024

Follow up #2150: Special chars in values of query strings #2191

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2116 Escaping unsafe pattern values of `Regex` constructor derived from URL query parameter values containing special `Regex` chars #2150

#2116 Escaping unsafe pattern values of `Regex` constructor derived from URL query parameter values containing special `Regex` chars #2150

int0x81 commented Sep 13, 2024 •

edited by raman-m

Loading

raman-m commented Sep 13, 2024 •

edited

Loading

raman-m left a comment

ggnaegi left a comment

raman-m Sep 19, 2024 •

edited

Loading

raman-m Sep 19, 2024 •

edited

Loading

ggnaegi Sep 19, 2024 •

edited

Loading

raman-m left a comment •

edited

Loading

ggnaegi commented Sep 20, 2024 •

edited

Loading

		[InlineData("api/debug()")] // no query
		[InlineData("api/debug%28%29")] // debug()

#2116 Escaping unsafe pattern values of Regex constructor ​​derived from URL query parameter values containing special Regex chars #2150

#2116 Escaping unsafe pattern values of Regex constructor ​​derived from URL query parameter values containing special Regex chars #2150

Conversation

int0x81 commented Sep 13, 2024 • edited by raman-m Loading

Fixes #2116

Bug

Solution

raman-m commented Sep 13, 2024 • edited Loading

raman-m left a comment

Choose a reason for hiding this comment

ggnaegi left a comment

Choose a reason for hiding this comment

raman-m Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

TODO Special chars in values of query strings

raman-m Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

ggnaegi Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

raman-m left a comment • edited Loading

Choose a reason for hiding this comment

ggnaegi commented Sep 20, 2024 • edited Loading

#2116 Escaping unsafe pattern values of `Regex` constructor derived from URL query parameter values containing special `Regex` chars #2150

#2116 Escaping unsafe pattern values of `Regex` constructor derived from URL query parameter values containing special `Regex` chars #2150

int0x81 commented Sep 13, 2024 •

edited by raman-m

Loading

raman-m commented Sep 13, 2024 •

edited

Loading

raman-m Sep 19, 2024 •

edited

Loading

raman-m Sep 19, 2024 •

edited

Loading

ggnaegi Sep 19, 2024 •

edited

Loading

raman-m left a comment •

edited

Loading

ggnaegi commented Sep 20, 2024 •

edited

Loading