spring-projects / spring-boot

Spring Boot helps you to create Spring-powered, production-grade applications and services with absolute minimum fuss.
https://spring.io/projects/spring-boot
Apache License 2.0
75.34k stars 40.72k forks source link

Spring Boot 3.2.2 breaks SpEL interpretation #39261

Closed aholland closed 10 months ago

aholland commented 10 months ago

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

bclozel commented 10 months ago

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

Thanks!

aholland commented 10 months ago

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.

bclozel commented 10 months ago

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.

philwebb commented 10 months ago

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 commented 10 months ago

It may be duplicate of https://github.com/spring-projects/spring-framework/issues/32087.

wilkinsona commented 10 months ago

Thanks, @quaff.

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

aholland commented 10 months ago

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
wilkinsona commented 10 months ago

Thanks very much, @aholland. That confirms that this is a duplicate of https://github.com/spring-projects/spring-framework/issues/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.

aholland commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

@scottfrederick thanks!