Fix bug 1388043 #718

benjaminapetersen · 2016-10-24T21:13:57Z

Fix bugzilla #1388043 link and #1388017 link
Fixes overly aggressive escaping to ensure error messages are readable. Example:

spadgett · 2016-10-25T12:14:30Z

app/scripts/controllers/membership.js

      // NOTE: these could all be moved out into a strings service.
      var messages = {
-        errorReason: _.template('Reason: "<%- httpErr %>"'),
+        errorReason: _.template('Reason: "<%= httpErr %>"'),


We shouldn't keep some plain text messages and some HTML markup messages in the same messages object. It's going to get too confusing to know when the message needs to be escaped. I'd create separate messages objects for alerts / html dialog messages. Or simply inline the messages.

That seems reasonable. I def prefer not going back to inline, I'd rather keep making steps to getting the alert handling in a service, probably the text string templates in another service.

At some point, we'll need a message catalog for translation. If we really want to pull the messages out of the controllers anyway, we should start using the format we'll eventually need for translation. But let's not worry about that for this PR.

Here is the difference, for reference:

Escaping error messages from server:

Not escaping messages from server:

Sure, we only want to HTML escape messages that will be passed as detailsMarkup to the dialog. But I have no way to know which is which just looking at the messages object with no context.

Note that the getErrorDetails filter is not guaranteed to return anything, so in some cases this could show Reason: with no content following it.

benjaminapetersen · 2016-10-25T14:16:15Z

@spadgett updated, split into escaped and unescaped message sets.

spadgett · 2016-10-25T14:43:14Z

app/scripts/controllers/membership.js

-            showAlert('rolebindingCreate', 'success', messages.update.subject.success({
+            showAlert('rolebindingCreate', 'success', escaped.update.subject.success({
              roleName: role.metadata.name,
              subjectName: _.escape(newSubject.name)


Won't this double escape subjectName (here and repeated below)?

(You won't see this problem in the UI unless subjectName has special characters.)

Ah, true, removing the _.escape( on these as redundant.
Though, even just with the template escape sequence <<<bar still appears as:

Right, we shouldn't HTML escape anything going into the alerts. Just messages passed as detailsMarkup to the dialog.

Just to make sure I'm with you, alerts still get user input text, I'm just removing the _.escape() in favor of using <%- in the templates, separating out the one template that processes server response with <%= (I could see this one being inline but would rather be consistent at this point).

Any template that will display a plain text message in an alert should use <%=.

Any template that will display HTML content in a dialog should use <%- to escape the HTML. The only time you need this is if you use ng-bind-html like in the dialog.

You shouldn't need to use _.escape anywhere if you use <%- for the HTML messages.

spadgett · 2016-10-31T12:16:50Z

app/scripts/controllers/membership.js

            resetForm();
            refreshRoleBindingList();
-            showAlert('rolebindingCreate', 'success', messages.update.subject.success({
+            showAlert('rolebindingCreate', 'success', escaped.update.subject.success({


This still doesn't seem right. The alerts should not have any escaped messages. Only dialogs.

The alerts can still have text from user input, not just the dialogs.

"Bob" was given the role "view". "Bob" needs to be escaped, correct?

"Bob" was given the role "view". "Bob" needs to be escaped, correct?

No, Angular will escape any special characters you have in the string since the alerts use angular expressions.

https://github.com/openshift/origin-web-console/blob/master/app/views/_alerts.html#L22

We only need to escape values that we pass to ng-bind-html in the dialog.

Ok I can update this. Something about it still bothers me, looking at the docs:

ngBindHTML:

Evaluates the expression and inserts the resulting HTML into the element in a secure way. By default, the resulting HTML content will be sanitized using the $sanitize service.

$sanitize:

Sanitizes an html string by stripping all potentially dangerous tokens.

It sounds like they strip things like <script> tags from the content, which is good. But if you have a user name with special characters like < and &, I suspect it won't render correctly.

spadgett · 2016-10-31T12:19:00Z

app/scripts/controllers/membership.js

+        errorReason: _.template('Reason: "<%= httpErr %>"')
+      };
+
+      var escaped = {


Maybe messages and messagesWithMarkup?

Well, since the real issue here seems to be security, the word 'escaped' seems to be more descriptive if the specific problem we are trying to solve. If I came back in a month I may wonder "why do I care which ones have markup?" and would have to dig. The word 'escaped' immediately tells me this is important for security.

Fixup HTML escaping in membership.js

benjaminapetersen · 2016-10-31T17:49:20Z

You can see all the places we are using templates in this screenshot:

I think the latest including @spadgett's change (not squashed) is where we want this, should be good to go.

spadgett · 2016-10-31T18:18:26Z

[merge]

openshift-bot · 2016-10-31T18:23:33Z

Evaluated for origin web console merge up to c9ee3a5

openshift-bot · 2016-10-31T18:23:33Z

[Test]ing while waiting on the merge queue

openshift-bot · 2016-10-31T18:31:15Z

Evaluated for origin web console test up to c9ee3a5

openshift-bot · 2016-10-31T18:45:34Z

Origin Web Console Test Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/611/) (Base Commit: d4becd8)

openshift-bot · 2016-10-31T19:07:20Z

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/613/) (Base Commit: 05de64a)

spadgett reviewed Oct 25, 2016

View reviewed changes

spadgett self-assigned this Oct 25, 2016

benjaminapetersen force-pushed the bug-1388043-membership-escape-characters branch from fd20f61 to ec69fdd Compare October 25, 2016 14:15

spadgett reviewed Oct 25, 2016

View reviewed changes

Fix bug 1388043

7e09995

benjaminapetersen force-pushed the bug-1388043-membership-escape-characters branch from ec69fdd to 7e09995 Compare October 25, 2016 16:13

spadgett requested changes Oct 31, 2016

View reviewed changes

spadgett and others added 2 commits October 31, 2016 12:17

Fixup HTML escaping in membership.js

67ed467

Merge pull request #4 from spadgett/ben-membership-escaping

c9ee3a5

Fixup HTML escaping in membership.js

spadgett approved these changes Oct 31, 2016

View reviewed changes

openshift-bot merged commit d3402a5 into openshift:master Oct 31, 2016

benjaminapetersen deleted the bug-1388043-membership-escape-characters branch October 31, 2016 19:27

Fix bug 1388043 #718

Fix bug 1388043 #718

Uh oh!

Conversation

benjaminapetersen commented Oct 24, 2016

Uh oh!

spadgett Oct 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjaminapetersen commented Oct 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spadgett Oct 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjaminapetersen commented Oct 31, 2016

Uh oh!

spadgett commented Oct 31, 2016

Uh oh!

openshift-bot commented Oct 31, 2016

Uh oh!

openshift-bot commented Oct 31, 2016

Uh oh!

openshift-bot commented Oct 31, 2016

Uh oh!

openshift-bot commented Oct 31, 2016

Uh oh!

openshift-bot commented Oct 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

spadgett Oct 25, 2016 •

edited

Loading

spadgett Oct 26, 2016 •

edited

Loading

openshift-bot commented Oct 31, 2016 •

edited

Loading