Opened 5 years ago

Closed 4 years ago

#4022 enhancement closed fixed (fixed)

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

Reported by: exarkun Owned by:
Priority: low Milestone:
Component: web Keywords: easy
Cc: rwall Branch:
Author: Launchpad Bug:

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 rwall 5 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 djfroofy 4 years ago.
Patch to only accept list value in setRawHeaders

Download all attachments as: .zip

Change History (8)

Changed 5 years ago by rwall

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

comment:1 Changed 5 years ago by rwall

  • Cc rwall added
  • Keywords review added
  • Owner changed from jknight to exarkun

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

comment:2 Changed 5 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to rwall

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 4 years ago by djfroofy

Patch to only accept list value in setRawHeaders

comment:3 Changed 4 years ago by djfroofy

  • Keywords review added
  • Owner changed from rwall to exarkun

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

comment:4 Changed 4 years ago by exarkun

  • Keywords review removed
  • Status changed from new to assigned

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

comment:5 Changed 4 years ago by exarkun

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

(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 3 years ago by <automation>

  • Owner exarkun deleted
Note: See TracTickets for help on using tickets.