fix decoding url #207

Eomm · 2021-10-06T12:35:35Z

Fixes #206

index.js

mav-rik · 2021-10-06T19:27:27Z

@Eomm I would rather put decodeURI right at sanitizeUrl output.

First of all decodeURI must be wrapped to be called safe:

function safeDecodeURI (v) {
    if (v.indexOf('%') < 0) return v
    try {
        return decodeURI(v)
    } catch (e) {
        return null
    }
}

and the sanitizeUrl:

function sanitizeUrl (url) {
  for (var i = 0, len = url.length; i < len; i++) {
    var charCode = url.charCodeAt(i)
    // Some systems do not follow RFC and separate the path and query
    // string with a `;` character (code 59), e.g. `/foo;jsessionid=123456`.
    // Thus, we need to split on `;` as well as `?` and `#`.
    if (charCode === 63 || charCode === 59 || charCode === 35) {
      return safeDecodeURI(url.slice(0, i))
    }
  }
  return safeDecodeURI(url)
}

All the calls of fastDecode should stay untached for further decoding of dynamic params.

Eomm · 2021-10-08T15:27:30Z

All the calls of fastDecode should stay untached for further decoding of dynamic params.

Right, and moreover

https://www.rfc-editor.org/rfc/rfc3986#section-2.4

Implementations must not
percent-encode or decode the same string more than once, as decoding
an already decoded string might lead to misinterpreting a percent
data octet as the beginning of a percent-encoding, or vice versa in
the case of percent-encoding an already percent-encoded string.

The main issue is that:

we want to decode the URL before executing the routing phase
we don't want to decode the /(%2F) because it defines the URL segment and it is a path parameter

I think the sanitizeUrl works fine, but I had to move the call into the find method, removing from the lookup.
I think this is overkill: we should sanitize only once and the find method is recursive.

I need to find how to archive it

Eomm · 2021-10-11T07:29:24Z

we should sanitize only once and the find method is recursive.

This was not true: the iteration happens inside the while loop:

find-my-way/index.js

Line 375 in c561d22

while (true) {

so the sanitizeUrl is executed once

index.js

Eomm · 2021-10-13T14:43:51Z

The @mav-rik solution works (thks for the help)

I have added a failing test to check this issue https://owasp.org/www-community/Double_Encoding
Checking how to solve ⏳

Eomm · 2021-10-13T14:49:41Z

test/on-bad-url.test.js

@@ -30,7 +30,7 @@ test('If onBadUrl is defined, then a bad url should be handled differently (look
      t.fail('Should not be defaultRoute')
    },
    onBadUrl: (path, req, res) => {
-      t.equal(path, '%world')
+      t.equal(path, '/hello/%world')


This could be a breaking change:
the onBadUrl was getting the path parameter when there were wrong URLs and not the whole path

index.js

Eomm · 2021-10-14T16:47:03Z

I have added two benches on both branches.
When there is an encoded path parameter, the performance drop is quite big

I think we can work on it a bit more: the decode-component should be executed only when there are one of these encoded chars:

; , / ? : @ & = + $ - _ . ! ~ * ' ( ) #

NOTE: this decoding is needed to keep supporting this feature:

The url is sanitized internally, all the parameters and wildcards are decoded automatically.

master branch

lookup static route x 77,362,766 ops/sec ±0.11% (595 runs sampled)
lookup dynamic route x 6,564,291 ops/sec ±0.07% (597 runs sampled)
lookup dynamic multi-parametric route x 3,502,668 ops/sec ±0.11% (596 runs sampled)
lookup dynamic multi-parametric route with regex x 2,688,043 ops/sec ±0.14% (596 runs sampled)
lookup long static route x 5,130,292 ops/sec ±0.09% (595 runs sampled)
lookup long dynamic route x 3,867,784 ops/sec ±0.29% (595 runs sampled)
lookup static route on constrained router x 26,144,108 ops/sec ±0.21% (588 runs sampled)
lookup static versioned route x 4,292,943 ops/sec ±0.11% (596 runs sampled)
lookup static constrained (version & host) route x 4,249,856 ops/sec ±0.38% (595 runs sampled)
find static route x 54,420,447 ops/sec ±0.37% (592 runs sampled)
find dynamic route x 7,250,609 ops/sec ±0.22% (595 runs sampled)
+find dynamic route with encoded parameter x 5,018,355 ops/sec ±0.16% (596 runs sampled)
find dynamic multi-parametric route x 3,886,136 ops/sec ±0.09% (596 runs sampled)
find dynamic multi-parametric route with regex x 2,944,841 ops/sec ±0.29% (595 runs sampled)
find long static route x 7,246,821 ops/sec ±0.25% (596 runs sampled)
find long dynamic route x 5,256,830 ops/sec ±0.13% (595 runs sampled)
find long nested dynamic route x 2,145,797 ops/sec ±0.11% (594 runs sampled)
+find long nested dynamic route with encoded parameter x 1,711,683 ops/sec ±0.09% (597 runs sampled)
find long nested dynamic route with other method x 4,399,929 ops/sec ±0.12% (595 runs sampled)
find long nested dynamic route x 2,115,145 ops/sec ±0.09% (596 runs sampled)
find long nested dynamic route with other method x 4,405,515 ops/sec ±0.09% (596 runs sampled)

this PR

lookup static route x 75,074,382 ops/sec ±0.17% (595 runs sampled)
lookup dynamic route x 6,722,545 ops/sec ±0.09% (597 runs sampled)
lookup dynamic multi-parametric route x 3,511,734 ops/sec ±0.11% (596 runs sampled)
lookup dynamic multi-parametric route with regex x 2,706,381 ops/sec ±0.14% (595 runs sampled)
lookup long static route x 4,966,484 ops/sec ±0.11% (595 runs sampled)
lookup long dynamic route x 3,873,694 ops/sec ±0.11% (595 runs sampled)
lookup static route on constrained router x 25,935,927 ops/sec ±0.24% (587 runs sampled)
lookup static versioned route x 4,210,813 ops/sec ±0.10% (596 runs sampled)
lookup static constrained (version & host) route x 4,201,175 ops/sec ±0.10% (596 runs sampled)
find static route x 50,637,288 ops/sec ±0.38% (590 runs sampled)
find dynamic route x 6,713,246 ops/sec ±0.13% (596 runs sampled)
-find dynamic route with encoded parameter x 1,328,750 ops/sec ±0.18% (594 runs sampled)
find dynamic multi-parametric route x 3,582,311 ops/sec ±0.11% (595 runs sampled)
find dynamic multi-parametric route with regex x 2,765,211 ops/sec ±0.10% (596 runs sampled)
find long static route x 5,171,196 ops/sec ±0.09% (596 runs sampled)
find long dynamic route x 3,942,188 ops/sec ±0.11% (596 runs sampled)
find long nested dynamic route x 1,997,218 ops/sec ±0.11% (596 runs sampled)
-find long nested dynamic route with encoded parameter x 719,727 ops/sec ±0.17% (594 runs sampled)
find long nested dynamic route with other method x 3,961,963 ops/sec ±0.12% (596 runs sampled)
find long nested dynamic route x 1,966,554 ops/sec ±0.11% (596 runs sampled)
find long nested dynamic route with other method x 3,955,305 ops/sec ±0.19% (596 runs sampled)

mcollina · 2021-10-14T17:21:59Z

I would land this as a semver-major.

mcollina · 2021-10-14T17:23:33Z

I'm not liking the decreased performance, but that's expectable.

Eomm · 2021-10-14T17:28:22Z

I'm not liking the decreased performance, but that's expectable.

Yes. I want to filter out cases like the space char %20 because in this case, we can skip decodeURIComponent
We should run only the code strictly needed for reducing the performance drop

index.js

lib/url-sanitizer.js

mcollina · 2021-10-20T07:11:27Z

How are we with benchmarks?

mcollina · 2021-10-20T07:15:19Z

I think this is just a bad bug we should fix, so I would release as a patch and then deal with the fallout in Fastify (if there is any).

Eomm · 2021-10-20T07:54:11Z

here the results
the encoded parameter benches have a -50% performance drop

comparing master-pr

  pr:     lookup static route x 76,674,656 ops/sec ±0.12% (595 runs sampled)
+ master: lookup static route x 79,131,796 ops/sec ±0.25% (594 runs sampled)

  pr:     lookup dynamic route x 6,259,776 ops/sec ±0.29% (596 runs sampled)
+ master: lookup dynamic route x 6,604,871 ops/sec ±0.19% (595 runs sampled)

  pr:     lookup dynamic multi-parametric route x 3,508,864 ops/sec ±0.29% (594 runs sampled)
+ master: lookup dynamic multi-parametric route x 3,555,251 ops/sec ±0.11% (595 runs sampled)

  pr:     lookup dynamic multi-parametric route with regex x 2,816,789 ops/sec ±0.46% (596 runs sampled)
+ master: lookup dynamic multi-parametric route with regex x 2,704,261 ops/sec ±0.15% (595 runs sampled)

  pr:     lookup long static route x 4,970,945 ops/sec ±0.11% (595 runs sampled)
+ master: lookup long static route x 5,166,473 ops/sec ±0.19% (593 runs sampled)

  pr:     lookup long dynamic route x 3,939,538 ops/sec ±0.10% (596 runs sampled)
+ master: lookup long dynamic route x 3,884,604 ops/sec ±0.96% (593 runs sampled)

  pr:     lookup static route on constrained router x 24,927,696 ops/sec ±0.20% (588 runs sampled)
+ master: lookup static route on constrained router x 24,286,435 ops/sec ±0.22% (590 runs sampled)

  pr:     lookup static versioned route x 4,206,344 ops/sec ±0.10% (596 runs sampled)
+ master: lookup static versioned route x 4,297,009 ops/sec ±0.17% (595 runs sampled)

  pr:     lookup static constrained (version & host) route x 4,206,567 ops/sec ±0.16% (596 runs sampled)
+ master: lookup static constrained (version & host) route x 4,335,753 ops/sec ±0.20% (596 runs sampled)

  pr:     find static route x 52,060,384 ops/sec ±0.38% (586 runs sampled)
+ master: find static route x 54,845,502 ops/sec ±0.41% (585 runs sampled)

  pr:     find dynamic route x 6,809,845 ops/sec ±0.13% (596 runs sampled)
+ master: find dynamic route x 7,363,324 ops/sec ±0.14% (595 runs sampled)

  pr:     find dynamic route with encoded parameter unoptimized x 1,794,848 ops/sec ±0.13% (596 runs sampled)
+ master: find dynamic route with encoded parameter unoptimized x 4,879,835 ops/sec ±0.15% (596 runs sampled)

  pr:     find dynamic route with encoded parameter optimized x 1,916,581 ops/sec ±0.15% (590 runs sampled)
+ master: find dynamic route with encoded parameter optimized x 4,955,861 ops/sec ±0.12% (596 runs sampled)

  pr:     find dynamic multi-parametric route x 3,347,742 ops/sec ±0.12% (594 runs sampled)
+ master: find dynamic multi-parametric route x 3,957,345 ops/sec ±0.10% (596 runs sampled)

  pr:     find dynamic multi-parametric route with regex x 2,686,346 ops/sec ±0.12% (597 runs sampled)
+ master: find dynamic multi-parametric route with regex x 2,938,081 ops/sec ±0.33% (596 runs sampled)

  pr:     find long static route x 4,479,007 ops/sec ±0.11% (595 runs sampled)
+ master: find long static route x 7,311,289 ops/sec ±0.16% (595 runs sampled)

  pr:     find long dynamic route x 3,392,116 ops/sec ±0.11% (592 runs sampled)
+ master: find long dynamic route x 5,357,651 ops/sec ±0.10% (595 runs sampled)

  pr:     find long nested dynamic route x 1,882,345 ops/sec ±0.12% (595 runs sampled)
+ master: find long nested dynamic route x 2,174,753 ops/sec ±0.09% (596 runs sampled)

  pr:     find long nested dynamic route with encoded parameter unoptimized x 944,112 ops/sec ±0.16% (596 runs sampled)
+ master: find long nested dynamic route with encoded parameter unoptimized x 1,732,654 ops/sec ±0.11% (596 runs sampled)

  pr:     find long nested dynamic route with encoded parameter optimized x 976,013 ops/sec ±0.15% (596 runs sampled)
+ master: find long nested dynamic route with encoded parameter optimized x 1,762,524 ops/sec ±0.11% (596 runs sampled)

  pr:     find long nested dynamic route with other method x 3,700,463 ops/sec ±0.12% (591 runs sampled)
+ master: find long nested dynamic route with other method x 4,476,532 ops/sec ±0.12% (596 runs sampled)

  pr:     find long nested dynamic route x 1,856,434 ops/sec ±0.26% (593 runs sampled)
+ master: find long nested dynamic route x 2,136,509 ops/sec ±0.10% (596 runs sampled)

  pr:     find long nested dynamic route with other method x 3,737,505 ops/sec ±0.12% (596 runs sampled)
+ master: find long nested dynamic route with other method x 4,478,782 ops/sec ±0.10% (595 runs sampled)

mcollina · 2021-10-20T08:09:00Z

test/on-bad-url.test.js

@@ -11,7 +11,7 @@ test('If onBadUrl is defined, then a bad url should be handled differently (find
      t.fail('Should not be defaultRoute')
    },
    onBadUrl: (path, req, res) => {
-      t.equal(path, '/%world')
+      t.equal(path, '/%world', { todo: 'this is not executed' })


why? Can we remove this assertion then?

I would leave it for a follow up PR

mcollina · 2021-10-20T08:11:20Z

Does this change impact at all Fastify benchmarks?

delvedor

Great work! Looks good to me. The drop in performances is not that much in normal cases, so I'm fine with it, and in any case, better to be correct from a spec perspective.

Just to be sure tho, I would release this in a major, just in time for Fastify v4.

README.md

RafaelGSS

LGTM.

Co-authored-by: Simone Busoli <[email protected]>

Eomm · 2021-10-25T07:20:56Z

Does this change impact at all Fastify benchmarks?

No, I think the bench does not check path parameters.

I run the benchmark script on fastify and here are the results (3 runs each, and get the best one).

main

[1] ┌─────────┬──────┬──────┬───────┬──────┬─────────┬─────────┬───────┐
[1] │ Stat    │ 2.5% │ 50%  │ 97.5% │ 99%  │ Avg     │ Stdev   │ Max   │
[1] ├─────────┼──────┼──────┼───────┼──────┼─────────┼─────────┼───────┤
[1] │ Latency │ 2 ms │ 5 ms │ 6 ms  │ 7 ms │ 4.84 ms │ 1.55 ms │ 75 ms │
[1] └─────────┴──────┴──────┴───────┴──────┴─────────┴─────────┴───────┘
[1] ┌───────────┬────────┬────────┬─────────┬─────────┬───────────┬─────────┬────────┐
[1] │ Stat      │ 1%     │ 2.5%   │ 50%     │ 97.5%   │ Avg       │ Stdev   │ Min    │
[1] ├───────────┼────────┼────────┼─────────┼─────────┼───────────┼─────────┼────────┤
[1] │ Req/Sec   │ 166015 │ 166015 │ 188287  │ 189823  │ 185962.67 │ 5577.08 │ 165919 │
[1] ├───────────┼────────┼────────┼─────────┼─────────┼───────────┼─────────┼────────┤
[1] │ Bytes/Sec │ 31 MB  │ 31 MB  │ 35.2 MB │ 35.5 MB │ 34.8 MB   │ 1.04 MB │ 31 MB  │
[1] └───────────┴────────┴────────┴─────────┴─────────┴───────────┴─────────┴────────┘

within this branch

[1] ┌─────────┬──────┬──────┬───────┬──────┬─────────┬─────────┬───────┐
[1] │ Stat    │ 2.5% │ 50%  │ 97.5% │ 99%  │ Avg     │ Stdev   │ Max   │
[1] ├─────────┼──────┼──────┼───────┼──────┼─────────┼─────────┼───────┤
[1] │ Latency │ 2 ms │ 5 ms │ 6 ms  │ 8 ms │ 4.81 ms │ 1.64 ms │ 72 ms │
[1] └─────────┴──────┴──────┴───────┴──────┴─────────┴─────────┴───────┘
[1] ┌───────────┬─────────┬─────────┬─────────┬────────┬───────────┬─────────┬─────────┐
[1] │ Stat      │ 1%      │ 2.5%    │ 50%     │ 97.5%  │ Avg       │ Stdev   │ Min     │
[1] ├───────────┼─────────┼─────────┼─────────┼────────┼───────────┼─────────┼─────────┤
[1] │ Req/Sec   │ 164863  │ 164863  │ 189951  │ 192255 │ 187118.94 │ 6384.67 │ 164805  │
[1] ├───────────┼─────────┼─────────┼─────────┼────────┼───────────┼─────────┼─────────┤
[1] │ Bytes/Sec │ 30.8 MB │ 30.8 MB │ 35.5 MB │ 36 MB  │ 35 MB     │ 1.19 MB │ 30.8 MB │
[1] └───────────┴─────────┴─────────┴─────────┴────────┴───────────┴─────────┴─────────┘

Eomm · 2021-10-26T15:50:47Z

@mcollina could you re-run the pipeline?
I think there were some flakiness on the last run

mcollina · 2021-10-26T19:54:26Z

@mcollina could you re-run the pipeline?

I think there were some flakiness on the last run

seems all passing now

darkgl0w · 2021-10-30T14:26:29Z

There is a little typo on the README.md file.
It should be :

**Note** that you must encode the parameters containing [reserved characters](https://www.rfc-editor.org/rfc/rfc3986#section-2.2).

README.md

mcollina

lgtm

mcollina · 2021-10-30T17:04:20Z

@delvedor could you land and ship a new major?

Co-authored-by: darkgl0w <[email protected]>

Eomm · 2021-11-08T11:24:11Z

@delvedor I may target a next branch if needed 👍🏽

Eomm added 2 commits October 6, 2021 14:34

add failing test

e11a893

fix decode uri before routing

d6a30e7

mav-rik reviewed Oct 6, 2021

View reviewed changes

index.js Outdated Show resolved Hide resolved

Eomm added 4 commits October 8, 2021 12:55

docs and test

a86b100

fix encoding static paths

1f1b8dd

todo: found wrong test

334cd60

restore test

28898e7

clean code

3d362bf

Eomm marked this pull request as ready for review October 11, 2021 07:27

mav-rik reviewed Oct 11, 2021

View reviewed changes

index.js Outdated Show resolved Hide resolved

Eomm added 2 commits October 13, 2021 16:32

fix encoding

d3d61c4

add failing test

0c35714

Eomm commented Oct 13, 2021

View reviewed changes

mcollina requested a review from delvedor October 13, 2021 15:38

mav-rik reviewed Oct 13, 2021

View reviewed changes

index.js Outdated Show resolved Hide resolved

Eomm added 2 commits October 14, 2021 17:25

fix double encoding

5bddbca

add optimization

bb96da9

Eomm added 2 commits October 15, 2021 11:31

remove decodeURIComponent call

73a1566

improve perf

c7c9f50

mcollina reviewed Oct 15, 2021

View reviewed changes

index.js Outdated Show resolved Hide resolved

Eomm added 2 commits October 15, 2021 14:57

sparse array

6b2bf45

remove fast-decode-uri-component

43e06c0

mcollina reviewed Oct 18, 2021

View reviewed changes

lib/url-sanitizer.js Outdated Show resolved Hide resolved

mcollina reviewed Oct 18, 2021

View reviewed changes

lib/url-sanitizer.js Outdated Show resolved Hide resolved

mcollina reviewed Oct 18, 2021

View reviewed changes

lib/url-sanitizer.js Outdated Show resolved Hide resolved

fix remove bind

150ffc9

Eomm requested a review from mcollina October 20, 2021 07:08

mcollina reviewed Oct 20, 2021

View reviewed changes

delvedor approved these changes Oct 20, 2021

View reviewed changes

simoneb reviewed Oct 22, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

RafaelGSS approved these changes Oct 24, 2021

View reviewed changes

Update README.md

c34e166

Co-authored-by: Simone Busoli <[email protected]>

Eomm mentioned this pull request Oct 28, 2021

Update find-my-way for v4 fastify/fastify#3399

Closed

2 tasks

Eomm requested a review from mcollina October 29, 2021 07:27

darkgl0w reviewed Oct 30, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

mcollina approved these changes Oct 30, 2021

View reviewed changes

Update README.md

000146b

Co-authored-by: darkgl0w <[email protected]>

delvedor approved these changes Nov 12, 2021

View reviewed changes

delvedor merged commit 6a6617a into delvedor:master Nov 12, 2021

climba03003 mentioned this pull request Nov 26, 2021

encoding/decoding uri is confused and misconfigured fastify/fastify#3356

Closed

2 tasks

ivan-tymoshenko mentioned this pull request Apr 19, 2022

Fix parameters decoding #253

Closed

jimmythomson mentioned this pull request Feb 19, 2024

Decoding of %2F #348

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix decoding url #207

fix decoding url #207

Eomm commented Oct 6, 2021

mav-rik commented Oct 6, 2021

Eomm commented Oct 8, 2021

Eomm commented Oct 11, 2021

Eomm commented Oct 13, 2021

Eomm Oct 13, 2021

Eomm commented Oct 14, 2021

mcollina commented Oct 14, 2021

mcollina commented Oct 14, 2021

Eomm commented Oct 14, 2021

mcollina commented Oct 20, 2021

mcollina commented Oct 20, 2021

Eomm commented Oct 20, 2021

mcollina Oct 20, 2021

Eomm Oct 20, 2021

mcollina commented Oct 20, 2021

delvedor left a comment

RafaelGSS left a comment

Eomm commented Oct 25, 2021

Eomm commented Oct 26, 2021

mcollina commented Oct 26, 2021

darkgl0w commented Oct 30, 2021 •

edited

Loading

mcollina left a comment

mcollina commented Oct 30, 2021

Eomm commented Nov 8, 2021

fix decoding url #207

fix decoding url #207

Conversation

Eomm commented Oct 6, 2021

mav-rik commented Oct 6, 2021

Eomm commented Oct 8, 2021

Eomm commented Oct 11, 2021

Eomm commented Oct 13, 2021

Eomm Oct 13, 2021

Choose a reason for hiding this comment

Eomm commented Oct 14, 2021

mcollina commented Oct 14, 2021

mcollina commented Oct 14, 2021

Eomm commented Oct 14, 2021

mcollina commented Oct 20, 2021

mcollina commented Oct 20, 2021

Eomm commented Oct 20, 2021

mcollina Oct 20, 2021

Choose a reason for hiding this comment

Eomm Oct 20, 2021

Choose a reason for hiding this comment

mcollina commented Oct 20, 2021

delvedor left a comment

Choose a reason for hiding this comment

RafaelGSS left a comment

Choose a reason for hiding this comment

Eomm commented Oct 25, 2021

Eomm commented Oct 26, 2021

mcollina commented Oct 26, 2021

darkgl0w commented Oct 30, 2021 • edited Loading

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Oct 30, 2021

Eomm commented Nov 8, 2021

darkgl0w commented Oct 30, 2021 •

edited

Loading