Code

Opened 5 years ago

Closed 5 years ago

Last modified 3 years ago

#10834 closed (fixed)

BaseHandler does not handle resolver returning None

Reported by: xerolas Owned by: ccahoon
Component: HTTP handling Version: master
Severity: Keywords:
Cc: chris.cahoon@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

django.core.handler.base.BaseHandler has the following call to the resolver:

            callback, callback_args, callback_kwargs = resolver.resolve(
	                    request.path_info)

However resolve() can return None (this happens to me when I get bogus HTTP requests from bots) and TypeError is not explicitly caught (for trying to unpack None).

The result is that 500 is returned instead of 404 and an email to the admins is sent (by default).

Attachments (2)

ticket10834.diff (1.7 KB) - added by ccahoon 5 years ago.
regression text and fix, updated to exclude the else statement
10834r11126.diff (588 bytes) - added by ccahoon 5 years ago.
Updated to pass regression tests.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 5 years ago by mtredinnick

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

This is probably addressing the wrong end of the issue. As things stand, there is a problem. But it isn't that we aren't handling None. It's that we are seeing None in the first place.

We should be raising an exception (either Http404 or Resolver404 might be appropriate). It's an exceptional condition and should enter the normal code paths for handling non-existent URLs, not require special handling in the non-exceptional path.

comment:2 Changed 5 years ago by ccahoon

  • Owner changed from nobody to ccahoon

comment:3 Changed 5 years ago by ccahoon

  • Has patch set

I raise a Resolver404 if self.regex.search(path) returns None. This happens if path is not a regular expression. The unit test gives some examples of test cases that caused resolver to return None before the patch.

Changed 5 years ago by ccahoon

regression text and fix, updated to exclude the else statement

comment:4 Changed 5 years ago by ccahoon

  • Cc malcolm@… added
  • milestone set to 1.1

comment:5 Changed 5 years ago by ccahoon

  • Cc chris.cahoon@… added

comment:6 Changed 5 years ago by russellm

  • Resolution set to fixed
  • Status changed from new to closed

(In [11120]) Fixed #10834 -- Added bucket condition to ensure that URL resolvers won't ever return None. Thanks to Chris Cahoon for the patch.

comment:7 follow-up: Changed 5 years ago by mwelch

This fixed breaks the debug screen. When setting up a new app and setting entries in the urls.py file, you will still get the default blue 'Welcome to django' screen instead of the 404 debug screen.

comment:8 Changed 5 years ago by mwelch

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:9 in reply to: ↑ 7 Changed 5 years ago by anonymous

Replying to mwelch:

This fixed breaks the debug screen. When setting up a new app and setting entries in the urls.py file, you will still get the default blue 'Welcome to django' screen instead of the 404 debug screen.

Please provide more information. Using r11126 I cannot recreate any problem getting the 404 debug screen to show up when I have DEBUG set to True and attempt to access a URL that has no mapping in my urlpatterns.

comment:10 Changed 5 years ago by kmtracey

Sorry, anonymous at 7:01:33 was me. (Normally the spam blocker lets me know when my login has expired but not this time.)

comment:11 Changed 5 years ago by ccahoon

That's very interesting. I was able to recreate showing the Welcome to Django screen if the following urlpattern is not covered:

r'^$'

However, the other urls still functioned properly. I looked around and it looks like it is because of this code in django/views/debug.py (this is on trunk r11126):

244 def technical_404_response(request, exception):
245     "Create a technical 404 error response. The exception should be the     
    Http404."
246     try:
247         tried = exception.args[0]['tried']
248     except (IndexError, TypeError):
249         tried = []
250     else:
251         if not tried:
252             # tried exists but is an empty list. The URLconf must've been   
    empty.
253             return empty_urlconf(request)

The content of the changeset (which was different from my patch) creates an empty "tried" list in the dictionary returned in the Resolver404 exception. Thus, debug thinks there is nothing in the urlconf. Attaching a patch.

I am pretty sure I ran into this issue when creating the fix -- only by not even having "tried" in the dictionary does the 404 behave properly. Now I am not sure if this is correct overall behavior, but for a patch this small, it seemed appropriate to just leave it out.

comment:12 Changed 5 years ago by russellm

@ccahoon: There must be something else going on here, because removing tried from the dictionary causes mass test failures in the test_client and test_client_regress unit tests. Your analysis regarding why tried should be omitted looks sound, though - we'll have to dig deeper to find the cause of the failures in the test_client tests.

comment:13 Changed 5 years ago by ccahoon

@russellm: I've had very recent experience with test_client_regress expecting behavior that is consistent with Django but not necessarily correct. I will take a look and see what is going on later tonight.

Changed 5 years ago by ccahoon

Updated to pass regression tests.

comment:14 Changed 5 years ago by ccahoon

  • Cc malcolm@… removed

That patch passes the test_client_regress tests and seems to behave properly in real use. The machine I made the change on can't handle running the full test suite at the moment, so I do not know about other regression tests.

comment:15 Changed 5 years ago by ccahoon

The patch above passes the regression test suite. @russellm, what do you think of the solution?

comment:16 Changed 5 years ago by russellm

@ccahoon - Not thrilled. The format of the error message with the provided patch is completely misleading. I suspect the patch will need to be more than a single line change - looking at the code, there may be a need to tweak the 404 template to differentiate between the error case described here, and the "new project, no URL's" message.

comment:17 Changed 5 years ago by russellm

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [11156]) [1.0.X] Fixed #10834 -- Corrected [11120] to ensure that there is a difference between catching a bad URL pattern and an new (no URLs) project. Thanks to Matt Welch for the report.

Merge of r11155 from trunk.

comment:18 Changed 5 years ago by russellm

(In [11155]) Fixed #10834 -- Corrected [11120] to ensure that there is a difference between catching a bad URL pattern and an new (no URLs) project. Thanks to Matt Welch for the report.

comment:19 Changed 3 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.