Skip to content

web: include offending value in Unsafe header ValueError message#3679

Draft
HrachShah wants to merge 1 commit into
tornadoweb:masterfrom
HrachShah:fix/unsafe-header-error-format
Draft

web: include offending value in Unsafe header ValueError message#3679
HrachShah wants to merge 1 commit into
tornadoweb:masterfrom
HrachShah:fix/unsafe-header-error-format

Conversation

@HrachShah

Copy link
Copy Markdown

_convert_header_value raised ValueError("Unsafe header value %r", retval) — a two-argument form that str(e) rendered as the tuple repr ('Unsafe header value %r', 'foo\r\nbar') with the actual offending value tucked into e.args[1]. Callers that logged the exception got the literal "Unsafe header value %r" with the bad value nowhere to be seen, which defeats the purpose of the message.

The fix is the obvious one: format the value into the message so str(e) actually contains it. The two existing integration tests (test_header_injection, test_set_header) still pass because they only assert the substring "Unsafe header value" is in str(e), which is true in both forms.

  • tornado/web.py: change raise ValueError("Unsafe header value %r", retval) to raise ValueError("Unsafe header value %r" % retval).
  • tornado/test/web_test.py: add a new ConvertHeaderValueTest class with 5 unit tests that exercise the str, bytes (latin1), int, empty-string, and unsafe-value code paths directly without going through a full handler. The unsafe-value test asserts that the bad value appears in the message, that len(e.args) == 1, and that "Unsafe header value" is in str(e) — so any future regression to a two-arg raise (or to a different format placeholder) would be caught.

Verification: pre-fix the new test_unsafe_value_message_includes_value fails with AssertionError: 2 != 1; post-fix all 5 new tests pass. Existing test_header_injection, test_set_header, and the 3 Accept-Language tests in AcceptLanguageTest (which also touch _convert_header_value indirectly) all still pass.

Drafted with Claude.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant