magnars / optimus

A Ring middleware for frontend performance optimization.
364 stars 23 forks source link

`realize-regex-paths` matches paths outside of the given public directory #39

Open tomdalling opened 9 years ago

tomdalling commented 9 years ago

With this directory structure:

resources/js/bootstrap.js
resources/js/google-analytics.js
resources/js/jquery-1.11.1.js
resources/static/gemmy/gemmy.js
resources/static/gemmy/phaser.js

I had this line of code:

(optimus.assets/load-bundle "js" "all.js" [#".*\.js"])

Which I expected to match the three files inside the js directory. This is what was actually matched:

(optimus.assets.creation/realize-regex-paths "js" #".*\.js")
#=> ("/bootstrap.js" 
     "/google-analytics.js"
     "/jquery-1.11.1.js"
     "Users/tom/proj/tomdalling.github.io/resources/static/gemmy/gemmy.js"
     "Users/tom/proj/tomdalling.github.io/resources/static/gemmy/phaser.js")

Which later on causes a FileNotFound exception for the path Users/tom/proj/tomdalling.github.io/resources/static/gemmy/gemmy.js.

I traced the problem back to this line: https://github.com/magnars/optimus/blob/512b2ede6a9350dc78abde41951ab467df821aa6/src/optimus/assets/creation.clj#L87

The realize-regex-paths function doesn't test whether a path is within the public-dir directory, it just checks whether the public-dir string exists anywhere within the path. In my case, I was trying to match a directory called "js", but it also matched all javascript files with the file extension "js".

magnars commented 9 years ago

Thanks for such a detailed report.

I could certainly add a / or two to the test, so that it only matches /js/ instead of js - but you would still get false matches with a path like resources/static/js/gemmy/gemmy.js.

Any ideas for a better solution?

tomdalling commented 9 years ago

Well, you could pass more detailed data up from optimus.class-path, such as {:root "/Users/tom/proj/tomdalling.github.io/resources" :path "static/gemmy/gemmy.js"}, which would allow for accurate filtering on the path.

An alternative is to pass public-dir down into optimus.class-path, and do the filtering there. I have this replacement for optimus.class-path/class-path-elements that should work:

(defn class-path-elements-under-directory [directory]
  (mapcat optimus.class-path/get-resource-paths
    (->> (optimus.class-path/class-path-elements)
         (map #(str % "/" directory))
         (filter #(let [f (File. %)] (and (.exists f) (.isDirectory f)))))))

That said, I'm not sure if people are relying on the current behaviour, because that would give you backwards compatibility issues.

magnars commented 9 years ago

Thanks. There is the added restriction that it must also work for resources in jars, but I'll see if I can figure out a way to separate the root and the path for both.

tomdalling commented 4 years ago

Closing due to age

magnars commented 4 years ago

Old or not, I think this is still a problem. It's just been forgotten about. Re-opening.