Hi Stefan, I think your concern is very valid. However, could you (and every list user) please not respond to an existing message when you are starting a new topic? It breaks the threading in thread-capable browsers and the post gets hidden in a thread that most often doesn''t have anything to do with your subject. It also causes two different threads get mixed up when both topics collect more replies. Thanks, Jarkko -- Jarkko Laine http://jlaine.net http://odesign.fi _______________________________________________ Rails mailing list Rails-1W37MKcQCpIf0INCOvqR/iCwEArCW2h5@public.gmane.org http://lists.rubyonrails.org/mailman/listinfo/rails
Jarkko Laine wrote:> I think your concern is very valid. However, could you (and every list > user) please not respond to an existing message when you are starting > a new topic? It breaks the threading in thread-capable browsers and > the post gets hidden in a thread that most often doesn''t have anything > to do with your subject. It also causes two different threads get > mixed up when both topics collect more replies.I noticed my misake about 1 minute after I sent the message. I''ll send it again, so we get a clean start. -- stefan
With the current implementation, every public method of a controller is a valid action name, which means that they are all callable via factoring a corresponding url. So every rails application is open to script kiddies. I think this is a major problem. I think it would be preferable to specify the valid actions using a class method in the controller declaration, e.g. like this: actions :index, :show, :edit:, ... This is easily implemented by adding a Hash containing the valid action names to the ActionController, which can be used by the dispatcher to determine whether the specified action name is valid. Scaffolding can easliy changed to add the names of the generated actions to the controller so this would not require much effort by the rails programmer. I know this would break a lot of apps, but IMHO, the current state of affairs is not acceptable. In order to mitigate this problem I suggest adding an option variable that can be set in the config file. To give you an idea what can be called via URI, I have listed the public methods available for tinkering below. -- stefan ======================================================================requestto_yaml_type instance_variables call_consider_all_requests_local fragment_cache_storefrozen? process __send__ to_a page_cache_directory action_name call_logger perform_action_without_benchmark call_ignore_missing_templates cache_erb_fragment page_cache_directoryclass call_perform_caching to_yaml_properties instance_variable_get expire_action controller_class_name cache_name_for expire_page render to_s perform_action_with_benchmark view_controller_internals send url_for clone nil? view_controller_internalsdisplay expire_fragment instance_variable_set controller_name inspect expire_actions instance_eval render_without_benchmark expire_matched_fragments perform_action_without_rescue dup remove_subclasses_of to_yaml equal? call_template_root active_layout methods require cache_page render_with_benchmark consider_all_requests_local method taint subclasses_of consider_all_requests_localeql? index hash call_fragment_cache_store showm params singleton_methods ignore_missing_templates paramsrender_with_layout extend instance_of? ignore_missing_templatesis_complex_yaml? perform_action_with_filters read_fragment id template_class session protected_methods tainted? response template_classkind_of? untaint sessionprocess_cgi responsebefore_action write_fragment call_template_class __id__ rendered_action_cache private_methods call_view_controller_internals =headers ==rendered_action_cachemodule_name freeze is_a? perform_caching logger headersrequire_gem after_action paginate object_id perform_cachingloggercookies call_page_cache_directory public_methods =~ assigns template_root request assignsrespond_to? fragment_cache_store template_roottype ============================================================================
* Stefan Kaes <skaes-hi6Y0CQ0nG0@public.gmane.org> [2005-02-14 09:24]:> script kiddies. I think this is a major problem. I think it would be > preferable to specify the valid actions using a class method in the > controller declaration, e.g. like this: > > actions :index, :show, :edit:, ...Two python web frameworks I''m aware of have the same issue, as they are "object publisher"-esque as well. == Quixote handles it like so: http://www.mems-exchange.org/software/quixote _q_exports = [''_q_index'', ''display'', ''login'', ...] == CherryPy, same idea, slightly different impl: http://www.cherrypy.org/ foo.exposed = True # ''foo'' being a method == Obviously Stefan''s proposed syntax is much more rubyish, but this seems a common and sensible practice for object publishing frameworks... -- ______________________________ toddgrimason*todd-AT-slack.net
> With the current implementation, every public method of a controller > is a valid action name, which means that they are all callable via > factoring a corresponding url. So every rails application is open to > script kiddies.Not really. If you would either try it out or inspect the code you would fine this: 1 def perform_action 2 if action_methods.include?(action_name) || action_methods.include?(''method_missing'') 3 send(action_name) 4 render unless performed? 5 elsif template_exists? && template_public? 6 render 7 else 8 raise UnknownAction, "No action responded to #{action_name}", caller 9 end 0 end Line #2 ensures that only actions part of the action_methods return value are accessible. And action_methods look like: def action_methods action_controller_classes = self.class.ancestors.reject{ |a| [Object, Kernel].include?(a) } action_controller_classes.inject([]) { |action_methods, klass| action_methods + klass.public_instance_methods(false) } end What it does is remove all of those of the object and kernel methods, so you can''t call things like clone or send. Then it makes sure that the only methods that ARE allowed are public instance methods within the hierarchy of that controller. With that said, it might be that we have some public methods available that shouldn''t. If you should spot and verify those as exposed and potentially harmful, please do post a ticket about it. Or even better, provide a fix. There''s no need to introduce additional gates, though. -- David Heinemeier Hansson, http://www.basecamphq.com/ -- Web-based Project Management http://www.rubyonrails.org/ -- Web-application framework for Ruby http://www.loudthinking.com/ -- Broadcasting Brain
> the only methods that ARE allowed are public instance methods within > the hierarchy of that controller.Doh! too new to ruby to remember it does have built-in access controls in classes unlike python and perl. Thanks for not publicly ridiculing me! So everyone ignore those 2 python examples please. Thank you. -- ______________________________ toddgrimason*todd-AT-slack.net
DHH wrote:> What [the perform_action method] does is remove all of those of the > object and kernel methods, so you can''t call things like clone or > send. Then it makes sure that the only methods that ARE allowed are > public instance methods within the hierarchy of that controller.And in addition, you use filters to be sure your public methods are safe to run, no? ActionController::Filters::ClassMethods http://rails.rubyonrails.com/classes/ActionController/Filters/ClassMethods.html
David Heinemeier Hansson wrote:>> With the current implementation, every public method of a controller >> is a valid action name, which means that they are all callable via >> factoring a corresponding url. So every rails application is open to >> script kiddies. > > > Not really. If you would either try it out or inspect the code you would > fine this: > > 1 def perform_action > 2 if action_methods.include?(action_name) || > action_methods.include?(''method_missing'') > 3 send(action_name) > 4 render unless performed? > 5 elsif template_exists? && template_public? > 6 render > 7 else > 8 raise UnknownAction, "No action responded to #{action_name}", > caller > 9 end > 0 end > > Line #2 ensures that only actions part of the action_methods return > value are accessible. And action_methods look like: > > def action_methods > action_controller_classes = self.class.ancestors.reject{ |a| > [Object, Kernel].include?(a) } > action_controller_classes.inject([]) { |action_methods, klass| > action_methods + klass.public_instance_methods(false) } > end > > What it does is remove all of those of the object and kernel methods, so > you can''t call things like clone or send. Then it makes sure that the > only methods that ARE allowed are public instance methods within the > hierarchy of that controller. > > With that said, it might be that we have some public methods available > that shouldn''t. If you should spot and verify those as exposed and > potentially harmful, please do post a ticket about it. Or even better, > provide a fix. > > There''s no need to introduce additional gates, thoughI don''t really have any problems with the current approach, but if there''s a reasonable claim to be made, you could easily fix this by doing the actions :show, :list thing in the same was as attr_accessible. That way old apps won''t even break. You can also do that from application.rb as a filter and a method ''action''. Something like: before_filter :validate_accessible_action protected def actions(*args) @accessible_actions = args.collect { |arg| arg.to_s } end def validate_accessible_action @accessible_actions.include? @params[''action''] end
David Heinemeier Hansson wrote:>> With the current implementation, every public method of a controller >> is a valid action name, which means that they are all callable via >> factoring a corresponding url. So every rails application is open to >> script kiddies. > > > Not really. If you would either try it out or inspect the code you > would fine this: >I must have overlooked this. Sorry for that mistake.> 1 def perform_action > 2 if action_methods.include?(action_name) || > action_methods.include?(''method_missing'') > 3 send(action_name) > 4 render unless performed? > 5 elsif template_exists? && template_public? > 6 render > 7 else > 8 raise UnknownAction, "No action responded to > #{action_name}", caller > 9 end > 0 end > > Line #2 ensures that only actions part of the action_methods return > value are accessible.Hm. what is ||action_methods.include?(''method_missing'') for? Looks to me like every name is valid if one of the classes has a missing method. This seems strange to me. Is it maybe related to class reloading?> And action_methods look like: > > def action_methods > action_controller_classes = self.class.ancestors.reject{ |a| > [Object, Kernel].include?(a) } > action_controller_classes.inject([]) { |action_methods, klass| > action_methods + klass.public_instance_methods(false) } > end > > What it does is remove all of those of the object and kernel methods, > so you can''t call things like clone or send. Then it makes sure that > the only methods that ARE allowed are public instance methods within > the hierarchy of that controller.Shouldn''t ActionController::Base be added too? And what about all the modules added through append_features (e.g. ActionController::Caching)? Or methods added by class_eval?> With that said, it might be that we have some public methods available > that shouldn''t. If you should spot and verify those as exposed and > potentially harmful, please do post a ticket about it. Or even better, > provide a fix.well, logging "action_methods" in log_processing like so logger.info " Action Methods:\n #{action_methods.sort.join"\n "}" displays the following functions: action_name active_layout after_action assigns assigns before_action cache_erb_fragment cache_page call_consider_all_requests_local call_fragment_cache_store call_ignore_missing_templates call_logger call_page_cache_directory call_perform_caching call_template_class call_template_root call_view_controller_internals consider_all_requests_local consider_all_requests_local controller_class_name controller_name cookies expire_action expire_fragment expire_page fragment_cache_store fragment_cache_store headers headers ignore_missing_templates ignore_missing_templates logger logger module_name page_cache_directory page_cache_directory paginate params params perform_action_with_benchmark perform_action_with_filters perform_action_without_benchmark perform_action_without_rescue perform_caching perform_caching process process_cgi read_fragment render render_with_benchmark render_with_layout render_without_benchmark rendered_action_cache rendered_action_cache request request response response session session template_class template_class template_root template_root url_for view_controller_internals view_controller_internals write_fragment Quite a few, unless I made a mistake in the logging code. I don''t know whether any of these are potentially harmful, and I don'' really want to look at the code at all. Frankly, I don''t want any of them to be callable, whether harmful or not.> There''s no need to introduce additional gates, though.I disagree. There is a fundamental difference between the approach I suggested and the code above: (a) You allow every public method initially and then filter out unwanted ones. (b) I want to disallow all public functions initially and then selectively enable some. With approach (a) it is very difficult to ensure that no unwanted method slips through. And you do have a maintenance problem too: every time a new public method or a feature module gets added it needs to be secured. With approach (b) it is very easy to get a secure system, because security is linked directly to funcionality: unless you declare the action it won''t work at all. And I don''t think it is too much to ask the programmer to declare a list of callable actions for each controller. It is also easy to generate the appropriate declarations when scaffolding is used. Moreover, the whole scheme can be implemented to fall back to the old method of determining valid action names when no such declaration has be been provided. Thus, you can make the system secure as an option, if you want to leave it unsecured just do so. Moreover, it is rather inefficient to compute all public methods of a controller (~80 in ), especially since this is done on each request. And then, there is also a design problem in approach (a): it makes every public method of controller instances of classes below ActionController::Base accessible via URL. But not all of these methods are template display functions. For example, I implemented user specific action caching for my 2 main entry pages. To get this working, I used a modification of the standard scheme with a filter class. The filter class needs to call a specialized fragment expiry method, defined in the controller class. Unless I am completely mistaken, this method needs to be public, at least with the current caching implementation. And I don''t want it to be callable via URL. Of course, all of this can be implemented via filters in ApplicationController. But this is cumbersome and requires action by each programmer for each system that needs to be secured. And IMHO, every system should be secure by default. Adding security as an afterthought just won''t work, look at the IE fiasco. Regards, -- stefan PS: I have already implemented the proposed changes in my local source copy. If you''re interested I can send the code for inspection, either to you or to the mailing list. But I won''t create a patch unless there is a chance of acceptance. PPS: Please don''t get me wrong, I really like RoR, I think it''s already a great system, but this doesn''t mean it can''t be improved. And having an easily securable system would surely add to its market value.
Just as an idea, you could use the newly implemented Routes to select every actions you want to make available ? -- Cheers, zimba http://zimba.oree.ch
zimba [tm] wrote:>Just as an idea, you could use the newly implemented Routes to select >every actions you want to make available ? >Of course, there are countless ways to do it. Could have used apache rewriting as well. Or filters. Or whatever. But that is besides the point. They system should be safe without extra effort on the programmers side. And IMHO this can only be achieved by changing the way actions are invoked in ActionController::Base. -- stefan
* Stefan Kaes <skaes-hi6Y0CQ0nG0@public.gmane.org> [2005-02-15 10:39]:> zimba [tm] wrote: > > >Just as an idea, you could use the newly implemented Routes to select > >every actions you want to make available ? > > > Of course, there are countless ways to do it. Could have used apache > rewriting as well. Or filters. Or whatever. But that is besides the > point. They system should be safe without extra effort on the > programmers side. And IMHO this can only be achieved by changing the way > actions are invoked in ActionController::Base.Hmm maybe my post re: the similar python frameworks and how they handle it (same as Stefan proposed, explicit declaration of ''visible'' or ''exported'' actions) wasn''t way off. I can''t argue over the internals and implementation, but I think the general infosec policy of "allowing the bare minimum permissions required" is pretty well-accepted these days across all applications, and sensible. As is a "lockdown out of the box" experience - i.e. linux and related OS''s all shipping with all ports closed unless explicity opened - yes it adds a gate, but a fair tradeoff - if you can''t handle the one-liner to turn something on, you probably don''t know enough to safely run it. Stefan listed the maintenance and sec issues as well at a code-level. This will also potentially be jumped on by other language communtiies, but that''s pure politics and unimportant (well it might impede evangelizing...) Anyway this is the viewpoint of a "random developer out there". i.e., those who will make up the future community. And as Stefan said, this is constructive criticism, not any sort of attack... -- ______________________________ toddgrimason*todd-AT-slack.net
I believe the main issue is that action methods are excluded by Object.methods
and not ActionController.methods. This will remove several of these such as
url_to. At the same time, I could not replicate several of the examples you
mention, especially the more critical ones such as instance_eval and send.
In truth, only the methods you list which accept 0 arguments are potentially
exploitable -- of course, it is still preferred that this door not even be
slightly ajar.
If no-one else tries this in the next few days I''ll try to get around
to it.
If you''d like to dive in, you might start with trying with (in
action_controller/base.rb)
def action_methods
(self.methods - ActionController.instance_methods).filter do |method_name|
self.method(method_name).arity == 0
end
end
That''s entirely off the top of my head, so no guarentees. :)
--
Nicholas Seckar aka. Ulysses
Beyond any security aspects, I like the self-documenting side effect of declaring exposed actions as Stefan proposed: actions :list :view :add :edit #etc This is nice right up top. But maybe thas just me... (and when TextMate gets its method menu/list/whatever I suppose it will be less of an issue - I still think this is a good idea though, but I don''t have lots (any, really) of legacy code to potentially worry about). -- ______________________________ toddgrimason*todd-AT-slack.net
> Beyond any security aspects, I like the self-documenting side effect > of declaring exposed actions as Stefan proposed: > > actions :list :view :add :edit #etcI don''t. At all. The convention is pretty clear. Accessible methods are the public ones. If you want to keep a method inaccessible, make it private or protected. This fits well with the Ruby accessibility model too. If you don''t say anything, the method is public. Declaring it private or protected changes that. I''ll work with Nicholas to close off any holes we might have with methods that shouldn''t be exposed. When that''s done, people shall be more than happy to submit bug reports if they should ever find ill-exposed methods. -- David Heinemeier Hansson, http://www.basecamphq.com/ -- Web-based Project Management http://www.rubyonrails.org/ -- Web-application framework for Ruby http://www.loudthinking.com/ -- Broadcasting Brain
Nicholas Seckar wrote:>I believe the main issue is that action methods are excluded by Object.methods >and not ActionController.methods. This will remove several of these such as >url_to. At the same time, I could not replicate several of the examples you >mention, especially the more critical ones such as instance_eval and send. > >Hmm, I didn''t explicitely mention instance_eval and send. After changing action methods to def action_methods action_controller_classes = self.class.ancestors.reject{ |a| [Object, Kernel, ActionController::Base].include?(a) } action_controller_classes.inject([]) { |action_methods, klass| action_methods + klass.public_instance_methods(false) } end I get the following list: active_layout after_action before_action cache_erb_fragment cache_page cookies expire_fragment expire_page paginate perform_action_with_benchmark perform_action_with_filters read_fragment render_with_benchmark render_with_layout write_fragment But the exacting listing depends on which modules get included in the particular controller. Just add logger.info " Action Methods:\n #{action_methods.sort.join"\n "}" to the log_processing method to check the list for your controllers.>In truth, only the methods you list which accept 0 arguments are potentially >exploitable -- of course, it is still preferred that this door not even be >slightly ajar. > >Not entirely true: methods with default arguments or argument lists are exploitable too.>If no-one else tries this in the next few days I''ll try to get around to it. >If you''d like to dive in, you might start with trying with (in >action_controller/base.rb) >def action_methods > (self.methods - ActionController.instance_methods).filter do |method_name| > self.method(method_name).arity == 0 > end >end > >That''s entirely off the top of my head, so no guarentees. :) >Well, as I''ve said before, I''ve already implemented and tested my suggestion, am absolutely satsified with it (it is faster, more flexible and more secure) and don''t see any reason to change it. BTW, here is the code that I have added/changed in AC::Base: class << self def valid_actions(*names) #explicit declaration of valid actions self.class_eval %q{cattr_accessor :valid_action_names} unless defined? self.valid_action_names self.valid_action_names ||= Hash.new names.each {|nm| self.valid_action_names[nm.id2name]=true} end def hidden_actions(*names) #removal of some public controller methods self.class_eval %q{cattr_accessor :hidden_action_names} unless defined? self.hidden_action_names self.hidden_action_names ||= Hash.new names.each {|nm| self.hidden_action_names[nm.id2name]=true} end end def perform_action if valid_action_name # was action_methods.include?(action_name) || action_methods.include?(''method_missing'') send(action_name) render unless performed? elsif template_exists? && template_public? render else raise UnknownAction, "No action responded to #{action_name}", caller end end def valid_action_name #new function if defined? self.class.valid_action_names self.class.valid_action_names.include?(action_name) else hidden = defined?(self.class.hidden_action_names) && self.class.hidden_action_names.include?(action_name) !hidden && (action_methods.include?(action_name) || action_methods.include?(''method_missing'')) end Which I believe to be completely compatible with all old applications. -- stefan
* David Heinemeier Hansson <david-OiTZALl8rpK0mm7Ywyx6yg@public.gmane.org> [2005-02-15 16:15]:> >Beyond any security aspects, I like the self-documenting side effect > >of declaring exposed actions as Stefan proposed: > > > >actions :list :view :add :edit #etc > > I don''t. At all. The convention is pretty clear. Accessible methods areHmm... so that''s a no? :-) -- ______________________________ toddgrimason*todd-AT-slack.net
> > I don''t. At all. The convention is pretty clear. Accessible methods are > > Hmm... so that''s a no? :-)In the rails world this is the equivalent to a blinking las-vegas style 50 feed tall sign which says "nop" :) -- Tobi http://www.snowdevil.ca - Snowboards that don''t suck http://www.hieraki.org - Open source book authoring http://blog.leetsoft.com - Technical weblog
On Tuesday 15 February 2005 18:06, Stefan Kaes wrote:> >In truth, only the methods you list which accept 0 arguments are > > potentially exploitable -- of course, it is still preferred that this > > door not even be slightly ajar. > > Not entirely true: methods with default arguments or argument lists are > exploitable too.Your definition of "accepts" is evidentally different from my use. I was using "accepts 0 arguments" to mean that object.send(method) will not raise an argument error regarding arity. (Although you are correct in that the code should have read method.arity <= 0)> Well, as I''ve said before, I''ve already implemented and tested my > suggestion, am absolutely satsified with it (it is faster, more flexible > and more secure) and don''t see any reason to change it. BTW, here is > the code that I have added/changed in AC::Base:I fail to see how declaring your actions explicitly is flexible. From my perspective this is a blatant violation of DRY. Of course, you are encouraged to customize Rails as you see fit, and I''m not going try to tell you what you should do -- Well, I will actually. Do whatever makes you happy. Within reason of course. :) I''ve submitted a ticket and provided a preview patch regarding a solution to this. I''ll try to finish this off tomorrow. I''ll save you looking at the patch code if you''d prefer to avoid that: This patch redefines action_methods to include all public instance methods of the controller except those that ActionController::Base also has. The array of allowed actions is cached. This is probably premature optimization, but I figure Array - Array is possibly an O(n**2) operation, so why not. :) [1] http://dev.rubyonrails.com/ticket/644 -- Nicholas Seckar aka. Ulysses
Nicholas Seckar wrote>I fail to see how declaring your actions explicitly is flexible. From my >It is more flexible because it gives me more control and does not force me to make a particular method protected or public, that is completely up to me.>perspective this is a blatant violation of DRY. Of course, you are encouraged >to customize Rails as you see fit, and I''m not going try to tell you what you >should do -- Well, I will actually. Do whatever makes you happy. Within >reason of course. :) > >I will. From what I read about DRY, I would say that DRY is roughly equivalent to:avoid redundancy. Not really a new concept, only a new name. However, it is well known, that not all redundancy is evil. I''d rather risk a little redundancy if it makes my system secure.>I''ve submitted a ticket and provided a preview patch regarding a solution to >this. I''ll try to finish this off tomorrow. > >I''ll save you looking at the patch code if you''d prefer to avoid that: This >patch redefines action_methods to include all public instance methods of the >controller except those that ActionController::Base also has. > >Never said I didn''t want to look at the patch, only said I don''t want to wade through the source code of ~80 functions to check whether they are potentially harmful or not.>The array of allowed actions is cached. This is probably premature >optimization, but I figure Array - Array is possibly an O(n**2) operation, so >why not. :) > >[1] http://dev.rubyonrails.com/ticket/644 >Fine, fine. Nice idea to optimize action_methods. However, I think it won''t have much effect, since apparently controllers get instantiated for each request and then thrown away: ActionController::Base: # Factory for the standard create, process loop where the controller is discarded after processing. def process(request, response) #:nodoc: new.process(request, response) end Dispatcher: controller_class(controller_name).process(request, response).out Oh, BTW, I have noticed that on my machine (XP, 1G Mem, 3Ghz) garbage collection kicks in after every 4-6 requests, depending of course on which pages pages are viewed. The ruby process, invoked from apache via fcgi, uses around 20M virtual memory, garbage collection takes around 100 msec. (Can this be actually measured exactly? Is there a way to do real profiling of ruby code?) So sub second behavior is almost impossible for even a moderate user base. Back to security: yes your code eliminates most of the unwanted methods from the actions list, as you can see from the following log excerpt: Action Methods: add_favorite advsearch alphabetic autrezsearch cache_name cache_name_for cache_name_for_action cat create delete_favorite destroy edit expire_actions exsearch favsearch index letter list myknzlpzl new newsearch printerview random search show update These methods are all declared as public in my controller class, but cache_name, cache_name_for, cache_name_for_action and expire_actions aren''t view methods. They are used by a cache filter class like so: (excuse the silly class names) class NamingCacheFilter def initialize(*actions) @actions = actions end def before(controller) return unless @actions.include?(controller.action_name.intern) if cache = controller.read_fragment(controller.cache_name_for_action) controller.rendered_action_cache = true controller.send(:render_text, cache) false end end def after(controller) return if !@actions.include?(controller.action_name.intern) || controller.rendered_action_cache controller.write_fragment(controller.cache_name_for_action, controller.response.body) end end The filter gets installed on the controller via a call to the following method: class ApplicationController ... def self.caches_named_action(*actions) return unless perform_caching around_filter(NamingCacheFilter.new(*actions)) end ... which is complemented by the following sweeper class Sweeper < ActiveRecord::Observer observe Rezept, Post, Comment, Menue, Favorite, Book Fragments = %w(lsb_0 lsb_1 lsb_2 mdb_0 mdb_1 mdb_2 rsb_0 rsb_1 rsb_2).collect!{|f| "rezept/" + f} @@records = [] def after_save(record) @@records << record end def after_destroy(record) @@records << record end def self.add_records(records) @@records.concat records end def filter(controller) frags = Fragments.clone @@records.each {|r| case r.class.name when /Rezept|Comment|Menue/ frags << "rezept/lsb_u_" + r.user_id.to_s frags << "rezept/rsb_u_" + r.user_id.to_s frags << "rezept/mdb_u_" + r.user_id.to_s when /Book|Favorite/ frags << "rezept/mdb_u_" + r.user_id.to_s end } @@records = [] frags.each {|f| controller.expire_fragment(f) } controller.expire_actions end end If you look at the code (or simply believe me), the filter and the sweeper need to call non-view methods of the controller. Therfore these must be declared as public methods. And, even if I have to repeat myself ;-) these methods should not be callable via URL. (BTW I think it is a mistake to attach the caching mechanism so closely to the controller, the cache is an independent object, which should be directly accessible, without going through the controller. If this were possible, the public controller methods would not be necessary. [And sometimes there is no controller available at all: some of my data gets imported into the database outside the running ruby process, using an activerecord batch file. How do I clear the cache then? By recoding the logic of the controller attached code. A severe case of redundancy.] I can see only three solutions to avoid the use of public methods on the controller that aren''t view functions (BTW in MVC controllers are named conrollers because they control view and methods, and usually they have a bunch of process abstractions attached to them, which should can be invoked from other controllers). Now what can I do? 1. give up on caching: that just won''t do, the action cache serves requests around 1000 times faster than going through action processing 2. provide a mechanism to declare methods as actions: works well and is 100% secure, but adds a little redundancy 3. provide a mechanism to declare some methods as uncallable via URL: works too, adds less redundancy than 2, but is more error prone (e.g. I need to look at the log file to find out which methods RoR thinks are view functions, just in case one has slipped through the action_methods check) If you have another solution, please tell me. I''d be more than happy to remove the nonview methods or make them protected. I''ve already mentioned that problem in one of my previous posts to dhh, but he chose not to react. And no matter how you implement action_method computation, it is not possible to automatically determine which controller methods should be callable. -- stefan
> If you have another solution, please tell me. I''d be more than happy > to remove the nonview methods or make them protected. > > I''ve already mentioned that problem in one of my previous posts to > dhh, but he chose not to react. > > And no matter how you implement action_method computation, it is not > possible to automatically determine which controller methods should be > callable.Hey, Stefan. I''m happy that you''re raising awareness about the methods accessible that shouldn''t be. With Nicholas'' patch and added protection for the caching methods, we''ll have that potential hole covered up in no time. I am, however, sensing an antagonistic tone in your emails. If this was caused by my lack of interest in an explicit gatekeeper declaration for accessible actions, I hope that you''ll accept my assurance that it was merely the idea and not your presentation of it that I didn''t like. We''ve had a few similar situations on the list before where some issue creates an unnecessarily uncomfortable tone, but we''ve always been able to address it by just recognizing its presence. I hope we can do the same here. -- David Heinemeier Hansson, http://www.basecamphq.com/ -- Web-based Project Management http://www.rubyonrails.org/ -- Web-application framework for Ruby http://www.loudthinking.com/ -- Broadcasting Brain
David Heinemeier Hansson wrote:> Hey, Stefan. I''m happy that you''re raising awareness about the methods > accessible that shouldn''t be. With Nicholas'' patch and added > protection for the caching methods, we''ll have that potential hole > covered up in no time. > > I am, however, sensing an antagonistic tone in your emails. If this > was caused by my lack of interest in an explicit gatekeeper > declaration for accessible actions, I hope that you''ll accept my > assurance that it was merely the idea and not your presentation of it > that I didn''t like. > > We''ve had a few similar situations on the list before where some issue > creates an unnecessarily uncomfortable tone, but we''ve always been > able to address it by just recognizing its presence. I hope we can do > the same here. >Hey, David. I didn''t mean to offend. Please excuse me if my emails created that impression. I was, however, slightly disappointed by not getting any reply to this mail (4211CFDE.7030706-hi6Y0CQ0nG0@public.gmane.org). Of course, you are probably very busy and don''t have enough time on your hands to answer all mails in detail. That was my first reaction. But then you explained someone else in a side branch of the main thread in great length why you didn''t like the idea and how were going to handle it. Well, I found that a bit antagonistic as well. And I sensed a bit of "not invented here". Maybe I got that wrong. I only wanted to help securing RoR (and avoid patching on each release of course ;-). And I usually strive for the best solution I can dream up. I am still convinced that only an explicit action declaration can make the system 100% secure and I have not seen a valid counterargument so far. But I think we should just lay that to rest now. It''s your baby, you decide what goes in. I have no problem with that. Regards, Stefan PS: what do you mean with "added protection for the caching methods"? I might have missed something.
I have changed the ticket status from RESEARCH to PATCH. In addition to stronger default exclusion, you can mark any action as hidden using hide_actions :action_one, :action_two, ... or using hide_action :a_action [1] http://dev.rubyonrails.com/ticket/644 -- Nicholas Seckar aka. Ulysses _______________________________________________ Rails mailing list Rails-1W37MKcQCpIf0INCOvqR/iCwEArCW2h5@public.gmane.org http://lists.rubyonrails.org/mailman/listinfo/rails
Hi, Wouldn''t the cleanest solution be to filter the public instance methods provided by ActionController::Base only? (in addition to Object methods, obviously). I can''t think of a scenario where ActionController::Base needs to define any actions. Public methods defined in classes deriving from ActionController::Base should be actions, imho, as those classes are "concrete controllers". This seems a vastly simpler approach, is easy to explain, and means you don''t have to update the "exclusion list" every time ActionController::Base gains a public instance method. Am I missing something? Leon
Nicholas Seckar wrote:> I have changed the ticket status from RESEARCH to PATCH. In addition > to stronger default exclusion, you can mark any action as hidden using > > hide_actions :action_one, :action_two, ... > > or using > > hide_action :a_action > > [1] http://dev.rubyonrails.com/ticket/644 >Cool. I think there is still a bug in the code. I have updated the ticket. Only posted here because I don''t know whether you will get a notification when someone changes the ticket. I am also interested in automatic notification, is there a way to do this without using the list? -- stefan