From 00a126aa532d32ee73c699ed92c5f6bd42fdbc17 Mon Sep 17 00:00:00 2001
From: Thomas Randolph <trandolph@gitlab.com>
Date: Wed, 10 Feb 2021 23:19:40 -0700
Subject: [PATCH 1/4] Add URL Utility for making safe-ER URL paths

---
 .../javascripts/lib/utils/url_utility.js      | 30 +++++++++++++++++++
 spec/frontend/lib/utils/url_utility_spec.js   | 23 ++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/app/assets/javascripts/lib/utils/url_utility.js b/app/assets/javascripts/lib/utils/url_utility.js
index 0b920ba8e7a8f0..2c9847dec0364b 100644
--- a/app/assets/javascripts/lib/utils/url_utility.js
+++ b/app/assets/javascripts/lib/utils/url_utility.js
@@ -16,6 +16,36 @@ function decodeUrlParameter(val) {
   return decodeURIComponent(val.replace(/\+/g, '%20'));
 }
 
+/**
+ * Safely encodes a string to be used as a path
+ *
+ * Note: This function DOES encode typical URL parts like ?, =, &, #, and +
+ * If you need to use search parameters or URL fragments, they should be
+ *     added AFTER calling this function, not before.
+ *
+ * @param {String} potentiallyUnsafePath
+ * @returns {String}
+ */
+export function encodeSaferUrl(potentiallyUnsafePath) {
+  const unencode = ['%2F'];
+  const encode = ['#', '!', '~', '\\*', "'", '\\(', '\\)'];
+  let saferPath = encodeURIComponent(potentiallyUnsafePath);
+
+  unencode.forEach((code) => {
+    saferPath = saferPath.replace(new RegExp(code, 'g'), decodeURIComponent(code));
+  });
+  encode.forEach((code) => {
+    const encodedValue = code
+      .codePointAt(code.length - 1)
+      .toString(16)
+      .toUpperCase();
+
+    saferPath = saferPath.replace(new RegExp(code, 'g'), `%${encodedValue}`);
+  });
+
+  return saferPath;
+}
+
 export function cleanLeadingSeparator(path) {
   return path.replace(PATH_SEPARATOR_LEADING_REGEX, '');
 }
diff --git a/spec/frontend/lib/utils/url_utility_spec.js b/spec/frontend/lib/utils/url_utility_spec.js
index 61df3aa19c2352..7812b2b7400fbd 100644
--- a/spec/frontend/lib/utils/url_utility_spec.js
+++ b/spec/frontend/lib/utils/url_utility_spec.js
@@ -880,4 +880,27 @@ describe('URL utility', () => {
       expect(urlUtils.getURLOrigin(url)).toBe(expectation);
     });
   });
+
+  describe('encodeSaferUrl', () => {
+    it.each`
+      character | input                 | output
+      ${' '}    | ${'/url/hello 1.jpg'} | ${'/url/hello%201.jpg'}
+      ${'#'}    | ${'/url/hello#1.jpg'} | ${'/url/hello%231.jpg'}
+      ${'!'}    | ${'/url/hello!.jpg'}  | ${'/url/hello%21.jpg'}
+      ${'~'}    | ${'/url/hello~.jpg'}  | ${'/url/hello%7E.jpg'}
+      ${'*'}    | ${'/url/hello*.jpg'}  | ${'/url/hello%2A.jpg'}
+      ${"'"}    | ${"/url/hello'.jpg"}  | ${'/url/hello%27.jpg'}
+      ${'('}    | ${'/url/hello(.jpg'}  | ${'/url/hello%28.jpg'}
+      ${')'}    | ${'/url/hello).jpg'}  | ${'/url/hello%29.jpg'}
+      ${'?'}    | ${'/url/hello?.jpg'}  | ${'/url/hello%3F.jpg'}
+      ${'='}    | ${'/url/hello=.jpg'}  | ${'/url/hello%3D.jpg'}
+      ${'+'}    | ${'/url/hello+.jpg'}  | ${'/url/hello%2B.jpg'}
+      ${'&'}    | ${'/url/hello&.jpg'}  | ${'/url/hello%26.jpg'}
+    `(
+      'properly escapes `$character` characters while retaining the integrity of the URL',
+      ({ input, output }) => {
+        expect(urlUtils.encodeSaferUrl(input)).toBe(output);
+      },
+    );
+  });
 });
-- 
GitLab


From 2cac4e4e6392f4ca2020e731a2ee56597b3b09c4 Mon Sep 17 00:00:00 2001
From: Thomas Randolph <trandolph@gitlab.com>
Date: Wed, 10 Feb 2021 23:20:08 -0700
Subject: [PATCH 2/4] Use encodeSaferURL utility to render image diff

---
 .../components/content_viewer/viewers/image_viewer.vue |  8 ++++++--
 .../content_viewer/viewers/image_viewer_spec.js        | 10 ++++++++++
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/app/assets/javascripts/vue_shared/components/content_viewer/viewers/image_viewer.vue b/app/assets/javascripts/vue_shared/components/content_viewer/viewers/image_viewer.vue
index 9ece6a52805df7..a49eb7fd6118ef 100644
--- a/app/assets/javascripts/vue_shared/components/content_viewer/viewers/image_viewer.vue
+++ b/app/assets/javascripts/vue_shared/components/content_viewer/viewers/image_viewer.vue
@@ -1,6 +1,7 @@
 <script>
 import { throttle } from 'lodash';
-import { numberToHumanSize } from '../../../../lib/utils/number_utils';
+import { numberToHumanSize } from '~/lib/utils/number_utils';
+import { encodeSaferUrl } from '~/lib/utils/url_utility';
 
 export default {
   props: {
@@ -43,6 +44,9 @@ export default {
     hasDimensions() {
       return this.width && this.height;
     },
+    safePath() {
+      return encodeSaferUrl(this.path);
+    },
   },
   beforeDestroy() {
     window.removeEventListener('resize', this.resizeThrottled, false);
@@ -84,7 +88,7 @@ export default {
 <template>
   <div data-testid="image-viewer" data-qa-selector="image_viewer_container">
     <div :class="innerCssClasses" class="position-relative">
-      <img ref="contentImg" :src="path" @load="onImgLoad" />
+      <img ref="contentImg" :src="safePath" @load="onImgLoad" />
       <slot
         name="image-overlay"
         :rendered-width="renderedWidth"
diff --git a/spec/frontend/vue_shared/components/content_viewer/viewers/image_viewer_spec.js b/spec/frontend/vue_shared/components/content_viewer/viewers/image_viewer_spec.js
index 31e843297faf3b..af3b63ad7e59d6 100644
--- a/spec/frontend/vue_shared/components/content_viewer/viewers/image_viewer_spec.js
+++ b/spec/frontend/vue_shared/components/content_viewer/viewers/image_viewer_spec.js
@@ -33,4 +33,14 @@ describe('Image Viewer', () => {
       },
     );
   });
+
+  describe('file path', () => {
+    it('should output a valid URL path for the image', () => {
+      wrapper = mount(ImageViewer, {
+        propsData: { path: '/url/hello#1.jpg' },
+      });
+
+      expect(wrapper.find('img').attributes('src')).toBe('/url/hello%231.jpg');
+    });
+  });
 });
-- 
GitLab


From 5e63282d0a45d4342d414725d6950ed27bebbdee Mon Sep 17 00:00:00 2001
From: Thomas Randolph <trandolph@gitlab.com>
Date: Tue, 2 Feb 2021 05:06:12 -0700
Subject: [PATCH 3/4] Add changelog for fixing image diff octothorpes

---
 .../tor-defect-urlencode-image-diff-octothorpe.yml           | 5 +++++
 1 file changed, 5 insertions(+)
 create mode 100644 changelogs/unreleased/tor-defect-urlencode-image-diff-octothorpe.yml

diff --git a/changelogs/unreleased/tor-defect-urlencode-image-diff-octothorpe.yml b/changelogs/unreleased/tor-defect-urlencode-image-diff-octothorpe.yml
new file mode 100644
index 00000000000000..3ca89861bb37e8
--- /dev/null
+++ b/changelogs/unreleased/tor-defect-urlencode-image-diff-octothorpe.yml
@@ -0,0 +1,5 @@
+---
+title: Fix some image diff URLs with special characters causing the diff to not show
+merge_request: 53638
+author:
+type: fixed
-- 
GitLab


From ef367d434532775e6ad7da89c6c9dc395c88f45d Mon Sep 17 00:00:00 2001
From: Thomas Randolph <trandolph@gitlab.com>
Date: Mon, 15 Feb 2021 11:20:32 -0700
Subject: [PATCH 4/4] Add base-case tests and more JSDoc description / examples
 (review)

---
 app/assets/javascripts/lib/utils/url_utility.js | 14 ++++++++++++++
 spec/frontend/lib/utils/url_utility_spec.js     | 10 ++++++++++
 2 files changed, 24 insertions(+)

diff --git a/app/assets/javascripts/lib/utils/url_utility.js b/app/assets/javascripts/lib/utils/url_utility.js
index 2c9847dec0364b..cc2cf787a8f9e8 100644
--- a/app/assets/javascripts/lib/utils/url_utility.js
+++ b/app/assets/javascripts/lib/utils/url_utility.js
@@ -23,6 +23,20 @@ function decodeUrlParameter(val) {
  * If you need to use search parameters or URL fragments, they should be
  *     added AFTER calling this function, not before.
  *
+ * Usecase: An image filename is stored verbatim, and you need to load it in
+ *     the browser.
+ *
+ * Example: /some_path/file #1.jpg      ==> /some_path/file%20%231.jpg
+ * Example: /some-path/file! Final!.jpg ==> /some-path/file%21%20Final%21.jpg
+ *
+ * Essentially, if a character *could* present a problem in a URL, it's escaped
+ *     to the hexadecimal representation instead. This means it's a bit more
+ *     aggressive than encodeURIComponent: that built-in function doesn't
+ *     encode some characters that *could* be problematic, so this function
+ *     adds them (#, !, ~, *, ', (, and )).
+ *     Additionally, encodeURIComponent *does* encode `/`, but we want safer
+ *     URLs, not non-functional URLs, so this function DEcodes slashes ('%2F').
+ *
  * @param {String} potentiallyUnsafePath
  * @returns {String}
  */
diff --git a/spec/frontend/lib/utils/url_utility_spec.js b/spec/frontend/lib/utils/url_utility_spec.js
index 7812b2b7400fbd..b60ddea81eed06 100644
--- a/spec/frontend/lib/utils/url_utility_spec.js
+++ b/spec/frontend/lib/utils/url_utility_spec.js
@@ -902,5 +902,15 @@ describe('URL utility', () => {
         expect(urlUtils.encodeSaferUrl(input)).toBe(output);
       },
     );
+
+    it.each`
+      character | input
+      ${'/, .'} | ${'/url/hello.png'}
+      ${'\\d'}  | ${'/url/hello123.png'}
+      ${'-'}    | ${'/url/hello-123.png'}
+      ${'_'}    | ${'/url/hello_123.png'}
+    `('makes no changes to unproblematic characters ($character)', ({ input }) => {
+      expect(urlUtils.encodeSaferUrl(input)).toBe(input);
+    });
   });
 });
-- 
GitLab