diff --git a/CHANGELOG.md b/CHANGELOG.md index 251168b..a40dfde 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ 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) +- 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 5e69e3d..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] @@ -160,13 +168,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 +189,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], @@ -201,9 +209,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? @@ -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 87b1852..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,12 +123,20 @@ 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 _(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 @@ -157,5 +172,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