fix(adapter): sanitize page parameter to prevent ArgumentError (#7830) - #7832
fix(adapter): sanitize page parameter to prevent ArgumentError (#7830)#7832UwaisBaig wants to merge 1 commit into
Conversation
WalkthroughThe change adds protected page sanitization to Merge Risk: 🟡 Moderate · up to The change prevents malformed page values from crashing searches, but requests that omit the page parameter can still produce an application error instead of defaulting to the first page. Merge should wait until missing values are normalized and covered by a regression test. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes address issue
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: 8fac2f69-962b-4299-8516-f1852db6ec6b
📒 Files selected for processing (4)
app/lib/adapters/base_adapter.rbapp/lib/adapters/pg_adapter.rbapp/lib/adapters/solr_adapter.rbspec/lib/adapters/pg_adapter_spec.rb
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
| protected | ||
|
|
||
| def sanitize_page(page) | ||
| page_num = page.to_i |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether production search callers can omit the page parameter.
rg -n -C 8 'search_(project|user)|query_params\[:page\]|page:' app specRepository: CircuitVerse/CircuitVerse
Length of output: 39640
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- repository conventions and learnings ---'
find /tmp/coderabbit-repo-knowledge/circuitverse-circuitverse-08827a34 \
-maxdepth 2 -type f -name '*.md' -print
printf '%s\n' '--- base adapter ---'
cat -n app/lib/adapters/base_adapter.rb
printf '%s\n' '--- direct search flow ---'
cat -n app/lib/adapters/pg_adapter.rb | sed -n '23,55p'
cat -n app/lib/adapters/solr_adapter.rb | sed -n '1,30p'
printf '%s\n' '--- query object and direct callers ---'
rg -n -C 6 'ProjectsQuery|UsersQuery|Adapters::|search_project|search_user' app specRepository: CircuitVerse/CircuitVerse
Length of output: 23244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- query base class ---'
fd -i 'generic_query|search' app/queries app/controllers app/helpers app/lib \
| sort
cat -n app/queries/generic_query.rb
printf '%s\n' '--- search-related controller flow ---'
rg -n -C 10 'def search|Search.new|query\(' app/controllers app/helpers app/queries app/lib
printf '%s\n' '--- relevant repository learnings ---'
cat /tmp/coderabbit-repo-knowledge/circuitverse-circuitverse-08827a34/learnings/null.md
cat /tmp/coderabbit-repo-knowledge/circuitverse-circuitverse-08827a34/learnings/app-controllers.mdRepository: CircuitVerse/CircuitVerse
Length of output: 12156
Handle an omitted page before calling to_i.
SearchController#search passes raw params to the query objects. If page is omitted, query_params[:page] is nil, and Adapters::PgAdapter#perform_search reaches BaseAdapter#sanitize_page, where nil.to_i raises NoMethodError before the fallback. Normalize the missing value before conversion and add a regression case.
Summary of Changes
Fixes #7830 (Sentry
CIRCUITVERSE-CORE-155) where passing non-integer or malicious SQL-like strings in thepagequery parameter (e.g.,"1' AND 1=1 UNION SELECT NULL-- -") causedwill_paginateto raiseArgumentError: invalid value for Integer(): ....Changes made:
app/lib/adapters/base_adapter.rb: Addedsanitize_page(page)helper method that coerces the input to an integer (page.to_i) and defaults non-positive / unparseable values to1.app/lib/adapters/pg_adapter.rb: Appliedsanitize_page(query_params[:page])inperform_searchbefore passing topaginate.app/lib/adapters/solr_adapter.rb: Appliedsanitize_page(query_params[:page])insearch_projectandsearch_user.spec/lib/adapters/pg_adapter_spec.rb: Added unit tests for page parameter sanitization.Related Issue
Summary by CodeRabbit
Bug Fixes
Tests