Add /option GET and PUT endpoints#798
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Summary of ChangesHello @yukieiji, 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 new API endpoints to programmatically manage application options. It provides a GET endpoint to fetch the current configuration and a PUT endpoint to modify individual option settings, enhancing external control and integration capabilities for the ExtremeRoles application. Highlights
Changelog
Activity
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. API calls now flow, Options fetched, then set with care, Code responds to change. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
| return; | ||
| } |
There was a problem hiding this comment.
エラー発生時にresponse.Abort()を使用すると、クライアントが意図したHTTPステータスコードやエラーメッセージを受け取れない可能性があります。ApiServer.csのreturnInternalErrorメソッドのように、ステータスコードを設定し、必要に応じてエラーメッセージを書き込んでからresponse.Close()を呼び出すのがより良いプラクティスです。これにより、APIの堅牢性とデバッグのしやすさが向上します。
IRequestHandler.SetStatusNG(response);
response.Close();| IRequestHandler.SetStatusNG(response); | ||
| response.Abort(); |
There was a problem hiding this comment.
上記と同様に、エラー発生時にresponse.Abort()を使用すると、クライアントが意図したHTTPステータスコードやエラーメッセージを受け取れない可能性があります。ステータスコードを設定し、必要に応じてエラーメッセージを書き込んでからresponse.Close()を呼び出すのがより良いプラクティスです。
IRequestHandler.SetStatusNG(response);
response.Close();
GitHub Actions MSBuilds
Translation Checker Report
|
1 similar comment
GitHub Actions MSBuilds
Translation Checker Report
|
Co-authored-by: yukieiji <40425798+yukieiji@users.noreply.github.com>
…ON for PUT Co-authored-by: yukieiji <40425798+yukieiji@users.noreply.github.com>
Co-authored-by: yukieiji <40425798+yukieiji@users.noreply.github.com>
yukieiji
force-pushed
the
feature/api-option-13826371918860551875
branch
from
3b7cde0 to
eda889b
Compare
|
/gemini review |
GitHub Actions MSBuilds
Translation Checker Report
|
There was a problem hiding this comment.
Code Review
このプルリクエストでは、オプションを管理するための新しいAPIエンドポイントが追加され、大規模なリファクタリングが行われています。
特に、RoleArgsとRolePropの導入により、役職定義の可読性と保守性が大幅に向上しています。また、IsAlive()やIsInValid()といった新しい拡張メソッドが一貫して使用されており、コードが整理されています。
新しいAPIハンドラのロジックにいくつかの軽微な問題と1つの潜在的なバグを見つけました。全体として、これは素晴らしい変更です。
Note: Security Review did not run due to the size of the PR.
Acknowledged. I have implemented the requested /option API with GET and PUT endpoints, featuring a hierarchical structure, recursive child options, and JSON-based parameters as requested. |
GitHub Actions MSBuilds
Translation Checker Report
|
|
/gemini review |
Acknowledged. I have further refined the /option API to include recursive child structures and ensured that the JSON fields exactly match your requested names (ID, TransedName, ValueRange, Selection, TransedFormat, Childs). I also improved the reflection logic to better extract value ranges for all option types. |
yukieiji
force-pushed
the
feature/api-option-13826371918860551875
branch
from
77bfe3e to
9be61dd
Compare
GitHub Actions MSBuilds
Translation Checker Report
|
GitHub Actions MSBuilds
Translation Checker Report
|
There was a problem hiding this comment.
Code Review
This pull request adds GET and PUT endpoints for /option, enabling external management of game options with well-structured DTOs and API handlers. However, a critical Broken Access Control (IDOR) vulnerability has been identified in the PutOption endpoint, which allows an authenticated host to modify any game option without proper authorization. Additionally, while the code generally adheres to the repository's style guide, there is a suggestion to improve error handling in OptionData.cs for enhanced robustness.
I am having trouble creating individual review comments. Click here to see my feedback.
ExtremeRoles/Module/ApiHandler/PutOption.cs (24)
The PutOption endpoint allows an authenticated user (specifically, the host) to update any game option by providing its ID and Value. The request.ID is used to construct tab, categoryId, and optionId without any authorization checks to ensure the user has permission to modify that specific option. This allows a malicious host to modify options they shouldn't have access to, potentially leading to privilege escalation or game manipulation.
Remediation: Implement robust authorization checks within the OptionManager.Update method or before calling it in PutOption.cs. These checks should verify that the authenticated user has the necessary permissions to modify the specific option identified by tab, categoryId, and optionId.
ExtremeRoles/Module/ApiHandler/OptionData.cs (138)
This empty catch block swallows any exceptions that might occur during the reflection process in getValueRange. This can make debugging very difficult if something goes wrong with retrieving the option range. It's better to log the exception so that potential issues are visible. You may need to add using ExtremeRoles; to access the logger.
catch (Exception e) { ExtremeRolesPlugin.Logger.LogWarning($"Failed to get value range for option '{option.Info.Name}' via reflection: {e.Message}"); }
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces GET and PUT endpoints for managing game options, a valuable addition. The overall implementation, including the new DTOs and API handlers, is well-structured. I've identified a few areas for improvement, including a potential bug in option processing, a typo, and some minor style inconsistencies. Additionally, I've suggested the removal of some unused code. The core changes to interfaces and their implementations to support option metadata are solid.
| { | ||
| foreach (var child in childs) | ||
| { | ||
| regsted.Add(option.Info.Id); |
There was a problem hiding this comment.
The parent option's ID is being added here, but the intention seems to be to add the child's ID to prevent it from being processed again in the CreateCategoryDto loop. Please change this to add child.Info.Id to fix the bug.
regsted.Add(child.Info.Id);References
- The current logic incorrectly marks the parent option as processed for each of its children, which fails to prevent child options from being processed multiple times if they appear as top-level items in the category's option list. This is a correctness issue.
| { | ||
| var options = new List<AuOptionDto>(role.AllGameSettings.Count + 1); | ||
|
|
||
| int count = curGameOptions.RoleOptions.GetNumPerGame(role.Role); |
There was a problem hiding this comment.
A full-width space is used here. Please replace it with a standard half-width space for coding style consistency.
int count = curGameOptions.RoleOptions.GetNumPerGame(role.Role);References
- The style guide emphasizes readability. Using inconsistent spacing, such as a full-width space where a half-width space is expected, harms readability. (link)
| return categoryDto; | ||
| } | ||
|
|
||
| private static ExROptionDto createOptionDTO(IOption option, HashSet<int> regsted) |
There was a problem hiding this comment.
The parameter name regsted appears to be a typo for registered. Please consider correcting it and its usages throughout the method to improve readability.
private static ExROptionDto createOptionDTO(IOption option, HashSet<int> registered)References
- The style guide emphasizes readability and using good names. A typo in a variable name makes the code harder to understand. (link)
1 participant
Added /option GET and PUT endpoints to the ApiServer.
GET /option returns the current option structure and data as JSON.
PUT /option updates a specific option and returns the updated options state.
Introduced OptionDTO, CategoryDTO, TabDTO and PutOptionRequest in OptionData.cs.
Implemented GetOption and PutOption handlers.
Registered the new handlers in ExtremeRolesPlugin.cs.
PR created automatically by Jules for task 13826371918860551875 started by @yukieiji