From b63f3ae1494f50b0305e5565d0904e37a78d4215 Mon Sep 17 00:00:00 2001 From: makaiver Date: Wed, 27 May 2026 10:07:18 +0200 Subject: [PATCH 1/7] fix: normalize nil group param to prevent NoMethodError in version/diffs views When a request is made to /node/version/view or /node/version/diffs without a group= query parameter, params[:group] returns nil. The HAML templates check `@info[:group] != ''` which evaluates to true for nil, then attempt `nil + '/' + node` causing: NoMethodError: undefined method `+' for nil Fix by normalizing params[:group] to '' when absent using `|| ''`, so the existing guards in the templates behave correctly. Also pass nil to get_version when group is empty, preserving the original API contract. Verified against oxidized/oxidized:latest (oxidized-web 0.18.1): curl "http://localhost:8888/node/version/view?node=test&num=1" Before: HTTP 500 NoMethodError After: no crash --- lib/oxidized/web/webapp.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/oxidized/web/webapp.rb b/lib/oxidized/web/webapp.rb index 5e69e3d..aa725a2 100644 --- a/lib/oxidized/web/webapp.rb +++ b/lib/oxidized/web/webapp.rb @@ -160,13 +160,13 @@ class WebApp < Sinatra::Base node, @json = route_parse :node @info = { node: node, - group: params[:group], + group: params[:group] || '', oid: params[:oid], time: Time.at(params[:epoch].to_i), num: params[:num] } - the_data = nodes.get_version node, @info[:group], @info[:oid] + the_data = nodes.get_version node, @info[:group].empty? ? nil : @info[:group], @info[:oid] if %w[json text].include?(params[:format]) @data = the_data else @@ -181,7 +181,7 @@ class WebApp < Sinatra::Base node, @json = route_parse :node @data = nil @info = { node: node, - group: params[:group], + group: params[:group] || '', oid: params[:oid], time: Time.at(params[:epoch].to_i), num: params[:num], From a48c1176d4b4080eb1269b4581857960a7e9dae5 Mon Sep 17 00:00:00 2001 From: makaiver Date: Wed, 27 May 2026 18:50:20 +0200 Subject: [PATCH 2/7] Fix get_diff called with '' instead of nil for ungrouped nodes The /node/version/diffs route correctly normalises params[:group] to '' and builds a local `group` variable (nil or value) for nodes.version, but then passed @info[:group] (which is '' for ungrouped nodes) directly to both nodes.get_diff calls. In oxidized's yield_repo_and_path, an empty string is truthy, so the path was built as "/node-name" instead of "node-name", causing get_diff to silently return 'no diffs' for all ungrouped nodes. Fix: use the already-correct local `group` variable in both get_diff calls. --- lib/oxidized/web/webapp.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/oxidized/web/webapp.rb b/lib/oxidized/web/webapp.rb index aa725a2..c2463da 100644 --- a/lib/oxidized/web/webapp.rb +++ b/lib/oxidized/web/webapp.rb @@ -201,9 +201,9 @@ class WebApp < Sinatra::Base @info[:num2] = num break end - @data = nodes.get_diff node, @info[:group], @info[:oid], oid2 + @data = nodes.get_diff node, group, @info[:oid], oid2 else - @data = nodes.get_diff node, @info[:group], @info[:oid], nil + @data = nodes.get_diff node, group, @info[:oid], nil end @stat = %w[null null] if @data != 'no diffs' && !@data.nil? From 0acd26b9e093eb3c2c020e673046bf69bb134c60 Mon Sep 17 00:00:00 2001 From: makaiver Date: Wed, 27 May 2026 19:00:16 +0200 Subject: [PATCH 3/7] Add tests for nil group param and update CHANGELOG - spec/web/node/version_spec.rb: add three tests covering the case where group= is entirely absent from the query string for /node/version/view and /node/version/diffs (the original NoMethodError scenario) - CHANGELOG.md: add Fixed entry under [Unreleased] for the nil group param NoMethodError fix --- CHANGELOG.md | 1 + spec/web/node/version_spec.rb | 40 ++++++++++++++++++++++++++++++++++- 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 251168b..0858b90 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Changed ### Fixed +- Fix `NoMethodError` when `group=` param is absent from `/node/version/view` and `/node/version/diffs` requests. `params[:group]` was assigned directly to `@info[:group]`; the HAML templates and `get_diff` calls then called methods on `nil`. Normalise to `''` on input and pass `nil` to the oxidized backend where required (@makaiver) ## [0.18.1 – 2026-01-19] diff --git a/spec/web/node/version_spec.rb b/spec/web/node/version_spec.rb index 87b1852..f3a89d4 100644 --- a/spec/web/node/version_spec.rb +++ b/spec/web/node/version_spec.rb @@ -122,6 +122,14 @@ def app _(last_response.ok?).must_equal true _(last_response.body).must_equal '["text &/<> \n"," ascii;"]' end + + it 'returns 200 when group param is absent (no NoMethodError)' do + @nodes.expects(:get_version).with('sw5', nil, 'c8aa93cab5').returns('Old configuration of sw5') + + get '/node/version/view?node=sw5&oid=c8aa93cab5&epoch=1738781340&num=2' + _(last_response.ok?).must_equal true + _(last_response.body.include?('Old configuration of sw5')).must_equal true + end end describe 'get /node/version/diffs' do @@ -142,7 +150,7 @@ def app "+changed line new2\n " \ "\n " \ "unchanged line\n", - stat: [1, 2] } + stat: [1, 2] } @nodes.expects(:version).with('sw5', nil).returns(@versions) @nodes.expects(:get_diff).returns(@diff) @@ -157,5 +165,35 @@ def app "#{Time.at(1738781340)}" )).must_equal true end + + it 'returns 200 when group param is absent (no NoMethodError)' do + @versions = [ + { oid: "C006", time: Time.parse("2025-02-05 19:49:00 +0100") }, + { oid: "C003", time: Time.parse("2025-02-05 19:03:00 +0100") } + ] + @diff = { patch: "diff --git a/sw5 b/sw5\nsome diff\n", stat: [1, 1] } + + @nodes.expects(:version).with('sw5', nil).returns(@versions) + @nodes.expects(:get_diff).with('sw5', nil, 'C006', nil).returns(@diff) + + get '/node/version/diffs?node=sw5&oid=C006&epoch=1738781340&num=2' + _(last_response.ok?).must_equal true + end + + it 'passes nil group to get_diff for ungrouped nodes' do + @versions = [ + { oid: "C006", time: Time.parse("2025-02-05 19:49:00 +0100") }, + { oid: "C003", time: Time.parse("2025-02-05 19:03:00 +0100") } + ] + @diff = { patch: "diff --git a/sw5 b/sw5\nsome diff\n", stat: [1, 1] } + + # Explicitly assert nil is passed, not '' — empty string would resolve + # to wrong git path in oxidized's yield_repo_and_path + @nodes.expects(:version).with('sw5', nil).returns(@versions) + @nodes.expects(:get_diff).with('sw5', nil, 'C006', nil).returns(@diff) + + get '/node/version/diffs?node=sw5&oid=C006&epoch=1738781340&num=2' + _(last_response.ok?).must_equal true + end end end From 6b77ed153af0ea3ae9e9a273562bf83dc02dd9ec Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 27 May 2026 17:05:22 +0000 Subject: [PATCH 4/7] Initial plan From cdb7ff3c430c8dc00ce79cf90a9dd3534f95ee59 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 27 May 2026 17:12:36 +0000 Subject: [PATCH 5/7] Fix RuboCop hash alignment in version spec --- spec/web/node/version_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/web/node/version_spec.rb b/spec/web/node/version_spec.rb index f3a89d4..ba5cb89 100644 --- a/spec/web/node/version_spec.rb +++ b/spec/web/node/version_spec.rb @@ -150,7 +150,7 @@ def app "+changed line new2\n " \ "\n " \ "unchanged line\n", - stat: [1, 2] } + stat: [1, 2] } @nodes.expects(:version).with('sw5', nil).returns(@versions) @nodes.expects(:get_diff).returns(@diff) From 301d607d6b6f9a207b94f97edb299811785daa0a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 1 Jun 2026 08:11:24 +0000 Subject: [PATCH 6/7] Initial plan From 6370b57370e6207c28ce6e29508a2e3b977b999a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 1 Jun 2026 08:30:42 +0000 Subject: [PATCH 7/7] Guard nil params and add crash-regression specs --- CHANGELOG.md | 1 + lib/oxidized/web/webapp.rb | 24 ++++++++++++++------- spec/web/conf_search_spec.rb | 40 +++++++++++++++++++++++++++++++++++ spec/web/node/version_spec.rb | 15 +++++++++---- 4 files changed, 68 insertions(+), 12 deletions(-) create mode 100644 spec/web/conf_search_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 0858b90..a40dfde 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed - Fix `NoMethodError` when `group=` param is absent from `/node/version/view` and `/node/version/diffs` requests. `params[:group]` was assigned directly to `@info[:group]`; the HAML templates and `get_diff` calls then called methods on `nil`. Normalise to `''` on input and pass `nil` to the oxidized backend where required (@makaiver) +- Fix `NoMethodError` and `RegexpError` crashes on `/node/version` and `/nodes/conf_search` when params are missing or contain a malformed regex; return empty results instead of a 500 (@makaiver) ## [0.18.1 – 2026-01-19] diff --git a/lib/oxidized/web/webapp.rb b/lib/oxidized/web/webapp.rb index c2463da..0253890 100644 --- a/lib/oxidized/web/webapp.rb +++ b/lib/oxidized/web/webapp.rb @@ -65,13 +65,21 @@ class WebApp < Sinatra::Base end post '/nodes/conf_search.?:format?' do - @to_research = Regexp.new params[:search_in_conf_textbox] - nodes_list = nodes.list.map + search_term = params[:search_in_conf_textbox].to_s @nodes_match = [] - nodes_list.each do |n| - node, @json = route_parse n[:name] - config = nodes.fetch node, n[:group] - @nodes_match.push({ node: n[:name], full_name: n[:full_name] }) if config[@to_research] + unless search_term.empty? + begin + @to_research = Regexp.new search_term + rescue RegexpError + @to_research = nil + end + if @to_research + nodes.list.map.each do |n| + node, @json = route_parse n[:name] + config = nodes.fetch node, n[:group] + @nodes_match.push({ node: n[:name], full_name: n[:full_name] }) if config[@to_research] + end + end end @data = @nodes_match out :conf_search @@ -142,7 +150,7 @@ class WebApp < Sinatra::Base @data = nil @group = nil @node = nil - node_full = params[:node_full] + node_full = params[:node_full].to_s if node_full.include? '/' node_full = node_full.rpartition("/") @group = node_full[0] @@ -254,7 +262,7 @@ def route_parse(param) e = if param.respond_to?(:to_str) param.split '.' else - params[param].split '.' + params[param].to_s.split '.' end if e.last == 'json' e.pop diff --git a/spec/web/conf_search_spec.rb b/spec/web/conf_search_spec.rb new file mode 100644 index 0000000..acad9e5 --- /dev/null +++ b/spec/web/conf_search_spec.rb @@ -0,0 +1,40 @@ +require_relative '../spec_helper' + +describe Oxidized::API::WebApp do + include Rack::Test::Methods + + def app + Oxidized::API::WebApp + end + + before do + @nodes = mock('Oxidized::Nodes') + app.set(:nodes, @nodes) + end + + describe 'post /nodes/conf_search' do + it 'returns 200 with empty results when search_in_conf_textbox is missing' do + post '/nodes/conf_search' + _(last_response.ok?).must_equal true + end + + it 'returns 200 with empty results when search_in_conf_textbox is blank' do + post '/nodes/conf_search', search_in_conf_textbox: '' + _(last_response.ok?).must_equal true + end + + it 'returns 200 with empty results when regex is malformed (no crash)' do + post '/nodes/conf_search', search_in_conf_textbox: '[' + _(last_response.ok?).must_equal true + end + + it 'returns 200 with matches when regex is valid' do + @nodes.expects(:list).returns( + [{ name: 'sw5', full_name: 'sw5', group: nil }] + ) + @nodes.expects(:fetch).with('sw5', nil).returns("hostname sw5\n") + post '/nodes/conf_search', search_in_conf_textbox: 'hostname' + _(last_response.ok?).must_equal true + end + end +end diff --git a/spec/web/node/version_spec.rb b/spec/web/node/version_spec.rb index ba5cb89..132c65a 100644 --- a/spec/web/node/version_spec.rb +++ b/spec/web/node/version_spec.rb @@ -71,11 +71,18 @@ def app "2025-02-05 19:49:00 +0100\n" )).must_equal true end + + it 'returns 200 when node_full is absent (no NoMethodError)' do + @nodes.expects(:version).with('', nil).returns([]) + + get '/node/version' + _(last_response.ok?).must_equal true + end end describe 'get /node/version/view.?:format?' do it 'fetches a previous version from git' do - @nodes.expects(:get_version).with('sw5', '', 'c8aa93cab5').returns('Old configuration of sw5') + @nodes.expects(:get_version).with('sw5', nil, 'c8aa93cab5').returns('Old configuration of sw5') get '/node/version/view?node=sw5&group=&oid=c8aa93cab5&epoch=1738781340&num=2' _(last_response.ok?).must_equal true @@ -90,7 +97,7 @@ def app end it 'does not display binary content' do - @nodes.expects(:get_version).with('sw5', '', 'c8aa93cab5').returns("\xff\x42 binary content\x00") + @nodes.expects(:get_version).with('sw5', nil, 'c8aa93cab5').returns("\xff\x42 binary content\x00") get '/node/version/view?node=sw5&group=&oid=c8aa93cab5&epoch=1738781340&num=2' _(last_response.ok?).must_equal true @@ -107,7 +114,7 @@ def app it 'does not encode html-chars in text-format' do configuration = "text &/<> \n ascii;" - @nodes.expects(:get_version).with('sw5', '', 'c8aa93cab5').returns(configuration) + @nodes.expects(:get_version).with('sw5', nil, 'c8aa93cab5').returns(configuration) get '/node/version/view?node=sw5&group=&oid=c8aa93cab5&epoch=1738781340&num=2&format=text' _(last_response.ok?).must_equal true @@ -116,7 +123,7 @@ def app it 'does not encode html-chars in json-format' do configuration = "text &/<> \n ascii;" - @nodes.expects(:get_version).with('sw5', '', 'c8aa93cab5').returns(configuration) + @nodes.expects(:get_version).with('sw5', nil, 'c8aa93cab5').returns(configuration) get '/node/version/view?node=sw5&group=&oid=c8aa93cab5&epoch=1738781340&num=2&format=json' _(last_response.ok?).must_equal true