added feature and tests for uploading and downloading cv#268
added feature and tests for uploading and downloading cv#268
Conversation
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
phillipc0
left a comment
phillipc0
left a comment
There was a problem hiding this comment.
Just looked @ the code, visual inspection will follow later
| deleteFileIfExists(path.join(DATA_UPLOADS_DIR, storedFileName)); | ||
| deleteFileIfExists(path.join(FRONTEND_UPLOADS_DIR, storedFileName)); | ||
| }; | ||
|
|
||
| const persistPdfFile = (storedFileName: string, fileBuffer: Buffer): void => { | ||
| ensureDirectoryExists(DATA_UPLOADS_DIR); | ||
| ensureDirectoryExists(FRONTEND_UPLOADS_DIR); | ||
|
|
||
| fs.writeFileSync(path.join(DATA_UPLOADS_DIR, storedFileName), fileBuffer); | ||
| fs.writeFileSync(path.join(FRONTEND_UPLOADS_DIR, storedFileName), fileBuffer); |
There was a problem hiding this comment.
thats why i was always hesitant of implementing profile picture uploads or similar, i am pretty sure its susceptible to path traversal attacks :(
But I guess since those functions can only be reached by authenticated users, its fine if you can "hack" your own system (as long as the pw is safe), still we should see if it can be hardened (or if it even is possible to do that attack here)
There was a problem hiding this comment.
interesting, haven't thought about that. Probably you can also fake mime types so you can upload any kind of file?
I also guess its not that critical here, but you can have a look pls. :)
| if (!BASE64_PATTERN.test(body.data)) { | ||
| return "Invalid file data"; | ||
| } |
There was a problem hiding this comment.
Is that common practise/necessary? How is the performance of that?
| navigate("/edit"); | ||
| }; | ||
|
|
||
| const handleCvDownload = () => { |
There was a problem hiding this comment.
Would it not fit better into the basic info tab, than the navbar?
In my mind, the navbar is more of a navigational/settings place and the pdf is more about all of the information
There was a problem hiding this comment.
I also though about that, in the Navbar was currently the easiest way. I will experiment where the button could be with respect to responsiveness on mobile.
| } | ||
| } | ||
|
|
||
| // Check optional CV document metadata |
There was a problem hiding this comment.
We should also check if the file exists if we plan on linking to it?
Although most of the time you use the feature when setting it up from scratch so there wont be a file, so do we even want to allow to import it then?
| expect(res.status).toHaveBeenCalledWith(405); | ||
| expect(res.setHeader).toHaveBeenCalledWith("Allow", ["POST"]); | ||
| expect(res.json).toHaveBeenCalledWith({ error: "Method not allowed" }); | ||
| }); |
There was a problem hiding this comment.
can we add a test for unauthenticated/unauthorized requests?
There was a problem hiding this comment.
Yes we can! I guess
|
2 participants
PR will be reviewed once all boxes are checked (check boxes if they do not apply to this pr)