Opened 6 years ago

Closed 4 years ago

#3684 defect closed fixed (fixed)

HEAD-handling code corrupts the HTTP channel in some cases, is generally redundant and confused in others

Reported by: glyph Owned by: washort
Priority: normal Milestone:
Component: web Keywords:
Cc: Branch: branches/head-3684
(diff, github, buildbot, log)
Author: washort Launchpad Bug:

Description

If an UnsupportedMethod exception is thrown for rendering a HEAD method, the default for Request.render (if GET is in allowedMethods) is to call render() again with the request mutated to have its method be GET. In some resources presumably this will return a body.

If the Resource's render_GET returns NOT_DONE_YET (i.e. defers writing its body to the channel), the body will actually be returned to the client, violating the RFC. If the method attribute were left as HEAD, then http.Request.write would enforce the no-data rule.

According to trial --coverage twisted.web, this code path is completely untested. It's not clear that it's even necessary, given that Resource.render_HEAD delegates to render_GET by default. (Or, vice versa.)

There's also more confused code at the end of server.Request.render that tries to specifically handle HEAD.

Change History (11)

comment:1 Changed 5 years ago by dreid

  • Owner changed from glyph to dash

comment:2 Changed 5 years ago by washort

  • Author set to washort
  • Branch set to branches/head-3684

(In [28406]) Branching to head-3684

comment:3 Changed 5 years ago by washort

  • Keywords review added
  • Owner dash deleted

comment:4 Changed 5 years ago by wsanchez

  • Owner set to wsanchez
  • Status changed from new to assigned

comment:5 Changed 5 years ago by wsanchez

First, I like that your branch is called "head"

comment:6 Changed 5 years ago by wsanchez

  • Keywords review removed
  • Owner changed from wsanchez to washort
  • Status changed from assigned to new

OK, so with this API, if you have allowedMethods = ["GET"], then even though you are telling us that you don't want to allow HEAD (for whatever reason), we're going to ignore you and allow it anyway.

This strikes me as strangely inconsistent/exceptional API, so that bothers me. What I would prefer is that if you say allowedMethods = ["GET", "HEAD"], but you don't implement the HEAD method, then we'll use your GET implementation to provide HEAD.

That is, we should do what you ask us to do, and not pretend that we know better.

comment:7 Changed 5 years ago by wsanchez

Otherwise, this patch looks fine

comment:8 Changed 4 years ago by washort

It's always done this. #4958 filed to address this problem.

comment:9 Changed 4 years ago by washort

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

(In [31179])
Merge 'head-3684'

Author: washort
Reviewer: wsanchez
Fixes: #3684

When render_GET is called to satisfy HEAD requests, don't write a body in the response.

comment:10 Changed 4 years ago by exarkun

  • Resolution fixed deleted
  • Status changed from closed to reopened

(In [31184]) Revert r31179 - test suite regression

twisted.web.test.test_distrib.UserDirectoryTests.test_interface and
twisted.web.test.test_wsgi.WSGIResourceTests.test_interfaces fail the
verifyObject check because they do not provide the allowedMethods
attribute.

Reopens #3684

comment:11 Changed 4 years ago by washort

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

(In [31195]) Merge 'head-3684'

Author: washort
Reviewer: wsanchez
Fixes: #3684

When render_GET is called to satisfy HEAD requests, don't write a body in the response.
Remerge of r15826 which was reverted because IResource was incorrectly changed.

Note: See TracTickets for help on using tickets.