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

Alternate for #866 Invalid field handling #868

Closed
wants to merge 1 commit into from

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Jul 7, 2021

Removed distinction between treating as malformed and sending a 400, since treating as malformed already
allows a 400 to be sent.

This uses some text from #866 and replaces that PR.

Removed distinction between treating as malformed and sending a 400, since treating as malformed already
allows a 400 to be sent.
@gregw
Copy link
Contributor Author

gregw commented Jul 7, 2021

The outcome I'm striving for here is to avoid the situation where the spec has two classes of invalid fields. With #866 it currently has that a message with one type of invalid character MUSTS be treated as malformed and that bad response code MAY be sent; but for some other types of invalid characters it just says a bad response code SHOULD be sent otherwise MUST treated as malformed (which then says a bad response code MAY be sent).

This PR simplies it to saying that all messages with invalid characters MUST be treated as malformed and that if the message is a request then a bad response code SHOULD be sent.
The definition of invalid characters optionally include those disallows by HTTP.

@gregw
Copy link
Contributor Author

gregw commented Jul 7, 2021

@mnot @martinthomson

@martinthomson
Copy link
Collaborator

martinthomson commented Jul 7, 2021

So this makes the recommendation to use 400 stronger; extending it to include more fields. I think that's OK, but I'm going to defer to @mnot on this. I'm OK with either; this might even be better.

@mnot
Copy link
Member

mnot commented Jul 7, 2021

I did a rewrite, PTAL. Happy to review this one but I'd be suggesting a number of edits here.

@gregw
Copy link
Contributor Author

gregw commented Jul 7, 2021

closed in favour of the updated #866

@gregw gregw closed this Jul 7, 2021
@gregw gregw deleted the invalid-fields branch July 7, 2021 22:23
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.

3 participants