Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable encoding on url segments #85

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rjimenezda
Copy link

I was trying to rewrite all urls that looked like a/b/:path+ to c/d/:path, so in practice:

a/b/file => c/d/file
a/b/dir/dir/dir/file => c/d/dir/dir/file

However, it didn't work. After some digging I found that path-to-regexp was encoding the slashes in the matched url segment.

However, you can pass a custom encoder, so I've added an option on rewrite objects to use an identity function (value => value), so no character gets encoded in the URL. I've named it raw but I'm open to suggestions.

There's a minor detail, you also have to provide the regex for the destintation, because by default they disallow certain characters there as well. I've added an example to the README mentioning this too. Also, a small test case.

@codecov
Copy link

codecov bot commented Apr 3, 2019

Codecov Report

Merging #85 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #85   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      2           
  Lines         324    324           
=====================================
  Hits          324    324
Impacted Files Coverage Δ
src/index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9101d15...1751eaf. Read the comment docs.

warren-bank added a commit to warren-bank/node-serve that referenced this pull request Dec 22, 2021
primary use case:
* regex pattern matches a subset of the URL pathname
  example (configs):
    - source:
        '/foo/:bar*'
    - destination:
        '/:bar'
    - purpose:
      * 'rewrites' rule would be used to silently ignore
        the leading 'foo/' directory in path
      * 'redirects' rule would be used to trigger a 301 redirect
        to a new URL that removes
        the leading 'foo/' directory in path
  example (behavior):
    - request URL path:
        '/foo/a/b/c/d/e/f.txt'
    - :bar interpolates to value (before encoding):
        'a/b/c/d/e/f.txt'
    - :bar interpolates to value (after default encoding):
        encodeURIComponent('a/b/c/d/e/f.txt') === 'a%2Fb%2Fc%2Fd%2Fe%2Ff.txt'
* if the corresponding 'rewrites' or 'redirects' rule includes the flag:
    {"raw": true}
  then the raw value will be returned without any encoding

based on:
* upstream PR 85
    vercel/serve-handler#85

references:
* pathToRegExp.compile(data, options)
    https://github.com/pillarjs/path-to-regexp#compile-reverse-path-to-regexp

road blocks:
* pathToRegExp bug
    pillarjs/path-to-regexp#260
    pillarjs/path-to-regexp#261
  status:
  - the desired behavior will remain broken until this PR is merged
  - 'source' patterns that match one or more '/' characters
    cause the library to throw an Error for a failed assertion
@a10nik
Copy link

a10nik commented Mar 16, 2024

I wonder what happened to this PR. Seems still valid to me.

@na-ji
Copy link

na-ji commented Jul 20, 2024

The downside of this PR is that it doesn't work with glob patterns. For instance:

{ source: 'folderA/**', destination: 'folderB/:0', raw: true }

Will throw this error:

 ⨯ TypeError: Expected "0" to match "[^\/]+?", but got "sub/folder/to/file.png"
    at /Users/user/www/myapp/node_modules/path-to-regexp/index.js:180:17
    at toTarget (/Users/user/www/myapp/node_modules/serve-handler/src/index.js:88:9)
    at applyRewrites (/Users/user/www/myapp/node_modules/serve-handler/src/index.js:105:18)
    at module.exports (/Users/user/www/myapp/node_modules/serve-handler/src/index.js:618:24)

In the meantime, I will use it as is with a regex, as in the test. Here is the patch-package I made for this. You just need to add this file patches/serve-handler+6.1.5.patch:

diff --git a/node_modules/serve-handler/src/index.js b/node_modules/serve-handler/src/index.js
index 05e3430..c29cb86 100644
--- a/node_modules/serve-handler/src/index.js
+++ b/node_modules/serve-handler/src/index.js
@@ -66,7 +66,7 @@ const sourceMatches = (source, requestPath, allowSegments) => {
 	return null;
 };
 
-const toTarget = (source, destination, previousPath) => {
+const toTarget = (source, destination, previousPath, raw) => {
 	const matches = sourceMatches(source, previousPath, true);
 
 	if (!matches) {
@@ -85,7 +85,7 @@ const toTarget = (source, destination, previousPath) => {
 		props[name] = results[index + 1];
 	}
 
-	return toPath(props);
+	return toPath(props, raw ? {encode: value => value} : {});
 };
 
 const applyRewrites = (requestPath, rewrites = [], repetitive) => {
@@ -101,8 +101,8 @@ const applyRewrites = (requestPath, rewrites = [], repetitive) => {
 	}
 
 	for (let index = 0; index < rewritesCopy.length; index++) {
-		const {source, destination} = rewrites[index];
-		const target = toTarget(source, destination, requestPath);
+		const {source, destination, raw} = rewrites[index];
+		const target = toTarget(source, destination, requestPath, raw);
 
 		if (target) {
 			// Remove rules that were already applied

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants