Conversation
Summary of ChangesHello @muhammadolammi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request fixes a bug in the ExtractZip function to correctly handle file paths during extraction, preventing files from being created in the current working directory. The changes also include significant improvements to resource management by fixing a file descriptor leak and using a more efficient method for copying file data. Additionally, a syntax and logic error in a loop within registry/model.go is corrected. My review identifies a critical security vulnerability (Zip Slip) in the updated ExtractZip function and provides a suggestion to mitigate it.
…rent /tmp sub folder instead of cwd Signed-off-by: Muhammad Akewukanwo <muhammadolammi@gmail.com>
…kipped file Signed-off-by: Muhammad Akewukanwo <muhammadolammi@gmail.com>
Signed-off-by: Muhammad Akewukanwo <muhammadolammi@gmail.com>
Signed-off-by: Muhammad Akewukanwo <muhammadolammi@gmail.com>
Signed-off-by: Muhammad Akewukanwo <muhammadolammi@gmail.com>
Signed-off-by: Muhammad Akewukanwo <muhammadolammi@gmail.com>
9dde1d5 to
d3cd80e
Compare
Signed-off-by: Muhammad Akewukanwo <muhammadolammi@gmail.com>
27f6546 to
fb4ff3d
Compare
|
@parthivsaikia, perhaps, you might offer review or @lekaf974 might have a comment or two. |
| return ErrExtractZip(err, path) | ||
| } | ||
| if !strings.HasPrefix(targetPath, destDir+string(os.PathSeparator)) && targetPath != destDir { | ||
| return fmt.Errorf("zipslip: illegal file path: %s", file.Name) |
There was a problem hiding this comment.
This must return a meshkit error, ErrExtractZip looks a good candidate
| fd, err := file.Open() | ||
| if err != nil { | ||
| return ErrExtractZip(err, path) | ||
| return err |
There was a problem hiding this comment.
this must be a meskit error as well
There was a problem hiding this comment.
Hi @muhammadolammi, thank you for this PR.
Please ensure all errors returned are meshkit errors
Documentation: https://docs.meshery.io/project/contributing/contributing-error
The errors in this function are wrapped with ErrExtractZip. The naked errors are in the closure function, where we call a function in the for loop that returns a Go error. Then, we check if it's a non-nil error and return an ErrExtractZip error of that error. I think it's a good design |
05fee38 to
9b3ddae
Compare
Signed-off-by: Muhammad Akewukanwo <muhammadolammi@gmail.com>
9b3ddae to
65937b7
Compare
Description
This fixes the bug in the ExtractZip function; it currently creates all subfolders of the zip inside cwd when using "filename". By using filepath, we make sure it's produced in /tm,p, which other functions calling ExtractZip (like the ProcessContent) expect and depend on
Notes for Reviewers
This PR depends on "fix syntax error in registry/model.go #884" Please merge #884 first.
Signed commits