Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strip trailing optional whitespace (OWS) from single-line header field values #3254

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

pajod
Copy link
Contributor

@pajod pajod commented Jul 31, 2024

Backwards incompatible behavior - but only in rare instances of trailing padding on header field values. Old behavior was wrong, new behavior is sanctioned by https://datatracker.ietf.org/doc/html/rfc9112#name-field-syntax

Copy link
Owner

@benoitc benoitc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the patch :) can you make it the default since it's part of the spec.

also please make sure seetiings.rst and other documentation fiile update is part of a separate commit

@@ -2241,6 +2241,24 @@ class PasteGlobalConf(Setting):
"""


class RefuseObsoleteFolding(Setting):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let just make it the default if it's not supported by the specidication.

Copy link
Contributor Author

@pajod pajod Aug 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You okay with just flipping it? You added test 016.http in d4ae13c - there may still be systems generating this stuff. If so, it would be forthcoming to offer people the usual flag to determine whether they are hit by this or a different change applied in the same release.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the spec says

Field values containing CR, LF, or NUL characters are invalid and dangerous, due to the varying ways that implementations might parse and interpret those characters; a recipient of CR, LF, or NUL within a field value MUST either reject the message or replace each of those characters with SP before further processing or forwarding of that message. Field values containing other CTL characters are also invalid; however, recipients MAY retain such characters for the sake of robustness when they appear within a safe context (e.g., an application-specific quoted string that will not be processed by any downstream HTTP parser).

So you're right there to keep that extra option.

Copy link
Contributor Author

@pajod pajod Aug 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Careful, there seems to be a mixup here! You quoted the excerpt relating to 2, but I was replying regarding 3:

  1. lone CR/LF or any NUL in field values - always bad, spec demands reject or replace (lets start rejecting, see Refuse requests with invalid and dangerous CR/LF/NUL in header field value, as demanded by rfc9110 section 5.5 #3253)
  2. control characters beyond those aforementioned 3 in field values - sometimes bad, spec still permits some leeway (lets pass them as-is for now)
  3. obsolete folding in regular requests - not bad in itself, but increases severity of other parser bugs, spec demands reject or replace (lets start rejecting and merely add a temporary escape hatch option to ease diagnostics)

(Sorry for submitting somewhat related, but in fact separate concerns in one PR. I can still split it up if that helps..)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is fine. Thanks for the explanation. Let's merge this change. I think we will deprate the option in next release to remove it completely from 24.

Copy link
Owner

@benoitc benoitc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pajod can you make the change none optionnnal and link it to major 23.0 coming ?

Strip whitespace also *after* header field value.
Simply refuse obsolete header folding (a default-off
option to revert is temporarily provided).
While we are at it, explicitly handle recently
introduced http error classes with intended status code.
@benoitc benoitc merged commit 06d537d into benoitc:master Aug 8, 2024
24 checks passed
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.

Gunicorn doesn't strip tabs and spaces from header values on the right
2 participants