upgrade formidable to v3 #10451
upgrade formidable to v3 #10451
Conversation
| RestLessFieldNames.Method | ||
| ] as string; | ||
| request.method = methodOverride; | ||
| ] as string[] | string; |
There was a problem hiding this comment.
Curious, was changing the type of variables produced by reading fields to string[] | string something related to formidable/inclusion?
There was a problem hiding this comment.
Yes, Formidable now always returns fields[*] as an array as of v3.0.0
| @@ -99,8 +108,9 @@ export class RestLessServer { | |||
| } | |||
|
|
|||
| private async parseStreamRequestFormBody(request: IncomingMessageEx): Promise<void> { | |||
| const formidable = (await inclusion("formidable")).default; | |||
There was a problem hiding this comment.
Does this mean that the formidable version we are bumping to does not allow what we used to have, like import formidable from "formidable";?
If that is the case, I'm curious: why do we need to use inclusion instead of doing something like
// eslint-disable-next-line @typescript-eslint/no-require-imports
import formidable = require("formidable");There was a problem hiding this comment.
This is due to Formidable switching to ESModules in v3.0.0. It's a really annoying problem, where packages that use ESModules cannot be imported using require(). You would think that import formidable as "formidable" would work, but Typescript is transpiling our code into CommonJS which uses require wherever there is an import (take a look at the dist/ folders locally). @tylerbutler helpfully pointed us to a forum thread with possible fixes (none worked) and the inclusion hack, which does work.
There was a problem hiding this comment.
Thanks for all the details!
| return new Promise<void>((resolve, reject) => { | ||
| const form = formidable({ multiples: true }); | ||
| const form = formidable(); |
There was a problem hiding this comment.
You mentioned in the other closed PR that multiples support was removed. How does that affect us?
There was a problem hiding this comment.
Formidable now always returns an array, where it used to be dependent on the number of form fields with that field name (1 field name would return string, multiple fieldnames would return array). We made use of this expecting multiple Header fields and handling the array possibility. However, for body and method, we expected 1, so a string. You can see the type changes above in the code (where you commented already). We still need to handle string because if the body is parsed by bodyparser instead, it will be a string | string[]
| // set up rudimentary authentication | ||
| const authMiddleware: () => express.RequestHandler = () => (req, res, next) => { | ||
| if (req.get("Authorization") !== `Bearer ${authToken}`) { | ||
| return res.sendStatus(403); | ||
| } | ||
| next(); | ||
| }; | ||
| app.use(authMiddleware()); | ||
| if (restLessBeforeBodyParser) { |
There was a problem hiding this comment.
Educate me: are we introducing these 2 modes of tests depending on the value of restLessBeforeBodyParser just to improve our test coverage, or is this also something related to formidable?
There was a problem hiding this comment.
This is to improve test coverage. Only 1 path of RestLess server was being tested (the formidable path). So I added this alternate variation to test the bodyparser path. All of our services use the formidable path, which is good, because you can see here I had to make a change to urlencoded to allow it to recognize a restless form body.
Upgrade Formidable to v3
Taking over #10434, since there are more braking changes than expected.
Description
The currently used version of "formidable" package has a vulnerability bug which has been fixed in the latest version (3.2.4). Bumping up the version to the latest to address the vulnerability issue.
Steps to Reproduce Bug and Validate Solution
PR Checklist
Does this introduce a breaking change?
Testing
Any relevant logs or outputs
Other information or known dependencies