Plugins

Comments and changes to this ticket

• 

  Chris Wanstrath November 13th, 2007 @ 06:43 PM

  • → Assigned user changed from “Chris Wanstrath” to “Mislav”
  • → State changed from “new” to “open”
• 

  martin November 20th, 2007 @ 11:18 PM

  On a related note, both versions don't handle existing, non-nil but non-numerical values of page.

  E.g. if someone accesses /.../index?page=Wurstbrot this will give an invalid SQL statement - page.to_i returns 0 and that creates the LIMIT -10 statement.

  While this is mostly harmless, I don't think outside users should be able to generate this kind of error by simply surfing the site.

• 

  Brad Greenlee December 13th, 2007 @ 04:43 PM

  We just ran into this as well at Wesabe, when a bot checking for security holes started throwing all kinds of non-numeric values at page. Seems the easiest solution is to just throw this in collection.rb:

      def initialize(page, per_page, total)
         @current_page  = page.to_i
         @current_page = 1 if @current_page == 0
  

  although a less user-friendly solution would be to throw an ActiveRecord::RecordNotFound

• 

  Mislav December 19th, 2007 @ 06:37 AM

  • → State changed from “open” to “invalid”

  In Ruby, try doing this:

  2 + "2"
  

  You'll get an error that String cannot be coerced into Fixnum. This is because Ruby is strictly typed and it won't try to do any guesswork - that's a feature, not a language flaw.

  This is how WP::Collection was designed: to be strict on the data it accepts. Other than converting input data to integers, it won't do absolutely any guesswork after that. So, if input data is "Schnitzel" instead of a valid number, errors should and will happen.

  AR::Base#paginate method was designed to be a smarter API than this. It accepts nil as current page and defaults to page 1 because that is the global convention with pagination on the Web.

  My mistake was not that I didn't implement more guesswork in these methods, but that I tolerated invalid input. In near future, I will change will_paginate to raise a WP::InvalidPage error whenever there is input such as "Schnitzel", zero, negative number or a blank string.

  To conclude: WP::Collection is a basic class that won't do any of your application logic. If you want to sanitize input or default to a certain page, you'll have to do that prior to calling it or its big sister, AR::Base#paginate.

• 

  Brad Greenlee December 19th, 2007 @ 01:12 PM

  Throwing an appropriate exception is fine. Letting it fall through to SQL as LIMIT -10 isn't good. Thanks for looking at it.

• 

  Mislav December 23rd, 2007 @ 03:26 PM

  • → State changed from “invalid” to “resolved”

  In revision [421], WillPaginate::InvalidPage error is thrown on invalid input in :page parameter. WillPaginate::Collection doesn't even accept nil as page value anymore, but paginate() API does and defaults to page 1.