V0 #33
V0 #33
Conversation
Signed-off-by: Jean <[email protected]>
To do for a v0
People that might be give a feedback: @gurgunday @climba03003 @melroy89 @metcoder95 @melroy89 Maybe for the future?Initially, I wanted more features like:
But, although useful from a pedagogical perspective, take the focus away from Fastify and its core plugins. Another problem is that more features leads to more work and maintenance, and I understand now that I will not have the help I expected to have in terms of contribution on this repo. So I think it's better to have a small, quality demo repo that I can commit to maintaining over the long term, than a more ambitious project that doesn't age well. FrontendI also wanted to build a frontend with Fastify/Vite, there is this PR for it, but I am not really confident with the TypeScript support of Fastify/Vite for now. Fastify typingI would like to work on improving TypeScript on Fastify to improve this demo, to get rid of merging declaration, but I see a lot of different PRs and opinions on the subject. CLI DXThere is also this PR from @climba03003 we should review to offer a better CLI experience. |
Twice my name 😆 . I'm just one person btw. |
|
Update: following a post on my LinkedIn, I have a lot of people reaching me to contribute on this repo. So there is a chance that we can add more features after this PR and getting more DX feedbacks from the community 🤞 |
jean-michelet
requested review from
melroy89,
Mathieuka,
metcoder95 and
Fdawgs
and removed request for
melroy89,
metcoder95 and
Mathieuka
|
I should do a thorough review, especially to make sure the doc is up to date and that we are consistent in the plugin (code in general) organization. But I think I can get most of your feedback already so I change the status to RFR! |
| } | ||
|
|
||
| const allowedMimeTypes = ['image/jpeg', 'image/png'] | ||
| if (!allowedMimeTypes.includes(file.mimetype)) { |
There was a problem hiding this comment.
I realize mime type is not guessed from file content, if I send one_line_csv.png, it does work...
There was a problem hiding this comment.
Should we use file-type?
const fileBuffer = await file.toBuffer();
const fileType = await fileTypeFromBuffer(fileBuffer);There was a problem hiding this comment.
No opinion about this @climba03003 @gurgunday?
There was a problem hiding this comment.
After this feedback, I will merge.
There was a problem hiding this comment.
I'd say yes, as it's a security risk. It's something I checked in one of my APIs using file-type's fromBuffer: https://github.com/Fdawgs/docsmith/blob/main/src/routes/pdf/html/index.js#L32-L51
There was a problem hiding this comment.
Great the link, this is a good reason for educating about addContentTypeParser utility.
But don't you think we should mention this security risk in the plugin documentation?
There was a problem hiding this comment.
There could be a mention, yeah
But it's ultimately the same idea of never trusting a client without input validation
There was a problem hiding this comment.
But do we offer this kind of validation, or does a package we use offer this kind of validation?
Doesn't look like we recommend real validation for content file by reading the doc.
There was a problem hiding this comment.
I have no very strong opinion on inspecting the file content for the content-type, most of the application only use .ext to content-type (e.g. Reverse Proxy, Browser, OS).
If it is an input file (batch processing), you should validate the actual content inside instead of the guessed content-type.
If it is an image file or the others, using wrong content-type does not make changes (malicious code can still be hide inside the correct content-type and execute when parsing).
IMO, the only valid reason of guessing the content-type is you want to process the content and don't want to waste time or resources on apparently invalid one.
package.json
Outdated
| @@ -9,29 +9,33 @@ | |||
| }, | |||
| "scripts": { | |||
| "start": "npm run build && fastify start -l info dist/app.js", | |||
| "build": "tsc", | |||
| "build": "rm -rf ./dist && tsc", | |||
There was a problem hiding this comment.
rm -rf won't work for Windows users using cmd or PowerShell.
You may need to either add a script to scripts/ that cleans up dist using node:fs's rm and call that, or explicitly state in documentation for users to use Git Bash which comes with Git for Windows.
There was a problem hiding this comment.
Or just don't use windows 🤣🤭
Sorry about that, I really wanted to achieve the main backend features. |
|
OK, I think I am gonna merge and will create issues to fix the existing or add new features. I suggest we wait a little before communicating on it. |
|
Thanks all of you for your reviews ❤️ |
#31 #10 #28 #34
The aim of this PR is to produce enough features to release a v0 (backend only).
I think it's simpler launch it this way, as the code changes a lot and it's easier to get feedback loop.