feat: encoding > allowReserved support #630
Conversation
karol-maciaszek
changed the title
karol-maciaszek
changed the title
karol-maciaszek
requested review from
XVincentX,
lag-of-death and
mallachari
| for (const encoding of encodings) { | ||
| const allowReserved = get(encoding, 'allowReserved', false); | ||
| const property = encoding.property; | ||
| const value = shallowParsedTarget[property]; |
There was a problem hiding this comment.
What does shallow specifically mean here?
There was a problem hiding this comment.
I had trouble naming that.
Basically it should mean that URL is split by & and the key-val pairs are extracted. But percent-encoded entities hadn't been decoded yet.
We need it in that form because:
- we need key-val pairs in order to match key names with relevant encoding definitions
- we need before-percent-decoding values in order to look for forbidden characters
There was a problem hiding this comment.
Path and queryParams?
There was a problem hiding this comment.
Quite unsure what do you mean by path and queryParams. We are dealing here with body payload.
test-harness/specs/validate-body-params/form-data-allow-reserved-fail.oas3.txt
Show resolved
Hide resolved
| @@ -27,7 +27,7 @@ paths: | |||
| ====server==== | |||
| mock -p 4010 | |||
| ====command==== | |||
| curl -i -X POST http://localhost:4010/path -H "Content-Type: application/x-www-form-urlencoded" --data "status=ooopsie!" | |||
| curl -i -X POST http://localhost:4010/path -H "Content-Type: application/x-www-form-urlencoded" --data "status=ooopsie%21" | |||
There was a problem hiding this comment.
Just to make sure I understand this correctly — since ! is a reserved character and allowReserved is false by default, you have effectively encoded it so that it passes the validation and goes through the following validation step, right?
There was a problem hiding this comment.
Exactly. And this is the place where I doubt in OpenApi 3 spec. I mean having allowReserved=false by default sounds reasonable. However, I think it will end with lots of people complaining that prism has a bug.
There was a problem hiding this comment.
It depends, usually the clients handle the encoding automatically and even curl has an option
--data-urlencode.
//cc @philsturgeon for awareness but I do not see this as a problem
There was a problem hiding this comment.
Updated to use --data-urlencode 👍
Changelog line added 👍 |
There was a problem hiding this comment.
Looks good; unfortunately we had to move some of the logic in the http package but I guess there's no really other way.
@philsturgeon do you want to have a quick look here just to make sure it's ok from a feature standpoint?
If we could NOT support this we'd keep the things a little bit better in the place they should be and be like man respect the standard
BTW — personal opinion, this OAS flag is just…non sense and I would love ❤️ to know the rationale behind this.
test-harness/specs/validate-body-params/form-data-allow-reserved-fail.oas3.txt
Show resolved
Hide resolved
| @@ -27,7 +27,7 @@ paths: | |||
| ====server==== | |||
| mock -p 4010 | |||
| ====command==== | |||
| curl -i -X POST http://localhost:4010/path -H "Content-Type: application/x-www-form-urlencoded" --data "status=ooopsie!" | |||
| curl -i -X POST http://localhost:4010/path -H "Content-Type: application/x-www-form-urlencoded" --data "status=ooopsie%21" | |||
There was a problem hiding this comment.
It depends, usually the clients handle the encoding automatically and even curl has an option
--data-urlencode.
//cc @philsturgeon for awareness but I do not see this as a problem
|
Looks great feature wise! |
Closes #497
WIP: needs code style refinement
Checklist
What kind of change does this PR introduce?
encoding.styleis only applied forapplication/application/x-www-form-urlencodedencoding.styleis applied to all uri parts (before it was only applied to first one)qspackage to decode UrlEncoded payload. It was used internally by fastify'sform-bodypluginDoes this PR introduce a breaking change?
No