Better handling for weird data passed to the API via limit and offset #12827
Conversation
Signed-off-by: snipe <snipe@snipe.net>
snipe
requested review from
uberbrady,
marcusmoore,
spencerrlongg and
Godmartinz
PR Summary
|
There was a problem hiding this comment.
This seems pretty reasonable, but I do have a couple of questions -
- We don't use the
intval()thing when comparing the$request->input('limit')value to theconfig('app.max_results')value - I don't know if that's a big deal or not though. - We are doing a second
max()call when we already compared these values before. Is that necessary? - All of this feels a little...repetitive. Is there something we can do that's just a little more systemic so that we aren't repeating ourselves all the time? Maybe something from here: https://laravel.com/docs/9.x/pagination ?
All of that being said, those are just tiny nits and I don't think sufficient to block this from acceptance, if I can do a couple of key tests first (which, I will do).
Please let me know your thoughts on this and I'll get to testing.
@uberbrady I think we can safely assume |
| ((config('app.max_results') >= $request->input('limit')) && ($request->filled('limit'))) ? $limit = $request->input('limit') : $limit = config('app.max_results'); | ||
| // Make sure the offset and limit are actually integers and do not exceed system limits | ||
| $offset = ($request->input('offset') > $accessories->count()) ? $accessories->count() : intval(request('offset')); | ||
| $limit = ($request->input('limit') > config('app.max_results')) ? config('app.max_results') : max(intval(request('offset')), config('app.max_results')); |
There was a problem hiding this comment.
I might be missing something but let's say MAX_RESULTS=50 and the user requests ?limit=10 then $limit would be set to 50. Is that expected?
There was a problem hiding this comment.
I don't think so?
[2023-04-12 11:58:32] local.DEBUG: Max Results: 200
[2023-04-12 11:58:32] local.DEBUG: Original Requested Limit: 10
[2023-04-12 11:58:32] local.DEBUG: Intval Limit: 10
[2023-04-12 11:58:32] local.DEBUG: Modified Limit: 10
[2023-04-12 11:58:32] local.DEBUG: Original Requested Offset: -2
[2023-04-12 11:58:32] local.DEBUG: Modified Offset: 0
[2023-04-12 11:58:32] local.DEBUG: Total Results: 209
I can dig deeper shortly, but I believe it's doing the right thing here.
I don't think we really need it there - if you pass us a real number, it wouldn't matter. If you pass us garbage, that will shake out when we finally decide what value it needs to be.
I'm not sure what you mean.
As discussed on the call this AM, yes and no. Bootstrap-table requires our data to exist in a specific format (which is why we have all of those API transformers, etc.) It's possible we can get the best of both worlds - it's something I've been thinking about for a while. This PR is just meant to be a middle-step so shitheels who bought Burpsuite for Dummies don't poop all over our logs, but it's very possible that there's a better longer term solution possible here.
|
|
An alternative to this PR would be a piece of middleware (or in a provider or error handler) that simply rejects |
Hm...I like that idea a lot... |
|
I'm going to merge this into develop now and keep testing |
So, every now and then, people think they're cute and they start passing bad data to the API to see how it breaks. Currently it will just return a 500 error, which is fine I suppose, but it tends to poop all over our logging system.
This should set us up with sane default, while also still making sure that you're never going to get more results than the
MAX_RESULTSin the.env.When I had some debugging code in there, I tested this with multiple different URL parameters, and the numbers all look the way I'd expect:
I've tested this and all looks well, however this touches basically every major listing endpoint we have, so I'd appreciate some additional testing :)