Skip to content

Conversation

@sven-n
Copy link
Member

@sven-n sven-n commented Feb 2, 2026

  • Fixed serialization for JSON
  • Changed model properties from LocalizedString? to LocalizedString and adapted all code usages
  • Added EF Core migration
  • I had to upgrade the source generator to use the incremental generators, because I had to upgrade Microsoft.CodeAnalysis.CSharp which doesn't support the old generators anymore. Otherwise, no new migrations could've been added to the project due to the requirement of a newer version of Microsoft.CodeAnalysis.CSharp.

Replaces the PR of #694

@sven-n sven-n added the bug label Feb 2, 2026
@gemini-code-assist
Copy link

Summary of Changes

Hello @sven-n, 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 significantly enhances the project's stability and maintainability by enforcing non-nullable LocalizedString properties across data models, backed by a new EF Core migration. It also modernizes the build process by upgrading source generators to the incremental model, driven by necessary dependency updates. These changes collectively improve data integrity, optimize build times, and ensure compatibility with the latest .NET ecosystem.

Highlights

  • LocalizedString Nullability Enforcement: Properties using LocalizedString across various data models (e.g., ConfigurationUpdate, MiniGameChangeEvent, MiniGameSpawnWave, PeriodicTaskConfiguration) have been changed from nullable (LocalizedString?) to non-nullable (LocalizedString). This change improves data integrity by ensuring these properties always hold a LocalizedString instance.
  • EF Core Migration for Non-Nullable LocalizedString: A new EF Core migration (20260202204523_NonNullableLocalizedString) has been added to update the database schema, making the corresponding columns for LocalizedString properties non-nullable with a default empty string. This aligns the database with the updated C# model.
  • Source Generator Modernization: The BasicModelGenerator, EfCoreModelGenerator, CloneableGenerator, and ResourceGenerator have been upgraded to utilize C# incremental source generators (IIncrementalGenerator). This was necessary due to an upgrade of Microsoft.CodeAnalysis.CSharp and aims to improve build performance by allowing the generator to operate more efficiently.
  • Dependency Updates: Numerous NuGet package versions have been updated in src/Directory.Packages.props, including Microsoft.CodeAnalysis.CSharp, Microsoft.EntityFrameworkCore components, and various Microsoft.Extensions packages, ensuring the project uses more recent and compatible libraries.
  • JSON Serialization Refactoring: JSON serialization logic for plugin configurations has been refactored into a shared CreateSerializerOptions helper method within PlugInConfigurationExtensions.cs. This centralizes the configuration of JsonSerializerOptions, including reference handling and indentation, for consistency.
  • Code Robustness Improvements: Added checks for null or empty messages in ShowMessageAsync and improved LocalizedString.ToString() and GetTranslation() methods to handle null/empty values more gracefully, preventing potential runtime errors.
  • Source Generator Helper Class: The ModelGeneratorBase class has been refactored and renamed to ModelGeneratorHelper.cs, becoming a static helper class. This change centralizes common logic used by multiple source generators, promoting code reuse and maintainability.

🧠 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.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces several significant changes, including fixing JSON serialization for LocalizedString, making LocalizedString properties non-nullable across various models, and upgrading source generators to use incremental generators due to a Microsoft.CodeAnalysis.CSharp update. A critical finding is a translation injection vulnerability identified in the LocalizedStringField.razor component, where invalid input containing the language separator is applied to the model despite failing validation. Furthermore, the change from nullable to non-nullable LocalizedString and its modified GetTranslation method behavior introduces subtle bugs or inconsistencies, specifically requiring updates to the null-coalescing logic for default messages to handle empty strings and an opportunity to use more robust translation methods.

@sven-n
Copy link
Member Author

sven-n commented Feb 2, 2026

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors LocalizedString to be non-nullable and its GetTranslation method to return a nullable string, modernizes source generators to use the incremental generator API, fixes JSON serialization for plugin configurations, and includes database migration. However, a critical security issue was identified in the LocalizedStringField component: validation for the internal separator string is performed but not enforced, allowing for 'Localization Injection' where a user can unauthorizedly modify translations for other languages by crafting a malicious input string. Additionally, minor issues were found where the new nullable return type of GetTranslation could lead to runtime exceptions.

}

var message = (configuration.Message?.ToString() ?? PlugInResources.BaseInvasionPlugIn_DefaultStartMessage).Replace("{mapName}", mapName.GetTranslation(player.Culture), StringComparison.InvariantCulture);
var message = (configuration.Message.GetTranslation(player.Culture) ?? PlugInResources.BaseInvasionPlugIn_DefaultStartMessage).Replace("{mapName}", mapName.GetTranslation(player.Culture), StringComparison.InvariantCulture);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The call to mapName.GetTranslation(player.Culture) can return null after the changes to LocalizedString. This would cause an ArgumentNullException in string.Replace because the second argument cannot be null. You should handle the possible null value, for example by using the null-coalescing operator.

        var message = (configuration.Message.GetTranslation(player.Culture) ?? PlugInResources.BaseInvasionPlugIn_DefaultStartMessage).Replace("{mapName}", mapName.GetTranslation(player.Culture) ?? string.Empty, StringComparison.InvariantCulture);


await this.ForEachPlayerAsync(player => player.InvokeViewPlugInAsync<IShowMessagePlugIn>(p =>
message.GetTranslation(player.Culture) is { Length: > 0 } translation
? p.ShowMessageAsync(string.Format(translation, args), MessageType.GoldenCenter)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Calling string.Format with an empty args array will throw a FormatException if the translation string contains any format specifiers (e.g., {0}). Some call sites of ShowGoldenMessageAsync pass an empty args array. To make this more robust, you could check if args has any elements before calling string.Format.

                    ? p.ShowMessageAsync(args.Length > 0 ? string.Format(translation, args) : translation, MessageType.GoldenCenter)

@sven-n
Copy link
Member Author

sven-n commented Feb 2, 2026

Validation prevents injection of additional localizations. Additionally, the "user" is an admin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants