proposal: net/url: provide AllowQuerySemicolons functionality for net/url rather than net/http

[chrisguiney]

#25192 was a breaking change and #45973 sought to alleviate that change with AllowQuerySemicolons. Implementing it as an http handler however ignores any other use case for parsing urls.

My understanding is that anything parsing doubleclick urls, which explicitly use semicolons, would still break.

https://support.google.com/richmedia/answer/117426?hl=en

It would be great if we could have some mechanism to allow for backward compatibility in net/url rather than net/http, so that all use cases can be accommodated.

[ianlancetaylor]

CC @FiloSottile

[FiloSottile]

Not saying that we necessarily shouldn’t do this, but it’s worth noting that if you have a URL, getting the original behavior back is a single call to strings.Replace(u, “;”, “&”, -1).

[chrisguiney]

One suggestion might be viable would be to simply return an error, such as url.ErrLegacyQuerySep or something to denote that it was parsed correctly, but contained semicolons. This would likely make all existing clients ignore the returned value, and bubble up the error.

Another option would be to make a function that takes a set of valid separators to use.

[gopherbot]

Change https://golang.org/cl/338750 mentions this issue: doc/go1.17: suggest method of restoring prior behavior of url.Parse

[neild]

Using strings.Replace seems simpler than adding additional API surface to url.Parse. Sent a CL to add this suggestion to the release notes, since it isn't immediately obvious that it's a valid workaround.

[rsc]

Given updated docs, it doesn't seem like we need to put this API in.

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[rsc]

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

[rsc]

No change in consensus, so declined.
— rsc for the proposal review group

[derekperkins]

It's obviously too late now, but this is incredibly frustrating. I honestly don't understand how this is somehow exempt from the backwards compatibility promise, especially without being able to opt back into the original behavior. There are plenty of documented incorrect behaviors in the stdlib that aren't being corrected because of the promise. We deal with large quantities of urls, and as mentioned, Google explicitly uses semi-colons, which is a non-trivial part of the internet.

Anyhow, rant over, time to comb through our entire codebase looking for the right places to inject strings.Replace(u, “;”, “&”, -1)

[ianlancetaylor]

@derekperkins Sorry for the difficulties. As discussed at #25192, this change was required to avoid security problems, even though it broke backward compatibility. Although they are unfortunate and uncommon, security issues are one of the cases where the compatibility guarantee (https://golang.org/doc/go1compat) permits exceptions.

[derekperkins]

I totally understand for security problems, and obviously I have to defer to @FiloSottile as the expert, but when the outcome of the discussion was "We deemed it not a backportable security fix (also because of the potential for disruption and the limited real-world impact), but it's conceivable that it could have security value for some applications," that doesn't feel like a strong enough reason to break backwards compatibility.

That being said, I appreciate your response and I get that things like this come with the package. It speaks volumes about the compatibility promise that this is the first time I've encountered a problem since starting with 1.1. I honestly spent nearly a full day debugging every other option and dependency, even shooting down someone else suggesting the stdlib, because I didn't think that was possible. Thanks to everyone involved for providing such a stable platform.

[FiloSottile]

@derekperkins My apologies too for the breakage. This was not a clear-cut decision and reasonable people will disagree, but we had to weight the security cost of being out of alignment with the specs and with developer expectations against the breakage cost. The security risk of off-spec and unexpected behavior increases slowly over time, as most of the ecosystem converges on something different from what we do. On the other hand, the best tool we have to reduce disruption of a breaking change is to put it in a major release. These two factors explain why we invoked the security exception to the backwards compatibility promise without putting the change in a patch release. You are correct there are many documented incorrect behaviors in the stdlib (as is unavoidable given its backwards compatibility promise), but what makes this different is that it's in a place where parser alignment with third party systems is a security requirement. Systems that require parser alignment are fragile, but alas fairly common. If there was any way we could have reached you with the change notes before you spent a lot of time looking for the issue elsewhere, I would be interested to hear suggestions. What we did was log a line to ErrorLog and put a subheading in the Go 1.17 release notes, I am sure there are other channels we could cover too.

[chrisguiney]

@FiloSottile thanks for following up on this. Understood that it's a very unfortunate situation all around. For my part, the frustration stems from providing backwards compatibility at the handler level and not the parser level. Given enough time, some amount of backwards incompatibility is inevitable, so it's not the end of the world, just frustrating.

If there was any way we could have reached you with the change notes before you spent a lot of time looking for the issue elsewhere, I would be interested to hear suggestions.

My lament is that I wasn't aware of the issue until after #45973 was merged, so I didn't have an opportunity to express alternatives in time. I'm not someone that really watches the go issues like a hawk (that's a good thing!), but I do tend to lurk in at least some go spaces to keep up to date on happenings. Perhaps when the security clause is being invoked for a compatibility break, there could be an effort to get the word out to different community spaces so people can provide that feedback before action is taken? Obviously it'd have to be a best-effort approach, and I totally respect that it'd be that much more time out of what I'm sure are very busy schedules -- I'm just throwing the thought out there.

I'll repeat the thanks already expressed in this thread -- thank you for all of your work ensuring that go remains stable and secure by default. It very much is appreciated :)