Guard remaining nil/invalid request params in version and config-search routes to prevent 500s#3
Merged
makaiver merged 2 commits intoJun 1, 2026
Conversation
Copilot
AI
changed the title
[WIP] Fix additional NoMethodError in node version requests
Guard remaining nil/invalid request params in version and config-search routes to prevent 500s
Jun 1, 2026
makaiver
marked this pull request as ready for review
June 1, 2026 08:33
Copilot stopped work on behalf of
makaiver due to an error
June 1, 2026 08:34
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.
route_parseagainst nil params (useparams[param].to_s.split('.'))/node/versionagainst missingnode_fullparam/nodes/conf_searchagainst missing/blank input and rescueRegexpErrorspec/web/conf_search_spec.rbregression specs (missing & malformed inputs)/node/versionno-node_fullregression specget_versionmock expectations tonilgroup (matches fix: normalize nil group param to prevent NoMethodError ytti/oxidized-web#441 behavior)## [Unreleased]→### Fixedbundle exec rake→ 47 runs, 0 failures, RuboCop cleanOriginal prompt
Goal
Extend the existing PR ytti#441 by committing directly onto its head branch
fix/nil-group-param-nomethoderrorin this fork (makaiver/oxidized-web). Do NOT create a new branch and do NOT open a new pull request — push commits ontofix/nil-group-param-nomethoderrorso they appear in the already-open PR ytti#441.PR ytti#441 already fixed a
NoMethodErrorwhen thegroup=param is absent from/node/version/viewand/node/version/diffs. That fix is correct but incomplete: several sibling request params on the same and adjacent routes are still used without a nil-guard and each crashes the app with HTTP 500 on a single unauthenticated request. Build on top of the existing changes — do NOT revert or duplicate the existinggroupnormalization (params[:group] || ''and the.empty? ? nil : .../group = nil if ...logic) or the two specs already added inspec/web/node/version_spec.rb.Empirically confirmed crashes (all return HTTP 500 on current branch head)
lib/oxidized/web/webapp.rbGET /node/version/view?oid=...&epoch=...&num=2(nonode=)NoMethodError: undefined method 'split' for nilroute_parse→params[param].splitGET /node/version/diffs?oid=...&epoch=...&num=2(nonode=)NoMethodError: undefined method 'split' for nilroute_parse→params[param].splitGET /node/version(nonode_full=)NoMethodError: undefined method 'include?' for nilnode_full.include? '/'(~line 145-146)POST /nodes/conf_search(nosearch_in_conf_textbox)TypeError: no implicit conversion of nil into StringRegexp.new params[:search_in_conf_textbox](~line 67-68)POST /nodes/conf_searchwithsearch_in_conf_textbox=(RegexpError: end pattern with unmatched parenthesisRegexp.new params[:search_in_conf_textbox]The
groupparam (already fixed by ytti#441) is the control and does NOT crash.Required code changes in
lib/oxidized/web/webapp.rbGuard
route_parseagainst a nil param. It currently doesparams[param].split '.', which raises whenparams[param]is nil. Normalize nil to an empty string before splitting (e.g.params[param].to_s.split('.')) so a missing param yields['', false]instead of crashing. Preserve existing behavior for present params and the.jsonsuffix detection (including the branch whereparam.respond_to?(:to_str)). This makes/node/version/viewand/node/version/diffsreturn a normal (non-500) response whennodeis absent.Guard
node_fullonGET /node/version.?:format?.node_full = params[:node_full]thenif node_full.include? '/'crashes when nil. Useparams[:node_full].to_s(or an explicit nil-guard) so an absentnode_fullis treated as an empty node name. Preserve the existing group/node splitting when a/is present.Guard
conf_searchonPOST /nodes/conf_search.?:format?.@to_research = Regexp.new params[:search_in_conf_textbox]crashes on nil (TypeError) and on malformed regex like((RegexpError). Normalize a missing/blank search term (return an empty result set instead of crashing) and wrap theRegexp.newso a malformed user pattern returns a clean response (empty results) rather than a 500 — rescueRegexpError(and handle nil/blank up front). Keep the existing valid-input search behavior. Note this is also a minor DoS surface (unvalidated user regex), so failing safe matters.Do not change unrelated behavior. Keep ytti#441's
grouphandling intact.Tests —
spec/web/node/version_spec.rb(+ conf_search spec if needed)The repo uses minitest + rack-test + mocha. Mirror the style of the two specs already added in ytti#441 (e.g.
it 'returns 200 when group param is absent (no NoMethodError)'). Use@nodes = mock('Oxidized::Nodes')and setexpects(...)only for backend calls that should still occur after the guard. For routes that crash before any backend call, set expectations so mocha doesn't fail on unmet/unexpected calls. For conf_search, the handler iteratesnodes.list, so stub@nodes.expects(:list).returns([])to isolate it.Add specs asserting a non-500 response after the fix for:
/node/version/viewwithoutnode=/node/version/diffswithoutnode=/node/versionwithoutnode_full=POST /nodes/conf_searchwithoutsearch_in_conf_textboxPOST /nodes/conf_searchwith malformed regexsearch_in_conf_textbox=(Run
bundle exec rake testand ensure the whole suite (including existing tests) passes.Rubocop
Run
rubocop(tryrubocop -A/--autocorrect) and ensure the final diff has no new offenses, respecting.rubocop.yml(plugins: rubocop-rails, rubocop-rake, rubocop-minitest;Style/StringLiteralsdisabled,Style/FrozenStringLiteralCommentdisabled, verbose hash methods,Layout/LineLengthdisabled).CHANGELOG.md
Append a new bullet to the existing
### Fixedsection under `...This pull request was created from Copilot chat.