breezewish / express-minify

Automatically minify and cache your javascript and css files.
https://npmjs.org/package/express-minify
MIT License
86 stars 18 forks source link

use SHA256 instead of SHA1 #30

Closed breezewish closed 7 years ago

Martii commented 8 years ago

@SummerWish Have a couple of questions for you.

  1. https://github.com/SummerWish/express-minify/blob/dc6cf75d150a8c2cae6ad8c2c0ec7648e9484151/index.js#L159 is where sha256 would go specific to this issue... are there more interactions elsewhere in the code?
  2. When using the cache... with Userscripts... someone may edit a script, edit it again, and edit it again all in succession... when it is served via cache will that create 3 entries in the file cache or will it just be one with the middleware? e.g. does it clean up after itself or is that a task that we would need to do periodically?

Thanks. :)

breezewish commented 8 years ago

Hi,

  1. You only need to change this in order to use SHA256. Since express-minify would calculate the hash every time user requests a javascript/css/... asset, change from SHA1 to SHA256 may cost noteable CPU time. I haven't tested it yet and SHA1 seems to be enough (no collision), thus I kept SHA1 in the repository.
  2. express-minify calculates a hash of your response output and then use it as a key to find cache or create cache. So you need to clear "outdated" cache by yourself. Normally you can delete the cache file which is too old by creation time, however please notice that creation time is "first hit time" instead of "last cache hit time".

In your case.... I think compress BEFORE requesting is better than compress WHEN requesting since you can control more. I mean, use express-minify is not a BEST choice for you. I think you can compress and store the JavaScript when user submits and serve the compressed JavaScript when user requests. This solution adds minimum system load (needn't calculate hash every js request) and doesn't require clearing cache. It does need more modifications to the code :-)

Martii commented 8 years ago

Thanks for the confirmation.

The idea I had was, and it brushes here with the hash so my apologies if it is misplaced or really the incorrect path to pursue, is to be able to have an emitter equivalent (preferably not a callback) with an event we could trap anywhere in our side of the code (this case it would be starting here so we would not have to re-perform a script lookup in the middleware portion of app.use which is clear over there) in express-minify that tells us what the calculated hash that is going to be used... we could then add a field in our schema (around here)... something like minifyHash which would store the old/current hash in our MongoDB and we could compare the old hash to the new hash then perform a file operation that would clean up the old hash file. e.g. script.installName is known at that time in the code and since we are in that function we would know to use a node fs.unlink. Loosely related to #6

Utiilizing the existing cache system in this package would allow us to use the current cache system in place and if express-minify is "disabled" with debug mode of node... it would just not do that task... which is where I come with chores/maintenance.

Does this sound like a viable thing to do? ... perhaps with express-minify?

I'm still trying my best to keep the ideas with your package as it's very nice and re-inventing the wheel as we've done already a bit seems a bit redundant on our end. Saving the actual minified source outside of the file cache in our DB doesn't feel real appealing at this time though... with the cache it could be handled both manually and optionally programmatically preferably without a "job" that is triggered on a periodic interval with an egg timer as I call it. There's probably a technical name for all of this but I'm an experience type of engineer not a theory engineer.

Thanks again for taking the time to share your thoughts.

Martii commented 8 years ago

asset, change from SHA1 to SHA256 may cost noteable CPU time.

Btw... might be a breaking change but not necessarily... but have an option to choose the hash type? defaulting to sha1 e.g. something like:

app.use(minify({cache: { path: __dirname + '/cache', algorithm: 'sha256' }));

... perhaps with some supported default validations on what is an acceptable (tested) string value for the algorithm property.

In the minute tests that I've done our VPS we didn't have a huge lag time with sha256... we might be more interested in SHA256 since SHA1 based TLS/SSL certificates are pretty much discouraged due to vulnerabilities... if they are prone to flaws then possibly a simple sum check could be as well. IDK for sure.

breezewish commented 8 years ago

Oh, holy crap, I missed your latest comments.. I like this idea (some config like algorithm).