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

net/url: url.Values.Encode uses = for empty value in key=value #30025

Closed
nathj07 opened this issue Jan 30, 2019 · 9 comments
Closed

net/url: url.Values.Encode uses = for empty value in key=value #30025

nathj07 opened this issue Jan 30, 2019 · 9 comments
Labels
FeatureRequest FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@nathj07
Copy link

nathj07 commented Jan 30, 2019

What version of Go are you using (go version)?

$ go version
go version go1.10.3 darwin/amd64

Also

go version go1.10.3 linux/amd64

Does this issue reproduce with the latest release?

I haven't tried the latest release directly I have tried on the playground though: https://play.golang.org/p/LSBliQAmXLc

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/ndavies/Library/Caches/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/ndavies/Projects/go"
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/nf/fhhgkcvd737ggxqm3_f4tlb0nst21y/T/go-build800209626=/tmp/go-build -gno-record-gcc-switches -fno-common"

And..

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/ndavies/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/ndavies/dev/go"
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build955487558=/tmp/go-build -gno-record-gcc-switches"

What did you do?

https://play.golang.org/p/LSBliQAmXLc

What did you expect to see?

I would expect the URL to not have the trailing =

What did you see instead?

There is a trailing =

This of course may be down to the owner of the site in question (anonymised in the playground) not having a fully valid URL - a query string key with no value. Though from my reading of https://tools.ietf.org/html/rfc3986#section-3.4 that seems valid if a little unconventional

@agnivade
Copy link
Contributor

/cc @bradfitz

@fraenkel
Copy link
Contributor

@nathj07 What you have is valid. Its up to the server side to decide how to decode the query component.
If you want the query component to represent a string or commonly a URI, use RawQuery. If you want key/value pairs, then use Values to help you manage it.
Trying to make Values produce a plain string isn't its documented purpose.

@nathj07
Copy link
Author

nathj07 commented Jan 31, 2019

Ok I see.

In our use case we use Query() to get the map of key value pairs and then as we iterate over that we normalize it and then resultant map is applied as using Encode() it is this final Encode() call on the map that adds the trailing equals:

if rawURL.RawQuery != "" {
		purge := parseURLPurgeMap
		params := rawURL.Query()
		for k := range params {
			if purge[strings.ToLower(k)] {
				delete(params, k)
			}
		}
		rawURL.RawQuery = params.Encode()
	}

So in the case of a URL such as "http://example.com/path/to/page?queryalue" thee result is "http://example.com/path/to/page?queryalue=".

It seems that issue is in the Encode() step: https://golang.org/src/net/url/url.go?s=25313:25344#L879 this looks to me like it applies the = regardless of a value.

@bradfitz bradfitz changed the title net/url parsing leaves a trailing = on the query params when there is only a single key net/url: parsing leaves a trailing = on the query params when there is only a single key Jan 31, 2019
@bradfitz
Copy link
Contributor

Related:

I supposed we could add another bool for this purpose. I forget what these URLs are called. They used to be generated from <isindex> page forms, IIRC? But that's long deprecated/dead. How did you generate them? Was this from a browser or what was the client?

@bradfitz bradfitz added this to the Go1.13 milestone Jan 31, 2019
@bradfitz bradfitz added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. FeatureRequest labels Jan 31, 2019
@nathj07
Copy link
Author

nathj07 commented Jan 31, 2019

@bradfitz this was discovered within our web crawler.

So the url belongs to another site and was originally generated there. Our crawler discovered it in its original form. The act of parsing and normalizing within our system results in the trailing '='

@fraenkel
Copy link
Contributor

fraenkel commented Feb 1, 2019

@bradfitz Values assumes you always have a value associated with every key. Values also allow you to have multiple keys, yet that would seem invalid for this case.
A bool could live on the Values to enforce the single key as the only allowable item. Query/ParseQuery could detect the lack of the = and set the bool.

Of course, this would be a breaking change unless there is something else you were thinking of.

@bradfitz
Copy link
Contributor

bradfitz commented Feb 1, 2019

Yeah, that'd only work for the case where it's purely the old-style <isindex> query style without any = or &.

I think this should just be put on hold until the package/type is redesigned.

At least the URL itself round-trips: https://play.golang.org/p/mlPt1b2Q6J7 ... It's only the url.Values map that doesn't.

@Asuan
Copy link
Contributor

Asuan commented Apr 26, 2019

Looks like bug is from func (v Values) Encode() string, @nathj07 use it for building URL with params.
https://github.com/golang/go/blob/master/src/net/url/url.go#L925-L927

			buf.WriteString(keyEscaped)
			buf.WriteByte('=') // <<-- need add only in case non empty v
			buf.WriteString(QueryEscape(v))

But the function is used for building POST form and GET query.
It is correct for POST form (have buz=&bah=foo but not so good for HTTP url query

@rsc rsc changed the title net/url: parsing leaves a trailing = on the query params when there is only a single key net/url: url.Values.Encode uses = for empty value in key=value May 1, 2019
@rsc
Copy link
Contributor

rsc commented May 1, 2019

The playground example is setting

u.RawQuery = params.Encode()

If you don't want a trailing =, you can remove it before assigning to u.RawQuery. Part of the reason for having the RawQuery field at all is so that users have complete control over the exact spelling of the query string.

The objection might be that params.Encode chooses to always use an = for empty values, but I don't believe that's something we would be wise to change at this point. It's easy to call a different function that behaves the way you want - package net/url is not forcing the use of this particular encoder. I don't think there's much we can do here (short of adding a second encoder entry point, which seems a bit overkill).

@rsc rsc closed this as completed May 1, 2019
@golang golang locked and limited conversation to collaborators Apr 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FeatureRequest FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

7 participants