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

Spring Boot 3.2.2 breaks SpEL interpretation #39261

Closed
aholland opened this issue Jan 22, 2024 · 13 comments
Closed

Spring Boot 3.2.2 breaks SpEL interpretation #39261

aholland opened this issue Jan 22, 2024 · 13 comments
Labels
for: external-project For an external project and not something we can fix status: duplicate A duplicate of another issue

Comments

@aholland
Copy link

My system compiles with zero warnings and all tests pass, under 3.2.1. But if I change just one character, ie 3.2.1 to 3.2.2 in my build.gradle file, the system still compiles with no warnings but I get the following error running my tests:

java.lang.IllegalArgumentException: Failed to evaluate expression 'hasAnyAuthority('SYSTEM') or (hasAnyAuthority('ADMIN', 'DEVICE_MANAGER') and #remoteCommand.verifyDevicesBelongToMerchant(principal.merchant))'
...
Caused by: org.springframework.expression.spel.SpelEvaluationException: EL1011E: Method call: Attempted to call method verifyDevicesBelongToMerchant(co.acme.domain.Merchant) on null context object

I've tried forcing spring-core and spring-web back to 6.1.2 in my dependencies section, but it makes no difference. I am not sure which part of Spring governs SpEL, but this looks like a bug to me.

Here is the class containing the SpEL in question:

package co.acme.repository.device;

import co.acme.domain.device.RemoteCommand;
import jakarta.persistence.LockModeType;
import java.util.UUID;
import org.springframework.data.jpa.repository.Lock;
import org.springframework.data.rest.core.annotation.RestResource;
import org.springframework.security.access.prepost.PreAuthorize;
import org.springframework.stereotype.Repository;

@Repository
public interface RemoteCommandRepository extends org.springframework.data.repository.Repository<RemoteCommand, UUID> {
  @PreAuthorize(
    "hasAnyAuthority('SYSTEM') or (hasAnyAuthority('ADMIN', 'DEVICE_MANAGER') and #remoteCommand.verifyDevicesBelongToMerchant(principal.merchant))"
  )
  RemoteCommand save(RemoteCommand remoteCommand);

  @RestResource(exported = false)
  @Lock(LockModeType.PESSIMISTIC_WRITE)
  Iterable<RemoteCommand> findAllBySentAtIsNull();
}

Please bear in mind that this works perfectly with Spring Boot 3.2.1

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 22, 2024
@bclozel
Copy link
Member

bclozel commented Jan 22, 2024

I can't think of a recent change or regression here. Can you share a minimal sample application that reproduces the problem?

Thanks!

@bclozel bclozel added the status: waiting-for-feedback We need additional information before we can continue label Jan 22, 2024
@wilkinsona wilkinsona changed the title Spring Boot 6.2.2 breaks SpEL interpretation Spring Boot 3.2.2 breaks SpEL interpretation Jan 22, 2024
@aholland
Copy link
Author

aholland commented Jan 22, 2024

I can't think of a recent change or regression here. Can you share a minimal sample application that reproduces the problem?

Thanks!

I'd like to and if you have any guidance on paring down a large API-server Spring Boot application to a minimal example which doesn't expose too much company code, let me know. This is my first time working with Spring.

@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 22, 2024
@bclozel
Copy link
Member

bclozel commented Jan 22, 2024

You can start from a a simple application created on https://start.spring.io with the relevant dependencies (Spring Data REST and Spring Security from what I can see) and add bit by bit classes to reproduce the problem. If you can't reproduce the issue when you've reached the point where the code snippet shown above is mostly there, this means that you'll need to investigate a bit more.

@bclozel bclozel added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Jan 22, 2024
@philwebb
Copy link
Member

You might also try adding a exception breakpoint for SpelEvaluationException to see if debugging can shed any insight. In these cases I often work back up the stacktrace and try to add a breakpoint somwhere before the exception is thrown, then debug the app again with the previous version to see why the exception isn't being thrown in that version.

@quaff
Copy link
Contributor

quaff commented Jan 23, 2024

It may be duplicate of spring-projects/spring-framework#32087.

@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 23, 2024
@wilkinsona
Copy link
Member

Thanks, @quaff.

@aholland, can you try with all of Spring Framework's modules, and spring-aop in particular, downgraded to 6.1.2?

@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Jan 23, 2024
@aholland
Copy link
Author

aholland commented Jan 23, 2024

The significant difference between 3.2.1 and 3.2.2 comes in a class called AopUtils:
In 3.2.1, spring-aop-6.1.2.jar!org.springframework.aop.support.AopUtils#getMostSpecificMethod:203, calls spring-core-6.1.2.jar!org.springframework.core.BridgeMethodResolver#findBridgedMethod:68,
which checks bridgeMethod.isBridge() (false) and so returns bridgeMethod:

68	public static Method findBridgedMethod(Method bridgeMethod) {
69		if (!bridgeMethod.isBridge()) {
70			return bridgeMethod;

In 3.2.2 however, spring-aop-6.1.3.jar!org.springframework.aop.support.AopUtils#getMostSpecificMethod:204, calls spring-core-6.1.3.jar!org.springframework.core.BridgeMethodResolver#getMostSpecificMethod:88 (shown below)
This then calls resolveBridgeMethod on line 94 and on line 95 compares targetClass (== class org.springframework.data.jpa.repository.support.SimpleJpaRepository) with bridgeMethod.getDeclaringClass() (== interface co.givealittle.repository.UserRepository), setting localBridge to false, and so lands up using the method filter on line 105. That uses isBridgedCandidateFor, line 127, which find that both Spring's SimpleJpaRepository and the aforementioned UserRepository have a method called save with exactly one parameter each. This becomes the only candidate method and is selected by line 108, which is how we end up with a MethodBasedEvaluationContext for the wrong method.

With the wrong method in hand, we eventually fail to find a suitable accessor in the loop at spring-expression-6.1.3-sources.jar!org.springframework.expression.spel.ast.PropertyOrFieldReference:208 and proceed to throw the exception on 224.

88	public static Method getMostSpecificMethod(Method bridgeMethod, @Nullable Class<?> targetClass) {
89		Method specificMethod = ClassUtils.getMostSpecificMethod(bridgeMethod, targetClass);
90		return resolveBridgeMethod(specificMethod,
91				(targetClass != null ? targetClass : specificMethod.getDeclaringClass()));
92	}
93
94	private static Method resolveBridgeMethod(Method bridgeMethod, Class<?> targetClass) {
95		boolean localBridge = (targetClass == bridgeMethod.getDeclaringClass());
.		if (!bridgeMethod.isBridge() && localBridge) {
.			return bridgeMethod;
.		}

		Object cacheKey = (localBridge ? bridgeMethod : new MethodClassKey(bridgeMethod, targetClass));
		Method bridgedMethod = cache.get(cacheKey);
		if (bridgedMethod == null) {
			// Gather all methods with matching name and parameter size.
			List<Method> candidateMethods = new ArrayList<>();
105			MethodFilter filter = (candidateMethod -> isBridgedCandidateFor(candidateMethod, bridgeMethod));
			ReflectionUtils.doWithMethods(targetClass, candidateMethods::add, filter);
			if (!candidateMethods.isEmpty()) {
108				bridgedMethod = (candidateMethods.size() == 1 ? candidateMethods.get(0) :
						searchCandidates(candidateMethods, bridgeMethod, targetClass));
			}
			if (bridgedMethod == null) {
				// A bridge method was passed in but we couldn't find the bridged method.
				// Let's proceed with the passed-in method and hope for the best...
				bridgedMethod = bridgeMethod;
.			}
.			cache.put(cacheKey, bridgedMethod);
.		}
118		return bridgedMethod;
119	}

	/**
	 * Returns {@code true} if the supplied '{@code candidateMethod}' can be
	 * considered a valid candidate for the {@link Method} that is {@link Method#isBridge() bridged}
	 * by the supplied {@link Method bridge Method}. This method performs inexpensive
	 * checks and can be used to quickly filter for a set of possible matches.
	 */
127	private static boolean isBridgedCandidateFor(Method candidateMethod, Method bridgeMethod) {
128		return (!candidateMethod.isBridge() &&
129				candidateMethod.getName().equals(bridgeMethod.getName()) &&
130				candidateMethod.getParameterCount() == bridgeMethod.getParameterCount());
131	}

image image

@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 23, 2024
@wilkinsona
Copy link
Member

wilkinsona commented Jan 23, 2024

Thanks very much, @aholland. That confirms that this is a duplicate of spring-projects/spring-framework#32087 as @quaff suspected. If you have time, please try Spring Framework 6.1.4-SNAPSHOT (available from https://repo.spring.io/snapshot) and confirm that it fixes the problem for you.

@wilkinsona wilkinsona closed this as not planned Won't fix, can't repro, duplicate, stale Jan 23, 2024
@wilkinsona wilkinsona added status: duplicate A duplicate of another issue for: external-project For an external project and not something we can fix and removed status: waiting-for-triage An issue we've not yet triaged status: feedback-provided Feedback has been provided labels Jan 23, 2024
@aholland
Copy link
Author

@wilkinsona Yes, that fixes it. I added the repo and the first two implementation lines below to my build.gradle, and now all my tests are passing again. I assume that's not a safe combination of dependencies though. Guess I'll wait for 6.1.4 or—can I assume?—Spring Boot 3.2.3.

dependencies {

    implementation 'org.springframework:spring-core:6.1.4-SNAPSHOT'
    implementation 'org.springframework:spring-aop:6.1.4-SNAPSHOT'

    // managed (ie version determined by plugin `io.spring.dependency-management`)
    annotationProcessor 'org.projectlombok:lombok'
    annotationProcessor 'org.springframework.boot:spring-boot-configuration-processor' // purely for IDEA autocomplete
    compileOnly 'org.projectlombok:lombok'
    implementation 'org.springframework.boot:spring-boot-starter-data-jpa'
    implementation 'org.springframework.boot:spring-boot-starter-data-rest'
    implementation 'org.springframework.boot:spring-boot-starter-jersey'
    implementation 'org.springframework.security:spring-security-config'
    implementation 'org.springframework.security:spring-security-oauth2-jose'
    implementation 'org.springframework.security:spring-security-oauth2-resource-server'
    runtimeOnly 'com.h2database:h2'
    runtimeOnly 'com.mysql:mysql-connector-j'
    runtimeOnly 'org.apache.httpcomponents.client5:httpclient5'
    runtimeOnly 'org.flywaydb:flyway-mysql'
    runtimeOnly 'org.springframework.boot:spring-boot-starter-actuator'
    runtimeOnly 'org.springframework.security:spring-security-data'
    testAnnotationProcessor 'org.projectlombok:lombok'
    testCompileOnly 'org.projectlombok:lombok'
    testImplementation 'org.springframework.boot:spring-boot-starter-test'
    testRuntimeOnly 'org.junit.platform:junit-platform-launcher'
    testRuntimeOnly 'org.springframework.security:spring-security-test'

    // fixed (ie version coordinate is a specific number)
    implementation 'com.auth0:auth0:2.10.0'
    implementation 'com.auth0:java-jwt:4.4.0'
    implementation 'com.devskiller.friendly-id:friendly-id:1.1.0'
    implementation 'com.google.guava:guava:33.0.0-jre'
    implementation 'com.mailjet:mailjet-client:5.2.5'
    implementation 'com.opencsv:opencsv:5.9' // vulnerability Cx78f40514-81ff
    implementation 'com.stripe:stripe-java:20.136.0'
    implementation 'net.javacrumbs.shedlock:shedlock-provider-jdbc-template:5.10.2'
    implementation 'net.javacrumbs.shedlock:shedlock-spring:5.10.2'
    implementation 'org.apache.commons:commons-vfs2:2.9.0'
    implementation 'org.reflections:reflections:0.10.2'
    testImplementation 'com.github.CautionYourBlast:java-faker:1.0.3'
}

@wilkinsona
Copy link
Member

I would not recommend using snapshots for anything important.

Spring Boot 3.2.3, to be released next month, will upgrade to Spring Framework 6.1.4. In the meantime, you may want to downgrade Spring Framework to 6.1.2 or even 6.1.1 (the latter to avoid being vulnerable to https://spring.io/security/cve-2024-22233/).

@aholland
Copy link
Author

aholland commented Jan 24, 2024

Thanks @wilkinsona . A final question if you possibly have some time - given the build.gradle you see above, what is the best way to revert to Spring Framework 6.1.1? Do I just change to an older version of Spring Boot (in the plugins block, not shown), or, if I do it in dependencies, which of the org.springframework._ dependencies above do you mean when you write "...may want to downgrade Spring Framework to ..."? Do I just put :6.1.2 after all of them? And doesn't mixing and matching risk things like method-not-found exceptions at runtime? To be clear, we intend to wait until Spring Boot 3.2.3 as our actual course of action but I'd like to be clear on what is meant by, and covered by, the term "Spring Framework".

@scottfrederick
Copy link
Contributor

given the build.gradle you see above, what is the best way to revert to Spring Framework 6.1.1?

The Gradle plugin documentation shows how to override managed dependency versions. In your case, you'd add something like this to your build.gradle.

ext['spring-framework.version'] = '6.1.1`

doesn't mixing and matching risk things like method-not-found exceptions at runtime?

It is generally safe to override the version of a managed Spring dependency to a different patch level (for example, from 6.1.3 to 6.1.2). Spring projects try very hard not to change public APIs in patch releases. Changing to an older minor version (for example, from 6.1.3 to 6.0.16) would have more risk of API problems.

@aholland
Copy link
Author

@scottfrederick thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: external-project For an external project and not something we can fix status: duplicate A duplicate of another issue
Projects
None yet
Development

No branches or pull requests

7 participants