Default payload validation #48753
Default payload validation #48753
Conversation
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
|
retest |
💚 Build Succeeded |
💚 Build Succeeded |
💔 Build Failed |
💔 Build Failed |
💚 Build Succeeded |
legrego
changed the title
legrego
added
release_note:skip
| // TODO: This 'validate' section can be removed once the legacy platform is completely removed. | ||
| // We are telling Hapi that NP routes can accept any payload, so that it can bypass the default | ||
| // validation applied in ./http_tools#getServerOptions | ||
| // (All NP routes are already required to specify their own validation in order to access the payload) |
There was a problem hiding this comment.
not sure that we can remove it. NP plugins might disable validation https://github.com/restrry/kibana/blob/841abd1162f1eb6aaebf27d004c92e8344636d91/src/core/server/http/router/route.ts#L78-L80
There was a problem hiding this comment.
Oh thanks for the heads-up! Are we sure we want to allow this behavior? Do we have concrete use cases for disabling the config-schema validation at this time?
There was a problem hiding this comment.
There are some cases when we don't know the shape of an object upfront.
https://github.com/restrry/kibana/blob/841abd1162f1eb6aaebf27d004c92e8344636d91/x-pack/legacy/plugins/security/server/routes/api/v1/authenticate.js#L204
https://github.com/elastic/kibana/pull/48413/files#diff-fbd7aa2d4390350fcd1ffd047c33af25
There was a problem hiding this comment.
There are some cases when we don't know the shape of an object upfront.
That's a fair point -- we do know that payloads will at least be objects or arrays though, right? Could we at least enforce something like schema.object({}, { allowUnknowns: true }) at route handlers?
We are proposing a followup PR to this one which will incorporate these default validations into the schema.object call itself, so that all NP routes will have these protections in place, unless they explicitly opt-out of them via schema.unsafeObject({}) or similar.
There was a problem hiding this comment.
Could we at least enforce something like schema.object({}, { allowUnknowns: true }) at route handlers?
We already enforce. Don't we? Plugins cannot get access to a body without declaring validation
https://github.com/elastic/kibana/blob/master/src/core/server/http/router/request.ts#L106-L109
Although they can lax the validation
schema.any()
schema.object({}, { allowUnknowns: true })There was a problem hiding this comment.
Ah sorry I misunderstood. I thought NP provided a way to prevent any validation (not even schema.any(), and still get access to the underlying payload. I think we're OK as-is then. Our followup PR should protect these routes as well.
| // This is a default payload validation which applies to all LP routes which do not specify their own | ||
| // `validate.payload` handler, in order to reduce the likelyhood of prototype pollution vulnerabilities. | ||
| // (All NP routes are already required to specify their own validation in order to access the payload) | ||
| payload: value => Promise.resolve(validateObject(value)), |
There was a problem hiding this comment.
what about query validation?
There was a problem hiding this comment.
We are mostly concerned with payload validation, as that's the most common offender for nested data structures. We may add similar validations for query, path, and header in the future
|
@elasticmachine merge upstream |
💔 Build Failed |
|
@elasticmachine merge upstream |
💔 Build Failed |
|
@elasticmachine merge upstream |
💚 Build Succeeded |
|
ACK: reviewing |
| { constructor: { foo: { prototype: null } } }, | ||
| { prototype: { foo: { constructor: null } } }, | ||
| ].forEach(value => { | ||
| ['headers', 'payload', 'query', 'params'].forEach(property => { |
There was a problem hiding this comment.
nit: Ensuring headers, payload, query and params are being checked isn't really relevant anymore. This is a remnant from when we were validating the entire request itself, as opposed to explicitly just doing payload validation.
…e-validation-audit
…kibana into security/route-validation-audit
💚 Build Succeeded |
|
@elasticmachine merge upstream |
💚 Build Succeeded |
* trial for default payload validation * relaxing default validation * some cleanup and testing * update xsrf integration test * adding API smoke tests * fixing types * removing Joi extensions * updating tests * documenting changes * fixing NP validation bypass * fix lint problems * Update src/legacy/server/http/integration_tests/xsrf.test.js * Update src/legacy/server/http/integration_tests/xsrf.test.js * revert test changes * simplifying tests Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
* trial for default payload validation * relaxing default validation * some cleanup and testing * update xsrf integration test * adding API smoke tests * fixing types * removing Joi extensions * updating tests * documenting changes * fixing NP validation bypass * fix lint problems * Update src/legacy/server/http/integration_tests/xsrf.test.js * Update src/legacy/server/http/integration_tests/xsrf.test.js * revert test changes * simplifying tests Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
| const [key, childValue] = entries[i]; | ||
| if (isObject(childValue)) { | ||
| if (seen.has(childValue)) { | ||
| throw new Error('circular reference detected'); |
There was a problem hiding this comment.
@legrego why did you add a throw here instead of simply a continue statement? Is it because the presence of a circular reference is an indication of a programming error during development?
This adds a default payload validation for all Legacy Platform routes which do not define their own
validate: payloadschema.Since New Platform routes handle and enforce their own validation, they are opted-out of this default behavior.