Process name exclusion (#46) #84
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @PoneyClairDeLune, 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 enhances the application's proxying capabilities by introducing a configurable process exclusion feature. It allows users to specify applications that should bypass the proxy, improving flexibility and control over network traffic management. The changes integrate this new functionality into the existing configuration system and core proxy logic, ensuring that specified processes are identified and handled appropriately before any proxy association occurs.
Highlights
- Process Exclusion Configuration: Introduced the ability to load a list of process names to be excluded from proxying directly from the application's configuration settings.
- Core Logic for Exclusion: Implemented a new mechanism within the core proxy routing logic to check against the excluded process list before attempting to associate an application with a proxy.
- API for Process Exclusion: Added a new public method,
ExcludeProcessName, to theSocksifierinterface and its underlying C++ implementation, allowing programmatic exclusion of processes. - Build Artifact Exclusion: Updated the
.gitignorefile to prevent the inclusion of generated build artifacts (binandobjdirectories) from theProxiFyreproject in version control.
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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| 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 issue 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
-
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. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a useful feature for excluding processes from being proxied. The changes are implemented across C#, C++/CLI, and native C++. While the overall approach is sound, I've identified several critical issues, particularly in the C++ and C++/CLI layers, that will prevent the code from compiling or cause incorrect behavior. These include a typo in the C# logging code, incorrect function definitions in C++/CLI, and a critical error in C++ where a static function attempts to access an instance member. I've also found a bug related to case-sensitive comparisons for exclusions and some performance inefficiencies. My review comments provide detailed explanations and suggestions to fix these issues. Given your note about not having a Windows machine for testing, addressing these points should help ensure the feature works as intended.
136c947 to
8bd325d
Compare
8bd325d to
dafcd7c
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces a process exclusion feature, allowing certain applications to bypass the proxy. The changes span C#, C++/CLI, and C++ code. While the feature is a valuable addition, there are a few issues in the C++ implementation that need attention. I've identified a correctness bug related to case-sensitive matching for excluded processes and an issue with error handling that makes part of the C# code unreachable. My review includes specific suggestions to address these points, aiming for clarity, especially given the note about limited C++ experience. Overall, a good feature addition with a few necessary fixes.
| process->path_name.find(excluded_entry) != std::wstring::npos || | ||
| process->name.find(excluded_entry) != std::wstring::npos |
There was a problem hiding this comment.
The process name matching for exclusion is case-sensitive. excluded_entry is converted to uppercase when it's added to excluded_list_, but process->path_name and process->name are not converted to uppercase before the find operation. This will cause the exclusion to fail if the casing doesn't match exactly. The other matching logic in this function correctly uses to_upper.
to_upper(process->path_name).find(excluded_entry) != std::wstring::npos ||
to_upper(process->name).find(excluded_entry) != std::wstring::npos| if (_socksify.ExcludeProcessName(excludedEntry)) { | ||
| LoggerInstance.Info($"Successfully excluded {excludedEntry} from being proxied."); | ||
| } else { | ||
| LoggerInstance.Info($"Failed to exclude {excludedEntry} from being proxied."); |
There was a problem hiding this comment.
Logging a failed exclusion as Info might cause configuration issues to be missed. It's better to use a higher severity level like Warn to make these events more visible in the logs.
LoggerInstance.Warn($"Failed to exclude {excludedEntry} from being proxied.");| /** | ||
| * Associates a process name to a specific proxy ID. Not sure if this function is thread-safe. | ||
| * @param excluded_entry the name of the process to exclude | ||
| * @return True if exclusion was successful, otherwise false | ||
| */ | ||
| bool exclude_process_name(const std::wstring& excluded_entry) | ||
| { | ||
| // Tried to mimic the lock as in associate_process_name_to_proxy | ||
| std::lock_guard lock(lock_); | ||
|
|
||
| // Append the excluded entry | ||
| excluded_list_.push_back(to_upper(excluded_entry)); | ||
|
|
||
| // If successful, return true | ||
| return true; | ||
| } |
There was a problem hiding this comment.
This function always returns true, which makes the else block in the C# code that calls it unreachable. While std::vector::push_back rarely fails for small strings, it can throw std::bad_alloc on memory exhaustion, which would terminate the service. It's better to handle potential exceptions and return false on failure. This would make the error handling in the C# code functional.
Also, your comment indicates uncertainty about thread safety. The use of std::lock_guard lock(lock_); does make this function thread-safe with respect to other methods using the same mutex (lock_), like associate_process_name_to_proxy. I've updated the documentation to reflect this.
/**
* @brief Adds a process name to the exclusion list. This function is thread-safe.
* @param excluded_entry the name of the process to exclude
* @return True if exclusion was successful, otherwise false
*/
bool exclude_process_name(const std::wstring& excluded_entry) noexcept
{
// The lock_guard makes this function thread-safe against other operations using `lock_`.
std::lock_guard lock(lock_);
try
{
// Append the excluded entry
excluded_list_.push_back(to_upper(excluded_entry));
}
catch (...)
{
return false;
}
return true;
}Fixes for PR #84 from https://github.com/PoneyClairDeLune - Fix case-insensitive matching in socks_local_router exclusion logic * Update exclusion checks to follow same path/name selection logic as pattern matching * Remove redundant to_upper() calls since network_process fields are already uppercase * Ensure consistent behavior between exclusion and association logic - Add comprehensive documentation for match_app_name function * Document two-stage matching process (exclusion then pattern matching) * Clarify intelligent field selection based on path separators * Add detailed parameter descriptions with examples - Update configuration documentation and examples * Add missing excludedList parameter documentation in ProxiFyreSettings constructor * Include "excludes" array in JSON configuration example with various exclusion patterns * Demonstrate support for process names, full paths, and partial path matching - Enhance code consistency across C++ and C# components * Align exclusion logic implementation with documented behavior * Improve documentation completeness for better maintainability These changes ensure robust process filtering with consistent case-insensitive matching and comprehensive documentation for configuration and usage.
|
Thanks a lot for bringing this up and for putting together the initial implementation 🙏. I’ve created another branch where I applied your idea along with some additional fixes and adjustments. Since the changes are now tracked there, I’m going to close this PR and replace it with a new one. Your contribution was really helpful in pointing me in the right direction. |
fix: enhance process exclusion logic and improve configuration documentation (PR #84)
Built upon the snippet from @NukaColaM (now deleted) in #46, this PR allows loading excluded processes from config, preceding actual process to proxy associations.
I have no experience with C++, and I do not have a Windows machine to test it, only building on your example as much as possible. If viable, please also consider adding a global default proxy endpoint, so that ProxiFyre could be run in whitelist mode, allowing for better user experience.