Ignore sticky bit when validating permissions. #177
Conversation
jupyter_core/paths.py
Outdated
| @@ -388,7 +388,7 @@ def get_file_mode(fname): | |||
| # Some filesystems (e.g., CIFS) auto-enable the execute bit on files. As a result, we | |||
| # should tolerate the execute bit on the file's owner when validating permissions - thus | |||
| # the missing one's bit on the third octet. | |||
| return stat.S_IMODE(os.stat(fname).st_mode) & 0o7677 # Use 4 octets since S_IMODE does the same | |||
| return stat.S_IMODE(os.stat(fname).st_mode) & 0o6677 # Use 4 octets since S_IMODE does the same | |||
There was a problem hiding this comment.
I agree we should ignore the sticky bit. Wondering if we shouldn't just ignore them all in that octet, but that's a larger discussion.
If we're only removing the sticky bit from the mask, then the comment should be updated to state why. I also had a brain cramp when I added the previous comment and we should change the wording of "one's bit" to "least significant bit" while we're here. So perhaps something like...
...thus the missing least significant bit on the third octet. In addition, we also tolerate the sticky bit being set, so the lsb from the fourth octet is also removed.
The other "repair" ask I have (sorry), is to fix the error message at the end of file. Could you please add some spacing before "Got" and put a period at the end of its sentence as well? That's been bugging me in the recent logs I've seen. 😄
There was a problem hiding this comment.
I'm going to go super nitpicky about the comments: I think we're using 'octet' inaccurately. An octet is 8 bits, aka a byte. Digits in octal represent 3 bits each. So I think we should be using the term 'octal digit' instead.
(I know this was already there, but I think it's worth fixing while we're looking at this bit of code anyway)
There was a problem hiding this comment.
Yeah, you're right - my bad. @WibbletheDuck - would you mind fixing up the occurrences of octet as well?
There was a problem hiding this comment.
@kevin-bates Done!
Fix wording of get_file_mode() comment and add text about allowing the sticky bit. Fix grammar of secure_write() permissions error message.
|
Thanks |
- Recent addition in jupyter_client caused this bug. This should be removed later, once a newer version of jupyter_core is released. The offending code was added to jupyter_client, moved to jupyter_core, then fixed in jupyter_core. - original patch: jupyter/jupyter_client#469 (2019-09-08) - fix in jupyter_core: jupyter/jupyter_core#177 (2019-11-16)
On Nextjournal we use a standard connection file which is mounted to docker containers in order to support Jupyter runtimes. Since the addition of
secure_writewe can get an initial startup just fine by setting permissions on the file. However, with some Jupyter kernels it seems the Sticky Bit gets set, and this persists on the mounted file through restarts and resets, causingsecure_writeto fail:I don't think the Sticky Bit is a security risk(?), so I see no reason it shouldn't be ignored. Tested the fork, and this change does clear up our problem.