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 ignore of requests with extensions #88

Merged
merged 3 commits into from
Dec 5, 2020

Conversation

adam-lynch
Copy link
Contributor

Before this change: If a URL happens to contain a . but it's not a request to a file, it'll be ignored (a 404 is returned). E.g. http://localhost:5000/51.89768130456134,-8.470441788798238.

The regex now only matches if the path contains valid filename characters all the way from the slash til the end, not just at the end.

Before this change: If a URL happens to contain a `.` but it's not a request to a file, it'll be ignored (a 404 is returned). E.g. http://localhost:5000/51.89768130456134,-8.470441788798238. 

The regex now only matches if the path contains valid filename characters all the way from the slash til the end, not just at the end.
Copy link
Owner

@lukeed lukeed left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the PR, but I don't think this is quite right.

This new RegExp does not match anything it should now:

'/foo.jpg';
//=> before: true
//=> after: false

'/hello/foo.jpg';
//=> before: true
//=> after: false

'/51.89,-8.45';
//=> before: true
//=> after: false

@codecov-io
Copy link

Codecov Report

Merging #88 (d0c5b14) into master (7d693e7) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #88   +/-   ##
=======================================
  Coverage   98.52%   98.52%           
=======================================
  Files           2        2           
  Lines          68       68           
=======================================
  Hits           67       67           
  Misses          1        1           

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 7d693e7...d0c5b14. Read the comment docs.

@adam-lynch
Copy link
Contributor Author

Ah sorry, fixed;

image

Copy link
Owner

@lukeed lukeed left a comment

Choose a reason for hiding this comment

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

Thanks! But sorry, another (common) case that this breaks:

'/bundle.min.js'
// before: matched
// after: unmatched

'/foo-bar.js'
// before: matched
// after: unmatched

'/foo.abc.123.js'
// before: matched
// after: unmatched

'/foo.abc-123.js'
// before: matched
// after: unmatched

'/foo-123.js'
// before: matched
// after: unmatched

This is because the RegExp is allowing 2 segments only (name + extension) and looks at the whole filename before the extension begins & - doesn't match \w.

RE 1st reason: This is/was why the original RegExp only looked at the ending, because there's no way to know/control how many dot-delimited segments there might be... just wanted to look at the last segment.

RE 2nd reason: This technically existed before too, but in a much narrower window. Basically, something like /foo-.jpg would fail but that's not very likely to happen.

The tests still/happen to pass because things that should 404 are still 404ing, but after more work to get there.

Am wondering if adding an --ignore value is the best choice here?

@lukeed
Copy link
Owner

lukeed commented Dec 2, 2020

I think this may do the trick:

/[/]([A-Za-z\s\d~$._-]+\.\w+){1,}$/

- still follows slash
- still disallows comma-delimited segment
- fix: allows any number of dot-delimited segments
@lukeed
Copy link
Owner

lukeed commented Dec 5, 2020

Thanks!

@lukeed lukeed merged commit 5e3d7a8 into lukeed:master Dec 5, 2020
@adam-lynch
Copy link
Contributor Author

Thanks! Sorry I didn't intend for you to spend much time on this 😄

@adam-lynch adam-lynch deleted the adam-lynch-patch-1 branch December 5, 2020 21:06
@lukeed
Copy link
Owner

lukeed commented Dec 5, 2020

All good, worth having :)
Published btw

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