[Ready for review] 直接替换maidata.txt谱面的功能 #32
Conversation
…是普通的导入谱面功能发起的。 2. ReplaceChart新增参数shift，仅在导入的是MA2时需要传入，表示延迟调整模式。 3. 并进一步重构 ReplaceChartModal.tsx，确保原来的直接替换MA2功能可用的同时，预留新功能的空间。
Summary of ChangesHello @Starrah, 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! 此拉取请求旨在扩展现有谱面替换功能，使其能够处理maidata.txt格式的谱面，而不仅仅是MA2格式。通过复用导入时的语法检查逻辑，确保替换的maidata.txt谱面质量。此外，还引入了将导入模式存储到音乐XML元数据中的机制，并对前端UI进行了调整，以更好地支持不同文件类型的替换流程，并为用户提供了关于替换功能适用范围的明确警告。 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR aims to refactor the replaceChart functionality to support maidata.txt charts, enhancing both backend API logic and frontend UI. However, a security audit revealed two significant vulnerabilities in ChartController.cs: a Denial of Service (DoS) risk from reading large files into memory without validation, and a Path Traversal vulnerability allowing arbitrary file overwrites due to unsanitized assetDir parameters. Addressing these security issues is critical. Additionally, there are opportunities to improve the efficiency and robustness of backend file processing.
| public void ReplaceChart(int id, int level, IFormFile file, string assetDir, | ||
| [FromForm] ImportChartController.ShiftMethod? shift) |
There was a problem hiding this comment.
The assetDir parameter, which is a route parameter and thus user-controlled, is used to construct file paths using Path.Combine (e.g., on line 106) without any validation or sanitization. An attacker can provide a path containing directory traversal sequences (e.g., ..) to escape the intended directory and overwrite arbitrary files on the server where the application has write permissions.
Remediation: Validate the assetDir parameter against an allowlist of expected directory names or sanitize it to remove any path traversal sequences before using it in file system operations.
There was a problem hiding this comment.
@clansty 这个用管吗😰 虽然他说有问题的这段并不是我写的，我只是加了个shift参数而已（
| string fileContent; | ||
| using (var reader = new StreamReader(file.OpenReadStream())) | ||
| { | ||
| fileContent = reader.ReadToEnd(); |
There was a problem hiding this comment.
The ReplaceChart method at this line reads the entire content of an uploaded file into memory using reader.ReadToEnd() without any size validation. This creates a Denial of Service (DoS) risk, as a large file could lead to an OutOfMemoryException and application crash, especially when passed to ParseTNumAllFromMa2. Additionally, the current implementation has two code quality issues: it attempts to read the file stream multiple times (file.CopyTo(stream) then file.OpenReadStream()), which can cause errors with non-seekable streams, and it includes a redundant stream.Close() within a using statement. It is recommended to implement a maximum file size limit, process files using a stream-based approach, and refactor to read the file content only once for both writing and parsing.
There was a problem hiding this comment.
这个不用太管吧，后端是在用户自己的电脑上运行的，我DoS我自己吗？
MaiChartManager/Front/src/components/DragDropDispatcher/ReplaceChartModal.tsx
Outdated
Show resolved
Hide resolved
方便之后可以直接复用ImportMaidata函数实现替换。 但是这里的问题是ImportMaidata无法是静态的（因为里面有大量调用到的函数不是静态的，甚至还有一个logger是WebServer框架提供的）。因此后续还需要二阶段重构。
…ortChartController调用这个Service。 这样，MaidataImportService便可以被其他地方如ReplaceChart复用。
…ch的自动sync功能。 如ReplaceChart等后端接口，可能会涉及对MusicXml中的信息进行修改后保存。此时前端updateMusicList时会发现相关数据出现变更，触发了MusicEdit/ChartPanel中的watch，造成发送了多个多余的edit请求、同时modified也被错误设置为true。 因此，对于涉及内部对xml进行修改后会自动保存的后端接口，可以在updateMusicList期间打开本选项，以阻止MusicEdit/ChartPanel中的sync动作。
Starrah
changed the title
|
/gemini summary |
|
/gemini review |
Summary of Changes此拉取请求显著增强了谱面管理器的功能，允许用户直接替换maidata.txt格式的谱面，而不再局限于MA2格式。通过将复杂的谱面导入和解析逻辑封装到独立的MaidataImportService中，并复用现有的语法检查API，确保了替换过程的健壮性和数据一致性。前端界面也进行了全面更新，以支持新的文件类型，提供详细的检查结果反馈，并智能地处理导入模式的选择，从而为用户提供了更灵活、更安全的谱面替换体验。 Highlights
Changelog
Activity
|
There was a problem hiding this comment.
Code Review
This Pull Request successfully refactors the chart replacement function to support maidata.txt files, enhancing chart checking through ImportChartCheckApi reuse and improving maintainability by extracting core import logic into MaidataImportService. UI components like ImportAlert and ShiftModeSelector have been effectively split, and storing ShiftMethod in XML custom fields is a good design. However, a critical path traversal vulnerability has been introduced. The assetDir parameter in the chart replacement and import logic lacks proper validation, potentially allowing attackers to write files to arbitrary server locations. Strict validation for the assetDir parameter is required to address this. Minor improvements are also suggested in ChartController.cs. Overall, this is a high-quality submission, but the security vulnerability needs immediate attention.
| { | ||
| var targetChart = music.Charts[level]; | ||
| targetChart.Path = $"{id:000000}_0{level}.ma2"; | ||
| using var stream = System.IO.File.Open(Path.Combine(StaticSettings.StreamingAssets, assetDir, "music", $"music{id:000000}", targetChart.Path), FileMode.Create); |
There was a problem hiding this comment.
The assetDir parameter is used to construct a file path without validation. An attacker could provide a value like ../../.. to perform a path traversal attack and write files outside the intended directory. This is especially dangerous here as the content of the written file is controlled by the user upload. Consider validating assetDir against an allow-list of expected directory names.
| // Fallback to maiLibChart if T_NUM_ALL not found | ||
| if (targetChart.MaxNotes == 0) targetChart.MaxNotes = maiLibChart.AllNoteNum; | ||
|
|
||
| File.WriteAllText(Path.Combine(Path.GetDirectoryName(music.FilePath)!, targetChart.Path), shiftedConverted); |
There was a problem hiding this comment.
The music.FilePath property, which is derived from the user-controlled assetDir parameter, is used to determine the directory where the chart file is written. This allows for a path traversal attack where an attacker can overwrite arbitrary files on the system by providing a malicious assetDir value. This issue affects multiple File.WriteAllText calls in this method (e.g., lines 363, 364, 365, 398, 401).
1 participant
2025年挖过的坑，现在开始填。
Ready for review
我知道这个改动有点大了（你看我就说这不是一个多简单的事对吧，牵涉的东西还挺多的），所以review的时候建议分commit review。比如有一个commit看起来有四五百行的+-，但其实因为是重构，只是把代码移动了一个地方，内容没有变。所以分commit 一个一个review可能会看的清楚一点。