spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.57k stars 38.13k forks source link

parts w/o filename in `Content-Disposition` header are not cleaned from temp folder (skipped by `StandardServletMultipartResolver`) #33511

Closed gbrehmer closed 1 week ago

gbrehmer commented 1 month ago

Affects: 6.1.12 (but can not say since when exactly, older versions are probably also affected) + Jetty 12.0.12

Parts w/o filename in content disposition header are missing in AbstractMultipartHttpServletRequest.getMultiFileMap(). Because of this part.delete() is skipped in the resolver. The relevant logic to filter out such parts is in https://github.com/spring-projects/spring-framework/blob/eff19ec9721381344f0eeacdebfda9da8d8bef07/spring-web/src/main/java/org/springframework/web/multipart/support/StandardMultipartHttpServletRequest.java#L101

the logic to ignore parts which are not part of the filemap is here: https://github.com/spring-projects/spring-framework/blob/eff19ec9721381344f0eeacdebfda9da8d8bef07/spring-web/src/main/java/org/springframework/web/multipart/support/StandardServletMultipartResolver.java#L123

Should we cleanup those parts by ourself? Not sure whether there are other dependencies (why it should not be part of filemap e.g.) but our fix is currently to overwrite the StandardServletMultipartResolver and delete the part nevertheless its contained or not in the filemap)

gbrehmer commented 1 month ago

I see all related code parts are VERY old, but I guess temp folder issues are sometimes hard to find, because normally temp folders are cleaned automatically on next boot. We found it because we limited the space for temp folder to a very low size (500M) when we normally have no big uploads or file generations. To fix the problem it would be enough to remove one of the mentioned checks/filters ;) But of course I have not all required informations/experiences with other servlet containers/use cases/corner cases etc. to make a decision like that

bclozel commented 1 week ago

This is due to #13937. In the meantime, I don't think Resin is still supported here (because there is no Jakarta release available), so I guess we can revert this workaround.