This won't work if multiple bits are set

These bits seem to typically manifest by replacing the 'x' field with either 's' (setuid/setgid+executable), 'S' (setuid/setgid WITHOUT executable), 't' (sticky+exec), or 'T' (sticky NO exec)

In commit "init: add option for rpccookie permissions" (7ad5349a2ec04399c99c8cbf71c8d3b4627132f8)

Could move this variable inside the if statement since it's not used afterwards.

Moved in 9eff5601059

nit: If touching this file again, may want to consider defining a default argument for cookie_perms. Since std::optional is used, I'm assuming the intent is not to force the caller to provide an optional object.

diff --git a/src/rpc/request.h b/src/rpc/request.h
index b40df16631..c7e723d962 100644
--- a/src/rpc/request.h
+++ b/src/rpc/request.h
@@ -19,7 +19,7 @@ std::string JSONRPCReply(const UniValue& result, const UniValue& error, const Un
 UniValue JSONRPCError(int code, const std::string& message);
 
 /** Generate a new RPC authentication cookie and write it to disk */
-bool GenerateAuthCookie(std::string* cookie_out, std::optional<fs::perms> cookie_perms);
+bool GenerateAuthCookie(std::string* cookie_out, std::optional<fs::perms> cookie_perms=std::nullopt);
 /** Read the RPC authentication cookie from disk */
 bool GetAuthCookie(std::string *cookie_out);
 /** Delete RPC authentication cookie from disk */

GenerateAuthCookie() is currently used with the argument provided, so not a big deal either way.

Added in 49d5bfdd7e7

In 31cc8bc305103baf2c9979d86ef1b43dec5628cb "test: add rpccookieperms test"

Since #29034, the preferred convention is to use platform.system() rather than os.name.

Thanks, updated in 49d5bfdd7e7

In 31cc8bc305103baf2c9979d86ef1b43dec5628cb "test: add rpccookieperms test"

This is incorrect. It causes the entire test file to be marked as skipped (different exit code, marked differently in the test runner output) even though everything else ran. The correct thing here is to skip this individual case, which can be achieved by simply returning.

Good catch. Might also be good to print a log message before returning for awareness.

Oh no! fixed in 49d5bfdd7e7

In 38af55e285336cdd3f42635938f24622451703b0 "util: add PermsToString and StringToPerms"

nit: This naming is a little bit confusing to me since it suggests that these functions are inverses of each other, but they're not as their string outputs are not compatible.

I agree on reflection, gave them new names in 49d5bfdd7e7

In 7ad5349a2ec04399c99c8cbf71c8d3b4627132f8 "init: add option for rpccookie permissions"

This comment is removed, but the umask is still what sets the permissions in the default case.

Since we can set the permissions explicitly in this function, I think it would make sense to always set them with owner as default rather than relying on the umask that happens somewhere else.

re: #28167 (comment)

This comment is removed, but the umask is still what sets the permissions in the default case.

Good catch, it would be good to add back the comment.

Since we can set the permissions explicitly in this function, I think it would make sense to always set them with owner as default rather than relying on the umask that happens somewhere else.

The PR was actually doing this originally, but I suggested doing the opposite in #28167 (comment). It makes sense to me that when -rpccookieperms option is not specified, bitcoind would not set permissions on this file, just like it is not setting permissions on other files.

It seems fragile to rely on the fs::permissions call doing the right thing on all filesystems, when its behavior is not specified anywhere and we have never relied on it before. The documentation points to some odd ways it can be behave like "changing some bits may automatically change others" and we don't know what unusual things it may do on fuse filesystems or filesystems that use ACLs.

I've un-removed the comment in 9eff5601059, as I agree with @ryanofsky that not changing the existant behaviour is the "safer" default, and new behaviour can be introduced with the new option.