i''m trying to debug something (file_column plugin) which makes use of RAILS_ROOT to determine a root storage path root_path = File::join RAILS_ROOT, "public" that''s well enough - but this same path is used throughout the code to generate urls for files under root_path. my understanding of RAILS_ROOT and the "public" subdir is that one should never be generating links from outside of "public" in this way since it subverts security at minimum and, at maximum, is broken since a url relative to RAILS_ROOT is not guaranteed to be visible since RAILS_ROOT is a file_system concept and is not in url space. is this correct? -a -- ==============================================================================| ara [dot] t [dot] howard [at] noaa [dot] gov | all happiness comes from the desire for others to be happy. all misery | comes from the desire for oneself to be happy. | -- bodhicaryavatara ===============================================================================
On Jan 4, 2006, at 12:54 AM, Ara.T.Howard wrote:> > i''m trying to debug something (file_column plugin) which makes use of > RAILS_ROOT to determine a root storage path > > root_path = File::join RAILS_ROOT, "public" > > that''s well enough - but this same path is used throughout the code to > generate urls for files under root_path. my understanding of > RAILS_ROOT and > the "public" subdir is that one should never be generating links > from outside > of "public" in this way since it subverts security at minimum and, > at maximum, > is broken since a url relative to RAILS_ROOT is not guaranteed to > be visible > since RAILS_ROOT is a file_system concept and is not in url space. > > is this correct?Yes, that''s correct as far as I understand it. I suppose this is in conjunction with the problem you ran in to with file_column and Family Connection? If so, I wonder if Sebastian has any comment on implementation choices that he had to make. Duane Johnson (canadaduane) http://blog.inquirylabs.com/
On Wed, 4 Jan 2006, Sebastian Kanthak wrote:>>> i''m trying to debug something (file_column plugin) which makes use of >>> RAILS_ROOT to determine a root storage path >>> >>> root_path = File::join RAILS_ROOT, "public" >>> >>> that''s well enough - but this same path is used throughout the code to >>> generate urls for files under root_path. my understanding of RAILS_ROOT >>> and the "public" subdir is that one should never be generating links from >>> outside of "public" in this way since it subverts security at minimum and, >>> at maximum, is broken since a url relative to RAILS_ROOT is not guaranteed >>> to be visible since RAILS_ROOT is a file_system concept and is not in url >>> space. is this correct? >> >> Yes, that''s correct as far as I understand it. I suppose this is in >> conjunction with the problem you ran in to with file_column and Family >> Connection? If so, I wonder if Sebastian has any comment on implementation >> choices that he had to make. > > well, my goal for file_column was to reduce the setup required to a > minimum. My basic assumption are that > > * RAILS_ROOT/public is a directory I can write files to > * files in RAILS_ROOT/public are accessible via the webapps base URL > (i.e., the URL that is used for url_for, etc.) > > These assumptions are true for webrick and most setups I''ve seen so > far. It can be changed by using the following configuration options > for file_column: > > * :root_path, this defaults to RAILS_ROOT > * :web_root, the URL where to reach :root_path, relative to the > rails app''s base URL, defaults to "" > > Ara, what security issues does this setup raise? Have you any > suggestions on how to improve it?here''s the issues and how i fixed them - the problem involves both the file_column plugin and the familyconnection software, but could easily affect other rails projects: assume the following - rails project living in /var/www/ror/howards - the public directory is exposed thus /var/www/html/howards -> /var/www/ror/howards/public where the first is a link ===============================================================problem one (filecolumn plugin): =============================================================== file: file_column/lib/file_column.rb 172 def absolute_path(subdir=nil) 173 if subdir 174 # File.expand_path(File.join(@dir, subdir, @filename)) # fix 175 File.join(@dir, subdir, @filename) 176 else 177 # File.expand_path(File.join(@dir, @filename)) # fix 178 File.join(@dir, @filename) 179 end 180 end the problem and fix are shown. RAILS_ROOT will have a value of something like /var/www/html/howards/../config/../ but recall that /var/www/html/howards/ is a link. when it is expanded it becomes something like /var/www/html/ which is obviously not correct. this is because: [root@localhost ahoward]# irb irb(main):001:0> path = "/var/www/html/howards/../config/../public/photo/image/1/FishRobert.jpg" => "/var/www/html/howards/../config/../public/photo/image/1/FishRobert.jpg" irb(main):002:0> File::exist? path => true irb(main):003:0> expanded_path = File::expand_path path => "/var/www/html/public/photo/image/1/FishRobert.jpg" irb(main):004:0> File::exist? expanded_path => false so the call to File::expand_path destroys the validity of the path if any links are encountered. this can be fixed using Pathname::new(path).realpath.to_s which deals with links correctly, but only on *nix. the suggested fix is simply not to expand the paths - it''s not needed for a method called ''absolute_path'' anyhow as the returned path is certainly absolute, if not expanded, with the fix. ===============================================================problem two (filecolumn plugin): =============================================================== file: file_column/lib/file_column_helper.rb 45 def url_for_file_column(object, method, subdir=nil) 46 case object 47 when String, Symbol 48 object = instance_variable_get("@#{object.to_s}") 49 end 50 relative_path = object.send("#{method}_relative_path", subdir) 51 return nil unless relative_path 52 url = "" 53 # url << @request.relative_url_root.to_s << "/" # fix 54 url << "/" 55 url << object.send("#{method}_options")[:base_url] << "/" 56 url << relative_path 57 end the issue here is that adding the ''relative_url_root'' causes it to be added twice since the rails trio of ''image_tag/image_path/compute_public_path'' already do this work. in short the urls returned should be relative to the application so the other rails helpers deal with them correctly. note that one would not see this behaviour of ''relative_url_root'' was empty! oncee that is fixed it causes... ===============================================================problem three (familyconnection project) =============================================================== file: app/views/news/_news_item.rhtml (and perhaps others!!) 6 <% if news_item.photos_count > 0 -%> 7 <% for photo in news_item.photos -%> 8 <% 9 thumb_url = url_for_image_column(photo, "image", 10 :size => "50x50", :crop => "1:1", :name => "thumb") 11 # fullsize_url = url_for_image_column(photo, "image", :name => "fullsize") # fix 12 fullsize_url = image_path(url_for_image_column(photo, "image", :name => "fullsize")) 13 %> 14 <%= link_to_function(image_tag(thumb_url), "popitup(''#{fullsize_url}'', 542, 532)") %> 15 <% end -%> 16 <div style="clear: both"></div> 17 <% end -%> once ''url_for_file_column'' is changed to return urls which are relative to the application we have to be careful to use all the rails'' helper methods to build correct urls. in this case without the fix link_to_function(image_tag(thumb_url), "popitup(''#{fullsize_url}'', 542, 532)") will correctly show the ''thumb_url'' but the ''popitup'' method will fail since ''fullsize_url'' will not have ''relative_url_root'' added. i don''t know a way around this since we need to pass urls from ''url_for_file_column'' to ''image_tag'', which will prepend the ''relative_url_root'' to them, but we''d assume a method named ''url_for_file_column'' would return a valid url - however this is not the case __unless__ our app is sitting in the webroot and ''relative_url_root'' is empty. it strikes me as odd that ''compute_public_path'' would add the ''relative_url_root'' to a path which has already had it added, but looking at file: actionpack-1.11.2/lib/action_view/helpers/asset_tag_helper.rb 148 def compute_public_path(source, dir, ext) 149 source = "/#{dir}/#{source}" unless source.first == "/" || source.include?(":") 150 source = "#{source}.#{ext}" unless source.include?(".") 151 source = "#{@controller.request.relative_url_root}#{source}" unless %r{^[-a-z]+://} =~ source 152 source = ActionController::Base.asset_host + source unless source.include?(":") 153 source 154 end we can see on line 151 that it certainly is __always__ added unless the url has a protocol attached (which seems an odd test doesn''t it?). another approach might be to make ''url_for_file_column'' return absolute urls of the form http://server:port/path/to/image.jpg and trust that rails helper methods will leave it alone since it''s certainly an absolute url... but that didn''t smell right to me. in any case, both peice of software certainly are great and i''ll be using both. a test run of the familyconnection is up at http://fortytwo.merseine.nu/howards/ and you can see all the fixes working here http://fortytwo.merseine.nu/howards/news in case either of you are doubting thomases like i am. thanks for the great software - hopefully this info will help you. cheers. -a -- ==============================================================================| ara [dot] t [dot] howard [at] noaa [dot] gov | all happiness comes from the desire for other be happy. all misery | comes from the desire for oneself to be happy. | -- bodhicaryavatara ===============================================================================
On Jan 4, 2006, at 4:13 PM, Ara.T.Howard wrote:> On Wed, 4 Jan 2006, Sebastian Kanthak wrote: > >>>> i''m trying to debug something (file_column plugin) which makes >>>> use of >>>> RAILS_ROOT to determine a root storage path >>>> >>>> root_path = File::join RAILS_ROOT, "public" >>>> >>>> that''s well enough - but this same path is used throughout the >>>> code to >>>> generate urls for files under root_path. my understanding of >>>> RAILS_ROOT >>>> and the "public" subdir is that one should never be generating >>>> links from >>>> outside of "public" in this way since it subverts security at >>>> minimum and, >>>> at maximum, is broken since a url relative to RAILS_ROOT is not >>>> guaranteed >>>> to be visible since RAILS_ROOT is a file_system concept and is >>>> not in url >>>> space. is this correct? >>> >>> Yes, that''s correct as far as I understand it. I suppose this is in >>> conjunction with the problem you ran in to with file_column and >>> Family >>> Connection? If so, I wonder if Sebastian has any comment on >>> implementation >>> choices that he had to make. >> >> well, my goal for file_column was to reduce the setup required to a >> minimum. My basic assumption are that >> >> * RAILS_ROOT/public is a directory I can write files to >> * files in RAILS_ROOT/public are accessible via the webapps base URL >> (i.e., the URL that is used for url_for, etc.) >> >> These assumptions are true for webrick and most setups I''ve seen so >> far. It can be changed by using the following configuration options >> for file_column: >> >> * :root_path, this defaults to RAILS_ROOT >> * :web_root, the URL where to reach :root_path, relative to the >> rails app''s base URL, defaults to "" >> >> Ara, what security issues does this setup raise? Have you any >> suggestions on how to improve it? > > here''s the issues and how i fixed them - the problem involves both the > file_column plugin and the familyconnection software, but could > easily affect > other rails projects: > > assume the following > > - rails project living in > > /var/www/ror/howards > > - the public directory is exposed thus > > /var/www/html/howards -> /var/www/ror/howards/public > > where the first is a link > > > ===============================================================> problem one (filecolumn plugin): > ===============================================================> > file: file_column/lib/file_column.rb > > 172 def absolute_path(subdir=nil) > 173 if subdir > 174 # File.expand_path(File.join(@dir, subdir, > @filename)) # fix > 175 File.join(@dir, subdir, @filename) > 176 else > 177 # File.expand_path(File.join(@dir, @filename)) # fix > 178 File.join(@dir, @filename) > 179 end > 180 end > > > the problem and fix are shown. RAILS_ROOT will have a value of > something like > > /var/www/html/howards/../config/../ > > but recall that > > /var/www/html/howards/ > > is a link. when it is expanded it becomes something like > > /var/www/html/ > > which is obviously not correct. this is because: > > > [root@localhost ahoward]# irb > irb(main):001:0> path = "/var/www/html/howards/../config/../ > public/photo/image/1/FishRobert.jpg" > => "/var/www/html/howards/../config/../public/photo/image/1/ > FishRobert.jpg" > > irb(main):002:0> File::exist? path > => true > > irb(main):003:0> expanded_path = File::expand_path path > => "/var/www/html/public/photo/image/1/FishRobert.jpg" > > irb(main):004:0> File::exist? expanded_path > => false > > > so the call to File::expand_path destroys the validity of the > path if any > links are encountered. this can be fixed using > > Pathname::new(path).realpath.to_s > > which deals with links correctly, but only on *nix. the > suggested fix is > simply not to expand the paths - it''s not needed for a method called > ''absolute_path'' anyhow as the returned path is certainly > absolute, if not > expanded, with the fix. > > > ===============================================================> problem two (filecolumn plugin): > ===============================================================> > file: file_column/lib/file_column_helper.rb > > 45 def url_for_file_column(object, method, subdir=nil) > 46 case object > 47 when String, Symbol > 48 object = instance_variable_get("@#{object.to_s}") > 49 end > 50 relative_path = object.send("#{method}_relative_path", > subdir) > 51 return nil unless relative_path > 52 url = "" > 53 # url << @request.relative_url_root.to_s << "/" # fix > 54 url << "/" > 55 url << object.send("#{method}_options")[:base_url] << "/" > 56 url << relative_path > 57 end > > > the issue here is that adding the ''relative_url_root'' causes it > to be added > twice since the rails trio of ''image_tag/image_path/ > compute_public_path'' > already do this work. in short the urls returned should be > relative to the > application so the other rails helpers deal with them correctly. > note that > one would not see this behaviour of ''relative_url_root'' was empty! > > oncee that is fixed it causes... > > > ===============================================================> problem three (familyconnection project) > ===============================================================> > file: app/views/news/_news_item.rhtml (and perhaps others!!) > > 6 <% if news_item.photos_count > 0 -%> > 7 <% for photo in news_item.photos -%> > 8 <% > 9 thumb_url = url_for_image_column(photo, "image", > 10 :size => "50x50", :crop => "1:1", :name => > "thumb") > 11 # fullsize_url = url_for_image_column(photo, > "image", :name => "fullsize") # fix > 12 fullsize_url = image_path(url_for_image_column > (photo, "image", :name => "fullsize")) > 13 %> > 14 <%= link_to_function(image_tag(thumb_url), > "popitup(''#{fullsize_url}'', 542, 532)") %> > 15 <% end -%> > 16 <div style="clear: both"></div> > 17 <% end -%> > > > once ''url_for_file_column'' is changed to return urls which are > relative to > the application we have to be careful to use all the rails'' > helper methods > to build correct urls. in this case without the fix > > link_to_function(image_tag(thumb_url), "popitup(''# > {fullsize_url}'', 542, 532)") > > will correctly show the ''thumb_url'' but the ''popitup'' method will > fail since > ''fullsize_url'' will not have ''relative_url_root'' added. > > i don''t know a way around this since we need to pass urls from > ''url_for_file_column'' to ''image_tag'', which will prepend the > ''relative_url_root'' to them, but we''d assume a method named > ''url_for_file_column'' would return a valid url - however this is > not the > case __unless__ our app is sitting in the webroot and > ''relative_url_root'' is > empty. > > it strikes me as odd that ''compute_public_path'' would add the > ''relative_url_root'' to a path which has already had it added, but > looking at > > file: actionpack-1.11.2/lib/action_view/helpers/ > asset_tag_helper.rb > > 148 def compute_public_path(source, dir, ext) > 149 source = "/#{dir}/#{source}" unless source.first > == "/" || source.include?(":") > 150 source = "#{source}.#{ext}" unless source.include? > (".") > 151 source = "#{@controller.request.relative_url_root} > #{source}" unless %r{^[-a-z]+://} =~ source > 152 source = ActionController::Base.asset_host + > source unless source.include?(":") > 153 source > 154 end > > > we can see on line 151 that it certainly is __always__ added > unless the url > has a protocol attached (which seems an odd test doesn''t it?). > > another approach might be to make ''url_for_file_column'' return > absolute urls > of the form > > http://server:port/path/to/image.jpg > > and trust that rails helper methods will leave it alone since > it''s certainly > an absolute url... but that didn''t smell right to me. > > > in any case, both peice of software certainly are great and i''ll be > using > both. a test run of the familyconnection is up at > > http://fortytwo.merseine.nu/howards/ > > and you can see all the fixes working here > > http://fortytwo.merseine.nu/howards/news > > in case either of you are doubting thomases like i am. > > thanks for the great software - hopefully this info will help you. > > cheers.Wow, Ara. Thanks for the thorough treatment and resolution of the problem. I checked out the website, and it is indeed working! :) (btw: It looks like you need to modify the application.css file so that your background images show up properly). As for the Rails core list, I''d like to find out why compute_public_path is doing what it''s doing. It seems to me, if we could eliminate that strangity, we wouldn''t have to work around things as you''ve done. Is anyone familiar with the asset_tag_helper and could chime in? Duane Johnson (canadaduane) http://blog.inquirylabs.com/
On Wed, 4 Jan 2006, Duane Johnson wrote:> Wow, Ara. Thanks for the thorough treatment and resolution of the problem. > I checked out the website, and it is indeed working! :) (btw: It looks > like you need to modify the application.css file so that your background > images show up properly).hmmm - i don''t have any background images though? am i supposed to?> As for the Rails core list, I''d like to find out why compute_public_path is > doing what it''s doing. It seems to me, if we could eliminate that > strangity, we wouldn''t have to work around things as you''ve done.it does seem a bit odd at first - but it really does make good sense: all the links you provide to rails'' helpers simply need to be absolute wrst to your application. in otherwords a link /bar.jpg works whether your application is at the top level or if it''s in a subdir like /foo/ - but you don''t have to care. all links simply need to be absolute wrst the app - it''s location doesn''t matter. at least that is my understanding... that being the case the filecolumn plugin definitely need the patches i suggested or some other type of fix. cheers. -a -- ==============================================================================| ara [dot] t [dot] howard [at] noaa [dot] gov | all happiness comes from the desire for others to be happy. all misery | comes from the desire for oneself to be happy. | -- bodhicaryavatara ===============================================================================
On 1/4/06, Ara.T.Howard <ara.t.howard@gmail.com> wrote:> it strikes me as odd that ''compute_public_path'' would add the > ''relative_url_root'' to a path which has already had it added, but looking at > > file: actionpack-1.11.2/lib/action_view/helpers/asset_tag_helper.rb > > 148 def compute_public_path(source, dir, ext) > 149 source = "/#{dir}/#{source}" unless source.first == "/" || source.include?(":") > 150 source = "#{source}.#{ext}" unless source.include?(".") > 151 source = "#{@controller.request.relative_url_root}#{source}" unless %r{^[-a-z]+://} =~ source > 152 source = ActionController::Base.asset_host + source unless source.include?(":") > 153 source > 154 end > > > we can see on line 151 that it certainly is __always__ added unless the url > has a protocol attached (which seems an odd test doesn''t it?).I ran into something similar in EdgeRails a few months ago, and my analysis led me to the method you''ve pointed out. From what I remember, isn''t @controller.request.relative_url_root only populated under Apache, and not under WEBrick or Lighty? Assuming that''s still true, that''s something to take into consideration.