Copilot commented on code in PR #3628:
URL: https://github.com/apache/thrift/pull/3628#discussion_r3544262354
##########
lib/netstd/Thrift/Transport/Server/TNamedPipeServerTransport.cs:
##########
@@ -189,7 +189,6 @@ private static SafePipeHandle CreatePipeNative(string name,
int inbuf, int outbu
pipesec.SetOwner(sidOwner);
pipesec.AddAccessRule(new PipeAccessRule(sidOwner,
PipeAccessRights.FullControl, AccessControlType.Allow));
- pipesec.AddAccessRule(new PipeAccessRule(sidWorld,
PipeAccessRights.ReadWrite, AccessControlType.Allow));
}
Review Comment:
Removing the WorldSid ReadWrite rule changes the effective named-pipe ACL to
(apparently) owner-only. That is a breaking behavior change for scenarios where
the server runs under one account (e.g., a service) and clients connect under
different local users. Consider making the allowed client SIDs configurable
(e.g., via a flag/option or a PipeSecurity callback), or updating the public
API/docs to clearly state the new default access semantics.
##########
lib/netstd/Thrift/Transport/Server/TNamedPipeServerTransport.cs:
##########
@@ -189,7 +189,6 @@ private static SafePipeHandle CreatePipeNative(string name,
int inbuf, int outbu
pipesec.SetOwner(sidOwner);
pipesec.AddAccessRule(new PipeAccessRule(sidOwner,
PipeAccessRights.FullControl, AccessControlType.Allow));
Review Comment:
`sidWorld` is now unused (the access rule granting WorldSid ReadWrite was
removed), and the nearby comment still states "everyone else read/write", which
no longer matches the effective ACL. This can produce build warnings and
mislead future changes. Consider removing the unused SID and updating the
comment to reflect the tightened access rules.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
