Use universal SID instead of locale-specific group names #2704
Conversation
Fixes agent failures on non-English Windows by using Well-Known SID "S-1-5-32-545" instead of hardcoded "BUILTIN\Users" in icacls commands.
Summary of ChangesHello @AndreasHanikel, 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 addresses a critical issue where agents running on non-English Windows operating systems would fail due to hardcoded, locale-specific group names in permission settings for temporary directories. By switching to a universal Security Identifier (SID), the change ensures that temporary directories are created with the correct permissions across all Windows locales, thereby improving the robustness and international compatibility of the agent. 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 pull request correctly replaces a locale-specific group name BUILTIN\Users with its universal SID S-1-5-32-545 to ensure compatibility with non-English Windows versions. This is a good fix. I've added a suggestion to also improve error handling for the icacls command and enhance readability by extracting the magic string into a variable.
| dirpath = tempfile.mkdtemp(suffix=suffix, prefix=prefix, dir=dirpath) | ||
| if sys.platform == "win32": | ||
| subprocess.call(["icacls", dirpath, "/inheritance:e", "/grant", "BUILTIN\\Users:(OI)(CI)(RX)"]) | ||
| subprocess.call(["icacls", dirpath, "/inheritance:e", "/grant", "*S-1-5-32-545:(OI)(CI)(RX)"]) |
There was a problem hiding this comment.
While using the universal SID is a great improvement for localization, this call to icacls can fail silently. The return code of subprocess.call is not checked, so if setting permissions fails, it could lead to subtle bugs later on.
It's better to use subprocess.check_call, which will raise a CalledProcessError if the command returns a non-zero exit code. This exception will be caught by the existing except block.
Additionally, extracting the magic string "*S-1-5-32-545:(OI)(CI)(RX)" into a well-named variable would improve code readability and maintainability.
| subprocess.call(["icacls", dirpath, "/inheritance:e", "/grant", "*S-1-5-32-545:(OI)(CI)(RX)"]) | |
| # Grant BUILTIN\Users Read & Execute permissions using its well-known SID to support non-English Windows. | |
| # See: https://learn.microsoft.com/en-us/windows/win32/secauthz/well-known-sids | |
| users_grant_arg = "*S-1-5-32-545:(OI)(CI)(RX)" | |
| subprocess.check_call(["icacls", dirpath, "/inheritance:e", "/grant", users_grant_arg]) |
|
Danke! |
2 participants
Fixes agent failures on non-English Windows by using the SID "S-1-5-32-545" instead of hardcoded "BUILTIN\Users" in icacls commands.
https://learn.microsoft.com/en-us/windows/win32/secauthz/well-known-sids