fix(permission): Address permissions issue for UNC paths on Windows #25132
Conversation
|
@dsherret please can review this change? |
yazan-abdalrahman
changed the title
…n-Windows' into Fix-UNC-Path-Permissions-Issue-on-Windows
This reverts commit 033370d.
|
@dsherret @bartlomieju deno.runtime_permissions_lib.rs.2024-08-21.17-24-59.mp4And when I ran the script, it didn't set failed to true, therefore it didn't output "One or more tests failed" and it didn't return 321. deno.special_main.js.2024-08-21.17-27-31.mp4 |
| // Check if the path is a UNC path (e.g., \\Server\Share\Folder) | ||
| // UNC paths typically contain "\\" at the start or somewhere in the middle | ||
| if s.windows(2).any(|window| window == b"\\\\") { | ||
| return true; |
There was a problem hiding this comment.
This might be a security issue. I think there's a reason why this isn't allowed without -A, but I can't remember at the moment.
There was a problem hiding this comment.
There was a problem hiding this comment.
@mmastrac Please check this change.
There was a problem hiding this comment.
I looked into this a bit and I think it's more complicated than it seems. Here's some points from an old internal document:
- If users are using
\\.\paths to access things like named pipes, raw devices, those should trigger--allow-sys.- Network resources should perform DNS lookup in permission checks, and the fully-resolved permission check object should include the the IP address. When we make the outgoing network requests, we must use the resolved IP.
That said, I'm not sure about what the right solution is here. At the moment, this just requires --allow-all permissions.
There was a problem hiding this comment.
I looked into this a bit and I think it's more complicated than it seems. Here's some points from an old internal document:
- If users are using
\\.\paths to access things like named pipes, raw devices, those should trigger--allow-sys.- Network resources should perform DNS lookup in permission checks, and the fully-resolved permission check object should include the the IP address. When we make the outgoing network requests, we must use the resolved IP.
That said, I'm not sure about what the right solution is here. At the moment, this just requires
--allow-allpermissions.
but I assume if it requires extra permission, it will prompt to ask it to grant the permission, thus actually at this moment it grants all the permission needed, no need to verify all permissions.
There was a problem hiding this comment.
This might be a security issue. I think there's a reason why this isn't allowed without
-A, but I can't remember at the moment.
This might be a security issue. I think there's a reason why this isn't allowed without
-A, but I can't remember at the moment.
@dsherret
What about this? I don't think it's a security issue, but why do sheared files require all permissions? Just some permissions like read, write, and net? and why it didn't prompt to give it all permission?
fix #24703