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

[fix] Breaking change: path params behaviour changed on 13.0.0 #185

Closed
3 tasks done
mgr-repo opened this issue Sep 6, 2024 · 14 comments
Closed
3 tasks done

[fix] Breaking change: path params behaviour changed on 13.0.0 #185

mgr-repo opened this issue Sep 6, 2024 · 14 comments
Assignees
Labels
enhancement New feature or request

Comments

@mgr-repo
Copy link

mgr-repo commented Sep 6, 2024

Describe the bug

Node.js version: 20 lts

OS version: linux

Description:
Since 13.0.0 plus signs are parsed to spaces in url path params.

Example Route:
router.get('/example/:exampleId/foo')

Access to path param:
const exampleId = ctx.params.exampleId

Path requested from Browser:

/example/value+value+value/foo

Actual behavior

In 13.0.0 exampleId is: value value value

The plus signs are replaced with spaces which is Ok for query params but from my point of view not for path params.
At least it is a breakin change.

Expected behavior

exampleId should be: value+value+value

Code to reproduce

Checklist

  • I have searched through GitHub issues for similar issues.
  • I have completely read through the README and documentation.
  • I have tested my code with the latest version of Node.js and this package and confirmed it is still not working.
@mgr-repo mgr-repo added the bug Something isn't working label Sep 6, 2024
@damianobarbati
Copy link

I see many paths broken now, like:

router.all('(.*)', () => {
router.get('/docs/(docs|openapi).(css|yml)', (ctx) => {
router.use(maintenance(['/health', '/ping']));

I've read the docs of path-to-regexp and the last one... I have no idea what's happening.

@3imed-jaberi 3imed-jaberi self-assigned this Sep 12, 2024
@3imed-jaberi 3imed-jaberi added enhancement New feature or request and removed bug Something isn't working labels Sep 12, 2024
@3imed-jaberi
Copy link
Member

@mgr-repo I will work to make this part more extensible here once I find a breath, and to resolve your case ASAP you can use the latest router version and patch it with patch-package (layer.js file).

-    return decodeURIComponent(text.replace(/\+/g, ' '));
+    return decodeURIComponent(text);

@3imed-jaberi
Copy link
Member

@damianobarbati could you please provide a full example?

@g00ds0n
Copy link

g00ds0n commented Sep 13, 2024

I think this is related. I see this error on v13.0.1

Code Example

const Router = require('koa-router');
const router = new Router();
const api = require('koa-router-version');

const versionFunctions = {
  '1.15.0': (ctx, next) => { /*...*/ },
  '2.10.0': (ctx, next) => { /*...*/ }
}

router.post('attachment.create', '/:version(v\d)?/attachment', api.version(versionFunctions));

Error Output

     TypeError: Unexpected ( at 9, expected END: https://git.new/pathToRegexpError
      at Iter.consume (node_modules/path-to-regexp/dist/index.js:136:15)
      at consume (node_modules/path-to-regexp/dist/index.js:193:16)
      at parse (node_modules/path-to-regexp/dist/index.js:197:20)
      at node_modules/path-to-regexp/dist/index.js:308:74
      at Array.map (<anonymous>)
      at pathToRegexp (node_modules/path-to-regexp/dist/index.js:308:25)
      at new Layer (node_modules/koa-router/lib/layer.js:54:39)
      at Router.register (node_modules/koa-router/lib/router.js:477:19)
      at Router.<computed> [as post] (node_modules/koa-router/lib/router.js:828:10)
      at module.exports.load (lib/routes.js:190:31)
      at main (index.js:188:27)
      at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
      at async Object.start_app (index.js:99:9)
      at async Context.<anonymous> (test/attachment_controller_test.js:39:13)

@iambumblehead
Copy link

iambumblehead commented Sep 15, 2024

https://blakeembrey.com/posts/2024-09-web-redos/

For version 8 [path-to-regexp], I've removed regex features. I know this is a huge pain for users of the library and apologize in advance. [...] custom regular expressions [can combine] to produce ReDoS vectors, the feature has been removed.

Maybe path-to-regex version 6 should continue to be used by @koa/router, instead of 8, though I am already using version 8 with latest version of @koa/router and have updated parts of my own applications for that.

Alternatively, people could be linked to an explanation one found at the path-to-regexp v8 release page https://github.com/pillarjs/path-to-regexp/releases/tag/v8.0.0

@bcurtin144
Copy link

Maybe path-to-regex version 6 should continue to be used by @koa/router, instead of 8, though I am already using version 8 with latest version of @koa/router and have updated parts of my own applications for that.

I agree with this. Jumping path-to-regex two major versions in one patch release of @koa/router is not how semver should work. All my regular expression routes were broken with this supposed patch upgrade.

@ajimix
Copy link

ajimix commented Sep 16, 2024

Following semver, I installed patch fix 13.0.1 and broke everything because of a major breaking change in regexp evaluation 😰

@sleewoo
Copy link

sleewoo commented Sep 16, 2024

@3imed-jaberi can you release 12.1.0 with path-to-regex updated tov6.3.0
Right now v12 is affected by a CVE and v13 broke most the paths.
Transitioning to v13 with new path-to-regex syntax might take some time.
Meanwhile having a v12 release with that vulnerability fixed would make much more sense.
Thanks.

@sleewoo
Copy link

sleewoo commented Sep 16, 2024

also adding breaking change warning to v13 would be nice, with migration instructions, like

v12 Optional Params:

"/users/:id?/delete"

v13 Optional Params:

"/users{/:id}/delete"

@3imed-jaberi
Copy link
Member

@3imed-jaberi can you release 12.1.0 with path-to-regex updated tov6.3.0 Right now v12 is affected by a CVE and v13 broke most the paths. Transitioning to v13 with new path-to-regex syntax might take some time. Meanwhile having a v12 release with that vulnerability fixed would make much more sense. Thanks.

@sleewoo Thanks, we are working on that! [look 👀]

@3imed-jaberi
Copy link
Member

3imed-jaberi commented Sep 16, 2024

Following semver, I installed patch fix 13.0.1 and broke everything because of a major breaking change in regexp evaluation 😰

@ajimix I apologize for the mistake; it was a typo made by a teammate. you can fix the issue by making your version strict to v13.0.0.

...
  "@koa/router": "13.0.0"
...

We are working to align the versions for v12, 13 and 14 (soon)!

@3imed-jaberi
Copy link
Member

@sleewoo v12.0.2 published 🎉!

@3imed-jaberi
Copy link
Member

v13.1.0 published with [email protected] and integration for path-to-regex@8+ will be shipped in major release 🎉!.

cc @titanism

@3imed-jaberi
Copy link
Member

I think all issues here are resolved by the latest versions (v13.1.0 and v12.0.2).

@mgr-repo If you want the old behavior for the query params you can follow my previous comment (patch-package solutions). Otherwise, you can continue using v12.

**I will work to make this part more extensible here once I find a breath (ref) **.

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

No branches or pull requests

8 participants