-
-
Notifications
You must be signed in to change notification settings - Fork 273
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
Update query parser options #5280
Conversation
@@ -45,12 +45,12 @@ export class RestApiServer { | |||
ajv: {customOptions: {coerceTypes: "array"}}, | |||
querystringParser: (str) => | |||
qs.parse(str, { | |||
// defaults to 20 but Beacon API spec allows max items of 30 | |||
arrayLimit: 30, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed arrayLimit
as it does not acutally work as expected, see ljharb/qs#294, in fact, completely removed array parsing (values like id[0]=1&id[1]=2&id[2]=3
) to make the parsing more predictable. This kind of array query strings were also not supported before updating the query parser in #5268, so I would assume nobody relies on that functionality at the moment.
comma: true, | ||
// default limit of 1000 seems unnecessarily high, let's reduce it a bit | ||
parameterLimit: 100, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parameter limit silently cuts off exceeding values which is not ideal, using the default value of 1000
here should be fine and is only relevant if someone sends array queries like this id=3&id=4&id=5
.
The parameter limit is anyhow not what should be used to limit DoS vectors, instead we should rely on maximum header size of the http server which is 16KB.
Some downstream tooling such as rocketpool might require this limit to be increased as they send requests like this eth/v1/beacon/states/head/validators?id=<1000 validator pubkeys, comma-separated>
which requires much higher max header size.
For now, this configurable using the --max-http-header-size
node cli option, see http.maxHeaderSize
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lodestar now also has a CLI flag --rest.headerLimit
to configure this
Performance Report✔️ no performance regression detected Full benchmark results
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
🎉 This PR is included in v1.7.0 🎉 |
Motivation
Follow up PR to #5268. Updates the parser options to make array query string parser more consistent/predictable.
Description
Update query parser options, drop support for array query strings like
id[0]=1&id[1]=2&id[2]=3
as those are not required to be OpenAPI spec compliant and results are inconsistent, see ljharb/qs#331. The schema validation will catch this and throw an error as parsed query string results in an object.Parsing of different formats
Here are just a few examples how array query strings will be parsed, most example query strings are from OpenAPI Parameter Serialization