Hi Tim,
> 1. I don't think `QueryParamParsingOptions::$maxQueryStringLength` is >> necessary or useful to have. Checking the input length is easy to do >> manually (it's just a `strlen()` call) and I don't see a practical way >> to abuse the API with large input lengths alone: The API will not “blow >> up” inputs into considerably larger data structures, the input strings >> are mostly just “copied” over, not too different from e.g. >> `str_replace()`. >> > > I was also thinking a lot about whether this option is necessary to have. > My main reason for including it was that people would otherwise easily > forget > to limit the input length, whereas the dedicated option enforces the limit > everywhere. > > So the real question is whether QueryParams really cannot be abused via > long strings, and whether $maxParamCount alone could mitigate those risks > ? > > And now by thinking about it more, neither I could identify any specific > vulnerability that > would become possible by not enforcing the upper limit. In the previous > iteration > of the RFC, limiting the length was more useful, because parsing would > have been > a more resource intensive operation due to the requirements of the old > "array API". > The operations that are performed now during parsing are cheap, so as far > as I can see, > it's possible to get rid of this option if no one can come up with a good > enough use-case. > Just after sending my previous reply, I realized that even if parsing long query strings isn't an expensive operation on its own, the rest of the operations supported by QueryParams may be. For example, sorting by long keys can cause significant slowdown. (To be fair: I haven't done any measurements yet). And in the light of the recent CVEs, increased memory consumption can cause troubles, so IMO an automatically enforced maximum length can still be useful. > Regarding accepting malformed input: uriparser's own query parameter > handling > functions (uriDissectQueryMalloc*()) behave the same way as the RFC currently > proposes. > Unfortunately, I could not really find any reasoning behind this choice of > uriparser. > > To be clear, I don't insist on following uriparser's behavior for RFC 1866 > and RFC 3986 (it's > not an option for WHATWG URL, since it specifies what needs to be done in > every detail), > and probably strict parsing is what is more consistent with the Uri class > indeed. > When implementing strict parsing for the RFC-based parsers, I realized that migration from the $_GET usage (via Uri\QueryParams::parseRfc1866($_SERVER["QUERY_STRING"] ?? "")) would be more difficult, because parseRfc1866() would start rejecting malformed input that PHP's existing query-string parser (php_default_treat_data()) currently accepts. I'm not really sure how big an issue this would be in practice, but at least it's worth pointing out. > One more thing which I'm probably going to change: it turned out that > WHATWG URL specifies > the sort() method (https://url.spec.whatwg.org/#dom-urlsearchparams-sort) > which must use UTF-16 based > comparisons (using 2 bytes). It is really not obvious from the WHATWG URL, > only the WHATWG Infra > specification defines it: > https://infra.spec.whatwg.org/#code-unit-less-than > > In order to restore compatibility with this requirement, I'd define two > sorting algorithms: > - sortByBytes(): the currently specified one which would use bytes as the > base of comparison > - sortByUtf16(): the one comparing UTF-16 sequences at once which would be > compatible with WHATWG URL > I've now added this to the RFC and also started to implement it (only locally). Máté
