Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork98
Prevent single wildcards from matching path separators for URL Redirects#501
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Prevent single wildcards from matching path separators for URL Redirects#501
Uh oh!
There was an error while loading.Please reload this page.
Conversation
semanticdiff-combot commentedNov 27, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Changed Files
|
e5b76da
toe591a0d
CompareThis ensures that * does not match the path separator.
e591a0d
toa58b67e
CompareWith # Glob groups 5[[advanced.redirects]]source ="**/{*}.{jpg,jpeg}"destination ="http://localhost/new-images/$2.$3"kind =302# Glob groups 6[[advanced.redirects]]source ="**/{*}.{ttf,otf,woff}"destination ="http://localhost/new-fonts/$2.woff"kind =302 For instance:
The additional path is now contained in |
You can try that. I wonder if we can take advantage of this PR and add a few more tests tackling other common cases. E.g. double/single -- Additionally, since Aside, I'm still wondering whyglobset does not go with |
Sure. Can you add more detail what you mean with those examples? It’s not totally clear to me. |
joseluisq commentedNov 27, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I just meant to add a couple of other unit test cases (not just |
I fixed the tests.
Added an extra admonition warning about this. Happy for any alternative suggestions on the text etc. Is the changelog automatically generated? Otherwise we would need to add it there too.
I wonder the same. So I asked here:BurntSushi/ripgrep#2940 Regarding the other tests. If you're ok with it I would do this in a separate PR. There are also some warnings that mkdocs reports that would be good to address. |
The changelog is not automatically generated but we will mention it in the upcoming release for sure.
That makes sense, take your time. The current PR though looks fine to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks!
96ed7df
intostatic-web-server:masterUh oh!
There was an error while loading.Please reload this page.
I'm also thinking that the URL Rewrites feature should follow the same approach. PR is welcome. |
Makes sense. Is this required for the next release? |
No. It isn't, but I have plans to release v2.34.0 on the weekend but take your time. Btw feel free to join Discord. |
Uh oh!
There was an error while loading.Please reload this page.
Description
globset
by default hasliteral_separator(false)
leading to*
matching the path separator.This PR ensures that
*
does not match the path separator in redirects.See:https://docs.rs/globset/0.4.15/globset/#syntax
Related Issue
See the documentation here:https://github.com/orgs/static-web-server/discussions/498
Motivation and Context
As per the discussion, the default of not matching the path separator is in line with how it works on Unix.
See also:https://en.wikipedia.org/wiki/Glob_(programming) andhttps://man7.org/linux/man-pages/man7/glob.7.html#:~:text=A%20'/'%20in%20a%20pathname%20cannot%20be%20matched
How Has This Been Tested?
Via test cases and manually.
Screenshots (if appropriate):