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

Comments

Projects
None yet
6 participants
@nathj07
Copy link

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

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

/cc @bradfitz

@fraenkel

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

@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

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

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

@nathj07

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

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 May 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.