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.

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

Tighten rules around profile naming #43176

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

YangSiJun528
Copy link

I added the validateProfiles() method to enforce stricter rules for Spring profile names.

Currently, only alphanumeric characters, hyphens (-), and underscores (_) are allowed. Please let me know if additional characters should be permitted - I'll update accordingly.

I've verified validation works in application.properties and @ActiveProfiles.

While @Profile doesn't perform validation, this seems acceptable since invalid profiles will fail during activation. Perhaps we could explore adding warnings as a potential enhancement in the future.

Note: this change may break applications using non-conforming profile names.

Fixes gh-34062

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 15, 2024
@mhalbritter mhalbritter changed the title Add validateProfiles for tighten rules around profile naming Tighten rules around profile naming Nov 20, 2024
@mhalbritter mhalbritter added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 20, 2024
@mhalbritter mhalbritter added this to the 3.5.x milestone Nov 20, 2024
@@ -163,6 +165,21 @@ private Set<StandardConfigDataReference> getProfileSpecificReferences(ConfigData
return references;
}

private void validateProfiles(Profiles profiles) {
Pattern validProfilePattern = Pattern.compile("^[a-zA-Z0-9_\\-]+$");
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be simplified to Pattern validProfilePattern = Pattern.compile("^[\\w-]+$");.

@mhalbritter
Copy link
Contributor

mhalbritter commented Nov 20, 2024

Hello! First of all, thank you for the PR!

This currently breaks:

  • org.springframework.boot.test.context.SpringBootContextLoaderTests#activeProfileWithComma
  • org.springframework.boot.test.context.SpringBootContextLoaderTests#testPropertyValuesShouldTakePrecedenceWhenInlinedPropertiesPresentAndProfilesActive

I think we can remove activeProfileWithComma and use a profile without a comma in testPropertyValuesShouldTakePrecedenceWhenInlinedPropertiesPresentAndProfilesActive.

It also breaks:

  • org.springframework.boot.SpringApplicationTests#logsActiveProfilesWithoutProfileAndMultipleDefaults: Here we can remove the comma.
  • org.springframework.boot.SpringApplicationTests#logsActiveProfilesWithMultipleProfiles: Same here.

@mhalbritter mhalbritter added status: noteworthy A noteworthy issue to call out in the release notes for: team-meeting An issue we'd like to discuss as a team to make progress labels Nov 20, 2024
@mhalbritter
Copy link
Contributor

mhalbritter commented Nov 20, 2024

Flagging this for team meeting as I want to talk about if the positive list is a good approach. Right now, it's very "western centric" by allowing only A-Z and digits.

I wonder if we should approach it like this:

	private void validateProfiles(Profiles profiles) {
		for (String profile : profiles) {
			validateProfile(profile);
		}
	}

	private void validateProfile(String profile) {
		Assert.notNull(profile, "Profile must not be null");
		Assert.hasText(profile, "Profile must not be empty");
		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));
		});
	}

This would also block ,, ( etc. but lets more non-ascii letters and digits through.

@YangSiJun528
Copy link
Author

YangSiJun528 commented Nov 24, 2024

I incorporated the feedback provided. @mhalbritter

  • Fixed the broken tests
  • Fixed validateProfiles to allow non-ascii letters

@YangSiJun528
Copy link
Author

YangSiJun528 commented Dec 4, 2024

To test empty string and null cases for active profile, I used mock(Environment.class) because AbstractEnvironment#setActiveProfiles() blocks direct testing. However, using mocks for testing is challenging because Profiles is used in many places.

Is there a better way to handle this testing?

I've included example tests in a demo branch (f4ed4d1).

@philwebb philwebb removed the for: team-meeting An issue we'd like to discuss as a team to make progress label Dec 4, 2024
@YangSiJun528

This comment was marked as outdated.

@mhalbritter

This comment was marked as outdated.

@YangSiJun528

This comment was marked as outdated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: noteworthy A noteworthy issue to call out in the release notes type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tighten rules around profile naming
5 participants