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

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

Closed
chrisguiney opened this issue Jul 27, 2021 · 14 comments

Comments

@chrisguiney
Copy link
Contributor

#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.

@gopherbot gopherbot added this to the Proposal milestone Jul 27, 2021
@ianlancetaylor
Copy link
Contributor

CC @FiloSottile

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jul 27, 2021
@FiloSottile
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link

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

@neild
Copy link
Contributor

neild commented Jul 30, 2021

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
Copy link
Contributor

rsc commented Aug 11, 2021

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

@rsc
Copy link
Contributor

rsc commented Aug 11, 2021

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 rsc moved this from Incoming to Active in Proposals (old) Aug 11, 2021
@rsc
Copy link
Contributor

rsc commented Aug 18, 2021

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

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Aug 18, 2021
@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Aug 25, 2021
@rsc
Copy link
Contributor

rsc commented Aug 25, 2021

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

@derekperkins
Copy link

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
Copy link
Contributor

@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
Copy link

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
Copy link
Contributor

@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
Copy link
Contributor Author

@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 :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

8 participants
@neild @rsc @chrisguiney @FiloSottile @ianlancetaylor @derekperkins @gopherbot and others