diff --git a/app/assets/javascripts/foreman_remote_execution/template_invocation.js b/app/assets/javascripts/foreman_remote_execution/template_invocation.js index f7c6e4162..175bba9fc 100644 --- a/app/assets/javascripts/foreman_remote_execution/template_invocation.js +++ b/app/assets/javascripts/foreman_remote_execution/template_invocation.js @@ -47,7 +47,9 @@ function show_preview_hosts_modal() { url: $('#previewHostsModal').attr('data-url') }).then(function(result){ var modal_window = $('#previewHostsModal'); - modal_window.find('.modal-body').html(result); + // Sanitize the result to prevent XSS attacks + var sanitizedResult = $('
').text(result).html(); + modal_window.find('.modal-body').html(sanitizedResult); modal_window.modal({'show': true}); modal_window.find('a[rel="popover-modal"]').popover(); }); diff --git a/app/controllers/cockpit_controller.rb b/app/controllers/cockpit_controller.rb index ccdefc1c7..0c8c4135d 100644 --- a/app/controllers/cockpit_controller.rb +++ b/app/controllers/cockpit_controller.rb @@ -8,15 +8,23 @@ def host_ssh_params def redirect return invalid_request unless params[:redirect_uri] - redir_url = URI.parse(params[:redirect_uri]) + begin + redir_url = URI.parse(params[:redirect_uri]) + rescue URI::InvalidURIError + return invalid_request + end + + # Validate URL scheme to prevent javascript: or data: schemes + return invalid_request unless %w[http https].include?(redir_url.scheme&.downcase) cockpit_url = ScriptExecutionProvider.cockpit_url_for_host('') - redir_url.query = if redir_url.hostname == URI.join(Setting[:foreman_url], cockpit_url).hostname - "access_token=#{request.session_options[:id]}" - else - "error_description=Sorry" - end - redirect_to(redir_url.to_s) + expected_hostname = URI.join(Setting[:foreman_url], cockpit_url).hostname + + # Only redirect to expected hostname to prevent open redirects + return invalid_request unless redir_url.hostname == expected_hostname + + redir_url.query = "access_token=#{request.session_options[:id]}" + redirect_to(redir_url.to_s, allow_other_host: true) end private diff --git a/test/functional/cockpit_controller_test.rb b/test/functional/cockpit_controller_test.rb index f726ea8c2..216fc2793 100644 --- a/test/functional/cockpit_controller_test.rb +++ b/test/functional/cockpit_controller_test.rb @@ -13,4 +13,69 @@ def setup response = ActiveSupport::JSON.decode(@response.body) assert response.key?('ssh_user'), 'ssh_params response must include ssh_user' end + + describe 'redirect action security tests' do + setup do + Setting[:foreman_url] = 'https://foreman.example.com' + # Mock the cockpit URL + ScriptExecutionProvider.stubs(:cockpit_url_for_host).returns('/cockpit') + end + + test "should reject redirect without redirect_uri parameter" do + get :redirect, session: set_session_user + assert_response :bad_request + end + + test "should reject redirect with invalid URI" do + get :redirect, params: { redirect_uri: 'not-a-valid-uri::' }, session: set_session_user + assert_response :bad_request + end + + test "should reject redirect with javascript scheme" do + get :redirect, params: { redirect_uri: 'javascript:alert("xss")' }, session: set_session_user + assert_response :bad_request + end + + test "should reject redirect with data scheme" do + get :redirect, params: { redirect_uri: 'data:text/html,' }, session: set_session_user + assert_response :bad_request + end + + test "should reject redirect with ftp scheme" do + get :redirect, params: { redirect_uri: 'ftp://evil.com/file' }, session: set_session_user + assert_response :bad_request + end + + test "should reject redirect to wrong hostname" do + get :redirect, params: { redirect_uri: 'https://evil.com/cockpit' }, session: set_session_user + assert_response :bad_request + end + + test "should reject redirect to subdomain attack" do + get :redirect, params: { redirect_uri: 'https://evil.foreman.example.com/cockpit' }, session: set_session_user + assert_response :bad_request + end + + test "should allow redirect to valid cockpit URL" do + valid_uri = 'https://foreman.example.com/cockpit/path' + get :redirect, params: { redirect_uri: valid_uri }, session: set_session_user + assert_response :redirect + + # Verify the redirect includes the access token + location = response.location + assert_includes location, 'access_token=' + assert_includes location, 'https://foreman.example.com/cockpit/path' + end + + test "should allow redirect with http scheme to same hostname" do + valid_uri = 'http://foreman.example.com/cockpit/path' + get :redirect, params: { redirect_uri: valid_uri }, session: set_session_user + assert_response :redirect + end + + test "should handle case insensitive scheme validation" do + get :redirect, params: { redirect_uri: 'HTTPS://foreman.example.com/cockpit' }, session: set_session_user + assert_response :redirect + end + end end diff --git a/webpack/JobInvocationDetail/JobInvocationConstants.js b/webpack/JobInvocationDetail/JobInvocationConstants.js index d851b4052..282e43291 100644 --- a/webpack/JobInvocationDetail/JobInvocationConstants.js +++ b/webpack/JobInvocationDetail/JobInvocationConstants.js @@ -74,7 +74,9 @@ const Columns = () => { return { title: __('Failed'), status: 1 }; case 'planned': return { title: __('Scheduled'), status: 2 }; - case 'running' || 'pending': + case 'running': + return { title: __('Pending'), status: 3 }; + case 'pending': return { title: __('Pending'), status: 3 }; case 'cancelled': return { title: __('Cancelled'), status: 4 }; diff --git a/webpack/JobInvocationDetail/JobInvocationHostTable.js b/webpack/JobInvocationDetail/JobInvocationHostTable.js index 9192720d1..4252b29ec 100644 --- a/webpack/JobInvocationDetail/JobInvocationHostTable.js +++ b/webpack/JobInvocationDetail/JobInvocationHostTable.js @@ -258,7 +258,7 @@ const JobInvocationHostTable = ({ setAPIOptions: filterApiCall, }; - const results = apiResponse.results ?? []; + const results = apiResponse?.results ?? []; const selectionToolbar = ( diff --git a/webpack/JobInvocationDetail/TemplateInvocation.js b/webpack/JobInvocationDetail/TemplateInvocation.js index 175b41061..e1e0c2182 100644 --- a/webpack/JobInvocationDetail/TemplateInvocation.js +++ b/webpack/JobInvocationDetail/TemplateInvocation.js @@ -212,10 +212,10 @@ export const TemplateInvocation = ({ ) : ( - 
{preview.plain}
+ 
{preview?.plain}
)} )} diff --git a/webpack/JobInvocationDetail/__tests__/TemplateInvocation.test.js b/webpack/JobInvocationDetail/__tests__/TemplateInvocation.test.js index 1634c8ea3..db8603baf 100644 --- a/webpack/JobInvocationDetail/__tests__/TemplateInvocation.test.js +++ b/webpack/JobInvocationDetail/__tests__/TemplateInvocation.test.js @@ -144,4 +144,76 @@ describe('TemplateInvocation', () => { expect(document.querySelectorAll('.pf-v5-c-skeleton')).toHaveLength(1); }); + + test('handles null preview object gracefully', async () => { + const mockResponseWithNullPreview = { + ...mockTemplateInvocationResponse, + preview: null, + }; + + selectors.selectTemplateInvocationStatus.mockImplementation(() => () => + 'RESOLVED' + ); + selectors.selectTemplateInvocation.mockImplementation(() => () => + mockResponseWithNullPreview + ); + + render( + + + + ); + + // Enable showCommand to trigger preview rendering + act(() => { + fireEvent.click(screen.getByText('Command')); + }); + + // Should not crash and should not display any preview content + expect(screen.queryByText('template-invocation-preview')).not.toBeInTheDocument(); + }); + + test('handles undefined preview.plain gracefully', async () => { + const mockResponseWithUndefinedPreview = { + ...mockTemplateInvocationResponse, + preview: {}, // preview object exists but plain is undefined + }; + + selectors.selectTemplateInvocationStatus.mockImplementation(() => () => + 'RESOLVED' + ); + selectors.selectTemplateInvocation.mockImplementation(() => () => + mockResponseWithUndefinedPreview + ); + + render( + + + + ); + + // Enable showCommand to trigger preview rendering + act(() => { + fireEvent.click(screen.getByText('Command')); + }); + + // Should not crash - the optional chaining should handle undefined preview.plain + expect(() => { + screen.getByText('template-invocation-preview'); + }).not.toThrow(); + }); }); diff --git a/webpack/JobWizard/JobWizard.js b/webpack/JobWizard/JobWizard.js index 7a22b8389..386b9a8fd 100644 --- a/webpack/JobWizard/JobWizard.js +++ b/webpack/JobWizard/JobWizard.js @@ -99,8 +99,10 @@ export const JobWizard = ({ rerunData }) => { if (inputs) { setTemplateValues(prev => { inputs.forEach(input => { - defaultTemplateValues[input.name] = - prev[input.name] || input?.default || ''; + if (input) { + defaultTemplateValues[input.name] = + prev[input.name] || input.default || ''; + } }); return defaultTemplateValues; }); @@ -108,8 +110,10 @@ export const JobWizard = ({ rerunData }) => { setAdvancedValues(currentAdvancedValues => { if (advancedInputs) { advancedInputs.forEach(input => { - advancedTemplateValues[input.name] = - currentAdvancedValues[input.name] || input?.default || ''; + if (input) { + advancedTemplateValues[input.name] = + currentAdvancedValues[input.name] || input.default || ''; + } }); } return { @@ -206,7 +210,7 @@ export const JobWizard = ({ rerunData }) => { const [isStartsAtError, setIsStartsAtError] = useState(false); useEffect(() => { const updateStartsError = () => { - if (scheduleValue.scheduleType === SCHEDULE_TYPES.FUTURE) { + if (scheduleValue?.scheduleType === SCHEDULE_TYPES.FUTURE) { setIsStartsAtError( !!scheduleValue?.startsAt?.length && new Date().getTime() >= new Date(scheduleValue.startsAt).getTime() @@ -216,7 +220,7 @@ export const JobWizard = ({ rerunData }) => { new Date().getTime() >= new Date(scheduleValue.startsBefore).getTime() ); - } else if (scheduleValue.scheduleType === SCHEDULE_TYPES.RECURRING) { + } else if (scheduleValue?.scheduleType === SCHEDULE_TYPES.RECURRING) { setIsStartsAtError( !!scheduleValue?.startsAt?.length && new Date().getTime() >= new Date(scheduleValue.startsAt).getTime() diff --git a/webpack/JobWizard/JobWizardPageRerun.js b/webpack/JobWizard/JobWizardPageRerun.js index f25938a99..82c75cfcc 100644 --- a/webpack/JobWizard/JobWizardPageRerun.js +++ b/webpack/JobWizard/JobWizardPageRerun.js @@ -173,10 +173,12 @@ const JobWizardPageRerun = ({ diff --git a/webpack/JobWizard/__tests__/JobWizard.null-inputs.test.js b/webpack/JobWizard/__tests__/JobWizard.null-inputs.test.js new file mode 100644 index 000000000..c92048e1b --- /dev/null +++ b/webpack/JobWizard/__tests__/JobWizard.null-inputs.test.js @@ -0,0 +1,171 @@ +import React from 'react'; +import { Provider } from 'react-redux'; +import { render } from '@testing-library/react'; +import { MockedProvider } from '@apollo/client/testing'; +import * as api from 'foremanReact/redux/API'; +import { JobWizard } from '../JobWizard'; +import * as selectors from '../JobWizardSelectors'; +import { testSetup, gqlMock } from './fixtures'; + +const store = testSetup(selectors, api); + +describe('JobWizard null input handling', () => { + beforeEach(() => { + jest.spyOn(selectors, 'selectRouterSearch'); + selectors.selectRouterSearch.mockImplementation(() => ({})); + + jest.spyOn(selectors, 'selectJobCategoriesStatus'); + selectors.selectJobCategoriesStatus.mockImplementation(() => 'RESOLVED'); + + jest.spyOn(selectors, 'selectJobTemplate'); + jest.spyOn(selectors, 'selectJobCategoriesResponse'); + jest.spyOn(selectors, 'selectTemplateError'); + jest.spyOn(selectors, 'selectIsSubmitting'); + jest.spyOn(selectors, 'selectIsLoading'); + + selectors.selectTemplateError.mockImplementation(() => false); + selectors.selectIsSubmitting.mockImplementation(() => false); + selectors.selectIsLoading.mockImplementation(() => false); + selectors.selectJobCategoriesResponse.mockImplementation(() => ({ + default_template: 1, + default_category: 'Test Category' + })); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + test('handles null inputs in template_inputs array gracefully', () => { + const templateResponseWithNullInputs = { + job_template: { + name: 'Test Template', + execution_timeout_interval: 60, + time_to_pickup: 30, + description_format: 'Test %{input1}', + job_category: 'Test Category', + }, + template_inputs: [ + { name: 'valid_input', default: 'test' }, + null, // This should be handled gracefully + { name: 'another_valid_input', default: 'test2' }, + undefined, // This should also be handled gracefully + ], + advanced_template_inputs: [], + effective_user: { value: 'root' }, + randomized_ordering: false, + ssh_user: 'test', + concurrency_control: { level: 1 }, + }; + + selectors.selectJobTemplate.mockImplementation(() => templateResponseWithNullInputs); + + api.get.mockImplementation(({ handleSuccess, ...action }) => { + if (action.key === 'JOB_TEMPLATE') { + handleSuccess && handleSuccess({ data: templateResponseWithNullInputs }); + } + return { type: 'get', ...action }; + }); + + // This should not throw an error despite null inputs + expect(() => { + render( + + + + + + ); + }).not.toThrow(); + }); + + test('handles null inputs in advanced_template_inputs array gracefully', () => { + const templateResponseWithNullAdvancedInputs = { + job_template: { + name: 'Test Template', + execution_timeout_interval: 60, + time_to_pickup: 30, + description_format: 'Test %{input1}', + job_category: 'Test Category', + }, + template_inputs: [ + { name: 'valid_input', default: 'test' }, + ], + advanced_template_inputs: [ + { name: 'valid_advanced_input', default: 'test' }, + null, // This should be handled gracefully + { name: 'another_valid_advanced_input', default: 'test2' }, + undefined, // This should also be handled gracefully + ], + effective_user: { value: 'root' }, + randomized_ordering: false, + ssh_user: 'test', + concurrency_control: { level: 1 }, + }; + + selectors.selectJobTemplate.mockImplementation(() => templateResponseWithNullAdvancedInputs); + + api.get.mockImplementation(({ handleSuccess, ...action }) => { + if (action.key === 'JOB_TEMPLATE') { + handleSuccess && handleSuccess({ data: templateResponseWithNullAdvancedInputs }); + } + return { type: 'get', ...action }; + }); + + // This should not throw an error despite null advanced inputs + expect(() => { + render( + + + + + + ); + }).not.toThrow(); + }); + + test('handles null schedule value gracefully', () => { + const templateResponse = { + job_template: { + name: 'Test Template', + execution_timeout_interval: 60, + time_to_pickup: 30, + description_format: 'Test template', + job_category: 'Test Category', + }, + template_inputs: [], + advanced_template_inputs: [], + effective_user: { value: 'root' }, + randomized_ordering: false, + ssh_user: 'test', + concurrency_control: { level: 1 }, + }; + + selectors.selectJobTemplate.mockImplementation(() => templateResponse); + + api.get.mockImplementation(({ handleSuccess, ...action }) => { + if (action.key === 'JOB_TEMPLATE') { + handleSuccess && handleSuccess({ data: templateResponse }); + } + return { type: 'get', ...action }; + }); + + // Test with rerunData that might have null scheduleValue + const rerunDataWithNullSchedule = { + job_category: 'Test Category', + template_invocations: [{ template_id: 1, effective_user: 'root' }], + targeting: { search_query: 'name ~ test' }, + schedule: null, // This should be handled gracefully + }; + + expect(() => { + render( + + + + + + ); + }).not.toThrow(); + }); +}); \ No newline at end of file diff --git a/webpack/JobWizard/steps/form/Formatter.js b/webpack/JobWizard/steps/form/Formatter.js index 33defd9b0..3aeaf26b2 100644 --- a/webpack/JobWizard/steps/form/Formatter.js +++ b/webpack/JobWizard/steps/form/Formatter.js @@ -51,7 +51,9 @@ const TemplateSearchField = ({ }; export const formatter = (input, values, setValue) => { - const isSelectType = !!input?.options; + if (!input) return null; + + const isSelectType = !!input.options; const inputType = input.value_type; const isTextType = inputType === 'plain' || !inputType; // null defaults to plain diff --git a/webpack/JobWizard/steps/form/ResourceSelect.js b/webpack/JobWizard/steps/form/ResourceSelect.js index 16389bea6..af839b5ca 100644 --- a/webpack/JobWizard/steps/form/ResourceSelect.js +++ b/webpack/JobWizard/steps/form/ResourceSelect.js @@ -48,7 +48,7 @@ export const ResourceSelect = ({ // eslint-disable-next-line react-hooks/exhaustive-deps }, []); let selectOptions = []; - if (response.subtotal > maxResults) { + if (response?.subtotal > maxResults) { selectOptions = [ maxResults) { + if (response?.subtotal > maxResults) { selectOptions = [ { if (variant === SelectVariant.typeahead) { - setSelected(response.results.find(r => r.id === selection)); // saving the name and id so we will have access to the name between steps + setSelected(response?.results?.find(r => r.id === selection)); // saving the name and id so we will have access to the name between steps } else if (variant === SelectVariant.typeaheadMulti) { if (selected.map(({ id }) => id).includes(selection)) { setSelected(currentSelected => @@ -69,7 +69,7 @@ export const SearchSelect = ({ } else { setSelected(currentSelected => [ ...currentSelected, - response.results.find(r => r.id === selection), + response?.results?.find(r => r.id === selection), ]); } } diff --git a/webpack/JobWizard/steps/form/__tests__/SelectSearch.test.js b/webpack/JobWizard/steps/form/__tests__/SelectSearch.test.js index 42d096b9e..ca2e09e62 100644 --- a/webpack/JobWizard/steps/form/__tests__/SelectSearch.test.js +++ b/webpack/JobWizard/steps/form/__tests__/SelectSearch.test.js @@ -2,7 +2,7 @@ import React from 'react'; import { render, fireEvent, screen } from '@testing-library/react'; import { SearchSelect } from '../SearchSelect'; -const apiKey = 'HOSTS_KEY'; +const TEST_API_KEY = 'HOSTS_KEY'; describe('SearchSelect', () => { it('too many', () => { @@ -12,7 +12,7 @@ describe('SearchSelect', () => { [ diff --git a/webpack/JobWizard/submit.js b/webpack/JobWizard/submit.js index f741fc5cf..aa2982d86 100644 --- a/webpack/JobWizard/submit.js +++ b/webpack/JobWizard/submit.js @@ -24,7 +24,7 @@ export const submit = ({ startsBefore, ends, purpose, - } = scheduleValue; + } = scheduleValue || {}; const { sshUser, effectiveUserValue, diff --git a/webpack/react_app/components/TargetingHosts/__tests__/fixtures.js b/webpack/react_app/components/TargetingHosts/__tests__/fixtures.js index 25cb6238a..d4ef98604 100644 --- a/webpack/react_app/components/TargetingHosts/__tests__/fixtures.js +++ b/webpack/react_app/components/TargetingHosts/__tests__/fixtures.js @@ -26,6 +26,30 @@ export const HostStatusFixtures = { renders: { status: 'success', }, + 'renders running status': { + status: 'running', + }, + 'renders pending status': { + status: 'pending', + }, + 'renders cancelled status': { + status: 'cancelled', + }, + 'renders N/A status': { + status: 'N/A', + }, + 'renders planned status': { + status: 'planned', + }, + 'renders warning status': { + status: 'warning', + }, + 'renders error status': { + status: 'error', + }, + 'renders unknown status': { + status: 'unknown_status', + }, }; export const TargetingHostsFixtures = { diff --git a/webpack/react_app/components/TargetingHosts/components/HostStatus.js b/webpack/react_app/components/TargetingHosts/components/HostStatus.js index e6500f831..9e34aee8b 100644 --- a/webpack/react_app/components/TargetingHosts/components/HostStatus.js +++ b/webpack/react_app/components/TargetingHosts/components/HostStatus.js @@ -17,7 +17,13 @@ const HostStatus = ({ status }) => { {__('Awaiting start')}
); - case 'running' || 'pending': + case 'running': + return ( +
+ {__('Pending')} +
+ ); + case 'pending': return (
{__('Pending')} diff --git a/webpack/react_app/components/TargetingHosts/index.js b/webpack/react_app/components/TargetingHosts/index.js index 159efe051..132a7a267 100644 --- a/webpack/react_app/components/TargetingHosts/index.js +++ b/webpack/react_app/components/TargetingHosts/index.js @@ -1,4 +1,4 @@ -import React, { useEffect, useState } from 'react'; +import React, { useEffect, useState, useCallback } from 'react'; import { useSelector, useDispatch } from 'react-redux'; import { get } from 'foremanReact/redux/API'; @@ -27,8 +27,8 @@ const buildSearchQuery = (query, stateFilter) => { ); return [query, filters] .flat() - .filter(x => x) - .map(x => `(${x})`) + .filter((x) => x) + .map((x) => `(${x})`) .join(' AND '); }; @@ -50,38 +50,47 @@ const WrappedTargetingHosts = () => { const [apiUrl, setApiUrl] = useState(getApiUrl(searchQuery, pagination)); const intervalExists = useSelector(selectIntervalExists); - const handleSearch = (query, status) => { - const defaultPagination = { page: 1, per_page: pagination.per_page }; - stopApiInterval(); + const handleSearch = useCallback( + (query, status) => { + const defaultPagination = { page: 1, per_page: pagination.per_page }; + stopApiInterval(); - setApiUrl(getApiUrl(buildSearchQuery(query, status), defaultPagination)); - setSearchQuery(query); - setPagination(defaultPagination); - }; + setApiUrl(getApiUrl(buildSearchQuery(query, status), defaultPagination)); + setSearchQuery(query); + setPagination(defaultPagination); + }, + [pagination.per_page, stopApiInterval] + ); - const handlePagination = args => { - stopApiInterval(); - setPagination(args); - setApiUrl(getApiUrl(buildSearchQuery(searchQuery, statusFilter), args)); - }; + const handlePagination = useCallback( + (args) => { + stopApiInterval(); + setPagination(args); + setApiUrl(getApiUrl(buildSearchQuery(searchQuery, statusFilter), args)); + }, + [stopApiInterval, searchQuery, statusFilter] + ); - const stopApiInterval = () => { + const stopApiInterval = useCallback(() => { if (intervalExists) { dispatch(stopInterval(TARGETING_HOSTS)); } - }; - - const getData = url => - withInterval( - get({ - key: TARGETING_HOSTS, - url, - handleError: () => { - dispatch(stopInterval(TARGETING_HOSTS)); - }, - }), - 1000 - ); + }, [intervalExists, dispatch]); + + const getData = useCallback( + (url) => + withInterval( + get({ + key: TARGETING_HOSTS, + url, + handleError: () => { + dispatch(stopInterval(TARGETING_HOSTS)); + }, + }), + 1000 + ), + [dispatch] + ); useEffect(() => { dispatch(getData(apiUrl)); @@ -93,18 +102,18 @@ const WrappedTargetingHosts = () => { return () => { dispatch(stopInterval(TARGETING_HOSTS)); }; - }, [dispatch, apiUrl, autoRefresh]); + }, [dispatch, apiUrl, autoRefresh, getData]); useEffect(() => { handleSearch(searchQuery, statusFilter); - }, [statusFilter, searchQuery]); + }, [statusFilter, searchQuery, handleSearch]); return ( chartFilter(null)(dispatch, null)} + statusFilterReset={(_x) => chartFilter(null)(dispatch, null)} apiStatus={apiStatus} items={items} totalHosts={totalHosts}