-
Notifications
You must be signed in to change notification settings - Fork 37.7k
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
Introduce hook in ResourceEntityResolver
to override fallback to remote access for DTD/XSD resolution
#29662
Comments
In which scenarios are you using |
It happens automatically when you happen to, for example, reference a schema with an import in it. If you accomodate the import in the appropriate way via spring.schemas, it works, but an accidental mistake on the classpath can leave the import un-hijacked, so to speak, and it falls through to remotely attempt resolution. That's how we noticed the behavior. We've frequently had to react to accidental remote fetches over the years, this time I just happened to have the time to bottom out the logic and locate the spot I needed to replace, but once I realized what it was doing, I felt it was worth mentioning. I don't argue that it's unreasonable to allow remote access (well, I would, but I'm not going to argue it here), but as a default, it didn't seem ideal to me, and there also didn't appear to be any direct means of preventing it without actually copying the Spring class and injecting the "part" of it that isn't doing that. |
I guess to be fair, there's another consideration here, which is that the documentation and behavior of Java's XML parser, with regard to the EntityResolver interface, is that returning null results in the systemId being directly resolved anyway. So another way to frame this I guess is, if all you're doing is fetching the systemId remotely, that's effectively what Java's doing already. So it's plausible to say "hey, write less code". So having said that, if you were to consider this behavior suboptimal and change it, you'd really have to change it to throw an exception, since removing the section of code outright won't change the behavior anyway. |
We could log a warning (or info statement) whenever we fall back to remote access there. After all, accidental remote access is unfortunate for cloud deployment, even more so for native images where one certainly intends to include all relevant resources. |
Another possible suggestion: rewrite the ResourceEntityResolver slightly to expose a protected method for doing the remote fetch step, so subclassers could easily block that step without having ot duplicate the earlier section of code with the local resolution. |
Let's go with a combination of the two proposals.
|
ResourceEntityResolver
to override fallback to remote access for DTD/XSD resolution
This commit extracts the DTD/XSD remote lookup fallback from the resolveEntity method into a protected method. A warn logging statement is added to the extracted fallback in order to make it clear that remote lookup happened. Overriding the protected method would allow users to avoid this fallback entirely if it isn't desirable, without the need to duplicate the local resolution code. Fixes spring-projectsgh-29662.
It appears the hook/fix for this was done without allowing the protected method to raise an IOException, so there's no way to properly signal back "don't do this" since returning null will just fall through. |
@scantor since the protected method is expected to be overridden, isn't it possible cover that case by having the override throw a |
I don't know what that will do exactly, since your base class doesn't catch that. It may be more fatal than it should otherwise be, but it will definitely cause non-localized logging and handling of the condition. |
the fact that the fallback is hit means the I'm not sure we can add the |
Ok, I did run a quick trial and it lands the exception in more or less the same try/catch block as it did before, so I can live with that answer, I just wanted to note the inconsistency. |
This is definitely true on Spring 6.0.2, I suspect it's equally true for 5.3.x and likely everything going back many years...
I would not characterize this as a security issue, but it seems like a poor default decision to allow the org.springframework.beans.factory.xml.ResourceEntityResolver class to fall through to remote http(s) resolution of XML entities.
I would think at minimum that ought to be off by default, or certainly an optional behavior at least, but there's no obvious way I saw to override that behavior save for duplicating the class and injecting the replacement into our custom bean defintion reader.
There's also a nasty bug in Java's XML parser that causes it to handle null coming back from an EntityResolver by just doing its own resolution, but throwing an exception out of the resolveEntity call does prevent that. So I think it would be a good improvement to either default throw, or at least provide some kind of option to do so.
The text was updated successfully, but these errors were encountered: