-
-
Notifications
You must be signed in to change notification settings - Fork 167
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
Fix [] was being escaped in query key position #445
Conversation
Codecov Report
@@ Coverage Diff @@
## master #445 +/- ##
=======================================
Coverage 99.24% 99.25%
=======================================
Files 2 2
Lines 664 667 +3
Branches 152 152
=======================================
+ Hits 659 662 +3
Misses 5 5
Continue to review full report at Codecov.
|
No idea if the version will be 1.4.3, I used that because the last released one according to https://github.com/aio-libs/yarl/blob/master/CHANGES.rst (as of 2311d02) is 1.4.2. Perhaps it deserves a minor bump even though it's a bug-fix patch because it could potentially disrupt users? |
Another bump for the weekend? (Forgot to do it yesterday.) |
The ``'[]'`` characters are not escaped in the keys of the query | ||
because some endpoints expect those to be present when specifying arrays. |
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.
Do you mind also referencing an RFC document that specifies this?
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.
I do not have any RFC on this, I only wrote this based on what you said in #443 (comment).
web-browsers use
varname[]
format for arrays
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.
So I've done some research and it turns out I was wrong. The square brackets are supposed to be encoded:
PHP hack uses them to turn input values into arrays but the client-side still encodes them.
I've checked it in Chrome and verified that it does encode []
. That being said, raw [
/]
are only allowed as a part of the host section to represent IPv6 addresses.
Okay, thanks for the investigation. So there's nothing for me to fix in #443 with regards to this and |
What do these changes do?
These changes fix a bug where
[]
was being escaped when used in the key of queries, namely:Are there changes in behavior for the user?
Yes, if users were relying on unintended, buggy behaviour (
[]
being escaped) this change may cause issues in their code.Related issue number
None as far as I know.
Checklist
CHANGES
folder<issue_id>.<type>
(e.g.588.bugfix
)issue_id
change it to the pr id after creating the PR.feature
: Signifying a new feature..bugfix
: Signifying a bug fix..doc
: Signifying a documentation improvement..removal
: Signifying a deprecation or removal of public API..misc
: A ticket has been closed, but it is not of interest to users.Fix issue with non-ascii contents in doctest text files.