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

Discuss limitations on types based on field def #2396

Merged
merged 9 commits into from Mar 8, 2023
Merged

Discuss limitations on types based on field def #2396

merged 9 commits into from Mar 8, 2023

Conversation

mnot
Copy link
Member

@mnot mnot commented Feb 2, 2023

Fixes #2393.

@mnot mnot requested a review from bsdphk as a code owner February 2, 2023 03:17
draft-ietf-httpbis-sfbis.md Outdated Show resolved Hide resolved
Co-authored-by: Julian Reschke <julian.reschke@gmx.de>
@mnot
Copy link
Member Author

mnot commented Feb 3, 2023

It's likely because the field is ignored when parsing fails, but 8941 s 2 allows a field to specify other error handling behaivour.

@reschke
Copy link
Contributor

reschke commented Feb 3, 2023

How so?

8941, Section 4.2 says:

If parsing fails -- including when calling another algorithm -- the entire field value MUST be ignored (i.e., treated as if the field were not present in the section). This is intentionally strict, to improve interoperability and safety, and specifications referencing this document are not allowed to loosen this requirement.

@mnot
Copy link
Member Author

mnot commented Feb 3, 2023

Section 2 has a bit more nuance:

When parsing fails, the entire field is ignored (see Section 4.2); in most situations, violating field-specific constraints should have the same effect. Thus, if a header is defined as an Item and required to be an Integer, but a String is received, the field will by default be ignored. If the field requires different error handling, this should be explicitly specified

@reschke
Copy link
Contributor

reschke commented Feb 3, 2023

You refer to:

If the field requires different error handling, this should be explicitly specified

?

My reading is that this applies to the preceding text about what happens when parsing does not fail, but additional constraints are not met.

If this is not the case, we should consider this to be an inconsistency in requirements and treat it as spec bug.

EDIT: opened #2399

Copy link
Contributor

@reschke reschke left a comment

Choose a reason for hiding this comment

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

I think "likely discarded" is very misleading; it's a requirement to discard.

Co-authored-by: Julian Reschke <julian.reschke@gmx.de>
draft-ietf-httpbis-sfbis.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-sfbis.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-sfbis.md Outdated Show resolved Hide resolved
Copy link
Contributor

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

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

A suggestion you might consider... I think that it would be helpful to include a more complete example here.

There is a hazard here in that any inconsistency how processing occurs creates interesting effects. That is, the field is destroyed if a Date if observed. That might lead to Heisenbugs.

Let's say you have a WAF that uses SF to parse, but it only looks at certain things. Maybe it doesn't look at parameters. This might ignore a Date on something like:

Example: ?0;t=@4098543601

Code that processes the t parameter might (rightfully) discard the field if it sees the Date.

draft-ietf-httpbis-sfbis.md Show resolved Hide resolved
Co-authored-by: Martin Thomson <mt@lowentropy.net>
Copy link
Contributor

@reschke reschke left a comment

Choose a reason for hiding this comment

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

(with two nits)

draft-ietf-httpbis-sfbis.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-sfbis.md Show resolved Hide resolved
Co-authored-by: Julian Reschke <julian.reschke@gmx.de>
@mnot mnot merged commit 7082ff1 into main Mar 8, 2023
2 checks passed
@mnot mnot deleted the mnot/2393 branch March 8, 2023 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Date as an extension in 8941-defined fields
5 participants