search

mozilla / connect-cachify

Express connect middleware to provide easy frontend caching for Node.js
Mozilla Public License 2.0
131 stars 21 forks source link

Simplify 'invalid cashes' verification #38

Closed pirxpilot closed 11 years ago

pirxpilot commented 11 years ago

Instead of checking cachify prefix against the md5 hash of the file content we just check if the said prefix is among the ones that we previously generated.

It provides a safeguard against someone maliciously attempting to clog the downstream cache by sending us a great number of invalid URLs and has an added benefit of beeing much simpler than 'recalculate the hash' method.

Fixes #37

pirxpilot commented 11 years ago

Hi @ozten. Thanks for looking at my code.

You wrote I don't believe this patch fixes Issue#31 mentioned in the code which is removed. I disagree. Issue #31 was about cache poisoning - that's a not a precise term, but as far as I can glean from @lloyd's description he was concerned about a malicious 3rd party generating a huge number of requests, that pre-0.14.0 version would happily fulfill thus clogging any downstream cache with a single copy of the resource. Any fix for #31 needs to reduce the space of valid URLs so that attacker can't fill the cache. And with #37 we have a very reasonable protection against that for a fraction of a price of recalculating the hash for every request.

You also wrote It's possible for a deployment error or some other reason that a file gets updated on the file system, but the service isn't restarted However connect-cachify in its current state (with or without my patch) simply does not work properly unless you require the restart after each file change. Look at hashify implementation and how the _cache variable is used. We only generate a hashed prefix once. The browser that already visited the site won't even ask for a new version of the file (that's why we gave it really long expiration after all). The fact that a new visitor browser might get either 404 (with #32) or a new version of the file (with #38) is largely irrelevant here. The bottom line is - if your deployment process allows for such 'errors' you have no business using connect-cachify at all.

Most importantly though I we need a fix for #37 (BTW any comments on that issue would be appreciated). The code that I removed as a part of #38 breaks reasonable deployment scenarios. Having both checks would not solve anything.

ozten commented 11 years ago

We had a production issue where users were reporting weird behavior. It was very hard to diagnose, but we eventually found this "cache poisoning" was new code being served up to new visitors under an old hash.

The post mortem was to send a 404 if we detect this situation, to fail early. A spike of 404s could help us see something was wrong with the deployment.

The term cache poisoning is misleading in Issue#31, it doesn't involve a malicious party making a large number of requests. That is an interesting attack vector, to be sure, but not what we were talking about.

However connect-cachify in its current state (with or without my patch) simply does not work properly unless you require the restart after each file change.

Yep

The bottom line is - if your deployment process allows for such 'errors' you have no business using connect-cachify at all.

Deployment software can have bugs. We should protect ourselves from them.

pirxpilot commented 11 years ago

Looks like I am trying to solve a problem, which you don't have, and vice versa ;-) I still think verifying deployment site integrity should be a part of some other software module, but clearly I don't have a full picture. Feel free to take whatever part of my patch you find useful.

In case someone wanders here trying to find a solution for #37 or a similar problem: check out connect-cachify-static middleware.