[go] net/url: normalize hex values before comparision
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded this change for review.
net/url: normalize hex values before comparision
"%3f" and "%3F" are identical strings. But currently, setPath() treats them
as different strings because of the lowercase f character in "%3f", causing
RawPath to be set.
This patch fixes this bug by normalizing the hexadecimal values of the two
strings being compared. Since `escp` is generated by escape(), all of its
hex values are uppercase. So, this patch only acts upon the `p` string,
which might contain lowercase hex values (since `p` is supplied by the user)
and converts all of the lowercase hex values to uppercase.
Fixes #33596
Change-Id: If01ac5025de340250f20c68651822641cdc2042c
GitHub-Last-Rev: d118eeb88d50af9bcf960f4c806a5b4d677d7bea
GitHub-Pull-Request: golang/go#53848
---
M src/net/url/url.go
M src/net/url/url_test.go
2 files changed, 58 insertions(+), 1 deletion(-)
diff --git a/src/net/url/url.go b/src/net/url/url.go
index e82ae6a..19f7917 100644
--- a/src/net/url/url.go
+++ b/src/net/url/url.go
@@ -673,7 +673,30 @@
return err
}
u.Path = path
- if escp := escape(path, encodePath); p == escp {
+ escp := escape(path, encodePath)
+ // p may contain lowercase hexadecimal letters.
+ // So, before comparing p with escp, we need to convert all lowercase
+ // hexadecimal digits to uppercase (since escp has uppercase hex)
+ var p_upper strings.Builder
+ p_upper.Grow(len(p))
+ for i := 0; i < len(p); i++ {
+ p_upper.WriteByte(p[i])
+ if p[i] != '%' {
+ continue
+ }
+ if p[i+1] > 'a' {
+ p_upper.WriteByte(p[i+1] + 'A' - 'a')
+ } else {
+ p_upper.WriteByte(p[i+1])
+ }
+ if p[i+2] > 'a' {
+ p_upper.WriteByte(p[i+2] + 'A' - 'a')
+ } else {
+ p_upper.WriteByte(p[i+2])
+ }
+ i += 2
+ }
+ if p_upper.String() == escp {
// Default encoding is fine.
u.RawPath = ""
} else {
diff --git a/src/net/url/url_test.go b/src/net/url/url_test.go
index 263eddf..ce8f05b 100644
--- a/src/net/url/url_test.go
+++ b/src/net/url/url_test.go
@@ -54,6 +54,17 @@
},
"",
},
+ // path with lowercase hex escaping (golang.org/issue/33596)
+ {
+ "http://www.google.com/file%3Fone%3ftwo",
+ &URL{
+ Scheme: "http",
+ Host: "www.google.com",
+ Path: "/file?one?two",
+ RawPath: "",
+ },
+ "",
+ },
// fragment with hex escaping
{
"http://www.google.com/#file%20one%26two",
To view, visit change 417395. To unsubscribe, or for help writing mail filters, visit settings.
Gopher Robot (Gerrit)
Congratulations on opening your first change. Thank you for your contribution!
Next steps:
A maintainer will review your change and provide feedback. See
https://go.dev/doc/contribute#review for more info and tips to get your
patch through code review.
Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.
During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11 or adds a tag like "wait-release", it means that this CL will be
reviewed as part of the next development cycle. See https://go.dev/s/release
for more details.
Ingo Oeser (Gerrit)
Attention is currently required from: Damien Neil, Russ Cox.
1 comment:
Patchset:
Could you implement it lazily? e.g. only convert after you did hit the first lower case hex value?
Could you adapt the comparison to compare hex values case insensitive instead?
To view, visit change 417395. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Damien Neil, Russ Cox.
Gerrit Bot uploaded patch set #2 to this change.
net/url: normalize hex values before comparision
"%3f" and "%3F" are identical strings. But currently, setPath() treats them
as different strings because of the lowercase f character in "%3f", causing
RawPath to be set.
This patch fixes this bug by normalizing the hexadecimal values of the two
strings being compared. Since `escp` is generated by escape(), all of its
hex values are uppercase. So, this patch only acts upon the `p` string,
which might contain lowercase hex values (since `p` is supplied by the user)
and converts all of the lowercase hex values to uppercase.
Fixes #33596
Change-Id: If01ac5025de340250f20c68651822641cdc2042c
GitHub-Last-Rev: 9728bc0ad734368b559de875509eeea26a48108a
GitHub-Pull-Request: golang/go#53848
---
M src/net/url/url.go
M src/net/url/url_test.go
2 files changed, 77 insertions(+), 8 deletions(-)
To view, visit change 417395. To unsubscribe, or for help writing mail filters, visit settings.
Subhaditya Nath (Gerrit)
Attention is currently required from: Damien Neil, Ingo Oeser, Russ Cox.
1 comment:
Patchset:
Could you adapt the comparison to compare hex values case insensitive instead?
Implemented in patchset 2
Could you implement it lazily? e.g. only convert after you did hit the first lower case hex value?
Patchset 2 doesn't convert the string anymore.
Still, I've tried to implement the logic "lazily". i.e. The byte-by-byte comparison only gets triggered if `==` fails AND the string contains the `%` character AND both the strings are of equal length.I've done so because I assumed that `len()`, `strings.IndexByte()` and `==` are faster than manually iterating over the string.
Am I right?
To view, visit change 417395. To unsubscribe, or for help writing mail filters, visit settings.
Subhaditya Nath (Gerrit)
Attention is currently required from: Damien Neil, Russ Cox.
1 comment:
Patchset:
This patchset contains a typo. These conditionals -
if p[i+1] > 'a'
if p[i+2] > 'a'
should have been -if p[i+1] >= 'a'
if p[i+2] >= 'a'
Please don't merge.
To view, visit change 417395. To unsubscribe, or for help writing mail filters, visit settings.
Subhaditya Nath (Gerrit)
Patchset:
This patchset contains a typo. These conditionals - […]
Fixed in Patchset 2
To view, visit change 417395. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Ingo Oeser, Russ Cox.
Patch set 2:Hold +1
1 comment:
Patchset:
The current handling of RawPath is working as intended; see my comment here:
https://github.com/golang/go/issues/33596#issuecomment-1185896838
To view, visit change 417395. To unsubscribe, or for help writing mail filters, visit settings.
Subhaditya Nath (Gerrit)
Attention is currently required from: Damien Neil, Ingo Oeser, Russ Cox.
1 comment:
Patchset:
The current handling of RawPath is working as intended; see my comment here: […]
I don't think so, because according to [RFC3986 §2.1](https://datatracker.ietf.org/doc/html/rfc3986#section-2.1), `"%3F"` and `"%3f"` are equivalent.
https://github.com/golang/go/issues/33596#issuecomment-1186071106
To view, visit change 417395. To unsubscribe, or for help writing mail filters, visit settings.
Subhaditya Nath (Gerrit)
Attention is currently required from: Damien Neil, Ingo Oeser.
1 comment:
Patchset:
I don't think so, because according to [RFC3986 §2.1](https://datatracker.ietf. […]
.
To view, visit change 417395. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Damien Neil, Ingo Oeser, Russ Cox.
Gerrit Bot uploaded patch set #3 to this change.
net/url: normalize hex values before comparision
"%3f" and "%3F" are identical strings. But currently, setPath() treats them
as different strings because of the lowercase f character in "%3f", causing
RawPath to be set.
This patch fixes this bug by normalizing the hexadecimal values of the two
strings being compared. Since `escp` is generated by escape(), all of its
hex values are uppercase. So, this patch only acts upon the `p` string,
which might contain lowercase hex values (since `p` is supplied by the user)
and converts all of the lowercase hex values to uppercase.
Fixes #33596
Change-Id: If01ac5025de340250f20c68651822641cdc2042c
GitHub-Last-Rev: 43ef9143e2190f83e9b21f03bec274c89a95733e
GitHub-Pull-Request: golang/go#53848
---
M src/net/url/url.go
M src/net/url/url_test.go
2 files changed, 54 insertions(+), 8 deletions(-)
To view, visit change 417395. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Ingo Oeser, Russ Cox, Subhaditya Nath.
Patch set 3:Hold +1
1 comment:
Patchset:
.
Please see my comments in https://go.dev/issue/33596. The current behavior is working as intended.
Further discussion to #33596.
To view, visit change 417395. To unsubscribe, or for help writing mail filters, visit settings.
Gopher Robot (Gerrit)
Gopher Robot abandoned this change![]( "Open in Gerrit")
Related details
Code-Review
No-Holds
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |