Opened 8 years ago

Closed 7 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 8 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 8 years ago.
Patch to only accept list value in setRawHeaders

Download all attachments as: .zip

Change History (8)

Changed 8 years ago by Richard Wall

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

comment:1 Changed 8 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 8 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 8 years ago by Drew Smathers

Patch to only accept list value in setRawHeaders

comment:3 Changed 8 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 7 years ago by Jean-Paul Calderone

Keywords: review removed
Status: newassigned

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

comment:5 Changed 7 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 7 years ago by <automation>

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