gh-127794: Validate header name according to rfc-5322#127820
Merged
picnixz merged 82 commits intopython:mainfrom Mar 30, 2025
Merged
gh-127794: Validate header name according to rfc-5322#127820picnixz merged 82 commits intopython:mainfrom
picnixz merged 82 commits intopython:mainfrom
Conversation
…ly printable ascii characters
srinivasreddy
changed the title
ZeroIntensity
added
topic-email
needs backport to 3.12
bitdancer
requested changes
Dec 12, 2024
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase And if you don't make the requested changes, you will be put in the comfy chair! |
bitdancer
requested changes
Dec 13, 2024
Member
bitdancer
left a comment
bitdancer
left a comment
There was a problem hiding this comment.
Thanks for the prompt response. Hopefully I can be reasonably prompt in return, but unfortunately no guarantees :(
picnixz
reviewed
Jan 2, 2025
Member
picnixz
left a comment
picnixz
left a comment
There was a problem hiding this comment.
Some final thoughts. Otherwise, it looks fine to me.
Misc/NEWS.d/next/Library/2024-12-11-17-44-36.gh-issue-127794.VwmRsp.rst
Outdated
Show resolved
Hide resolved
picnixz
reviewed
Jan 6, 2025
Member
picnixz
left a comment
picnixz
left a comment
There was a problem hiding this comment.
I forgot to post my review 3 days ago..
Misc/NEWS.d/next/Library/2024-12-11-17-44-36.gh-issue-127794.VwmRsp.rst
Outdated
Show resolved
Hide resolved
…wmRsp.rst Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Member
|
Great. The suggestion I proposed was meant to be checked with the online docs but RTD is currently failing for all other PRs. Have you tried rendering Can you check it locally & show me a screenshot comparing the two? Thanks in advance. |
Member
|
Nevermind, we use the long form anyway. No need to change: $ git grep -P ':rfc:`.+ <\d{4}#section.+>' Doc/
Doc/howto/logging-cookbook.rst::rfc:`relevant section of the specification <5424#section-6>`.)
Doc/library/codecs.rst:based on the separator characters defined in :rfc:`section 3.1 of RFC 3490 <3490#section-3.1>`
Doc/library/http.client.rst: an absolute path to conform with :rfc:`RFC 2616 §5.1.2 <2616#section-5.1.2>`
Doc/library/http.client.rst: with the request. A :rfc:`Host header <2616#section-14.23>`
Doc/library/http.client.rst: must be provided to conform with :rfc:`RFC 2616 §5.1.2 <2616#section-5.1.2>`
Doc/library/uuid.rst: :rfc:`RFC 9562, §5.8 <9562#section-5.8>`.
Doc/library/xmlrpc.client.rst: :rfc:`RFC 2045 section 6.8 <2045#section-6.8 |
picnixz
approved these changes
Jan 6, 2025
Member
picnixz
left a comment
picnixz
left a comment
There was a problem hiding this comment.
Once RTD is back online, we can merge this one I think.
Contributor
Author
|
@picnixz RTD passed as well. We can merge 🚀 🚀 |
Member
|
@bitdancer Is there anything else you want the OP to address or is the PR good for you? |
picnixz
reviewed
Feb 24, 2025
Member
picnixz
left a comment
picnixz
left a comment
There was a problem hiding this comment.
Friendly ping @bitdancer
I still don't know whether to treat it as a bug or a feature. I would say a bugfix because before it didn't work at all. However, we are now raising an exception; yet the code that previously didn't work also didn't raise (at least, according to the issue description). We can say that it's an example of "garbage in -> garbage out" (even though the garbage out is not the expected garbage out). Now that we raise, existing (but faulty) code could actually be raising exceptions which could not be handled by an application. And this can be considered a breaking change.
Therefore, should we backport this? 3.12 is nearing it's EOL in terms of bugfix so maybe it's a bit late to backport to it? I don't really know the policy in this case. cc @encukou @serhiy-storchaka @vstinner as they know better what to do in this situation.
bitdancer
requested changes
Mar 3, 2025
Member
bitdancer
left a comment
bitdancer
left a comment
There was a problem hiding this comment.
Sorry it took me so long to get back to this :(
Misc/NEWS.d/next/Library/2024-12-11-17-44-36.gh-issue-127794.VwmRsp.rst
Outdated
Show resolved
Hide resolved
Member
|
Oh, and the bug question: I think it is technically a "maybe breaking change" bugfix, and therefor should not be backported. Code that "worked" before won't be improved by this fix, so better to leave any (unlikely!) possible breakage to a new release only. |
picnixz
reviewed
Mar 30, 2025
picnixz
reviewed
Mar 30, 2025
Misc/NEWS.d/next/Library/2024-12-11-17-44-36.gh-issue-127794.VwmRsp.rst
Outdated
Show resolved
Hide resolved
picnixz
reviewed
Mar 30, 2025
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.