Skip to content

Commit 32752a3

Browse files
bart-vmwarebart-vmware
authored
Crashfix in Config Server client when Environment is a comma-separated list (SteeltoeOSS#1567)
* Escape comma in Config Server environment setting, to fix crash in URL masking * Additional hardening against special characters in Config Server URL * Fixed: allow colon to appear in password * Throw early when Config Server URL(s) is/are invalid * Code cleanup
1 parent 4e259f8 commit 32752a3

12 files changed

+131
-61
lines changed

‎src/Common/src/Common/Extensions/UriExtensions.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ public static bool TryGetUsernamePassword(this Uri uri, [NotNullWhen(true)] out
4848

4949
string userInfo = uri.GetComponents(UriComponents.UserInfo, UriFormat.UriEscaped);
5050

51-
string[] parts = userInfo.Split(':');
51+
string[] parts = userInfo.Split(':', 2);
5252

5353
if (parts.Length == 2)
5454
{

‎src/Common/test/Common.Test/Extensions/UriExtensionsTest.cs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,4 +40,16 @@ public void DoNotMaskIfNoBasicAuthentication()
4040

4141
masked.Should().Be(expected);
4242
}
43+
44+
[Fact]
45+
public void TryGetUsernamePassword_AllowsColonInPassword()
46+
{
47+
var uri = new Uri("http://username:pass::[email protected]");
48+
49+
bool result = uri.TryGetUsernamePassword(out string? username, out string? password);
50+
51+
result.Should().BeTrue();
52+
username.Should().Be("username");
53+
password.Should().Be("pass::word");
54+
}
4355
}

‎src/Configuration/src/ConfigServer/ConfigServerClientOptions.cs‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ public sealed class ConfigServerClientOptions : IValidateCertificatesOptions
3232
public bool FailFast { get; set; }
3333

3434
/// <summary>
35-
/// Gets or sets the environment used when accessing configuration data. Default value: "Production".
35+
/// Gets or sets a comma-separated list of environments used when accessing configuration data. Default value: "Production".
3636
/// </summary>
3737
[ConfigurationKeyName("Env")]
3838
public string? Environment { get; set; } = "Production";
@@ -143,8 +143,18 @@ public bool ValidateCertificatesAlt
143143
/// </summary>
144144
public IDictionary<string, string> Headers { get; } = new Dictionary<string, string>();
145145

146-
internal IList<string> GetUris()
146+
internal List<Uri> GetUris()
147147
{
148-
return !string.IsNullOrEmpty(Uri) ? Uri.Split(CommaDelimiter, StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries) : [];
148+
return Uri == null ? [] : Uri.Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries).Select(ParseSingleUri).ToList();
149+
}
150+
151+
private static Uri ParseSingleUri(string uri)
152+
{
153+
if (!System.Uri.TryCreate(uri, UriKind.Absolute, out Uri? result))
154+
{
155+
throw new ConfigServerException("One or more Config Server URIs in configuration are invalid.");
156+
}
157+
158+
return result;
149159
}
150160
}

‎src/Configuration/src/ConfigServer/ConfigServerConfigurationProvider.cs‎

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -223,24 +223,22 @@ public override void Load()
223223
// Adds client settings (e.g. spring:cloud:config:uri, etc.) to the Data dictionary
224224
AddConfigServerClientOptions();
225225

226-
string logUri = string.Join(',', ClientOptions.GetUris().Select(uri => new Uri(uri).ToMaskedString()));
227-
228226
if (ClientOptions is { Retry.Enabled: true, FailFast: true })
229227
{
230228
int attempts = 0;
231229
int backOff = ClientOptions.Retry.InitialInterval;
232230

233231
do
234232
{
235-
_logger.LogDebug("Fetching configuration from server at: {Uri}", logUri);
233+
_logger.LogDebug("Fetching configuration from server(s).");
236234

237235
try
238236
{
239237
return await DoLoadAsync(updateDictionary, cancellationToken);
240238
}
241239
catch (ConfigServerException exception)
242240
{
243-
_logger.LogWarning(exception, "Failed fetching configuration from server at: {Uri}.", logUri);
241+
_logger.LogWarning(exception, "Failed fetching configuration from server(s).");
244242
attempts++;
245243

246244
if (attempts < ClientOptions.Retry.MaxAttempts)
@@ -258,16 +256,16 @@ public override void Load()
258256
while (true);
259257
}
260258

261-
_logger.LogDebug("Fetching configuration from server at: {Uri}", logUri);
259+
_logger.LogDebug("Fetching configuration from server(s).");
262260
return await DoLoadAsync(updateDictionary, cancellationToken);
263261
}
264262

265263
internal async Task<ConfigEnvironment?> DoLoadAsync(bool updateDictionary, CancellationToken cancellationToken)
266264
{
267265
Exception? error = null;
268266

269-
// Get arrays of Config Server uris to check
270-
IList<string> uris = ClientOptions.GetUris();
267+
// Get list of Config Server uris to check
268+
List<Uri> uris = ClientOptions.GetUris();
271269

272270
try
273271
{
@@ -543,7 +541,7 @@ private void AddConfigServerClientOptions(Dictionary<string, string?> data)
543541
}
544542
}
545543

546-
internal async Task<ConfigEnvironment?> RemoteLoadAsync(IEnumerable<string> requestUris, string? label, CancellationToken cancellationToken)
544+
internal async Task<ConfigEnvironment?> RemoteLoadAsync(List<Uri> requestUris, string? label, CancellationToken cancellationToken)
547545
{
548546
_logger.LogTrace("Entered {Method}", nameof(RemoteLoadAsync));
549547

@@ -552,11 +550,13 @@ private void AddConfigServerClientOptions(Dictionary<string, string?> data)
552550

553551
Exception? error = null;
554552

555-
foreach (string requestUri in requestUris)
553+
foreach (Uri requestUri in requestUris)
556554
{
557555
// Make Config Server URI from settings
558556
Uri uri = BuildConfigServerUri(requestUri, label);
559557

558+
_logger.LogDebug("Trying to connect to Config Server at {RequestUri}", uri.ToMaskedString());
559+
560560
// Get the request message
561561
_logger.LogTrace("Building HTTP request message");
562562
HttpRequestMessage request = await GetRequestMessageAsync(uri, cancellationToken);
@@ -622,23 +622,23 @@ private void AddConfigServerClientOptions(Dictionary<string, string?> data)
622622
/// <returns>
623623
/// The request URI for the Configuration Server.
624624
/// </returns>
625-
internal Uri BuildConfigServerUri(string serverUri, string? label)
625+
internal Uri BuildConfigServerUri(Uri serverUri, string? label)
626626
{
627-
ArgumentException.ThrowIfNullOrWhiteSpace(serverUri);
627+
ArgumentNullException.ThrowIfNull(serverUri);
628628

629-
var uriBuilder = new UriBuilder(new Uri(serverUri));
629+
var uriBuilder = new UriBuilder(serverUri);
630630

631631
if (!string.IsNullOrEmpty(ClientOptions.Username))
632632
{
633-
uriBuilder.UserName = ClientOptions.Username;
633+
uriBuilder.UserName = WebUtility.UrlEncode(ClientOptions.Username);
634634
}
635635

636636
if (!string.IsNullOrEmpty(ClientOptions.Password))
637637
{
638-
uriBuilder.Password = ClientOptions.Password;
638+
uriBuilder.Password = WebUtility.UrlEncode(ClientOptions.Password);
639639
}
640640

641-
string pathSuffix = $"{ClientOptions.Name}/{ClientOptions.Environment}";
641+
string pathSuffix = $"{WebUtility.UrlEncode(ClientOptions.Name)}/{WebUtility.UrlEncode(ClientOptions.Environment)}";
642642

643643
if (!string.IsNullOrWhiteSpace(label))
644644
{
@@ -648,7 +648,7 @@ internal Uri BuildConfigServerUri(string serverUri, string? label)
648648
label = label.Replace("/", "(_)", StringComparison.Ordinal);
649649
}
650650

651-
pathSuffix = $"{pathSuffix}/{label.Trim()}";
651+
pathSuffix = $"{pathSuffix}/{WebUtility.UrlEncode(label)}";
652652
}
653653

654654
if (!uriBuilder.Path.EndsWith('/'))

‎src/Configuration/src/ConfigServer/ConfigurationSchema.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@
9090
},
9191
"Env": {
9292
"type": "string",
93-
"description": "Gets or sets the environment used when accessing configuration data. Default value: \"Production\"."
93+
"description": "Gets or sets a comma-separated list of environments used when accessing configuration data. Default value: \"Production\"."
9494
},
9595
"FailFast": {
9696
"type": "boolean",

‎src/Configuration/src/RandomValue/RandomValueProvider.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ private long GetLong()
165165

166166
private int GetNextIntInRange(string range)
167167
{
168-
string[] tokens = range.Split(',');
168+
string[] tokens = range.Split(',', 2);
169169
int.TryParse(tokens[0], CultureInfo.InvariantCulture, out int start);
170170

171171
if (tokens.Length == 1)
@@ -179,7 +179,7 @@ private int GetNextIntInRange(string range)
179179

180180
private long GetNextLongInRange(string range)
181181
{
182-
string[] tokens = range.Split(',');
182+
string[] tokens = range.Split(',', 2);
183183
long.TryParse(tokens[0], CultureInfo.InvariantCulture, out long start);
184184

185185
if (tokens.Length == 1)

‎src/Configuration/test/ConfigServer.Test/ConfigServerConfigurationBuilderExtensionsCoreTest.cs‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,7 @@ public void AddConfigServer_WithLoggerFactorySucceeds()
4343

4444
IList<string> logMessages = loggerProvider.GetAll();
4545

46-
logMessages.Should().Contain(
47-
"DBUG Steeltoe.Configuration.ConfigServer.ConfigServerConfigurationProvider: Fetching configuration from server at: http://localhost:8888/");
46+
logMessages.Should().Contain("DBUG Steeltoe.Configuration.ConfigServer.ConfigServerConfigurationProvider: Fetching configuration from server(s).");
4847
}
4948

5049
[Fact]

‎src/Configuration/test/ConfigServer.Test/ConfigServerConfigurationBuilderExtensionsTest.cs‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -184,8 +184,7 @@ public void AddConfigServer_WithLoggerFactorySucceeds()
184184

185185
IList<string> logMessages = loggerProvider.GetAll();
186186

187-
logMessages.Should().Contain(
188-
"DBUG Steeltoe.Configuration.ConfigServer.ConfigServerConfigurationProvider: Fetching configuration from server at: http://localhost:8888/");
187+
logMessages.Should().Contain("DBUG Steeltoe.Configuration.ConfigServer.ConfigServerConfigurationProvider: Fetching configuration from server(s).");
189188
}
190189

191190
[Theory]

‎src/Configuration/test/ConfigServer.Test/ConfigServerConfigurationProviderTest.Loading.cs‎

Lines changed: 28 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -13,18 +13,6 @@ namespace Steeltoe.Configuration.ConfigServer.Test;
1313

1414
public sealed partial class ConfigServerConfigurationProviderTest
1515
{
16-
[Fact]
17-
public async Task RemoteLoadAsync_InvalidUri()
18-
{
19-
var options = new ConfigServerClientOptions();
20-
using var provider = new ConfigServerConfigurationProvider(options, null, null, NullLoggerFactory.Instance);
21-
22-
// ReSharper disable once AccessToDisposedClosure
23-
Func<Task> action = async () => await provider.RemoteLoadAsync([@"foobar\foobar\"], null, TestContext.Current.CancellationToken);
24-
25-
await action.Should().ThrowExactlyAsync<UriFormatException>();
26-
}
27-
2816
[Fact]
2917
public async Task RemoteLoadAsync_HostTimesOut()
3018
{
@@ -35,9 +23,10 @@ public async Task RemoteLoadAsync_HostTimesOut()
3523

3624
var httpClientHandler = new SlowHttpClientHandler(1.Seconds(), new HttpResponseMessage());
3725
using var provider = new ConfigServerConfigurationProvider(options, null, httpClientHandler, NullLoggerFactory.Instance);
26+
List<Uri> requestUris = [new("http://localhost:9999/app/profile")];
3827

3928
// ReSharper disable once AccessToDisposedClosure
40-
Func<Task> action = async () => await provider.RemoteLoadAsync(["http://localhost:9999/app/profile"], null, TestContext.Current.CancellationToken);
29+
Func<Task> action = async () => await provider.RemoteLoadAsync(requestUris, null, TestContext.Current.CancellationToken);
4130

4231
(await action.Should().ThrowExactlyAsync<TaskCanceledException>()).WithInnerExceptionExactly<TimeoutException>();
4332
}
@@ -506,6 +495,32 @@ public async Task Load_MultipleConfigServers_ReturnsNotFoundStatus__DoesNotConti
506495
startup.RequestCount.Should().Be(1);
507496
}
508497

498+
[Fact]
499+
public async Task Load_UriInvalid_FailFastEnabled()
500+
{
501+
using var startup = new TestConfigServerStartup();
502+
startup.ReturnStatus = [500];
503+
504+
await using WebApplication app = TestWebApplicationBuilderFactory.Create().Build();
505+
startup.Configure(app);
506+
await app.StartAsync(TestContext.Current.CancellationToken);
507+
508+
using TestServer server = app.GetTestServer();
509+
server.BaseAddress = new Uri("http://localhost:8888");
510+
511+
ConfigServerClientOptions options = GetCommonOptions();
512+
options.Uri = "http://username:p@ssword@localhost:8888";
513+
options.FailFast = true;
514+
515+
using var httpClientHandler = new ForwardingHttpClientHandler(server.CreateHandler());
516+
using var provider = new ConfigServerConfigurationProvider(options, null, httpClientHandler, NullLoggerFactory.Instance);
517+
518+
// ReSharper disable once AccessToDisposedClosure
519+
Func<Task> action = async () => await provider.LoadInternalAsync(true, TestContext.Current.CancellationToken);
520+
521+
await action.Should().ThrowExactlyAsync<ConfigServerException>().WithMessage("One or more Config Server URIs in configuration are invalid.");
522+
}
523+
509524
[Fact]
510525
public async Task Load_ConfigServerReturnsBadStatus_FailFastEnabled()
511526
{

‎src/Configuration/test/ConfigServer.Test/ConfigServerConfigurationProviderTest.Settings.cs‎

Lines changed: 49 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -79,9 +79,9 @@ public void GetConfigServerUri_NoLabel()
7979
};
8080

8181
using var provider = new ConfigServerConfigurationProvider(options, null, null, NullLoggerFactory.Instance);
82-
string path = provider.BuildConfigServerUri(options.Uri!, null).ToString();
82+
string uri = provider.BuildConfigServerUri(new Uri(options.Uri!), null).ToString();
8383

84-
path.Should().Be($"{options.Uri}/{options.Name}/{options.Environment}");
84+
uri.Should().Be($"{options.Uri}/{options.Name}/{options.Environment}");
8585
}
8686

8787
[Fact]
@@ -95,9 +95,9 @@ public void GetConfigServerUri_WithLabel()
9595
};
9696

9797
using var provider = new ConfigServerConfigurationProvider(options, null, null, NullLoggerFactory.Instance);
98-
string path = provider.BuildConfigServerUri(options.Uri!, options.Label).ToString();
98+
string uri = provider.BuildConfigServerUri(new Uri(options.Uri!), options.Label).ToString();
9999

100-
path.Should().Be($"{options.Uri}/{options.Name}/{options.Environment}/{options.Label}");
100+
uri.Should().Be($"{options.Uri}/{options.Name}/{options.Environment}/{options.Label}");
101101
}
102102

103103
[Fact]
@@ -111,9 +111,9 @@ public void GetConfigServerUri_WithLabelContainingSlash()
111111
};
112112

113113
using var provider = new ConfigServerConfigurationProvider(options, null, null, NullLoggerFactory.Instance);
114-
string path = provider.BuildConfigServerUri(options.Uri!, options.Label).ToString();
114+
string uri = provider.BuildConfigServerUri(new Uri(options.Uri!), options.Label).ToString();
115115

116-
path.Should().Be($"{options.Uri}/{options.Name}/{options.Environment}/myLabel(_)version");
116+
uri.Should().Be($"{options.Uri}/{options.Name}/{options.Environment}/myLabel(_)version");
117117
}
118118

119119
[Fact]
@@ -127,9 +127,9 @@ public void GetConfigServerUri_WithExtraPathInfo()
127127
};
128128

129129
using var provider = new ConfigServerConfigurationProvider(options, null, null, NullLoggerFactory.Instance);
130-
string path = provider.BuildConfigServerUri(options.Uri, null).ToString();
130+
string uri = provider.BuildConfigServerUri(new Uri(options.Uri), null).ToString();
131131

132-
path.Should().Be($"http://localhost:9999/myPath/path/{options.Name}/{options.Environment}");
132+
uri.Should().Be($"http://localhost:9999/myPath/path/{options.Name}/{options.Environment}");
133133
}
134134

135135
[Fact]
@@ -143,9 +143,9 @@ public void GetConfigServerUri_WithExtraPathInfo_NoEndingSlash()
143143
};
144144

145145
using var provider = new ConfigServerConfigurationProvider(options, null, null, NullLoggerFactory.Instance);
146-
string path = provider.BuildConfigServerUri(options.Uri, null).ToString();
146+
string uri = provider.BuildConfigServerUri(new Uri(options.Uri), null).ToString();
147147

148-
path.Should().Be($"http://localhost:9999/myPath/path/{options.Name}/{options.Environment}");
148+
uri.Should().Be($"http://localhost:9999/myPath/path/{options.Name}/{options.Environment}");
149149
}
150150

151151
[Fact]
@@ -159,9 +159,9 @@ public void GetConfigServerUri_NoEndingSlash()
159159
};
160160

161161
using var provider = new ConfigServerConfigurationProvider(options, null, null, NullLoggerFactory.Instance);
162-
string path = provider.BuildConfigServerUri(options.Uri, null).ToString();
162+
string uri = provider.BuildConfigServerUri(new Uri(options.Uri), null).ToString();
163163

164-
path.Should().Be($"http://localhost:9999/{options.Name}/{options.Environment}");
164+
uri.Should().Be($"http://localhost:9999/{options.Name}/{options.Environment}");
165165
}
166166

167167
[Fact]
@@ -175,8 +175,43 @@ public void GetConfigServerUri_WithEndingSlash()
175175
};
176176

177177
using var provider = new ConfigServerConfigurationProvider(options, null, null, NullLoggerFactory.Instance);
178-
string path = provider.BuildConfigServerUri(options.Uri, null).ToString();
178+
string uri = provider.BuildConfigServerUri(new Uri(options.Uri), null).ToString();
179179

180-
path.Should().Be($"http://localhost:9999/{options.Name}/{options.Environment}");
180+
uri.Should().Be($"http://localhost:9999/{options.Name}/{options.Environment}");
181+
}
182+
183+
[Fact]
184+
public void GetConfigServerUri_MultipleEnvironments_EncodesComma()
185+
{
186+
var options = new ConfigServerClientOptions
187+
{
188+
Name = "myName",
189+
Environment = "one,two",
190+
Label = "demo"
191+
};
192+
193+
using var provider = new ConfigServerConfigurationProvider(options, null, null, NullLoggerFactory.Instance);
194+
string uri = provider.BuildConfigServerUri(new Uri(options.Uri!), options.Label).ToString();
195+
196+
uri.Should().Be("http://localhost:8888/myName/one%2Ctwo/demo");
197+
}
198+
199+
[Fact]
200+
public void GetConfigServerUri_EncodesSpecialCharacters()
201+
{
202+
var options = new ConfigServerClientOptions
203+
{
204+
Name = "my$<>:,\"'Name",
205+
Environment = "@/&?:\"'",
206+
Label = "^&$@#*<>+",
207+
Username = "a\":?'*,b/\\&",
208+
Password = "#&:$<>'/so,\"me"
209+
};
210+
211+
using var provider = new ConfigServerConfigurationProvider(options, null, null, NullLoggerFactory.Instance);
212+
string uri = provider.BuildConfigServerUri(new Uri(options.Uri!), options.Label).ToString();
213+
214+
uri.Should().Be(
215+
"http://a%22%3A%3F%27*%2Cb%2F%5C%26:%23%26%3A%24%3C%3E%27%2Fso%2C%22me@localhost:8888/my%24<>%3A%2C\"%27Name/%40%2F%26%3F%3A\"%27/^%26%24%40%23*<>%2B");
181216
}
182217
}

0 commit comments

Comments
 (0)