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

Consider disabling ;-separated query parameters by default #100

Open
samth opened this issue Mar 23, 2021 · 4 comments
Open

Consider disabling ;-separated query parameters by default #100

samth opened this issue Mar 23, 2021 · 4 comments

Comments

@samth
Copy link
Sponsor Member

samth commented Mar 23, 2021

Recently, people have pointed out that the combination of common caching proxies and web frameworks that treat ; as a query separator can lead to security problems; see here: https://snyk.io/blog/cache-poisoning-in-popular-open-source-packages/

This led Python to change the default behavior: https://bugs.python.org/issue42967

See also this article: https://lwn.net/Articles/846847/

We have a few choices here:

  1. Do nothing (probably a bad idea)
  2. Enable users of the web server to disable ; as a separator.
  3. Like 2, but make that the default. (This is what Python did)
@jeapostrophe
Copy link
Contributor

If I understand correctly, Racket already does this in via current-alist-separator-mode

https://github.com/racket/racket/blob/master/racket/collects/net/uri-codec.rkt#L249-L255

As far as the Web server goes, I believe you can just change that parameter and all is well. So are you asking to change the default?

Now, alternatively, you might be talking about the fact that ; is a reserved character in URI path segments called a "parameter" (https://tools.ietf.org/html/rfc3986#section-3.3), which is why the net/url has the path/param structure. The Web server uses that to embed the continuation ids in parallel to any query parameters. Changing this would be very invasive, because there's no other channel we could use that wouldn't interfere with the user's program...

@samdphillips
Copy link

samdphillips commented Nov 4, 2021

It would seem to me that setting the default for (current-alist-separator-mode) to 'amp would be the most sensible approach and similar to the choice the python devs made.

@samth
Copy link
Sponsor Member Author

samth commented Nov 4, 2021

Yes, that's what I was thinking.

@LiberalArtist
Copy link
Contributor

Substantively, this seems like a good change to me.

Changing the default for current-alist-separator-mode would, strictly speaking, be a backwards incompatible change. That may be the right choice for all clients of net/uri-codec. (I don't think I've ever seen a url in the wild that uses ; to separate query parameters.) But we do have the option of handling this within the web server via the "safety limits" construct: we could add an #:alist-separator-mode argument defaulting to 'amp for make-safety-limits, but to 'amp-or-semi for make-unlimited-safety-limits, and then parameterize accordingly when reading requests.

My recollection from when we made a breaking change for the sake of security and added the "safety limits" construct in the process is that it would have been good to have had broader discussion first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants