Opened 11 years ago

Closed 10 years ago

#4022 enhancement closed fixed (fixed)

twisted.web.http_headers.Headers allows itself to be constructed from strings instead of lists

Reported by: Jean-Paul Calderone Owned by:
Priority: low Milestone:
Component: web Keywords: easy
Cc: Richard Wall Branch:
Author:

Description

Headers is basically an abstraction around a dict mapping str to list of str. So its initializer will accept such a dict. However, it will also happily accept a dict mapping str to str and populate itself with garbage. It should reject this case.

Attachments (2)

strict-headers-constructor-4022.patch (2.2 KB) - added by Richard Wall 11 years ago.
Add validation for the addHeader value argument and refactor the setHeaders method to use it.
strict-setRawHeaders-4022.diff (1.1 KB) - added by Drew Smathers 10 years ago.
Patch to only accept list value in setRawHeaders

Download all attachments as: .zip

Change History (8)

Changed 11 years ago by Richard Wall

Add validation for the addHeader value argument and refactor the setHeaders method to use it.

comment:1 Changed 11 years ago by Richard Wall

Cc: Richard Wall added
Keywords: review added
Owner: changed from jknight to Jean-Paul Calderone

This looked easy enough so I had a go. Patch attached.

comment:2 Changed 11 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Richard Wall

Thanks. It looks like the patch is doing a bunch of things beyond just what's described on this ticket:

  1. What's the purpose for the change to __init__?
  2. Same question for addRawHeader
  3. The change that looks like it actually addresses this ticket, in setRawHeaders, looks like it's actually doing three things:
    1. It rejects str values (ostensibly the change desired by this ticket)
    2. It rejects unicode values too. This is cool, but there's no unit test.
    3. It converts any non-str, non-unicode iterable into a list, whereas the previous code didn't. This might be a good change... I'm not sure. This is the low-level API. The primary purpose isn't to be friendly. Accepting a whole mess of different types might not be called for. At the very least, it also needs documentation and testing.

Thanks

Changed 10 years ago by Drew Smathers

Patch to only accept list value in setRawHeaders

comment:3 Changed 10 years ago by Drew Smathers

Keywords: review added
Owner: changed from Richard Wall to Jean-Paul Calderone

I'm personally not a fan of explicitly type checking if the documentation around accepted types is clear. Anyhow, patch attached.

comment:4 Changed 10 years ago by Jean-Paul Calderone

Keywords: review removed
Status: newassigned

Thanks, looks good to me, I'll apply.

comment:5 Changed 10 years ago by Jean-Paul Calderone

Resolution: fixed
Status: assignedclosed

(In [28479]) Apply strict-setRawHeaders-4022.diff adding type checking to Header's initializer

Author: djfroofy Reviewer: exarkun Fixes: #4022

Reject any values passed to twisted.web.http_headers.Headers.setRawHeaders which are not lists.

comment:6 Changed 9 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.