Skip to content

Conversation

ryanroe
Copy link

@ryanroe ryanroe commented May 21, 2024

fix #550

@Copilot Copilot AI review requested due to automatic review settings August 22, 2025 07:32
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements a new "MeaningfulWords" renaming mode for the Confuser obfuscation tool and fixes a WPF relative resource renaming issue. The main purpose is to replace random character-based obfuscation with natural-looking names based on configurable word patterns, making obfuscated code appear more legitimate.

Key changes:

  • Adds MeaningfulWords renaming mode with configurable word lists and patterns
  • Implements configuration loading from XML with inline support
  • Fixes WPF BAML resource renaming to use filenames instead of full paths

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
Confuser.Renamer/MeaningfulWordsRenamer.cs New file implementing the core meaningful words generation logic
Confuser.Renamer/NameService.cs Adds meaningful words support and configuration loading
Confuser.Renamer/RenamePhase.cs Updates generic parameter renaming to support meaningful words mode
Confuser.Renamer/RenameMode.cs Adds MeaningfulWords enum value
Confuser.Renamer/Analyzers/WPFAnalyzer.cs Fixes resource renaming to use filenames instead of full paths
Confuser.Core/Project/ConfuserProject.cs Adds support for inline XML arguments in project configuration
Confuser.Core/Project/ConfuserPrj.xsd Updates schema to support meaningful words configuration
Tests/... Comprehensive test coverage for the new functionality

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

bamlRef.Rename(module, decodedName, decodedNewName);
bamlRef.Rename(module, encodedName, encodedNewName);
bamlRef.Rename(module, decodedFileName, fileName);
bamlRef.Rename(module, encodedFileName, fileName);
Copy link
Preview

Copilot AI Aug 22, 2025

Choose a reason for hiding this comment

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

The Rename method is being called with fileName as the new name, but fileName is generated using service.RandomName(renameMode).ToLowerInvariant(). This appears inconsistent with the original logic that used full paths (decodedNewName, encodedNewName). The change from full path renaming to filename-only renaming may break resource resolution if the BAML references expect full paths.

Suggested change
bamlRef.Rename(module, encodedFileName, fileName);
bamlRef.Rename(module, decodedName, decodedNewName);
bamlRef.Rename(module, encodedName, encodedNewName);

Copilot uses AI. Check for mistakes.


string GetMeaningfulName() {
EnsureMeaningfulWordsInitialized();
return meaningfulWordsGenerator.GenerateUniqueName(identifiers);
Copy link
Preview

Copilot AI Aug 22, 2025

Choose a reason for hiding this comment

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

The identifiers HashSet is being passed to GenerateUniqueName, but this set may contain identifiers that are not strings or may have different casing rules. The MeaningfulWordsGenerator.GenerateUniqueName method expects a HashSet<string> with case-insensitive comparison, but identifiers field type is not visible in this diff and its comparison behavior is unclear.

Suggested change
return meaningfulWordsGenerator.GenerateUniqueName(identifiers);
return meaningfulWordsGenerator.GenerateUniqueName(
new HashSet<string>(identifiers.OfType<string>(), StringComparer.OrdinalIgnoreCase)
);

Copilot uses AI. Check for mistakes.

}

// If still having conflicts, add random numbers to ensure uniqueness
if (attempts > maxAttempts / 2 && (IsNameUsed(name, existingNames) || usedNames.Contains(name))) {
Copy link
Preview

Copilot AI Aug 22, 2025

Choose a reason for hiding this comment

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

The condition checks IsNameUsed(name, existingNames) and usedNames.Contains(name) inside the loop after already checking the same condition in the while loop condition. This results in redundant checks that could impact performance in scenarios where many unique names need to be generated.

Suggested change
if (attempts > maxAttempts / 2 && (IsNameUsed(name, existingNames) || usedNames.Contains(name))) {
if (attempts > maxAttempts / 2) {

Copilot uses AI. Check for mistakes.

else {
// Use inner XML as value when no value attribute is present
value = i.InnerXml;
}
Copy link
Preview

Copilot AI Aug 22, 2025

Choose a reason for hiding this comment

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

Using i.InnerXml directly as a value without validation or sanitization could potentially allow XML injection or malformed XML to be processed. The code should validate that the inner XML content is safe and properly formed before using it as a configuration value.

Suggested change
}
else {
// Use inner XML as value when no value attribute is present
// Validate that the inner XML is well-formed before using it
string innerXml = i.InnerXml;
if (!string.IsNullOrWhiteSpace(innerXml)) {
try {
var doc = new XmlDocument();
// Prevent DTD processing for security
doc.XmlResolver = null;
doc.LoadXml("<root>" + innerXml + "</root>");
} catch (XmlException ex) {
throw new InvalidDataException("Malformed inner XML in configuration value for '" + name + "'.", ex);
}
}
value = innerXml;
}

Copilot uses AI. Check for mistakes.

name = name.Substring(0, config.MaxLength);
// Re-check uniqueness after truncation
if (IsNameUsed(name, existingNames) || usedNames.Contains(name)) {
name = name.Substring(0, Math.Max(1, config.MaxLength - 4)) + random.NextInt32(1000, 9999);
Copy link
Preview

Copilot AI Aug 22, 2025

Choose a reason for hiding this comment

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

The calculation Math.Max(1, config.MaxLength - 4) could still result in a name that exceeds config.MaxLength when adding the 4-digit random number (1000-9999). If config.MaxLength is less than or equal to 4, this will create names that are longer than the configured maximum length.

Suggested change
name = name.Substring(0, Math.Max(1, config.MaxLength - 4)) + random.NextInt32(1000, 9999);
int maxDigits = Math.Min(4, config.MaxLength);
string randomDigits = random.NextInt32((int)Math.Pow(10, maxDigits - 1), (int)Math.Pow(10, maxDigits)).ToString();
int prefixLength = config.MaxLength - maxDigits;
if (prefixLength > 0) {
name = name.Substring(0, prefixLength) + randomDigits;
} else {
name = randomDigits;
}

Copilot uses AI. Check for mistakes.

@ryanroe ryanroe closed this Aug 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Application doesn't work when resource directory contains relative resource
1 participant