APPEND_SLASH behavior

[Tidiane Dia]

Hello, I posted this on #django-users but I think here is the right place to post it.

To give more context, this is the related issue on Wagtail which lead me here.

In general, if a user defines a "catch all non-recognized patterns" URLpattern (let's call it catch-all-404) as Wagtail admin does, something like:

# Default view (will show 404 page)

urlpatterns = [

        ... (some patterns here) ...

        re_path(r'^', home.default),

]

the APPEND_SLASH setting has no effect.

For example, if /account/ is a valid path, when a user requests /account, he will be routed to the catch-all-404 view, regardless of the APPEND_SLASH setting. That is due to the check being done here which considers that the current path is a valid one since it matches the catch-all-404 pattern.

This is not the desired behavior, for Wagtail admin at least.

Instead of calling the should_redirect_with_slash in the process_response method, a trick I found to solve this issue is to call a should_force_redirect_with_slash method that directly checks if appending a slash to the current path turns it into a valid one.

Its only difference with the should_redirect_with_slash method is skipping the line highlighted above.

This seems very specific and I don't know if Django wants to handle this itself (and if so, I don't know if it's going to be this way too) or rather let the user adapt.

In both cases however, the current check being done in the process_response method for the CommonMiddleware(here) seems irrelevant to me unless I am missing something.

[Adam Johnson]

I don't think Django should change here. The current APPEND_SLASH behaviour is conservative and expected. Django can't tell the difference between a catch-all view that "shouldn't really catch the URL", and any other view.

Notably your suggestion would undo the work in django 3.2 to add a catch-all view to the admin to prevent potentially sensitive URL's being enumerated: https://docs.djangoproject.com/en/dev/releases/3.2/#django-contrib-admin

You could instead change *your* catch-all view to do similar processing to CommonMiddleware - check if appending a slash to the URL and resolving it leads to a view (other than your catch-all view), and redirect.

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/6d39c5c0-bd79-48ec-8ae4-bad4ae023237n%40googlegroups.com.

[Tidiane Dia]

Yes I suggested doing that  work at the middleware level, but it's not the preferred solution due to maintanability  concerns.

However, you haven't mentionned the unneccesary check (I think) being done in the CommonMiddleware's process_response method ?

[Tidiane Dia]

It seems that my first link doesn't work.

I was saying that I ran the coverage of the project and found that this line(here) is never hit because the two conditions can never be met at the same time.

Indeed the process_request method already handles if a slash should be appended.
When APPEND_SLASH is set to True, if we have a 404 error after process_request has been called, the condition should_redirect_with_slash can never be met.

[Florian Apolloner]

On Thursday, April 29, 2021 at 4:23:57 PM UTC+2 atd...@gmail.com wrote:

In both cases however, the current check being done in the process_response method for the CommonMiddleware(here) seems irrelevant to me unless I am missing something.

You are most likely not missing anything and we also noticed that when fixing the admin enumeration stuff. But we did not get around to fixing it yet. That said, I think doing this in process_response would be preferable over doing it in process_request so the extra checks when the URL is valid (the common case) are reduced. Resolving URLs can take a bit, especially when the urlconf is long and as such I'd like to get that check out of the "hot" code path. Only doing the redirect in the case of a failing resolve in the first place would probably make this more efficient.

Cheers,

Florian

[Tidiane Dia]

Ok I see better.

Talking about efficiency, I take this opportunity to link here the following draft PR I made: Backtracking URL Resolver and Provide ability to abort URL resolution early, which, if implemented in the right way, may help make URL resolving more efficient and more flexible too.

I understand that you may not have time to review  or maybe this is not something you are planning to put into Django right now, but I would be very happy to have any feedback on it!

[Florian Apolloner]

Hi,

I took a quick glance (literally just that) at the pull requests. I do like the one that offers a way to abort early inside a prefix -- this is a nice optimization and as well might open interesting options for specialized catch all views. I am not convinced about the backtracking PR, which problems does this solve in reality? What does this mean for features like atomic requests -- it is still just one request after all…

Cheers,

Florian

[Tidiane Dia]

Hi, thanks for giving it a look.

The PR are based on existing tickets so these are not my ideas. The relevant ticket for the backtracking URL contains valuable information about its benefits and why the author requested the feature. The idea is to  "provide a way to pass control back to the URL router to try the remaining routes" if a certain route matched at first but doesn't want to handle this request and want to pass the control to another view -- see this comment for an example of issue it would solve in reality --

[Adam Johnson]

That said, I think doing this in process_response would be preferable over doing it in process_request so the extra checks when the URL is valid (the common case) are reduced. Resolving URLs can take a bit, especially when the urlconf is long and as such I'd like to get that check out of the "hot" code path. Only doing the redirect in the case of a failing resolve in the first place would probably make this more efficient.

I agree. I guess it might need a deprecation cycle to move it to process_response though?

--

You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/41821059-b9a5-433b-b08d-78e3d99d84dcn%40googlegroups.com.

[chris.j...@gmail.com]

On Thursday, May 6, 2021 at 10:32:59 AM UTC-7 f.apo...@gmail.com wrote:

I took a quick glance (literally just that) at the pull requests. I do like the one that offers a way to abort early inside a prefix -- this is a nice optimization and as well might open interesting options for specialized catch all views. I am not convinced about the backtracking PR, which problems does this solve in reality? What does this mean for features like atomic requests -- it is still just one request after all…

I'm the one that filed the "abort early" ticket. I was curious about the other one though after reading some of the discussion. With the suggested work-around of having a view call other views, would a view be able to continue URL resolution in that case? To avoid the problems with the bad interaction with the request machinery, another approach that came to mind would be to allow inserting / including a function at any point in the URLconf. The function would return whether the pattern should be skipped or claimed. That would have the advantage of taking place before any view is started.

--Chris

[Florian Apolloner]

On Thursday, May 6, 2021 at 10:19:42 PM UTC+2 Adam Johnson wrote:

That said, I think doing this in process_response would be preferable over doing it in process_request so the extra checks when the URL is valid (the common case) are reduced. Resolving URLs can take a bit, especially when the urlconf is long and as such I'd like to get that check out of the "hot" code path. Only doing the redirect in the case of a failing resolve in the first place would probably make this more efficient.

I agree. I guess it might need a deprecation cycle to move it to process_response though?

Maybe, maybe not. It already (partially?) does exist in process_response as well. If we can show that this doesn't break existing stuff we can do without deprecation.

[Florian Apolloner]

Hi Chris,

nice hearing from you.

On Friday, May 7, 2021 at 6:20:44 PM UTC+2 chris.j...@gmail.com wrote:

With the suggested work-around of having a view call other views, would a view be able to continue URL resolution in that case?

Not without many code changes I fear and I am not sure about the gains.

 

To avoid the problems with the bad interaction with the request machinery, another approach that came to mind would be to allow inserting / including a function at any point in the URLconf. The function would return whether the pattern should be skipped or claimed. That would have the advantage of taking place before any view is started.

I'll put it like this: Interesting idea but I am afraid of the outcome :D Before I'd support such a change we'd really want to gather usecases first and think hard what it could/would break… And now I cannot stop thinking about such a function ala "lambda: random.choice([True, False])". Thank you for that ;)

Cheers,

Florian

[Adam Johnson]

To avoid the problems with the bad interaction with the request machinery, another approach that came to mind would be to allow inserting / including a function at any point in the URLconf. The function would return whether the pattern should be skipped or claimed. That would have the advantage of taking place before any view is started.

One can already do this by reaching a little into the internals... but I'm also not sure it's a good idea.

You can do it like so:

from functools import partial

from django.urls.conf import _path
from django.urls.resolvers import RoutePattern


class FilteringRoutePattern(RoutePattern):
    def match(self, path):
        matched = super().match(path)
        if matched and my_other_logic():
            matched = None
        return matched

filtering_path = partial(_path, Pattern=FilteringRoutePattern)

I tried it with my_other_logic() doing random.choice([True, False]) and it works.

--

You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/0c72e667-0223-4a28-8e4f-83c5d0f3c758n%40googlegroups.com.