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.

Sign up for GitHub

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

Jump to bottom

Fix the invalid matching of the ":name*" parameter #261

Merged
merged 3 commits into from
Feb 11, 2022
Merged

Fix the invalid matching of the ":name*" parameter #261

merged 3 commits into from
Feb 11, 2022

Conversation

kettanaito
Copy link
Contributor

GitHub

Changes

  • Adds a test case for matching/parsing/compiling the :name* problematic parameter pattern.

@kettanaito kettanaito marked this pull request as draft August 26, 2021 20:20
@kettanaito
Copy link
Contributor Author

I've added a failing test that captures the issue. The test result:

  ● path-to-regexp › rules › ':name*' › match › should match 'foobar'

    expect(received).toEqual(expected) // deep equality

    - Expected  - 1
    + Received  + 1

      Array [
        "foobar",
    -   "foobar",
    +   "r",
      ]

The test result seems to reproduce the unexpected behavior described in #260.

I'll provision the fix as suggested sometime tomorrow. Thank you for your effort in reviewing this. Feel free to point out if I'm adding things in the wrong order/structure.

kettanaito
kettanaito commented Aug 26, 2021
View reviewed changes
src/index.spec.ts Outdated
],
[[null, "/test"]]
[[null, "/test"]],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unexpected autoformatting, although I have no autoformatting enabled. I suspect the commit hook has formatted the files using the wrong Prettier configuration or something. Will resolve.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running npm run prettier src/**/*.ts in a freshly cloned repository produces massive changes in formatting. I suspect that Prettier was introduced to the code base but the code was never formatted. I'd also recommend having an explicit Prettier configuration defined in the project.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've raised this as a separate issue in #262.

@kettanaito kettanaito mentioned this pull request Aug 26, 2021
Closed
@kettanaito kettanaito force-pushed the 260-asterisk-parameter branch 4 times, most recently from 79b5591 to 5df644e Compare August 26, 2021 21:20
@kettanaito kettanaito changed the title Add test for ":name*" parameter matching Fix the invalid matching of the ":name*" parameter Aug 26, 2021
@kettanaito kettanaito force-pushed the 260-asterisk-parameter branch 2 times, most recently from 4208215 to 80d4d03 Compare August 27, 2021 11:38
@kettanaito kettanaito marked this pull request as ready for review August 27, 2021 11:38
@kettanaito kettanaito mentioned this pull request Aug 27, 2021
Closed
warren-bank added a commit to warren-bank/node-serve that referenced this pull request Dec 22, 2021
@warren-bank
flag to disable encoding of regex matches in 'rewrites' and 'redirects'
47d5e3f 
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
wanderview added a commit to wanderview/urlpattern-polyfill that referenced this pull request Jan 7, 2022
@wanderview
Fix matched values for :name* patterns.
5739db9 
This fixes the problem where:

  const p = new URLPattern({ pathname: ':name*' });
  const r = p.exec('foobar');
  console.log(r.pathname.groups.name);

Would log 'r' instead of 'foobar'.

This is an upstream bug in path-to-regexp:

  pillarjs/path-to-regexp#260

This commit essentially applies the proposed fix in:

  pillarjs/path-to-regexp#261

And adds the WPT tests from:

  https://chromium-review.googlesource.com/c/chromium/src/+/3123654
wanderview added a commit to wanderview/urlpattern-polyfill that referenced this pull request Jan 7, 2022
@wanderview
Fix matched values for :name* patterns. (kenchris#126)
f110e9a 
This fixes the problem where:

  const p = new URLPattern({ pathname: ':name*' });
  const r = p.exec('foobar');
  console.log(r.pathname.groups.name);

Would log 'r' instead of 'foobar'.

This is an upstream bug in path-to-regexp:

  pillarjs/path-to-regexp#260

This commit essentially applies the proposed fix in:

  pillarjs/path-to-regexp#261

And adds the WPT tests from:

  https://chromium-review.googlesource.com/c/chromium/src/+/3123654
wanderview added a commit to wanderview/urlpattern-polyfill that referenced this pull request Jan 11, 2022
@wanderview
Fix matched values for :name* patterns. (kenchris#126)
3cbfc6a 
This fixes the problem where:

  const p = new URLPattern({ pathname: ':name*' });
  const r = p.exec('foobar');
  console.log(r.pathname.groups.name);

Would log 'r' instead of 'foobar'.

This is an upstream bug in path-to-regexp:

  pillarjs/path-to-regexp#260

This commit essentially applies the proposed fix in:

  pillarjs/path-to-regexp#261

And adds the WPT tests from:

  https://chromium-review.googlesource.com/c/chromium/src/+/3123654
@wanderview wanderview mentioned this pull request Jan 11, 2022
Merged
kenchris pushed a commit to kenchris/urlpattern-polyfill that referenced this pull request Jan 11, 2022
@wanderview @kenchris
Fix matched values for :name* patterns. (#126)
7a5adfe 
This fixes the problem where:

  const p = new URLPattern({ pathname: ':name*' });
  const r = p.exec('foobar');
  console.log(r.pathname.groups.name);

Would log 'r' instead of 'foobar'.

This is an upstream bug in path-to-regexp:

  pillarjs/path-to-regexp#260

This commit essentially applies the proposed fix in:

  pillarjs/path-to-regexp#261

And adds the WPT tests from:

  https://chromium-review.googlesource.com/c/chromium/src/+/3123654
@codecov
Copy link

codecov bot commented Feb 11, 2022 •
edited

Codecov Report

Merging #261 (27766e2) into master (b45871b) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #261   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines          243       246    +3     
  Branches        96        98    +2     
=========================================
+ Hits           243       246    +3
Impacted Files Coverage Δ
src/index.ts 100.00% <100.00%> (ø)

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 b45871b...27766e2. Read the comment docs.

@blakeembrey blakeembrey merged commit 762bc6b into pillarjs:master Feb 11, 2022
@kettanaito kettanaito deleted the 260-asterisk-parameter branch February 11, 2022 13:54
@blakeembrey blakeembrey mentioned this pull request May 6, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

incorrect group value for :name* pattern
2 participants
@kettanaito @blakeembrey