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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 7 commits into from
Sep 20, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f2d773a
regex escape handling for url templates
int0x81 Sep 12, 2024
549aa56
refactored regex method to lamda version
int0x81 Sep 13, 2024
43c652b
Merge branch 'develop' into bugfix/2116_regex_handling
raman-m Sep 14, 2024
32f16ec
Quick code review by @raman-m
raman-m Sep 15, 2024
6dc998a
added acceptance test for url regex bug
int0x81 Sep 17, 2024
4848e11
moved acceptance test to routing tests
int0x81 Sep 17, 2024
0c150e5
Convert to theory: define 2 test cases
raman-m Sep 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Ocelot/DownstreamUrlCreator/Middleware/DownstreamUrlCreatorMiddleware.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,8 @@ private static void RemoveQueryStringParametersThatHaveBeenUsedInTemplate(Downst
foreach (var nAndV in templatePlaceholderNameAndValues)
{
var name = nAndV.Name.Trim(OpeningBrace, ClosingBrace);

var rgx = new Regex($@"\b{name}={nAndV.Value}\b");
var value = Regex.Escape(nAndV.Value); // to ensure a placeholder value containing special Regex characters from URL query parameters is safely used in a Regex constructor, it's necessary to escape the value
var rgx = new Regex($@"\b{name}={value}\b");

if (rgx.IsMatch(downstreamRequest.Query))
{
Expand Down
19 changes: 19 additions & 0 deletions test/Ocelot.AcceptanceTests/Routing/RoutingTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using Microsoft.AspNetCore.Http;
using Ocelot.Configuration.File;
using System.Web;

namespace Ocelot.AcceptanceTests.Routing
{
Expand Down Expand Up @@ -1167,6 +1168,24 @@ public void should_fix_issue_271()
.BDDfy();
}

[Theory]
[Trait("Bug", "2116")]
[InlineData("debug()")] // no query
[InlineData("debug%28%29")] // debug()
public void Should_change_downstream_path_by_upstream_path_when_path_contains_malicious_characters(string path)
{
var port = PortFinder.GetRandomPort();
var configuration = GivenDefaultConfiguration(port, "/api/{path}", "/routed/api/{path}");
var decodedDownstreamUrlPath = $"/routed/api/{HttpUtility.UrlDecode(path)}";
this.Given(x => x.GivenThereIsAServiceRunningOn($"http://localhost:{port}", decodedDownstreamUrlPath, HttpStatusCode.OK, string.Empty))
.And(x => _steps.GivenThereIsAConfiguration(configuration))
.And(x => _steps.GivenOcelotIsRunning())
.When(x => _steps.WhenIGetUrlOnTheApiGateway($"/api/{path}")) // should be encoded
.Then(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.OK))
.And(x => ThenTheDownstreamUrlPathShouldBe(decodedDownstreamUrlPath))
.BDDfy();
}

private void GivenThereIsAServiceRunningOn(string baseUrl, string basePath, HttpStatusCode statusCode, string responseBody)
{
_serviceHandler.GivenThereIsAServiceRunningOn(baseUrl, basePath, async context =>
Expand Down
37 changes: 37 additions & 0 deletions test/Ocelot.UnitTests/DownstreamUrlCreator/DownstreamUrlCreatorMiddlewareTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -611,6 +611,43 @@ public void Should_map_when_query_parameters_has_same_names_with_placeholder()
ThenTheQueryStringIs($"?roleId={roleid}&{everything}");
}

[Theory]
[Trait("Bug", "2116")]
[InlineData("api/debug()")] // no query
[InlineData("api/debug%28%29")] // debug()
Comment on lines +616 to +617
Copy link
Member

@raman-m raman-m Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

@raman-m raman-m Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

@ggnaegi ggnaegi Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

public void ShouldNotFailToHandleUrlWithSpecialRegexChars(string urlPath)
{
// Arrange
var withGetMethod = new List<string> { "Get" };
var downstreamRoute = new DownstreamRouteBuilder()
.WithUpstreamPathTemplate(new UpstreamPathTemplateBuilder()
.WithOriginalValue("/routed/api/{path}")
.Build())
.WithDownstreamPathTemplate("/api/{path}")
.WithUpstreamHttpMethod(withGetMethod)
.WithDownstreamScheme(Uri.UriSchemeHttp)
.Build();
GivenTheDownStreamRouteIs(new DownstreamRouteHolder(
new List<PlaceholderNameAndValue>
{
new("{path}", urlPath),
},
new RouteBuilder().WithDownstreamRoute(downstreamRoute)
.WithUpstreamHttpMethod(withGetMethod)
.Build()
));
GivenTheDownstreamRequestUriIs($"http://localhost:5000/{urlPath}");
GivenTheServiceProviderConfigIs(new ServiceProviderConfigurationBuilder().Build());
GivenTheUrlReplacerWillReturn($"routed/{urlPath}");

// Act
WhenICallTheMiddleware();

// Assert
ThenTheDownstreamRequestUriIs($"http://localhost:5000/routed/{urlPath}");
Assert.Equal((int)HttpStatusCode.OK, _httpContext.Response.StatusCode);
}

private void GivenTheServiceProviderConfigIs(ServiceProviderConfiguration config)
{
var configuration = new InternalConfiguration(null, null, config, null, null, null, null, null, null, null);
Expand Down