Revert "Detect and modify invalid URL characters (#1664)" (#1684)

This reverts commit a555427.
percy · Aug 2, 2024 · ed69ff9 · ed69ff9
1 parent 2f7608e
commit ed69ff9
Show file tree

Hide file tree

Showing 6 changed files with 3 additions and 239 deletions.
diff --git a/packages/core/src/network.js b/packages/core/src/network.js
@@ -1,7 +1,7 @@
 import { request as makeRequest } from '@percy/client/utils';
 import logger from '@percy/logger';
 import mime from 'mime-types';
-import { DefaultMap, createResource, decodeAndEncodeURLWithLogging, hostnameMatches, normalizeURL, waitFor } from './utils.js';
+import { DefaultMap, createResource, hostnameMatches, normalizeURL, waitFor } from './utils.js';
 
 const MAX_RESOURCE_SIZE = 25 * (1024 ** 2); // 25MB
 const ALLOWED_STATUSES = [200, 201, 301, 302, 304, 307, 308];
@@ -207,14 +207,6 @@ export class Network {
     // do not handle data urls
     if (request.url.startsWith('data:')) return;
 
-    // Browsers handle URL encoding leniently, but invalid characters can break tools like Jackproxy.
-    // This code checks for issues such as `%` and leading spaces and warns the user accordingly.
-    decodeAndEncodeURLWithLogging(request.url, this.log, {
-      meta: { ...this.meta, url: request.url },
-      shouldLogWarning: true,
-      warningMessage: `An invalid URL was detected for url: ${request.url} - the snapshot may fail on Percy. Please verify that your asset URL is valid.`
-    });
-
     if (this.intercept) {
       this.#pending.set(requestId, event);
       if (this.captureMockedServiceWorker) {

diff --git a/packages/core/src/snapshot.js b/packages/core/src/snapshot.js
@@ -7,8 +7,7 @@ import {
   request,
   hostnameMatches,
   yieldTo,
-  snapshotLogName,
-  decodeAndEncodeURLWithLogging
+  snapshotLogName
 } from './utils.js';
 import { JobData } from './wait-for-job.js';
 
@@ -25,21 +24,6 @@ function validURL(url, base) {
   }
 }
 
-function validateAndFixSnapshotUrl(snapshot) {
-  let log = logger('core:snapshot');
-  // encoding snapshot url, if contians invalid URI characters/syntax
-  let modifiedURL = decodeAndEncodeURLWithLogging(snapshot.url, log, {
-    meta: { snapshot: { name: snapshot.name || snapshot.url } },
-    shouldLogWarning: true,
-    warningMessage: `Invalid URL detected for url: ${snapshot.url} - the snapshot may fail on Percy. Please confirm that your website URL is valid.`
-  });
-
-  if (modifiedURL !== snapshot.url) {
-    log.debug(`Snapshot URL modified to: ${modifiedURL}`);
-    snapshot.url = modifiedURL;
-  }
-}
-
 // used to deserialize regular expression strings
 const RE_REGEXP = /^\/(.+)\/(\w+)?$/;
 
@@ -102,10 +86,8 @@ function mapSnapshotOptions(snapshots, context) {
     // transform snapshot URL shorthand into an object
     if (typeof snapshot === 'string') snapshot = { url: snapshot };
 
-    validateAndFixSnapshotUrl(snapshot);
-
-    let url = validURL(snapshot.url, context?.baseUrl);
     // normalize the snapshot url and use it for the default name
+    let url = validURL(snapshot.url, context?.baseUrl);
     snapshot.name ||= `${url.pathname}${url.search}${url.hash}`;
     snapshot.url = url.href;
 

diff --git a/packages/core/src/utils.js b/packages/core/src/utils.js
@@ -375,31 +375,6 @@ export function base64encode(content) {
     .toString('base64');
 }
 
-// This function replaces invalid character that are not the
-// part of valid URI syntax with there correct encoded value.
-// Also, if a character is a part of valid URI syntax, those characters
-// are not encoded
-// Eg: [abc] -> gets encoded to %5Babc%5D
-// ab c -> ab%20c
-export function decodeAndEncodeURLWithLogging(url, logger, options = {}) {
-  // In case the url is partially encoded, then directly using encodeURI()
-  // will encode those characters again. Therefore decodeURI once helps is decoding
-  // partially encoded URL and then after encoding it again, full URL get encoded
-  // correctly.
-  const { meta, shouldLogWarning, warningMessage } = options;
-  try {
-    let decodedURL = decodeURI(url); // This can throw error, so handle it will trycatch
-    let encodedURL = encodeURI(decodedURL);
-    return encodedURL;
-  } catch (error) {
-    logger.debug(error, meta);
-    if (error.name === 'URIError' && shouldLogWarning) {
-      logger.warn(warningMessage);
-    }
-    return url;
-  }
-}
-
 export function snapshotLogName(name, meta) {
   if (meta?.snapshot?.testCase) {
     return `testCase: ${meta.snapshot.testCase}, ${name}`;

diff --git a/packages/core/test/discovery.test.js b/packages/core/test/discovery.test.js
@@ -647,49 +647,6 @@ describe('Discovery', () => {
     ]));
   });
 
-  it('debug error log only for invalid network url', async () => {
-    let err = new Error('Some Invalid Error');
-    spyOn(global, 'decodeURI').and.callFake((url) => {
-      if (url === 'http://localhost:8000/style.css') {
-        throw err;
-      }
-      return url;
-    });
-
-    percy.loglevel('debug');
-    await percy.snapshot({
-      name: 'test snapshot',
-      url: 'http://localhost:8000',
-      domSnapshot: testDOM
-    });
-
-    expect(logger.stderr).toEqual(jasmine.arrayContaining([
-      '[percy:core:page] Navigate to: http://localhost:8000/',
-      '[percy:core:discovery] Handling request: http://localhost:8000/',
-      '[percy:core:discovery] - Serving root resource',
-      `[percy:core:discovery] ${err.stack}`,
-      '[percy:core:discovery] Handling request: http://localhost:8000/style.css',
-      '[percy:core:discovery] Handling request: http://localhost:8000/img.gif'
-    ]));
-  });
-
-  it('detect invalid network url', async () => {
-    percy.loglevel('debug');
-    await percy.snapshot({
-      name: 'test snapshot',
-      url: 'http://localhost:8000',
-      domSnapshot: testDOM.replace('style.css', 'http://localhost:404/%style.css')
-    });
-
-    expect(logger.stderr).toEqual(jasmine.arrayContaining([
-      '[percy:core:discovery] Handling request: http://localhost:8000/',
-      '[percy:core:discovery] - Serving root resource',
-      '[percy:core:discovery] An invalid URL was detected for url: http://localhost:404/%style.css - the snapshot may fail on Percy. Please verify that your asset URL is valid.',
-      '[percy:core:discovery] Handling request: http://localhost:404/%style.css',
-      '[percy:core:discovery] Handling request: http://localhost:8000/img.gif'
-    ]));
-  });
-
   it('allows setting a custom discovery user-agent', async () => {
     let userAgent;
 

diff --git a/packages/core/test/snapshot.test.js b/packages/core/test/snapshot.test.js
@@ -132,100 +132,6 @@ describe('Snapshot', () => {
     ]);
   });
 
-  describe('snapshot urls', () => {
-    beforeEach(() => {
-      percy.loglevel('debug');
-    });
-
-    describe('with invalid snapshot URL', () => {
-      const sharedExpectModifiedSnapshotURL = (expectedURL) => {
-        expect(logger.stderr).toEqual(jasmine.arrayContaining([
-          `[percy:core:snapshot] Snapshot URL modified to: ${expectedURL}`
-        ]));
-      };
-
-      it('modifies if url contains space', async () => {
-        await percy.snapshot([
-          { url: 'http://localhost:8000/ a' }
-        ]);
-
-        sharedExpectModifiedSnapshotURL('http://localhost:8000/%20a');
-      });
-
-      it('modifies if url contains [ ] to %5B and %5D', async () => {
-        let givenURL = 'http://localhost:8000/advanced-search/cf1a5848-f658-4939-be11-dcb7fd081043/results/[%229db14794-47f0-406a-a3b4-3f89280122b7%22]/structures/by-ccdc-number/123457';
-        await percy.snapshot([
-          { url: givenURL }
-        ]);
-
-        sharedExpectModifiedSnapshotURL('http://localhost:8000/advanced-search/cf1a5848-f658-4939-be11-dcb7fd081043/results/%5B%229db14794-47f0-406a-a3b4-3f89280122b7%22%5D/structures/by-ccdc-number/123457');
-      });
-
-      it('modifies if url contains {} to %7B and %7D', async () => {
-        let givenURL = 'http://localhost:8000/advanced-search/cf1a5848-f658-4939-be11-dctwo-{ abc [dbd] }/2253';
-        await percy.snapshot([
-          { url: givenURL }
-        ]);
-
-        sharedExpectModifiedSnapshotURL('http://localhost:8000/advanced-search/cf1a5848-f658-4939-be11-dctwo-%7B%20abc%20%5Bdbd%5D%20%7D/2253');
-      });
-
-      it('modifies if url is partially encoded', async () => {
-        // Here some character are encoded properly, this test ensure those encoed character should not gets encoded again
-        let partiallyEncodedURL = 'http://localhost:8000/advanced-search/cf1a5848-f658-4939-be11-dctwo-%7B%20abc%20[dbd]%20%7D/2253';
-        await percy.snapshot([
-          { url: partiallyEncodedURL }
-        ]);
-
-        sharedExpectModifiedSnapshotURL('http://localhost:8000/advanced-search/cf1a5848-f658-4939-be11-dctwo-%7B%20abc%20%5Bdbd%5D%20%7D/2253');
-      });
-
-      it('warns for invalid characters that can not get encoded properly', async () => {
-        let givenURL = 'http://localhost:8000/advanced-search/cf1a5848%-f658-4939-be11-dct';
-        await percy.snapshot([
-          { url: givenURL }
-        ]);
-
-        expect(logger.stderr).toEqual(jasmine.arrayContaining([
-          `[percy:core:snapshot] Invalid URL detected for url: ${givenURL} - the snapshot may fail on Percy. Please confirm that your website URL is valid.`,
-          `[percy:core:discovery] An invalid URL was detected for url: ${givenURL} - the snapshot may fail on Percy. Please verify that your asset URL is valid.`
-        ]));
-      });
-
-      it('debug logs and does not warn if error other than URIError', async () => {
-        spyOn(global, 'decodeURI').and.callFake(() => {
-          throw new Error('Some Invalid Error');
-        });
-
-        let givenURL = 'http://localhost:8000/';
-        await percy.snapshot([
-          { url: givenURL }
-        ]);
-
-        expect(logger.stderr).not.toEqual(jasmine.arrayContaining([
-          `[percy] Invalid URL detected for url: ${givenURL}`
-        ]));
-      });
-    });
-
-    describe('with valid snpashot url', () => {
-      const sharedExpectNonModifiedSnapshotURL = (expectedURL) => {
-        expect(logger.stdout).not.toEqual(jasmine.arrayContaining([
-          `[percy] Snapshot URL modified to: ${expectedURL}`
-        ]));
-      };
-
-      it('not modifies if the url does not invalid characters', async () => {
-        let givenURL = 'http://localhost:8000/advanced-search/cf1a5848-f658-4939-be11-dx3';
-        await percy.snapshot([
-          { url: givenURL }
-        ]);
-
-        sharedExpectNonModifiedSnapshotURL(givenURL);
-      });
-    });
-  });
-
   it('errors if the url is invalid', async () => {
     await percy.snapshot({
       name: 'test snapshot',

diff --git a/packages/core/test/utils.test.js b/packages/core/test/utils.test.js