No encode, decode in createLocation (#465)

* Do not encode pathnames, decode in createLocation * Throw when creating bad path, warn for bad location * Rethrow with clearer message on URIError * Throw URIError, add tests to memory history * Remove unnecessary test case * Add note to CHANGES.md
remix-run · Jun 10, 2017 · 4aeb669 · 4aeb669
1 parent 632dd9b
commit 4aeb669
Show file tree

Hide file tree

Showing 13 changed files with 261 additions and 74 deletions.
diff --git a/CHANGES.md b/CHANGES.md
@@ -1,3 +1,7 @@
+## HEAD
+
+- Rely on the user/browser to encode pathname portion of the URL.
+
 ## [v4.6.1]
 > Mar 15, 2017
 

diff --git a/modules/LocationUtils.js b/modules/LocationUtils.js
@@ -33,6 +33,19 @@ export const createLocation = (path, state, key, currentLocation) => {
       location.state = state
   }
 
+  try {
+    location.pathname = decodeURI(location.pathname)
+  } catch (e) {
+    if (e instanceof URIError) {
+      throw new URIError(
+        'Pathname "' + location.pathname + '" could not be decoded. ' +
+        'This is likely caused by an invalid percent-encoding.'
+      )
+    } else {
+      throw e
+    }
+  }
+
   if (key)
     location.key = key
 

diff --git a/modules/PathUtils.js b/modules/PathUtils.js
@@ -29,8 +29,6 @@ export const parsePath = (path) => {
     search = pathname.substr(searchIndex)
     pathname = pathname.substr(0, searchIndex)
   }
-
-  pathname = decodeURI(pathname)
 
   return {
     pathname,
@@ -42,13 +40,14 @@ export const parsePath = (path) => {
 export const createPath = (location) => {
   const { pathname, search, hash } = location
 
-  let path = encodeURI(pathname || '/')
+  let path = pathname || '/'
 
   if (search && search !== '?')
     path += (search.charAt(0) === '?' ? search : `?${search}`)
 
   if (hash && hash !== '#')
     path += (hash.charAt(0) === '#' ? hash : `#${hash}`)
 
+
   return path
 }
diff --git a/modules/__tests__/BrowserHistory-test.js b/modules/__tests__/BrowserHistory-test.js
@@ -71,6 +71,12 @@ describeHistory('a browser history', () => {
       })
     })
 
+    describe('push with an invalid path string (bad percent-encoding)', () => {
+      it('throws an error', (done) => {
+        TestSequences.PushInvalidPathname(history, done)
+      })
+    })
+
     describe('replace a new path', () => {
       it('calls change listeners with the new location', (done) => {
         TestSequences.ReplaceNewLocation(history, done)
@@ -89,6 +95,24 @@ describeHistory('a browser history', () => {
       })
     })
 
+    describe('replace  with an invalid path string (bad percent-encoding)', () => {
+      it('throws an error', (done) => {
+        TestSequences.ReplaceInvalidPathname(history, done)
+      })
+    })
+
+    describe('location created by encoded and unencoded pathname', () => {
+      it('produces the same location.pathname', (done) => {
+        TestSequences.LocationPathnameAlwaysDecoded(history, done)
+      })
+    })
+
+    describe('location created with encoded/unencoded reserved characters', () => {
+      it('produces different location objects', (done) => {
+        TestSequences.EncodedReservedCharacters(history, done)
+      })
+    })
+
     describe('goBack', () => {
       it('calls change listeners with the previous location', (done) => {
         TestSequences.GoBack(history, done)

diff --git a/modules/__tests__/HashHistory-test.js b/modules/__tests__/HashHistory-test.js
@@ -74,6 +74,12 @@ describeHistory('a hash history', () => {
       })
     })
 
+    describe('push with an invalid path string (bad percent-encoding)', () => {
+      it('throws an error', (done) => {
+        TestSequences.PushInvalidPathname(history, done)
+      })
+    })
+
     describe('replace a new path', () => {
       it('calls change listeners with the new location', (done) => {
         TestSequences.ReplaceNewLocation(history, done)
@@ -92,6 +98,24 @@ describeHistory('a hash history', () => {
       })
     })
 
+    describe('replace  with an invalid path string (bad percent-encoding)', () => {
+      it('throws an error', (done) => {
+        TestSequences.ReplaceInvalidPathname(history, done)
+      })
+    })
+
+    describe('location created by encoded and unencoded pathname', () => {
+      it('produces the same location.pathname', (done) => {
+        TestSequences.LocationPathnameAlwaysDecoded(history, done)
+      })
+    })
+
+    describe('location created with encoded/unencoded reserved characters', () => {
+      it('produces different location objects', (done) => {
+        TestSequences.EncodedReservedCharacters(history, done)
+      })
+    })
+
     describeGo('goBack', () => {
       it('calls change listeners with the previous location', (done) => {
         TestSequences.GoBack(history, done)

diff --git a/modules/__tests__/LocationUtils-test.js b/modules/__tests__/LocationUtils-test.js
@@ -112,6 +112,25 @@ describe('createLocation', () => {
     })
   })
 
+  describe('with a path that cannot be decoded', () => {
+
+    describe('given as a string', () => {
+      it('throws custom message when decodeURI throws a URIError', () => {
+        expect(() => {
+          createLocation('/test%')
+        }).toThrow('Pathname "/test%" could not be decoded.')
+      })
+    })
+
+    describe('given as an object', () => {
+      it('throws custom message when decodeURI throws a URIError', () => {
+        expect(() => {
+          createLocation({ pathname: '/test%' })
+        }).toThrow('Pathname "/test%" could not be decoded.')
+      })
+    })
+  })
+
   describe('key', () => {
     it('has a key property if a key is provided', () => {
       const location = createLocation('/the/path', undefined, 'key')

diff --git a/modules/__tests__/MemoryHistory-test.js b/modules/__tests__/MemoryHistory-test.js
@@ -62,6 +62,12 @@ describe('a memory history', () => {
       })
     })
 
+    describe('push with an invalid path string (bad percent-encoding)', () => {
+      it('throws an error', (done) => {
+        TestSequences.PushInvalidPathname(history, done)
+      })
+    })
+
     describe('replace a new path', () => {
       it('calls change listeners with the new location', (done) => {
         TestSequences.ReplaceNewLocation(history, done)
@@ -80,6 +86,24 @@ describe('a memory history', () => {
       })
     })
 
+    describe('replace  with an invalid path string (bad percent-encoding)', () => {
+      it('throws an error', (done) => {
+        TestSequences.ReplaceInvalidPathname(history, done)
+      })
+    })
+
+    describe('location created by encoded and unencoded pathname', () => {
+      it('produces the same location.pathname', (done) => {
+        TestSequences.LocationPathnameAlwaysDecoded(history, done)
+      })
+    })
+
+    describe('location created with encoded/unencoded reserved characters', () => {
+      it('produces different location objects', (done) => {
+        TestSequences.EncodedReservedCharacters(history, done)
+      })
+    })
+
     describe('goBack', () => {
       it('calls change listeners with the previous location', (done) => {
         TestSequences.GoBack(history, done)

diff --git a/modules/__tests__/TestSequences/EncodedReservedCharacters.js b/modules/__tests__/TestSequences/EncodedReservedCharacters.js
@@ -0,0 +1,38 @@
+import expect from 'expect'
+import execSteps from './execSteps'
+
+export default (history, done) => {
+  const steps = [
+    () => {
+      // encoded string
+      const pathname = '/view/%23abc'
+      history.replace(pathname)
+    },
+    (location) => {
+      expect(location).toMatch({
+        pathname: '/view/%23abc'
+      })
+
+      // encoded object
+      const pathname = '/view/%23abc'
+      history.replace({ pathname })
+    },
+    (location) => {
+      expect(location).toMatch({
+        pathname: '/view/%23abc'
+      })
+      // unencoded string
+      const pathname = '/view/#abc'
+      history.replace(pathname)
+    }
+    ,
+    (location) => {
+      expect(location).toMatch({
+        pathname: '/view/',
+        hash: '#abc'
+      })
+    }
+  ]
+
+  execSteps(steps, history, done)
+}
diff --git a/modules/__tests__/TestSequences/LocationPathnameAlwaysDecoded.js b/modules/__tests__/TestSequences/LocationPathnameAlwaysDecoded.js
@@ -0,0 +1,45 @@
+import expect from 'expect'
+import execSteps from './execSteps'
+
+export default (history, done) => {
+  const steps = [
+    () => {
+      // encoded string
+      const pathname = '/%E6%AD%B4%E5%8F%B2'
+      history.replace(pathname)
+    },
+    (location) => {
+      expect(location).toMatch({
+        pathname: '/歴史'
+      })
+
+      // encoded object
+      const pathname = '/%E6%AD%B4%E5%8F%B2'
+      history.replace({ pathname })
+    },
+    (location) => {
+      expect(location).toMatch({
+        pathname: '/歴史'
+      })
+      // unencoded string
+      const pathname = '/歴史'
+      history.replace(pathname)
+    }
+    ,
+    (location) => {
+      expect(location).toMatch({
+        pathname: '/歴史'
+      })
+      // unencoded object
+      const pathname = '/歴史'
+      history.replace({ pathname })
+    },
+    (location) => {
+      expect(location).toMatch({
+        pathname: '/歴史'
+      })
+    }
+  ]
+
+  execSteps(steps, history, done)
+}
diff --git a/modules/__tests__/TestSequences/PushInvalidPathname.js b/modules/__tests__/TestSequences/PushInvalidPathname.js
@@ -0,0 +1,16 @@
+import expect from 'expect'
+import execSteps from './execSteps'
+
+export default (history, done) => {
+  const steps = [
+    () => {
+      expect(() => {
+        history.push('/hello%')
+      }).toThrow(
+        'Pathname "/hello%" could not be decoded. This is likely caused by an invalid percent-encoding.'
+      )
+    }
+  ]
+
+  execSteps(steps, history, done)
+}
diff --git a/modules/__tests__/TestSequences/ReplaceInvalidPathname.js b/modules/__tests__/TestSequences/ReplaceInvalidPathname.js
@@ -0,0 +1,16 @@
+import expect from 'expect'
+import execSteps from './execSteps'
+
+export default (history, done) => {
+  const steps = [
+    () => {
+      expect(() => {
+        history.replace('/hello%')
+      }).toThrow(
+        'Pathname "/hello%" could not be decoded. This is likely caused by an invalid percent-encoding.'
+      )
+    }
+  ]
+
+  execSteps(steps, history, done)
+}
diff --git a/modules/__tests__/TestSequences/index.js b/modules/__tests__/TestSequences/index.js
@@ -4,15 +4,18 @@ export BlockPopWithoutListening from './BlockPopWithoutListening'
 export DenyPush from './DenyPush'
 export DenyGoBack from './DenyGoBack'
 export DenyGoForward from './DenyGoForward'
+export EncodedReservedCharacters from './EncodedReservedCharacters'
 export GoBack from './GoBack'
 export GoForward from './GoForward'
 export HashbangHashPathCoding from './HashbangHashPathCoding'
 export HashChangeTransitionHook from './HashChangeTransitionHook'
 export InitialLocationNoKey from './InitialLocationNoKey'
 export InitialLocationHasKey from './InitialLocationHasKey'
 export Listen from './Listen'
+export LocationPathnameAlwaysDecoded from './LocationPathnameAlwaysDecoded'
 export NoslashHashPathCoding from './NoslashHashPathCoding'
 export PushEncodedLocation from './PushEncodedLocation'
+export PushInvalidPathname from './PushInvalidPathname'
 export PushNewLocation from './PushNewLocation'
 export PushMissingPathname from './PushMissingPathname'
 export PushSamePath from './PushSamePath'
@@ -21,6 +24,7 @@ export PushState from './PushState'
 export PushStateWarning from './PushStateWarning'
 export PushRelativePathname from './PushRelativePathname'
 export PushUnicodeLocation from './PushUnicodeLocation'
+export ReplaceInvalidPathname from './ReplaceInvalidPathname'
 export ReplaceNewLocation from './ReplaceNewLocation'
 export ReplaceSamePath from './ReplaceSamePath'
 export ReplaceState from './ReplaceState'