Merge branch 'tor/defect/urlencode-image-diff-octothorpe' into 'master'

Fix unencoded octothorpes (and spaces) in Image Diff URLs

See merge request gitlab-org/gitlab!53638

app/assets/javascripts/lib/utils/url_utility.js

@@ -16,6 +16,50 @@ 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.
+ *
+ * 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}
+ */
+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, '');
 }

app/assets/javascripts/vue_shared/components/content_viewer/viewers/image_viewer.vue

 <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"

changelogs/unreleased/tor-defect-urlencode-image-diff-octothorpe.yml

+---
+title: Fix some image diff URLs with special characters causing the diff to not show
+merge_request: 53638
+author:
+type: fixed

spec/frontend/lib/utils/url_utility_spec.js

@@ -880,4 +880,37 @@ 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);
+      },
+    );
+
+    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);
+    });
+  });
 });

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');
+    });
+  });
 });