Fixes #25169 - fix xss on pages with breadcrumbs #6132

amirfefer · 2018-10-10T19:53:03Z

breadcrumbs replaced the title on pages by using @page_header variable.
in two places, @page_header contains html such as image for the operating system in details host page, and span with id (#hostFQDN) in order to manipulate the title with jquery.

In this PR, I removed dangerouslySetInnerHTML from the breadcrumb component, which caused this issue, while keeping the same behavior that I mentioned above.
I haven't found any other extracted html in @page_header except these two in foreman core and plugins.

theforeman-bot · 2018-10-10T19:53:26Z

Issues: #25169

ares · 2018-10-11T05:48:26Z

one integration test seemed to fail on all rubies, rerunning tests to be sure, but is probably related
[test foreman]


Capybara::ElementNotFound: Unable to find css "#hostFQDN"
    test/integration/host_js_test.rb:602:in `block (3 levels) in <class:HostJSTest>' (Capybara::ElementNotFound)
/usr/local/rvm/gems/ruby-2.4.3@test_develop_pr_core-0/gems/capybara-3.9.0/lib/capybara/node/finders.rb:297

sharvit

Thanks @amirfefer, left some comments about the React/Redux part

sharvit · 2018-10-11T08:22:52Z

webpack/assets/javascripts/foreman_breadcrumbs.js

@@ -0,0 +1,6 @@
+import store from './react_app/redux';
+
+import * as BreadcrumbsAction from './react_app/components/BreadcrumbBar/BreadcrumbBarActions';


nitpick, import { updateBreadcrumbTitle } from '...';

sharvit · 2018-10-11T08:25:06Z

webpack/assets/javascripts/react_app/components/BreadcrumbBar/BreadcrumbBar.js

+          title
+          items={breadcrumbItems}
+          isTitle={isTitle}
+          titleAddition={this.props.titleAddition}


Can you put it with all the other props in the const { ... } = this.props?

Can you add propType and defaultProp?

sharvit · 2018-10-11T08:27:16Z

webpack/assets/javascripts/react_app/components/BreadcrumbBar/BreadcrumbBarActions.js

@@ -25,6 +26,13 @@ export const removeSearchQuery = resource => (dispatch) => {
  loadSwitcherResourcesByResource(resource)(dispatch);
 };

+export const updateBreadcrumbTitle = title => (dispatch) => {


You don't need the dispatch in this case, this should be enough:

export const updateBreadcrumbTitle = title => ({ type: BREADCRUMB_BAR_UPDATE_TITLE, payload: title, });

sharvit · 2018-10-11T08:28:21Z

webpack/assets/javascripts/react_app/components/BreadcrumbBar/BreadcrumbBarReducer.js

@@ -27,6 +28,9 @@ export default (state = initialState, action) => {
    case BREADCRUMB_BAR_CLEAR_SEARCH:
      return state.set('searchQuery', '');

+    case BREADCRUMB_BAR_UPDATE_TITLE:
+      return state.set('titleAddition', payload);


Don't we want some titleAddition value to be in the initialState?

isn't it enough have a default value defined in the Proptype?

In this case it will achieve the same results, but I wouldn't say it is enough, they are playing different roles.

Different defaultProps when not passing a value vs the actual initial state of the application.

sharvit · 2018-10-11T08:35:18Z

...ascripts/react_app/components/BreadcrumbBar/components/__snapshots__/Breadcrumb.test.js.snap

-  />
+    title="root"
+  >
+    root false


The false is here accidentally?

amirfefer · 2018-10-11T13:01:02Z

@sharvit thanks for the review !
could you have another look ?

@ares - the integration test failure occurs due to the the legacy manipulation on the title, so i'm rewrite it.

coveralls · 2018-10-11T13:08:11Z

Coverage decreased (-0.1%) to 75.396% when pulling 04be4a3 on amirfefer:25169 into 7011d23 on theforeman:develop.

coveralls · 2018-10-11T13:08:15Z

Coverage decreased (-0.1%) to 75.557% when pulling f51a47c on amirfefer:25169 into 882d451 on theforeman:develop.

houndci-bot · 2018-10-11T14:39:29Z

webpack/assets/javascripts/react_app/components/BreadcrumbBar/components/Breadcrumb.scss

@@ -0,0 +1,3 @@
+#hostFQDN {
+  vertical-align: 0px


Unexpected unit length-zero-no-unit
Expected a trailing semicolon declaration-block-trailing-semicolon

amirfefer · 2018-10-11T16:40:09Z

@ares integration test has been fixed :)

ares · 2018-10-11T18:12:16Z

Ok, I can confirm test failure is now unrelated. Normally we try to keep security fixes minimal, because of backporting. But if there's no better way of handling the title in interfaces form, I guess that's fine. I'll be testing this shortly and waiting for @sharvit for code ack.

sharvit

LGTM! Thanks @amirfefer 👍

tstrachota · 2018-10-23T13:30:17Z

webpack/assets/javascripts/react_app/components/BreadcrumbBar/components/Breadcrumb.js

+            {' '}
+            {text || caption}
+            {' '}
+            { active && <span id='hostFQDN' > {titleAddition} </span>}


'hostFQDN' id seems too resource specific for this generic component.

I didn't do any testing, but besides that the design looks fine to me.

you're right, I chose it because the integration test catches this id. I'll change the id to something more generic here and in the test.

tbrisker · 2018-10-24T13:23:48Z

ping @amirfefer @tstrachota @sharvit - what is the status here?

tstrachota

I did some testing and found one regression on the host edit page. I'm sorry for that @amirfefer

tstrachota · 2018-10-24T13:24:31Z

webpack/assets/javascripts/react_app/components/BreadcrumbBar/components/Breadcrumb.js

+            {' '}
+            {text || caption}
+            {' '}
+            { active && <span id='title-addition' > {titleAddition} </span>}


I found out this is causing issues when I change fqdn of a primary interface on an existing host:

How about removing the addition and replacing the whole caption on BREADCRUMB_BAR_UPDATE_TITLE ?

so we have two options I can think of:

replace the entire title when fqdn is changed

add this title's addition only on new host page (keeping today's behavior )

I prefer 1. It means copying some strings into js code but in general I find it more re-usable.

I think it makes sense to do two things - 1. change the entire title instead of adding to it (so if needed we can do that elsewhere easily, where it might not make sense to only add text) 2. only do this on new hosts since it's confusing if you are editing an existing one - you expect the old name to be there rather than the new

@tbrisker I agree. Changing it on the edit page would be confusing.

amirfefer · 2018-11-01T15:20:22Z

thanks @tbrisker ! I've removed the readOnly prop, and changed the legacy js function.

amirfefer · 2018-11-01T15:29:53Z

app/helpers/layout_helper.rb

@@ -62,7 +62,7 @@ def layout_data

  def title(page_title, page_header = nil)
    content_for(:title, page_title.to_s)
-    @page_header ||= page_header || @content_for_title || page_title.to_s


@content_for_title doesn't exist

amirfefer · 2018-11-01T15:30:40Z

test/integration/host_js_test.rb

@@ -599,7 +599,6 @@ class HostJSTest < IntegrationTestWithJavascript
        modal.find(:button, "Ok").click

        assert table.find('td.fqdn').has_content?('name.' + domain.name)
-        assert page.find('#hostFQDN').has_content?('| name.' + domain.name)


moved to jest

tbrisker

just a tiny comment inline

tbrisker · 2018-11-01T15:39:58Z

app/helpers/operatingsystems_helper.rb

@@ -44,7 +44,9 @@ def icon(record, opts = {})
               return "" if record.family.blank?
               record.family
             end
-
+    if opts[:path]
+      return image_path(family + ".png")


nitpick:

return image_path(family + ".png") if opts[:path]

is a more common idiom in ruby

amirfefer · 2018-11-01T16:14:44Z

thanks @tbrisker :)

mmoll · 2018-11-02T13:11:49Z

test failure unrelated 💚

amirfefer · 2018-11-04T14:59:30Z

@tbrisker could you have another look please?

tbrisker

missed one change from earlier version

tbrisker · 2018-11-04T15:04:19Z

app/views/hosts/new.html.erb

-<% new_host_title %>
-
+<% title _("Create Host") %>
+<%= breadcrumbs(read_only_title: false) %>


leftover read_only_title

amirfefer · 2018-11-04T21:55:48Z

thanks for noticing :)

tbrisker

Thanks @amirfefer !

fixed

tbrisker · 2018-11-05T15:26:45Z

🍒 :
1.20 - a768857
1.19 - bd57e02
1.18 - 8a2077e

theforeman-bot added Needs testing Not yet reviewed UI labels Oct 10, 2018

sharvit suggested changes Oct 11, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Oct 11, 2018

amirfefer force-pushed the 25169 branch from 07f9c9d to 04be4a3 Compare October 11, 2018 13:00

theforeman-bot added Needs re-review and removed Waiting on contributor labels Oct 11, 2018

amirfefer force-pushed the 25169 branch from 04be4a3 to 2a33364 Compare October 11, 2018 14:39

houndci-bot reviewed Oct 11, 2018

View reviewed changes

amirfefer force-pushed the 25169 branch from 2a33364 to 82095e0 Compare October 11, 2018 16:39

sharvit previously approved these changes Oct 14, 2018

View reviewed changes

theforeman-bot removed the Needs re-review label Oct 14, 2018

tstrachota reviewed Oct 23, 2018

View reviewed changes

amirfefer dismissed sharvit’s stale review via ee5fe80 October 23, 2018 14:48

amirfefer force-pushed the 25169 branch from 82095e0 to ee5fe80 Compare October 23, 2018 14:48

tstrachota previously requested changes Oct 24, 2018

View reviewed changes

theforeman-bot added the Waiting on contributor label Oct 24, 2018

amirfefer force-pushed the 25169 branch from ee5fe80 to bdc65eb Compare October 25, 2018 13:18

theforeman-bot added Needs re-review and removed Waiting on contributor labels Oct 25, 2018

theforeman-bot added Waiting on contributor and removed Needs re-review labels Oct 31, 2018

amirfefer force-pushed the 25169 branch from a87cf4f to 83095c6 Compare November 1, 2018 15:19

theforeman-bot added Needs re-review and removed Waiting on contributor labels Nov 1, 2018

amirfefer force-pushed the 25169 branch from 83095c6 to 2d3f15b Compare November 1, 2018 15:29

amirfefer commented Nov 1, 2018

View reviewed changes

tbrisker reviewed Nov 1, 2018

View reviewed changes

amirfefer force-pushed the 25169 branch from 2d3f15b to a5c21e6 Compare November 1, 2018 16:13

mmoll added this to the 1.18.3 milestone Nov 2, 2018

tbrisker suggested changes Nov 4, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Nov 4, 2018

Fixes #25169 - fix xss on pages with breadcrumbs

ce8fd20

amirfefer force-pushed the 25169 branch from a5c21e6 to ce8fd20 Compare November 4, 2018 21:54

theforeman-bot added Needs re-review and removed Waiting on contributor labels Nov 4, 2018

tbrisker approved these changes Nov 5, 2018

View reviewed changes

theforeman-bot removed the Needs re-review label Nov 5, 2018

tbrisker merged commit 3a0c10c into theforeman:develop Nov 5, 2018

dLobatog mentioned this pull request Dec 27, 2023

[Snyk] Security upgrade axios from 0.17.1 to 1.6.3 dLobatog/foreman#51

Open

dLobatog mentioned this pull request Jan 5, 2024

[Snyk] Security upgrade axios from 0.17.1 to 1.6.4 dLobatog/foreman#52

Open

		@@ -0,0 +1,6 @@
		import store from './react_app/redux';

		import * as BreadcrumbsAction from './react_app/components/BreadcrumbBar/BreadcrumbBarActions';

Fixes #25169 - fix xss on pages with breadcrumbs #6132

Fixes #25169 - fix xss on pages with breadcrumbs #6132

Conversation

amirfefer commented Oct 10, 2018 • edited Loading

theforeman-bot commented Oct 10, 2018

ares commented Oct 11, 2018

sharvit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirfefer commented Oct 11, 2018 • edited Loading

coveralls commented Oct 11, 2018

coveralls commented Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

amirfefer commented Oct 11, 2018

ares commented Oct 11, 2018

sharvit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirfefer Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

tbrisker commented Oct 24, 2018

tstrachota left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirfefer commented Nov 1, 2018

amirfefer Nov 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbrisker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirfefer commented Nov 1, 2018

mmoll commented Nov 2, 2018

amirfefer commented Nov 4, 2018

tbrisker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirfefer commented Nov 4, 2018

tbrisker left a comment

Choose a reason for hiding this comment

tbrisker commented Nov 5, 2018

amirfefer commented Oct 10, 2018 •

edited

Loading

amirfefer commented Oct 11, 2018 •

edited

Loading

coveralls commented Oct 11, 2018 •

edited

Loading

amirfefer Oct 23, 2018 •

edited

Loading

amirfefer Nov 1, 2018 •

edited

Loading