Raise OutOfRangeError for page input below min about pagy HOT 22 CLOSED

Both API clients and user behaviour can either result in invalid input due to bugs or malicious behaviour.

Thanks for this, because you save me the time to answer you point by point. As you just wrote, the whole discussion is focused on the only 2 possible cause of that kind of error: bugs and malicious behaviour, and both of them are VERY worth an exception.

• Bugs: please, do raise exceptions to give someone the possibility to fix them. Regardless whether bugs happen inside the app or in an API client, the developers should be able to get an exception, and that could help fixing even bigger problems, discovered (and not ignored) thanks to the exception. I am tired to scratch my head when I get unexpected results and no exceptions and no log entries in the legacy apps I am working on, because some smart-ass implemented a rescue do_something_else somewhere!!!

• Malicious behavior: in this case if I could, I would even "spit in the eye" of the wannabe hacker (very Italian way to despise someone :)), but since it is quite difficult to do so over a connection, the very next thing is giving the guy an exception to suck!

I think that should be handled by the gem due to arguments listed above.

The gem offers the overflow extra that provides a few pre-defined ways to handle OverflowError exceptions, which - as explained - are neither bugs nor argument errors, but it does raise the exceptions in all the other cases.

from pagy.

I read all the discussion, and, actually, don't think it is a problem.

But, I do understand @espen 's point of view(had similar issue with "Postgres integer out of range"). ArgumentError is not nice. If pagy in this case provides the Pagy::ArgumentError, very few programmers will notice and appreciate it. Rare situation.

Because, if user somehow managed to enter "foo", it seems that he wants to brake the logic and not to receive real data. So this case may be ignored by the programmer. Unless it is requirement of a customer.

If you care about serializing user input for params[:page], it is easy to do with this example:

@pagy, @collection = pagy(scope, page: (params[:page].to_i rescue 1))

This is "bullet proof" handling of user input, including floating numbers.

Such example may be mentioned in "FAQ", and it will be enough.
Is such solution OK for you project, @espen ?

from pagy.

@espen, @workgena FYI: from version 3.3.1 all the exceptions are Pagy:: namespaced (i.e no more generic ArgumentError exceptions). They are all sub-classes of Pagy::VariableError (included the Pagy::OverfolowError) and since the Pagy::VariableError is a sub-class of ArgumentError it will not break any legacy rescues.

The Pagy::VariableError provides also the variable symbol of the offending variable and its value as accessors of the exception object.

IMO that's not a dramatic improvement, but it will be simpler to rescue even very specific errors, even for the most pointless reasons :)

HTH

from pagy.

How comes that :page get set to 0?
Are you doing something like page: params[:page].to_i explicitly?

from pagy.

Because it's user input from querystring (?page=foo) that is used when calling pagy(). Pagy is doing to_i here I believe: https://github.com/ddnexus/pagy/blob/master/lib/pagy.rb#L22

from pagy.

The :page can be an explicit integer >= 1. It can also be nil or "" and it would be set as the default 1 so it would work as expected.

Again, how comes that a user can set the input to a string like "foo" in your UI? If you are going to allow the user to set the page input in your UI, then use a numeric field with constraints. For example, all the *_compact navs have an input field where the user can choose any integer between 1 and @pagy.pages.

from pagy.

It's a querystring so the user can input anything they want in the URL. It's also an API so API clients can have a bug that they ask for page 0 or a string. Regardless a client side constraint, which can be bypassed, would still leave the backend open to resulting in a server error.

from pagy.

It's a querystring so the user can input anything they want in the URL.

Not really. No (final) user is supposed to enter anything in the query-string directly. It's the app that should handle the query-string. And if the "user" is the developer, then s/he is supposed to handle it correctly, i.e. ensuring it is a integer (or a "strigyfied" integer) and not a string, or (for easy handling) just passing the param straight and getting the default in case of blank param.

It's also an API so API clients can have a bug that they ask for page 0 or a string.

The goal of Pagy is not fixing the bugs of any API clients. If that is a goal of the app that implements the API (and it would be a silly goal IMO), the app itself could fix the API call before passing the wrong value to Pagy. Again, not a Pagy concern.

Regardless a client side constraint, which can be bypassed, [...]

If the client side constraint gets bypassed, than it means that the final user is tampering with the UI. Fixing a tampering attempt is not a concern of Pagy and indeed it is handled with a proper ArgumentError

[...] would still leave the backend open to resulting in a server error.

Indeed passing a string instead of an expected integer (or a stringyfied integer) IS resulting in a server error. Specifically a ArgumentError.

In conclusion I guess that the core of your concern is that you would like to have a Pagy::OutOfRangeError instead of an ArgumentError when the page is invalid (and generating a 0 when converted to a string in Pagy itself).

The PagyOutOfRangeError is just a special case of ArgumentError that is worth a special exception, because rescuing that specific exception could be useful. The known range accepted by Pagy is 1..last. Since the range-start is always 1 (constant), while the range-end is a variable integer, the exception is referring only to the variable range-end.

Strictly speaking that is a poor naming choice. I just realize that something like PageOverflowError would be a lot more appropriate name for that exception.

Also, the error feedback should report the value of the :page variable passed to the constructor and not the value after applying to_i, so if you pass page: 'foo' then you get an argument error saying expected :page >= 1; got "foo" instead of expected :page >= 1; got 0.

I will open a new issue to rename it and fix the feedback.
Thanks

from pagy.

If needed, please, continue the discussion in #103.

from pagy.

I disagree with many statements here. To sum up my view:

• The backend should be able to handle any input without raising an error. It should either result in a specific manageable raised error or silently fail by fallback input value to nil.
• What the user is supposed to do is irrelevant - the backend should not expect user input to be well-formed. Both API clients and user behaviour can either result in invalid input due to bugs or malicious behaviour.
• This is a pagy concern because it provides a GUI helper and also the backend call uses params directly. If I had to provide these variables to pagy then I would agree it should be handled outside pagy, but that is not the case.

IMHO ArgumentError should be a result of a developer using invalid argument in the code. Dynamic user input is not a developer argument error. This should raise Pagy::OutOfRangeError which can then be handled by the developer or just result in the overflow handler for example :empty_page.

The way it is now there is no way to catch this error without catching the generic ArgumentError. So the best case is to make a wrapper method that does input validation. I think that should be handled by the gem due to arguments listed above.

from pagy.

What is the difference between a user inputting an invalid page integer and a string input? Both are invalid and should result in an manageable error. But only the integer is possible to handle with the current error raising. Also only the invalid integer is handled internally by pagy.

There is not only bugs and malicious behaviour but also user errors (for example the pagy_nav_compact gives an input which the user can enter any page). Now this will result in a logged error which will cause error reporting based on user input - this should not be the case.

How do you suggest this should be handled when implementing pagy?

from pagy.

The string or integer is not a problem with Pagy IF the string actually contains the page number.

• OK: 23, "23", nil, ""
• OverflowError: 100000000000, "100000000000"
• ArgumentError: -2, "-2", 0, "0", "foo"

As you can understand if the user enters "foo" there is something wrong with the UI or the user behavior, hence Pagy raises an ArgumentError. You should actually be happy about it, because if that happens you want to have an exception and fix your bug or serving an error.

It's not true that "the user can enter any page" with the pagy_nav_compact! The helper creates an input tag that accepts only integers in the range 1..<last page> so it cannot generate any ArgumentError. However it could generate an OverflowError if the data changed after the rendering, but that case is totally covered, and you are not complaining about it.

Now this will result in a logged error which will cause error reporting based on user input - this should not be the case.

Indeed it is absolutely NOT the case! There will be no ArgumentError logged, because no ArgumentError can be generated by its helpers... if you create any special input helper you should do the same (i.e. restrict the input to avoid the ArgumentError).

from pagy.

The helper creates an input tag that accepts only integers in the range 1.. so it cannot generate any ArgumentError

If you rely on the browser restrictions and validations. Which on the backend you shouldn't.

For any high usage web-app this will be an issue. If every gem resulted in ArgumentError for invalid user input then error logs would be full of errors. Let's for example take a CMS which would have crawlers and bots doing all sort of things to forms - this would lead to many variations of user input.

As you can understand if the user enters "foo" there is something wrong with the UI or the user behavior, hence Pagy raises an ArgumentError. You should actually be happy about it, because if that happens you want to have an exception and fix your bug or serving an error.

The are two problems with this approach:

1. A backend in high usage web-app will get a lot of weird input regardless of UI. The backend should handle it. With your approach I have to handle invalid input in every gem and every situation.
2. If I implement a wrapper for pagy() and use it in my controllers I would have an issue if I do a mistake somewhere. Since I am now catching ArgumentError it will not only rescue that when user input is invalid but also when my code has a bug (for example I execute pagy() without any arguments). So this leads to a situation where I have a bug in my code but it will not be reported to my error service because I have to rescue ArgumentError.

Now this will result in a logged error which will cause error reporting based on user input - this should not be the case.

Indeed it is absolutely NOT the case!

What? It surely is. User input is "foo" and it will result in an error.

We obviously disagree on this. But I don't think I am the only one you would disagree with. This is not an issue in other gems. If I for example supply an invalid date to a gem handling dates it would either result in a specific error (ie InvalidDateError) or it will just silently "fail" and yield nil.

How do you suggest this should be handled when implementing pagy?

from pagy.

Which on the backend you shouldn't.

... if you are protecting Fort Knox and data that will get stored. In input that is only related to displaying stuff on the screen browser side restrictions and validation are good enough! Besides, I don't give a hut if a tampered input get an exception, and you should do the same. Did you read my previous answers?

For any high usage web-app this will be an issue.

Nope. It's not a quantitative problem: it's a qualitative problem, and the trigger of errors could only be bugs or malicious behaviour so, again I don't give a hut about those generating an exception or one million exceptions, and you should do the same.

Let's for example take a CMS which would have crawlers and bots doing all sort of things to forms - this would lead to many variations of user input.

so what?

The backend should handle it. With your approach I have to handle invalid input in every gem and every situation.

If you have a paranoid problem for input that just decides the display of a page, then write a paranoid extra and create a PR. Not sure it will get accepted though :)

So this leads to a situation where I have a bug in my code but it will not be reported to my error service because I have to rescue ArgumentError.

Seriously?

...
rescue ArgumentError => e
  if e.message.match(/....../)
    do_something
  else
    raise
  end
end

And BTW, rescue-ing is a bad idea when instead you could use the constructor properly by sanitizing/normalizing the input before using it. ArgumentError exceptions are there to remember you that you should not pass that kind of input to a method. So if you really want to handle malicious behavior gently and obfuscate bugs instead of do the right thing, please, feel free to do it in your code or write your paranoid extra that way.

What? It surely is. User input is "foo" and it will result in an error.

User input is "foo" only for that 2 causes that I already explained endless times and you seems confused with, and that indeed must raise an exception.

How do you suggest this should be handled when implementing pagy?

Sanitize/normalize your input before passing it to the constructor either in your code (best option) or propose a change in the backend (that will very likely be rejected) or write a paranoid extra that does that optionally (again not sure I will include that in the main repo, but I would certainly link it in the README if you write it).

I am afraid I exhausted myself and the time I can dedicate to this thread, so if you are still not convinced, please, submit some code. Thanks.

from pagy.

@workgena: params[:page].to_i rescue 1 is nice but still generates an ArgumentError for strings only, that to_i converts to 0.

from pagy.

Perhaps draft implementation of paranoid-extra might "open our eyes"(in a good way) on this use-case?

from pagy.

Very low priority IMO. It is still better to implement param normalization in the specific app code, if you really need that, and again, you would need that only for "ideological" reasons. :)

from pagy.

def normalized_current_page
    page = current_page.to_i rescue 1
    return 1 if page == 0
    max_pages = records.count.fdiv(per_page).ceil
    if page > max_pages
      max_pages
    else
      page
    end
end

This could help

from pagy.

I think the issue should be handled by pagy gem. If my url asks for page > available, give me the last page.

The amount of records can change between when the page gets generated and the 'next' page is requested. This is very likely with a multi-user system.

Also by the way if any other bad parameter is passed... just return page 1

Otherwise I have to rescue all calls to pagy. If you want the option to raise error, you can make it an option.

from pagy.

Very low priority IMO. It is still better to implement param normalization in the specific app code, if you really need that, and again, you would need that only for "ideological" reasons. :)

I don't believe that is true for out of bounds page number. The number of pages can change in another session/thread and now i have to check the the page is within range. Page is already checking, why should I rewrite code.
A page_default :first or :last should be implemented.

from pagy.

@weiserma you don't have to rewrite any code nor check if the page is within range.

Pagy does it for you, you can use the Overflow extra and/or go deeper at any level of detail by handling exceptions

from pagy.

# config/initializers/pagy.rb
require 'page/extras/overflow'

Pagy::DEFAULT[:overflow] = :empty_page

# controllers
@pagy, @collection = pagy(scope, page: [params[:page].to_i, 1].max)

I prefer this.

from pagy.