Smidge icon indicating copy to clipboard operation
Smidge copied to clipboard

GetCombinedStreamAsync > GetRequiredFileInfo exception should be handled gracefully to prevent DDoS

Open LennardF1989 opened this issue 1 year ago • 2 comments
trafficstars

Upon inspecting logs of an Umbraco website, we saw an unhandled exception similar to below occur quite often:

System.IO.FileNotFoundException: No such file exists 202303303/019e3e65.js (mapped from 202303303/019e3e65.js)

File name: '202303303/019e3e65.js'
   at Smidge.InMemory.MemoryCacheFileSystem.GetRequiredFileInfo(String filePath)
   at System.Linq.Enumerable.SelectListPartitionIterator`2.MoveNext()
   at System.Linq.Enumerable.WhereSelectEnumerableIterator`2.ToList()
   at Smidge.Controllers.SmidgeController.GetCombinedStreamAsync(IEnumerable`1 files, BundleContext bundleContext)
   at Smidge.Controllers.SmidgeController.Composite(CompositeFileModel file)

After investigation, this is relatively easy to trigger by taking the URL and purposely including a wrong file-id, for example: https://domain.tld/sc/598e8198.1234abcd.js.v202303303 where 1234abcd is a non-existing file.

This code currently throws an exception, but is never properly handled, causing a 500: https://github.com/Shazwazza/Smidge/blob/994e9451b9838e74669fd47b29489509dbfe92c7/src/Smidge.InMemory/MemoryCacheFileSystem.cs#L34

(This is also the case for other FileSystem types).

This particular line seems to be the reason for the actual 500 happening: https://github.com/Shazwazza/Smidge/blob/994e9451b9838e74669fd47b29489509dbfe92c7/src/Smidge/Controllers/SmidgeController.cs#L189-L191

GetCombinedStreamAsync tries to do an if-Exists check, but you can't check .Exists on something that is not there. A possible solution instead of throwing an Exception is just logging that it occured, but returning a default FileInfo.

While this particular issue may seem like a non-issue and only occurs when caching is out of sync with the actual smidge files, being able to consistently trigger a 500 can be considered a DDoS vulnerability. As a lot of exceptions in a row will make IIS go into panic mode and restart the website.

LennardF1989 avatar Dec 18 '23 13:12 LennardF1989

Hi, thanks for the report. Any chance can make a PR to resolve this? I'd be happy to log this instead of throw.

Shazwazza avatar Dec 18 '23 23:12 Shazwazza

Sure, I can take a look :)

LennardF1989 avatar Dec 19 '23 10:12 LennardF1989