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

With strict_slashes disabled, leaf paths mask branch paths. #1074

Closed
Parakleta opened this issue Feb 17, 2017 · 2 comments · Fixed by #2458
Closed

With strict_slashes disabled, leaf paths mask branch paths. #1074

Parakleta opened this issue Feb 17, 2017 · 2 comments · Fixed by #2458
Labels
Milestone

Comments

@Parakleta
Copy link

When running the following:

from werkzeug.routing import Map, Rule

srv = Map([
    Rule('/path1', endpoint='leaf'),
    Rule('/path1/', endpoint='branch'),
    Rule('/path2', endpoint='leaf', strict_slashes=False),
    Rule('/path2/', endpoint='branch'),
    Rule('/path3', endpoint='leaf'),
    Rule('/path3/', endpoint='branch', strict_slashes=False),
    Rule('/path4', endpoint='leaf', strict_slashes=False),
    Rule('/path4/', endpoint='branch', strict_slashes=False),
], strict_slashes=False).bind('')

print(srv.match('/path1'), srv.match('/path1/'))
print(srv.match('/path2'), srv.match('/path2/'))
print(srv.match('/path3'), srv.match('/path3/'))
print(srv.match('/path4'), srv.match('/path4/'))

the output is:

(('leaf', {}), ('branch', {}))
(('leaf', {}), ('leaf', {}))
(('leaf', {}), ('branch', {}))
(('leaf', {}), ('leaf', {}))

The issue appears to be in werkzeug/routing.py#L745 which allows leaf paths to consume and discard (via werkzeug/routing.py#L776) the trailing slash, and I assume by the route scoring rules the leaf is given priority. This has the effect of silently masking the rule with the trailing slash.

The change that implemented this was in commit 39cbdfd but I'm not sure why it was introduced. It is possible that it was to prevent a KeyError from being thrown by the pop that is now at werkzeug/routing.py#L770 but this can be achieved by just reordering the if statement so that it is if not self.is_leaf and not groups.pop('__suffix__') and .... If this change is made then the following elif ... del can also be removed.

Alternatively if it is intended that turning off strict_slashes will actually make the trailing slash in both the rules and the requests become irrelevant (which is more in line with what the parameter name suggests) then it is unfortunate that it appears the documentation and most of the commentary on the internet is suggesting that this parameter is used to disable the automatic redirection to the trailing slash URL.

I think the current behaviour is a bit confused whatever the intention is. Currently we can have sloppy rule slashes with strict request slashes (no redirection) or strict rule slashes with sloppy request slashes (with redirection). I need strict slashes for both the rules and requests.

If the change from 39cbdfd is reverted then the only capability that would be lost would be when strict_slashes is disabled having /path and /path/ both map to the same endpoint without redirection (/path if it exists, otherwise /path/) but this behaviour can easily be restored by just manually defining both paths to have the same endpoint.

I think having this available would also help provide work-arounds for a range of related issues such as #397 #773 pallets/flask#1783.

@Parakleta
Copy link
Author

Parakleta commented Feb 19, 2017

I've played with it a bit more, and obviously you have issue of backwards compatibility to consider, but my preferred implementation for a do-over would be to replace the if ... elif from lines 769-777 with the following:

    if (not self.is_leaf or not self.strict_slashes) and \
            not groups.pop('__suffix__') and \
            (method is None or self.methods is None or \
            method in self.methods):
        if self.strict_slashes:
            # This is a branch that doesn't match strictly
            return
        elif not self.is_leaf:
            # This is a non-strict match for a branch, so raise a
            # redirect exception to the URI with the trailing slash
            raise RequestSlash()

This has the effect of making strict_slashes strict in both rule and request, and then having non-strict leaf rules silently match both leaf and branch requests, and non-strict branch rules redirect leaf requests to branch requests. This covers all possible permutations of rule and request strictness and handling except for a redirect from a branch to a leaf.

@pgjones
Copy link
Member

pgjones commented Jul 1, 2022

@Parakleta #2433 should now meet your expectations

@davidism davidism added this to the 2.2.0 milestone Jul 1, 2022
pgjones added a commit to pgjones/werkzeug that referenced this issue Jul 22, 2022
The new router prevents a leaf from consuming the branch if there is a
branch match as well.

See also pallets#1074.
openstack-mirroring pushed a commit to openstack/sushy-tools that referenced this issue Jul 27, 2022
Version 2.2.0 of Werkzeug brings a fix for an issue [1] related to
discarding trailing slash that modifies redirect behavior for
branches, effectively applying strict_slashes rule.

[1] pallets/werkzeug#1074

Change-Id: Id08668d943be0ff94a191a1f06ba93eecbf74eb5
openstack-mirroring pushed a commit to openstack/openstack that referenced this issue Jul 27, 2022
* Update sushy-tools from branch 'master'
  to 58d7f8df316c99446e24d89545703e1da1002351
  - Fix for werkzeug 2.2 routing change
    
    Version 2.2.0 of Werkzeug brings a fix for an issue [1] related to
    discarding trailing slash that modifies redirect behavior for
    branches, effectively applying strict_slashes rule.
    
    [1] pallets/werkzeug#1074
    
    Change-Id: Id08668d943be0ff94a191a1f06ba93eecbf74eb5
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants