Throw exception if multiple meta-annotations are found by AnnotationUtils [SPR-13015]

[spring-projects-issues]

Juha Komulainen opened SPR-13015 and commented

When an element is annotated with multiple composed annotations that are meta-annotated with the target annotation, the first meta-annotation is returned, and the rest are silently discarded. This seems quite dangerous.

Consider the following example using Spring Security:

@Retention(RUNTIME)
@PreAuthorize("isAuthenticated()")
public @interface RequireAuthenticated { }

@Retention(RUNTIME)
@PreAuthorize("hasRole('FROBNICATION')")
public @interface RequireFrobnication { }

Now consider a method annoted with both:

@RequireAuthenticated
@RequireFrobnication
public void myMethod() { }

Spring Security will query the annotation with AnnotationUtils.getAnnotation(method, PreAuthorize.class), but the call will silently ignore the second annotation and produce a security vulnerability.

Of course there are probably other similar issues in Spring that have nothing to do with Spring Security.

---

Affects: 4.1.6

Reference URL: https://gist.github.com/komu/cadd2e1d3ce9b193fe55

Issue Links:

• Explicit failure if multiple @BootstrapWith annotations are used on the same test [SPR-12602] #17203 Explicit failure if multiple @BootstrapWith annotations are used on the same test

1 votes, 7 watchers

[spring-projects-issues]

Sam Brannen commented

When an element is annotated with multiple annotations that have the meta-annotation class, the first meta-annotation is returned and the rest are silently discarded. This seems quite dangerous.

This is by design. In fact, the Javadoc for AnnotationUtils.getAnnotation(Method, Class<A>) explicitly states:

Get a single Annotation of annotationType from the supplied Method, where the method is either directly annotated or meta-annotated with the annotationType.

Correctly handles bridge Methods generated by the compiler.

Note that this method supports only a single level of meta-annotations. For support for arbitrary levels of meta-annotations, use findAnnotation(Method, Class) instead.

Pay special attention to the phrase "Get a single Annotation". AnnotationUtils.getAnnotation(Method, Class<A>) therefore cannot return multiple annotations.

The fact that you can even declare @PreAuthorize as a meta-annotation is likely purely accidental. @PreAuthorize is declared with @Target({METHOD, TYPE}); however, as far as I know, the ElementType.TYPE declaration is intended to support usage at the class-level but not necessarily on an annotation as a meta-annotation. Furthermore, the Spring Security reference manual only explicitly states that the @AuthenticationPrincipal annotation may be used as a meta-annotation.

In other words, it appears to me that what you are attempting to do is not supported by Spring Security. If it were supported, the Spring Security team would use AnnotatedElementUtils.getAllAnnotationAttributes(AnnotatedElement, String) instead of AnnotationUtils.getAnnotation(Method, Class<A>).

The proper way to achieve your goal of combining multiple boolean SpEL expressions configured via @PreAuthorize is not to use @PreAuthorize as a meta-annotation on multiple composed annotations but rather to declare all boolean expressions in a single declaration of @PreAuthorize (which may be declared either directly on your method or as a single meta-annotation on a composed annotation that is declared directly on your method).

For example, the following will work:

@PreAuthorize("isAuthenticated() and hasRole('FROBNICATION')")
public void myMethod() { }

As will the following:

@Retention(RUNTIME)
@PreAuthorize("isAuthenticated() and hasRole('FROBNICATION')")
public @interface RequireAuthenticatedAndFrobnication {}

@RequireAuthenticatedAndFrobnication
public void myMethod() { }

If you would like for Spring Security to support the use of multiple @PreAuthorize annotations as meta-annotations with the SpEL expressions combined from multiple meta-annotations, you will need to open a JIRA issue against the Spring Security project requesting this as a New Feature.

I hope this clarifies what's going on for you.

Regards,

Sam

[spring-projects-issues]

Sam Brannen commented

Resolving this issue as "Works as Designed".

See previous comments for details.

[spring-projects-issues]

Juha Komulainen commented

Yes, I agree that it's probably purely accidental that you can use @PreAuthorize as a meta-annotation in Spring Security, but that just reinforces my point: the method is too easy to use in a way that produces unintended failure modes. It seems that Spring Security should not be using this method at all, but I guess a similar issue can be made against most other uses of this method. So using Spring Security as an example was perhaps unfortunate and sidetracked this issue.

When considering the clients of the method (whether Spring Security or something completely different), it seems clear to me that the caller is either prepared to handle multiple annotations (in which case the caller would use another method to begin with), or is expecting only one matching annotation to be defined (in which case the presence of multiple annotations is a violation of invariants and should be an exception). I fail to see the use case in which it would be sensible to return one annotation arbitrarily and ignore the others.

When it comes to the documentation, it's not explicit about what happens when there are multiple matching annotations. It says that it will return "the matching annotation", but it is completely silent about the situation with multiple matches. So what I'm arguing is that the semantics of this change (i.e., throw an exception on multiple matches) would be completely compatible with the currently documented contract.

However, I can see that changing the semantics of the method might not be feasible. It is a breaking change after all, even though in almost all cases where it would break it would probably uncover bugs in existing code. So if you're unwilling to change this method, I plead you to consider deprecating this method and adding a safe alternative.

[spring-projects-issues]

Sam Brannen commented

Juha Komulainen, you make some valid points.

I fail to see the use case in which it would be sensible to return one annotation arbitrarily and ignore the others.

I tend to agree. That's why I created a fix for #17203. That's actually the same scenario that you describe here but limited to look-ups for @BootstrapWith in the testing framework.

When it comes to the documentation, it's not explicit about what happens when there are multiple matching annotations. It says that it will return "the matching annotation", but it is completely silent about the situation with multiple matches.

Good point! I'll update the Javadoc to consistently mention that the search algorithms halt once the first matching annotation has been found.

So what I'm arguing is that the semantics of this change (i.e., throw an exception on multiple matches) would be completely compatible with the currently documented contract.

You are correct that changing the semantics of the method would be a breaking change. So with that in mind, I doubt that we can make such a change for existing methods; however, we will take your points into consideration, and I'll re-open this issue in order to track this.

Cheers,

Sam

[spring-projects-issues]

Sam Brannen commented

I have changed the title of this issue to reflect the scope more accurately.

[spring-projects-issues]

Sam Brannen commented

Improved documentation as described in GitHub commit 52153bd:

Document search scope in Ann*[Element]Utils

This commit improves the documentation for AnnotationUtils and AnnotatedElementUtils by explaining that the scope of most annotation searches is limited to finding the first such annotation, resulting in additional such annotations being silently ignored.

[spring-projects-issues]

Juha Komulainen commented

Thanks for reconsidering the issue!

One more thing to consider: I don't believe that Java specifies any particular ordering (or even stable ordering between calls) for AnnotatedElement.getAnnotations, making AnnotationUtils.getAnnotation nondeterministic in presence of multiple matching annotations. This could cause program to behave differently especially when running in different virtual machines.

There's even an issue in OpenJDK about the ordering being unstable. As far as I understand they are not implying that the unstable ordering of OpenJDK's getAnnotations would be against Java specification -- just that it's inconvenient because it causes random failures in javac's tests and should be fixed because of that. So in any case stable ordering does not seem to be something one can rely on.