Conversation
Summary of ChangesHello, 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 introduces a significant architectural improvement to Xmake's project creation functionality. By refactoring template-related logic into a new, centralized Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
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 |
![gemini-code-assist[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly refactors the project template creation system, moving logic to a dedicated actions.create.template module and simplifying template structure for improved maintainability. However, it introduces several security vulnerabilities, including path traversal in template discovery and project creation, and a command injection vulnerability leading to Remote Code Execution (RCE) via xmake.lua due to improper handling of user-supplied input. Additionally, there are suggestions for improving edge case handling, such as ensuring case-insensitivity for xmake.lua detection and refining shell completion. It is critical to address these security concerns by implementing strict validation and sanitization for all user-controlled parameters, especially those used in file path construction and template variable substitution.
| assert(tempinst and tempinst:scriptdir(), "invalid template id: %s!", templateid) | ||
|
|
||
| -- get project directory | ||
| local projectdir = path.absolute(option.get("project") or path.join(os.curdir(), targetname)) |
There was a problem hiding this comment.
The projectdir is constructed using the user-supplied targetname or project option without any validation or sanitization. An attacker could provide a malicious path containing traversal sequences (e.g., ../../etc) to point the project directory to a sensitive system location. Combined with the path traversal vulnerability in template selection, this could lead to arbitrary file writes on the system.
| function templatedir(lang, templateid) | ||
| assert(lang) | ||
| assert(templateid) | ||
| for _, rootdir in ipairs(rootdirs()) do | ||
| local dir = path.join(rootdir, lang, templateid) | ||
| if os.isdir(dir) then | ||
| return dir | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
The lang and templateid parameters are used to construct a directory path using path.join without any validation. An attacker can use path traversal sequences like .. to escape the intended template directory and point sourcedir to any directory on the system. This allows for unauthorized information disclosure or, when combined with project creation logic, arbitrary file writes.
| function templates(lang) | ||
| assert(lang) | ||
| local found = hashset.new() | ||
| for _, rootdir in ipairs(rootdirs()) do | ||
| local templateroot = path.join(rootdir, lang) | ||
| local templatedirs = os.dirs(path.join(templateroot, "*")) | ||
| if templatedirs then | ||
| for _, templatedir in ipairs(templatedirs) do | ||
| if os.isfile(path.join(templatedir, "xmake.lua")) then | ||
| local name = path.filename(templatedir) | ||
| found:insert(name) | ||
| end | ||
| end | ||
| end | ||
| end | ||
| local results = found:to_array() | ||
| table.sort(results) | ||
| return results | ||
| end |
There was a problem hiding this comment.
The lang parameter is used in path.join and subsequently passed to os.dirs without sanitization. This allows an attacker to perform path traversal and list the contents of arbitrary directories on the system during shell completion or template discovery.
| function copy_files(sourcedir, projectdir) | ||
| local createdfiles = {} | ||
| for _, srcfile in ipairs(os.files(path.join(sourcedir, "**"))) do | ||
| local relpath = path.relative(srcfile, sourcedir) | ||
| local dstfile = path.absolute(path.join(projectdir, relpath)) | ||
| os.mkdir(path.directory(dstfile)) | ||
| os.cp(srcfile, dstfile, {writeable = true}) | ||
| table.insert(createdfiles, dstfile) | ||
| end | ||
| return createdfiles | ||
| end |
There was a problem hiding this comment.
This implementation of copy_files does not copy empty directories from the template source to the project directory. os.files only iterates over files, so if a template contains an empty directory, it will not be created in the new project. The previous implementation using os.filedirs and os.cp handled this correctly. To fix this, you could first iterate over all directories (including empty ones) and create them, and then copy the files.
function copy_files(sourcedir, projectdir)
local createdfiles = {}
for _, dir in ipairs(os.dirs(path.join(sourcedir, "**"))) do
os.mkdir(path.join(projectdir, path.relative(dir, sourcedir)))
end
for _, srcfile in ipairs(os.files(path.join(sourcedir, "**"))) do
local relpath = path.relative(srcfile, sourcedir)
local dstfile = path.absolute(path.join(projectdir, relpath))
os.cp(srcfile, dstfile, {writeable = true})
table.insert(createdfiles, dstfile)
end
return createdfiles
end
| local templates = {} | ||
| local languages = opt.language and {opt.language} or template.languages() | ||
| for _, l in ipairs(languages) do | ||
| table.join2(templates, template.templates(l)) | ||
| end | ||
| , values = function (complete, opt) | ||
| if not complete then return end | ||
|
|
||
| -- import template | ||
| import("core.project.template") | ||
|
|
||
| -- get templates | ||
| local templates = {} | ||
| for _, l in ipairs(opt.language and {opt.language} or template.languages()) do | ||
| for _, t in ipairs(template.templates(l)) do | ||
| table.insert(templates, t:name()) | ||
| end | ||
| end | ||
|
|
||
| return templates | ||
| end } | ||
|
|
||
| , {} | ||
| , {nil, "target", "v", nil, "Create the given target." | ||
| , "Uses the project name as target if not exists." } | ||
| } | ||
| } | ||
|
|
||
|
|
||
|
|
||
| return templates |
There was a problem hiding this comment.
Using table.join2 here can result in duplicate template names in the completion list if a template is available for multiple languages (e.g., 'console' for both C and C++). For a better user experience with tab-completion, it's best to provide a unique, sorted list of template names. You can use a hashset to collect unique template names and then convert it to a sorted array.
import("core.base.hashset")
local templates_set = hashset.new()
local languages = opt.language and {opt.language} or template.languages()
for _, l in ipairs(languages) do
for _, t in ipairs(template.templates(l)) do
templates_set:insert(t)
end
end
local templates = templates_set:to_array()
table.sort(templates)
return templates
1 participant
Uh oh!
There was an error while loading. Please reload this page.