Fix: rescue malformed page param in Pagy::Keyset and KeynavJsPaginator#907
Merged
Merged
Conversation
Two sibling locations call `JSON.parse(B64.urlsafe_decode(page))` on an attacker-controlled request param without rescue. Any malformed ?page= value (invalid base64, or valid base64 of non-JSON) raises uncaught and propagates as a 500. This patch rescues both `ArgumentError` and `JSON::ParserError` at both call sites, matching `Pagy::Request#resolve_page`'s existing silent-fallback-to-page-1 convention for malformed integer page params. Before: GET /items?page=garbage -> 500 (JSON::ParserError or ArgumentError) After: GET /items?page=garbage -> 200 first page Sites fixed: - gem/lib/pagy/classes/keyset/keyset.rb (Pagy::Keyset) - gem/lib/pagy/toolbox/paginators/keynav_js.rb (KeynavJsPaginator) Tests cover both ArgumentError and JSON::ParserError paths against both Pet (ActiveRecord) and PetSequel (Sequel) models for Keyset, and Pet (ActiveRecord) for KeynavJsPaginator.
750465a to
669e0d7
Compare
Owner
|
@7a6163 Thank you! This is one of the best contribution Pagy ever received! 🙌 While reviewing your changes, I noticed that the my original Thank you. |
Contributor
Author
|
Thanks @ddnexus — much cleaner. Tests green locally. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two sibling locations call
JSON.parse(B64.urlsafe_decode(page))on an attacker-controlled request param without rescuing exceptions. Any malformed?page=value (invalid base64, or valid base64 of non-JSON) raises uncaught and propagates as a 500:Affected sites:
gem/lib/pagy/classes/keyset/keyset.rb—Pagy::Keyset#assign_pagegem/lib/pagy/toolbox/paginators/keynav_js.rb—KeynavJsPaginator.paginateThis patch rescues both
ArgumentErrorandJSON::ParserErrorat both call sites, matchingPagy::Request#resolve_page's existing silent-fallback-to-page-1 convention for malformed integer page params.Why
This is not a security issue — an attacker doesn't gain data, access, or availability, and Ruby/puma handles 500 responses as fast as 200s so there's no DoS amplification. It's filed as a robustness fix.
But the small change improves operational behavior:
Changes
gem/lib/pagy/classes/keyset/keyset.rb— method-levelrescue JSON::ParserError, ArgumentErrorthat nils@pagegem/lib/pagy/toolbox/paginators/keynav_js.rb—begin/rescuearound the inline JSON.parse sooptions[:page]stays unset on parse failuretest/unit/pagy/classes/keyset/keyset_test.rb— 2 new tests covering both exception paths against bothPet(ActiveRecord) andPetSequel(Sequel) via the existing[Pet, PetSequel].eachlooptest/unit/pagy/toolbox/paginators/keynav_test.rb— 2 new tests covering both exception paths viaMockAppTest plan
bundle exec ruby -Ilib:test -Igem/lib -e 'ARGV.each{|f| require "./#{f}"}' test/unit/pagy/classes/keyset/keyset_test.rb— 43 tests, 92 assertions passbundle exec ruby -Ilib:test -Igem/lib -e 'ARGV.each{|f| require "./#{f}"}' test/unit/pagy/toolbox/paginators/keynav_test.rb— 6 tests, 14 assertions passbundle exec thor test:api:all— current + next API both green, 0 failures, 0 regressionsbundle exec rubocopon all 4 changed files — 0 offensesbundle exec thor test:api:coverageis currently 95.46% line / 98.70% branch; this patch goes to 95.47% / 98.71%, net positive)Notes for reviewer
@page = nilon rescue inKeyset#assign_pageis intentional — downstreamattr_reader :pagethen returns nil, matching the "no page param given" code path.@prior_cutoffstays unset (since theJSON.parseline that would set it didn't complete), sofetch_recordscorrectly skips theapply_where(...)branch.KeynavJsPaginator.paginate, leavingoptions[:page]unset on rescue meansKeyset::Keynav#assign_page(line 49) takes the@page = @last = 1branch — same first-page behavior as if no page param had been sent.masterper CONTRIBUTING.md.