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

Escaping logic in PlaceholderParser does not handle escaped backslashes #34315

Closed
bitb4ker opened this issue Jan 24, 2025 · 7 comments
Closed
Labels
status: declined A suggestion or change that we don't feel we should currently apply

Comments

@bitb4ker
Copy link

Up until spring-boot 3.3.8, and escaped backslash followed by a placeholder like:

prop1=value1  
prop2=value2\\${prop1}  

Was resulting prop2 resolving to:

value2\value1

Starting with spring-boot 3.4.0, the result is:

value2value1

Doubling the backslashes like this:

prop1=value1  
prop2=value2\\\\${prop1}  

Results in:

value2${prop1}

This is due to the escaping logic introduced in org.springframework.util.org.springframework.util.PlaceholderParser which does two passes of PlaceholderParser.SimplePlaceholderPart.resolveRecursively(PartResolutionContext, String), each calling PlaceholderParser.parse(String, boolean) in which the escaping logic is implemented.
This last method does not handle escaping the escape character, causing the issue.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 24, 2025
@bclozel bclozel transferred this issue from spring-projects/spring-boot Jan 24, 2025
@snicoll
Copy link
Member

snicoll commented Jan 24, 2025

The parser in 6.2 has been rewritten to fix, amongst other things, #9628. Unfortunately, this means that if the placeholder is escaped, it's rendered as is (i.e. not evaluated).

I wrote a test:

@Test
void gh34315() {
    Properties properties = new Properties();
    properties.setProperty("prop1", "value1");
    properties.setProperty("prop2", "value2\\${prop1}");
    PlaceholderParser parser = new PlaceholderParser("${", "}", ":", '\\', true);
    assertThat(parser.replacePlaceholders("${prop2}", properties::getProperty)).isEqualTo("value2${prop1}");
}

and it passes. I've also added those two properties in an empty Spring 6.2 app and it resolved the same way. Can you share a sample that actually fails the way you've described?

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Jan 24, 2025
@bitb4ker
Copy link
Author

bitb4ker commented Jan 24, 2025

props-spring-3.3.zip

props-spring-3.4.zip

To run:

gradlew bootRun

The 3.3 one shows the expected behavior, the 3.4 shows what I described as the broken behavior in both '\' and '\\' in the original post.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 24, 2025
@snicoll
Copy link
Member

snicoll commented Jan 26, 2025

I got it now. The problem is PropertySourcesPlaceholderConfigurer getting in the way of the placeholder resolution. It does its own round, removes the backslash to render the placeholder as is. As a result, the parser continues its work and sees a new placeholder to resolve.

@snicoll
Copy link
Member

snicoll commented Jan 27, 2025

The issue as reported can't be fixed without re-introducing the bug that we fixed. The side effect that was found as part of the report is going to be handled by #34326.

For that case above, you'll need to restructure your configuration to move the backlash elsewhere to avoid the escaping, something like:

prop1=\\value1
prop2=value2${prop1}

@snicoll snicoll closed this as not planned Won't fix, can't repro, duplicate, stale Jan 27, 2025
@snicoll snicoll added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels Jan 27, 2025
@bitb4ker
Copy link
Author

bitb4ker commented Jan 27, 2025

Couldn't we introduce a way to escape the escape character?
This is essentially the root of the issue I raised.
Why does it break previous fixes?

@snicoll
Copy link
Member

snicoll commented Jan 28, 2025

Everything is possible, I guess but we’re not keen to make the parser more complex at this time.

@bitb4ker
Copy link
Author

My issue is that this broke something that was working for years.
We have dozen of config files now broken because we build windows path and windows logins with placeholders.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

3 participants