Create `tf-wbr-string` for safe word-breaking #2586

wchargin · 2019-08-21T21:03:29Z

Summary:
Prior to this commit, tf-runs-selector used innerHTML to inset word
breaks into text, without explicit attention to sanitization. It
happened to “accidentally” to a pretty good job of preventing XSS
because it inserts  after each any equals sign, making it
impossible(?) to set any attribute value without closing the tag. But
some lesser injection vectors, like remote CSS fetching, are still
possible with a bit of thought.

This commit introduces a new component, tf-wbr-string, that performs
the word-breaking safely with Polymer APIs.

Test Plan:
Run TensorBoard, and verify that the data location at the bottom of the
runs selector is still rendered properly. Inspect the tf-wbr-string in
the dev tools, and note that wbrs appear in the appropriate places.
Then, select the tf-wbr-string element and use $0.value = "…" to
make sure that it breaks correctly in a variety of cases (empty string,
consecutive delimiters, leading delimiters, trailing delimiters).

Note that git grep inner tensorboard/components/tf_runs_selector no
longer returns any results.

wchargin-branch: tf-wbr-string

Summary: Prior to this commit, `tf-runs-selector` used `innerHTML` to inset word breaks into text, without explicit attention to sanitization. It happened to “accidentally” to a pretty good job of preventing XSS because it inserts `` after each any equals sign, making it impossible(?) to set any attribute value without closing the tag. But some lesser injection vectors, like remote CSS fetching, are still possible with a bit of thought. This commit introduces a new component, `tf-wbr-string`, that performs the word-breaking safely with Polymer APIs. Test Plan: Run TensorBoard, and verify that the data location at the bottom of the runs selector is still rendered properly. Inspect the `tf-wbr-string` in the dev tools, and note that `wbr`s appear in the appropriate places. Then, select the `tf-wbr-string` element and use `$0.value = "…"` to make sure that it breaks correctly in a variety of cases (empty string, consecutive delimiters, leading delimiters, trailing delimiters). Note that `git grep inner tensorboard/components/tf_runs_selector` no longer returns any results. wchargin-branch: tf-wbr-string

nfelt

#628 (comment) 🤣 🥝 (couldn't resist)

tensorboard/components/tf_runs_selector/tf-wbr-string.html

nfelt · 2019-08-21T21:33:39Z

tensorboard/components/tf_runs_selector/tf-wbr-string.html

+            result.push({contents: value, last: true});
+            break;
+          } else {
+            const part = value.slice(0, idx + 1);


Isn't idx + 1 only correct if the delimiter match is exactly 1 character? If that's all we want to support it should be documented as such, and then in that case maybe we should construct the regex ourselves to control for this.

Yes; good point. I’ll make the pattern private.

nfelt · 2019-08-21T21:36:10Z

tensorboard/components/tf_runs_selector/tf-wbr-string.html

+        while (true) {
+          const idx = value.search(delimiterPattern);
+          if (idx === -1) {
+            result.push({contents: value, last: true});


If value was empty (i.e. the last delimiter matched is at the end of the string), then we'll be pushing an extra  onto the end, which seems unnecessary.

Seems like a fix would just be to do while (value) instead?

Either seems fine with me (they should have the same effect), but sure.

Using while (value) doesn’t actually prevent the last-added part from
having last: false on an input like "a/", and thus doesn’t prevent
the extra . Given that the unnecessary s do no harm, and
this code is clearly proving too complicated for my tiny brain, I’ve
gone the other way and just included the  after every part. Much
simpler. Everybody’s a winner.

wchargin · 2019-08-21T23:29:07Z

#628 (comment)

Truth be told, I did not remember writing that comment while writing
this PR, but each one of those thoughts did go through my head once
again. :sigh:

wchargin-source: 4dfbf39f201a0498b48577c870859bf2788bd35e wchargin-branch: tf-wbr-string

wchargin

In the course of implementing your suggestion, I have reversed course
entirely. PTAL.

wchargin · 2019-08-22T00:11:00Z

tensorboard/components/tf_runs_selector/tf-wbr-string.html

+         */
+        delimiterPattern: {
+          type: Object /* RegExp */,
+          value: /([\/=\-_,])/,


I’ve also taken this opportunity to reformat the /[\/=\-_,]/ pattern
for clarity: it does not match a backslash, it does match a hyphen, and
it does not match a caret (ASCII value 0x5E between backslash 0x5C and
underscore 0x5F). This is of course unambiguous in the original form,
but I don’t think that it was as obvious as with /[/=_,-]/.

wchargin · 2019-08-22T00:11:37Z

tensorboard/components/tf_runs_selector/tf-wbr-string.html

+        while (true) {
+          const idx = value.search(delimiterPattern);
+          if (idx === -1) {
+            result.push({contents: value, last: true});


Using while (value) doesn’t actually prevent the last-added part from
having last: false on an input like "a/", and thus doesn’t prevent
the extra . Given that the unnecessary s do no harm, and
this code is clearly proving too complicated for my tiny brain, I’ve
gone the other way and just included the  after every part. Much
simpler. Everybody’s a winner.

wchargin-source: 12011a79f5e3316129e975f92494a11d5fc3481f wchargin-branch: tf-wbr-string

stephanwlee · 2019-08-22T15:35:36Z

tensorboard/components/tf_runs_selector/tf-wbr-string.html

+
+<!--
+  tf-wbr-string safely renders a string, with <wbr> elements inserted
+  around select delimiters.


am I reading it correctly that wbr will be added at the end? Also, since delimiter is no longer configurable, can we just write out all the delimiter this would break? e.g., this will be "noop" for input like aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa right?

am I reading it correctly that wbr will be added at the end?

Yes. (#2586 (comment))

Also, since delimiter is no longer configurable, can we just write out
all the delimiter this would break?

Not sure what you mean—are you asking that we specify the delimiter
pattern in this doc comment? Sure, if you like.

e.g., this will be "noop" for input like
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa right?

Yes (as the previous implementation was).

wchargin-source: d4e09ef7a43d7779e2cfdec4d32952e1de750614 wchargin-branch: tf-wbr-string

stephanwlee · 2019-08-22T16:07:14Z

tensorboard/components/tf_runs_selector/tf-wbr-string.html

+        while (true) {
+          const idx = value.search(delimiterPattern);
+          if (idx === -1) {
+            result.push(value);


Stating obvious: the value can be an empty string and you are pushing an empty string into the result which can cause  which probably has zero visual impact.

wchargin added the core:frontend label Aug 21, 2019

wchargin requested a review from stephanwlee August 21, 2019 21:03

googlebot added the cla: yes label Aug 21, 2019

nfelt reviewed Aug 21, 2019

View reviewed changes

[update patch]

ad1eea8

wchargin-source: 4dfbf39f201a0498b48577c870859bf2788bd35e wchargin-branch: tf-wbr-string

wchargin commented Aug 22, 2019

View reviewed changes

wchargin requested a review from nfelt August 22, 2019 00:25

[update patch]

668d0c9

wchargin-source: 12011a79f5e3316129e975f92494a11d5fc3481f wchargin-branch: tf-wbr-string

stephanwlee reviewed Aug 22, 2019

View reviewed changes

[update patch]

944475a

wchargin-source: d4e09ef7a43d7779e2cfdec4d32952e1de750614 wchargin-branch: tf-wbr-string

stephanwlee approved these changes Aug 22, 2019

View reviewed changes

wchargin merged commit 56a9fd0 into master Aug 22, 2019

wchargin deleted the wchargin-tf-wbr-string branch August 22, 2019 16:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create `tf-wbr-string` for safe word-breaking #2586

Create `tf-wbr-string` for safe word-breaking #2586

wchargin commented Aug 21, 2019

nfelt left a comment

nfelt Aug 21, 2019

wchargin Aug 21, 2019

nfelt Aug 21, 2019

wchargin Aug 21, 2019

wchargin Aug 22, 2019

wchargin commented Aug 21, 2019

wchargin left a comment

wchargin Aug 22, 2019

wchargin Aug 22, 2019

stephanwlee Aug 22, 2019

wchargin Aug 22, 2019

stephanwlee Aug 22, 2019 •

edited

Loading

wchargin Aug 22, 2019

Create tf-wbr-string for safe word-breaking #2586

Create tf-wbr-string for safe word-breaking #2586

Conversation

wchargin commented Aug 21, 2019

nfelt 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

wchargin commented Aug 21, 2019

wchargin 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

stephanwlee Aug 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Create `tf-wbr-string` for safe word-breaking #2586

Create `tf-wbr-string` for safe word-breaking #2586

stephanwlee Aug 22, 2019 •

edited

Loading