-
Notifications
You must be signed in to change notification settings - Fork 9.1k
v3.1.2: Fix guidance on headers and RFC6570 percent-encoding #4819
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
Conversation
After much debate and research, we agreed that percent-encoding was never meant to be applied to headers. Exactly how to handle RFC6570 and cookie parameters remains TBD. For now, this preserves (but streamlines) the existing guidance for cookies.
@ralfhandl the "no spaces inside code span element" is causing a linting failure even though the space is there intentionally. Is it possible to disable the rule for this particular use? Or would it need to be disabled for the whole document? |
@ralfhandl I can't even get it to turn off locally. I reworded the sentence- I dislike this option but I care more about getting this merged than blocking on linter pickiness. |
LGTM, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. 👍
Do you want to mention Structured Headers in the appendix?
I'd rather not, as it was previously mentioned only to given an example of conflicting escaping guidance, and there is no longer a conflict. Structured Headers are still quite rare, and bringing that in is more likely to add confusion than clarity, I think. For any header, you now need to look up the escaping rules yourself. |
@ralfhandl I have tried to ensure that all of the formal requirements are in non-appendix sections (this may not be entirely true, I should go through and check it once we finish the new PRs), so I do not want to remove those sections in favor of a link to Appendix D. I have clarified them. I have also broken up Appendix D so that the comment about what no longer applies to headers is more clear. Note that for 3.2, PR #4822 would remove this part as well. |
Sorry, I’m not familiar with that linter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Two very, very minor observations that should not hold up merging this change in the slightest.
Note: Since this is just correcting incorrect guidance rather than changing the actual behavior, it is suitable for a patch release. I will port it to 3.2, where it will be paired with a fix to
allowReserved
that is not needed in 3.1.2, once merged.Fixes part of #4798.
After much debate and research, we agreed that percent-encoding was never meant to be applied to headers.
Exactly how to handle RFC6570 with cookie parameters remains TBD. For now, this preserves (but streamlines) the existing guidance for cookies.