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

Reject null and empty SpEL expressions #30371

Closed
ryan-c-wicks opened this issue Apr 25, 2023 · 3 comments
Closed

Reject null and empty SpEL expressions #30371

ryan-c-wicks opened this issue Apr 25, 2023 · 3 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: backported An issue that has been backported to maintenance branches type: enhancement A general enhancement
Milestone

Comments

@ryan-c-wicks
Copy link

ryan-c-wicks commented Apr 25, 2023

We have upgraded from Spring Framework 5.2.23 to 5.2.24, and now we are getting some NullPointerExceptions.

Looking in the stack traces we find that the issue is in InternalSpelExpressionParser.

With 5.2.24 there is now a 'checkExpressionLength(expressionString);' which calls:

	private void checkExpressionLength(String string) {
		if (string.length() > MAX_EXPRESSION_LENGTH) {
			throw new SpelEvaluationException(SpelMessage.MAX_EXPRESSION_LENGTH_EXCEEDED, MAX_EXPRESSION_LENGTH);
		}
	}

If string is null then string.length() results in a NullPointerException.

Is this the intended behavior of checkExpressionLength(), or is this a bug?

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 25, 2023
@quaff
Copy link
Contributor

quaff commented Apr 25, 2023

Why string is null?

@bclozel bclozel added the in: core Issues in core modules (aop, beans, core, context, expression) label Apr 25, 2023
@sbrannen sbrannen changed the title NullPointerException in InternalSpelExpressionParser.java (no null check on String) SpEL: NullPointerException in InternalSpelExpressionParser Apr 25, 2023
@sbrannen sbrannen changed the title SpEL: NullPointerException in InternalSpelExpressionParser NullPointerException if SpEL expression is null Apr 25, 2023
@sbrannen
Copy link
Member

sbrannen commented Apr 25, 2023

The expressionString should never be null. If it is null, that is a user error.

org.springframework.expression.ExpressionParser.parseExpression(String) and related methods accepting the expressionString do not specify it as @Nullable.

Interestingly enough, prior to #30325, the following test passed.

	@Test
	void nullExpression() {
		ExpressionParser parser = new SpelExpressionParser();
		String expression = null;
		Expression expr = parser.parseExpression(expression);
		Object result = expr.getValue();
		assertThat(result).isNull();
	}

However, that was not by design. Rather, that was by accident.

Supplying a null expression internally resulted in an expression created by concatenating null with "\0" in the constructor for org.springframework.expression.spel.standard.Tokenizer.. Thus, supplying null as the expression was effectively equivalent to supplying "null" as the expression.

We currently do not have any planned releases for 5.2.x. Thus if you are encountering this issue with Spring Framework 5.2.x, please ensure that you do not supply a null reference as the expression string to parse. Alternatively, if you rely on the behavior of a null expression for some reason, you could alter your code to supply "null" as the expression string instead of a null reference.

For Spring Framework 5.3.x and 6.x, I think we should introduce an explicit Assert.notNull(...) or Assert.hasText(...) check for the user-supplied expression string.

@sbrannen sbrannen self-assigned this Apr 25, 2023
@sbrannen sbrannen added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Apr 25, 2023
@sbrannen
Copy link
Member

In light of the above, I am repurposing this ticket to fail early if null or an empty string is provided as the SpEL expression to parse.

@sbrannen sbrannen added this to the 6.0.9 milestone Apr 25, 2023
@sbrannen sbrannen changed the title NullPointerException if SpEL expression is null Reject null and empty SpEL expressions Apr 25, 2023
@github-actions github-actions bot added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-5.3.x labels Apr 25, 2023
sbrannen added a commit that referenced this issue Apr 25, 2023
Prior to gh-30325, supplying a null reference for a SpEL expression was
effectively equivalent to supplying the String "null" as the
expression. Consequently, evaluation of a null reference expression
always evaluated to a null reference. However, that was accidental
rather than by design.

Due to the introduction of the checkExpressionLength(String) method in
InternalSpelExpressionParser (in conjunction with gh-30325), an attempt
to evaluate a null reference as a SpEL expression now results in a
NullPointerException.

To address both of these issues,
TemplateAwareExpressionParser.parseExpression() and
SpelExpressionParser.parseRaw() now reject null and empty SpEL
expressions.

See gh-30371
Closes gh-30373
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: backported An issue that has been backported to maintenance branches type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

5 participants