Ticket #4022 enhancement closed fixed

Opened 5 years ago

Last modified 4 years ago

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

strict-headers-constructor-4022.patch Download (2.2 KB) - added by rwall 4 years ago.
Add validation for the addHeader value argument and refactor the setHeaders method to use it.
strict-setRawHeaders-4022.diff Download (1.1 KB) - added by djfroofy 4 years ago.
Patch to only accept list value in setRawHeaders

Change History

Changed 4 years ago by rwall

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

1

Changed 4 years ago by rwall

  • keywords review added
  • cc rwall added
  • owner changed from jknight to exarkun

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

2

Changed 4 years ago by exarkun

  • owner changed from exarkun to rwall
  • keywords review removed

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

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.

4

Changed 4 years ago by exarkun

  • status changed from new to assigned
  • keywords review removed

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

5

Changed 4 years ago by exarkun

  • status changed from assigned to closed
  • resolution set to fixed

(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.

6

Changed 3 years ago by <automation>

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