From 0be0bed88cef0fe49d1e1e5ab3ece16b82f90e6c Mon Sep 17 00:00:00 2001 From: sijun-yang Date: Fri, 15 Nov 2024 22:51:11 +0900 Subject: [PATCH 1/2] Tighten rules around profile naming Profiles are only allowed to use dashes, underscores, digits or letters. See gh-43176 --- .../context/SpringBootContextLoaderTests.java | 14 +--- .../StandardConfigDataLocationResolver.java | 22 +++++ .../boot/SpringApplicationTests.java | 9 +- ...andardConfigDataLocationResolverTests.java | 83 ++++++++++++++++++- 4 files changed, 110 insertions(+), 18 deletions(-) diff --git a/spring-boot-project/spring-boot-test/src/test/java/org/springframework/boot/test/context/SpringBootContextLoaderTests.java b/spring-boot-project/spring-boot-test/src/test/java/org/springframework/boot/test/context/SpringBootContextLoaderTests.java index 3f6a4888d1..e59d496734 100644 --- a/spring-boot-project/spring-boot-test/src/test/java/org/springframework/boot/test/context/SpringBootContextLoaderTests.java +++ b/spring-boot-project/spring-boot-test/src/test/java/org/springframework/boot/test/context/SpringBootContextLoaderTests.java @@ -60,6 +60,7 @@ import static org.assertj.core.api.Assertions.assertThatIllegalStateException; * @author Stephane Nicoll * @author Scott Frederick * @author Madhura Bhave + * @author Sijun Yang */ class SpringBootContextLoaderTests { @@ -127,11 +128,6 @@ class SpringBootContextLoaderTests { assertThat(getActiveProfiles(MultipleActiveProfiles.class)).containsExactly("profile1", "profile2"); } - @Test - void activeProfileWithComma() { - assertThat(getActiveProfiles(ActiveProfileWithComma.class)).containsExactly("profile1,2"); - } - @Test // gh-28776 void testPropertyValuesShouldTakePrecedenceWhenInlinedPropertiesPresent() { TestContext context = new ExposedTestContextManager(SimpleConfig.class).getExposedTestContext(); @@ -314,14 +310,8 @@ class SpringBootContextLoaderTests { } - @SpringBootTest(classes = Config.class) - @ActiveProfiles({ "profile1,2" }) - static class ActiveProfileWithComma { - - } - @SpringBootTest(properties = { "key=myValue" }, classes = Config.class) - @ActiveProfiles({ "profile1,2" }) + @ActiveProfiles({ "profile1" }) static class ActiveProfileWithInlinedProperties { } diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/config/StandardConfigDataLocationResolver.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/config/StandardConfigDataLocationResolver.java index beb076ad86..d29efafa96 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/config/StandardConfigDataLocationResolver.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/config/StandardConfigDataLocationResolver.java @@ -54,6 +54,7 @@ import org.springframework.util.StringUtils; * @author Madhura Bhave * @author Phillip Webb * @author Scott Frederick + * @author Sijun Yang * @since 2.4.0 */ public class StandardConfigDataLocationResolver @@ -154,6 +155,7 @@ public class StandardConfigDataLocationResolver private Set getProfileSpecificReferences(ConfigDataLocationResolverContext context, ConfigDataLocation[] configDataLocations, Profiles profiles) { Set references = new LinkedHashSet<>(); + validateProfiles(profiles); for (String profile : profiles) { for (ConfigDataLocation configDataLocation : configDataLocations) { String resourceLocation = getResourceLocation(context, configDataLocation); @@ -163,6 +165,26 @@ public class StandardConfigDataLocationResolver return references; } + private void validateProfiles(Profiles profiles) { + for (String profile : profiles) { + validateProfile(profile); + } + } + + private void validateProfile(String profile) { + Assert.hasText(profile, "Profile must contain text"); + Assert.state(!profile.startsWith("-") && !profile.startsWith("_"), + () -> String.format("Invalid profile '%s': must not start with '-' or '_'", profile)); + Assert.state(!profile.endsWith("-") && !profile.endsWith("_"), + () -> String.format("Invalid profile '%s': must not end with '-' or '_'", profile)); + profile.codePoints().forEach((codePoint) -> { + if (codePoint == '-' || codePoint == '_' || Character.isLetterOrDigit(codePoint)) { + return; + } + throw new IllegalStateException(String.format("Invalid profile '%s': must contain only letters or digits or '-' or '_'", profile)); + }); + } + private String getResourceLocation(ConfigDataLocationResolverContext context, ConfigDataLocation configDataLocation) { String resourceLocation = configDataLocation.getNonPrefixedValue(PREFIX); diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/SpringApplicationTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/SpringApplicationTests.java index b606329686..dc7a64c734 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/SpringApplicationTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/SpringApplicationTests.java @@ -165,6 +165,7 @@ import static org.mockito.Mockito.spy; * @author Moritz Halbritter * @author Tadaya Tsuyukubo * @author Yanming Zhou + * @author Sijun Yang */ @ExtendWith(OutputCaptureExtension.class) class SpringApplicationTests { @@ -252,13 +253,13 @@ class SpringApplicationTests { @Test void logsActiveProfilesWithoutProfileAndMultipleDefaults(CapturedOutput output) { MockEnvironment environment = new MockEnvironment(); - environment.setDefaultProfiles("p0,p1", "default"); + environment.setDefaultProfiles("p0", "default"); SpringApplication application = new SpringApplication(ExampleConfig.class); application.setWebApplicationType(WebApplicationType.NONE); application.setEnvironment(environment); this.context = application.run(); assertThat(output) - .contains("No active profile set, falling back to 2 default profiles: \"p0,p1\", \"default\""); + .contains("No active profile set, falling back to 2 default profiles: \"p0\", \"default\""); } @Test @@ -273,9 +274,9 @@ class SpringApplicationTests { void logsActiveProfilesWithMultipleProfiles(CapturedOutput output) { SpringApplication application = new SpringApplication(ExampleConfig.class); application.setWebApplicationType(WebApplicationType.NONE); - application.setAdditionalProfiles("p1,p2", "p3"); + application.setAdditionalProfiles("p1", "p2"); application.run(); - assertThat(output).contains("The following 2 profiles are active: \"p1,p2\", \"p3\""); + assertThat(output).contains("The following 2 profiles are active: \"p1\", \"p2\""); } @Test diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/StandardConfigDataLocationResolverTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/StandardConfigDataLocationResolverTests.java index 6b221e15f2..61a964a175 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/StandardConfigDataLocationResolverTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/StandardConfigDataLocationResolverTests.java @@ -44,6 +44,7 @@ import static org.mockito.Mockito.mock; * @author Madhura Bhave * @author Phillip Webb * @author Moritz Halbritter + * @author Sijun Yang */ class StandardConfigDataLocationResolverTests { @@ -254,8 +255,8 @@ class StandardConfigDataLocationResolverTests { @Test void resolveProfileSpecificReturnsProfileSpecificFiles() { ConfigDataLocation location = ConfigDataLocation.of("classpath:/configdata/properties/"); - Profiles profiles = mock(Profiles.class); - given(profiles.iterator()).willReturn(Collections.singletonList("dev").iterator()); + this.environment.setActiveProfiles("dev"); + Profiles profiles = new Profiles(this.environment, this.environmentBinder, Collections.emptyList()); List locations = this.resolver.resolveProfileSpecific(this.context, location, profiles); assertThat(locations).hasSize(1); @@ -293,6 +294,84 @@ class StandardConfigDataLocationResolverTests { assertThatNoException().isThrownBy(() -> this.resolver.resolve(this.context, location)); } + @Test + void resolveProfileSpecificWhenProfileIsValidShouldNotThrowException() { + ConfigDataLocation location = ConfigDataLocation.of("classpath:/configdata/properties/"); + this.environment.setActiveProfiles("dev-test_123"); + Profiles profiles = new Profiles(this.environment, this.environmentBinder, Collections.emptyList()); + assertThatNoException() + .isThrownBy(() -> this.resolver.resolveProfileSpecific(this.context, location, profiles)); + } + + @Test + void resolveProfileSpecificWithNonAsciiCharactersShouldNotThrowException() { + ConfigDataLocation location = ConfigDataLocation.of("classpath:/configdata/properties/"); + this.environment.setActiveProfiles("dev-테스트_123"); + Profiles profiles = new Profiles(this.environment, this.environmentBinder, Collections.emptyList()); + assertThatNoException() + .isThrownBy(() -> this.resolver.resolveProfileSpecific(this.context, location, profiles)); + } + + @Test + void resolveProfileSpecificWithAdditionalValidProfilesShouldNotThrowException() { + ConfigDataLocation location = ConfigDataLocation.of("classpath:/configdata/properties/"); + this.environment.setActiveProfiles("dev-test"); + Profiles profiles = new Profiles(this.environment, this.environmentBinder, List.of("prod-test", "stage-test")); + assertThatNoException() + .isThrownBy(() -> this.resolver.resolveProfileSpecific(this.context, location, profiles)); + } + + @Test + void resolveProfileSpecificWhenProfileStartsWithSymbolThrowsException() { + ConfigDataLocation location = ConfigDataLocation.of("classpath:/configdata/properties/"); + this.environment.setActiveProfiles("-dev"); + Profiles profiles = new Profiles(this.environment, this.environmentBinder, Collections.emptyList()); + assertThatIllegalStateException() + .isThrownBy(() -> this.resolver.resolveProfileSpecific(this.context, location, profiles)) + .withMessageStartingWith("Invalid profile '-dev': must not start with '-' or '_'"); + } + + @Test + void resolveProfileSpecificWhenProfileStartsWithUnderscoreThrowsException() { + ConfigDataLocation location = ConfigDataLocation.of("classpath:/configdata/properties/"); + this.environment.setActiveProfiles("_dev"); + Profiles profiles = new Profiles(this.environment, this.environmentBinder, Collections.emptyList()); + assertThatIllegalStateException() + .isThrownBy(() -> this.resolver.resolveProfileSpecific(this.context, location, profiles)) + .withMessageStartingWith("Invalid profile '_dev': must not start with '-' or '_'"); + } + + @Test + void resolveProfileSpecificWhenProfileEndsWithSymbolThrowsException() { + ConfigDataLocation location = ConfigDataLocation.of("classpath:/configdata/properties/"); + this.environment.setActiveProfiles("dev-"); + Profiles profiles = new Profiles(this.environment, this.environmentBinder, Collections.emptyList()); + assertThatIllegalStateException() + .isThrownBy(() -> this.resolver.resolveProfileSpecific(this.context, location, profiles)) + .withMessageStartingWith("Invalid profile 'dev-': must not end with '-' or '_'"); + } + + @Test + void resolveProfileSpecificWhenProfileEndsWithUnderscoreThrowsException() { + ConfigDataLocation location = ConfigDataLocation.of("classpath:/configdata/properties/"); + this.environment.setActiveProfiles("dev_"); + Profiles profiles = new Profiles(this.environment, this.environmentBinder, Collections.emptyList()); + assertThatIllegalStateException() + .isThrownBy(() -> this.resolver.resolveProfileSpecific(this.context, location, profiles)) + .withMessageStartingWith("Invalid profile 'dev_': must not end with '-' or '_'"); + } + + @Test + void resolveProfileSpecificWhenProfileContainsInvalidCharactersThrowsException() { + ConfigDataLocation location = ConfigDataLocation.of("classpath:/configdata/properties/"); + this.environment.setActiveProfiles("dev*test"); + Profiles profiles = new Profiles(this.environment, this.environmentBinder, Collections.emptyList()); + assertThatIllegalStateException() + .isThrownBy(() -> this.resolver.resolveProfileSpecific(this.context, location, profiles)) + .withMessageStartingWith( + "Invalid profile 'dev*test': must contain only letters or digits or '-' or '_'"); + } + private String filePath(String... components) { return "file [" + String.join(File.separator, components) + "]"; } From 5322352919e7718db4ff2154728be138d8f705c2 Mon Sep 17 00:00:00 2001 From: Moritz Halbritter Date: Tue, 14 Jan 2025 14:32:37 +0100 Subject: [PATCH 2/2] Polish "Tighten rules around profile naming" See gh-43176 --- .../config/StandardConfigDataLocationResolver.java | 9 +++++---- .../org/springframework/boot/SpringApplicationTests.java | 3 +-- .../config/StandardConfigDataLocationResolverTests.java | 5 ++--- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/config/StandardConfigDataLocationResolver.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/config/StandardConfigDataLocationResolver.java index d29efafa96..3a1561b549 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/config/StandardConfigDataLocationResolver.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/config/StandardConfigDataLocationResolver.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2024 the original author or authors. + * Copyright 2012-2025 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -149,13 +149,13 @@ public class StandardConfigDataLocationResolver @Override public List resolveProfileSpecific(ConfigDataLocationResolverContext context, ConfigDataLocation location, Profiles profiles) { + validateProfiles(profiles); return resolve(getProfileSpecificReferences(context, location.split(), profiles)); } private Set getProfileSpecificReferences(ConfigDataLocationResolverContext context, ConfigDataLocation[] configDataLocations, Profiles profiles) { Set references = new LinkedHashSet<>(); - validateProfiles(profiles); for (String profile : profiles) { for (ConfigDataLocation configDataLocation : configDataLocations) { String resourceLocation = getResourceLocation(context, configDataLocation); @@ -172,7 +172,7 @@ public class StandardConfigDataLocationResolver } private void validateProfile(String profile) { - Assert.hasText(profile, "Profile must contain text"); + Assert.hasText(profile, "'profile' must contain text"); Assert.state(!profile.startsWith("-") && !profile.startsWith("_"), () -> String.format("Invalid profile '%s': must not start with '-' or '_'", profile)); Assert.state(!profile.endsWith("-") && !profile.endsWith("_"), @@ -181,7 +181,8 @@ public class StandardConfigDataLocationResolver if (codePoint == '-' || codePoint == '_' || Character.isLetterOrDigit(codePoint)) { return; } - throw new IllegalStateException(String.format("Invalid profile '%s': must contain only letters or digits or '-' or '_'", profile)); + throw new IllegalStateException( + String.format("Invalid profile '%s': must contain only letters or digits or '-' or '_'", profile)); }); } diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/SpringApplicationTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/SpringApplicationTests.java index dc7a64c734..c3355a355d 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/SpringApplicationTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/SpringApplicationTests.java @@ -258,8 +258,7 @@ class SpringApplicationTests { application.setWebApplicationType(WebApplicationType.NONE); application.setEnvironment(environment); this.context = application.run(); - assertThat(output) - .contains("No active profile set, falling back to 2 default profiles: \"p0\", \"default\""); + assertThat(output).contains("No active profile set, falling back to 2 default profiles: \"p0\", \"default\""); } @Test diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/StandardConfigDataLocationResolverTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/StandardConfigDataLocationResolverTests.java index 61a964a175..9feb13a4f0 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/StandardConfigDataLocationResolverTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/StandardConfigDataLocationResolverTests.java @@ -309,7 +309,7 @@ class StandardConfigDataLocationResolverTests { this.environment.setActiveProfiles("dev-테스트_123"); Profiles profiles = new Profiles(this.environment, this.environmentBinder, Collections.emptyList()); assertThatNoException() - .isThrownBy(() -> this.resolver.resolveProfileSpecific(this.context, location, profiles)); + .isThrownBy(() -> this.resolver.resolveProfileSpecific(this.context, location, profiles)); } @Test @@ -368,8 +368,7 @@ class StandardConfigDataLocationResolverTests { Profiles profiles = new Profiles(this.environment, this.environmentBinder, Collections.emptyList()); assertThatIllegalStateException() .isThrownBy(() -> this.resolver.resolveProfileSpecific(this.context, location, profiles)) - .withMessageStartingWith( - "Invalid profile 'dev*test': must contain only letters or digits or '-' or '_'"); + .withMessageStartingWith("Invalid profile 'dev*test': must contain only letters or digits or '-' or '_'"); } private String filePath(String... components) {