Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability for server to receive file permissions for Open and Mkdir requests #546

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

capnspacehook
Copy link

With this change, SFTP clients that support sending file attributes in SFTP open and mkdir requests can now specify the permissions of files and directories when they are created. To avoid breaking backwards compatibility and to keep this PR as small as possible, the SFTP client is not changed so support this.

@capnspacehook capnspacehook force-pushed the capnspacehook/server-side-open-perms branch from 2004cc3 to 2434a4f Compare April 12, 2023 21:10
Copy link
Collaborator

@puellanivis puellanivis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m a bit warry at providing asymmetric capabilities. This code might allow the server to support this, but the client still wouldn’t be able to support this. As well, I’m unsure how this would impact the request-server-side of the implementation.

But it could still have some utility, so I’m not hostile to the idea of patching this in.

Comment on lines +23 to +24
defaultFileMode = 0o644
defaultDirMode = 0o755
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These constants should be grouped individually in their own const so that a block-level documentation can cover them both.

Prefer to also have them be typed constants, so that we can simply use mode := defaultDirMode .

Comment on lines +225 to +226
if p.Attrs != nil {
attrs, _ := unmarshalFileStat(p.Flags, p.Attrs.([]byte))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either p.Attrs should be typed []byte or we should properly handle the case where it is not []byte.

It should be sufficient to use:

switch pattrs := p.Attrs.(type) {
case []byte:
  …
case *FileStat:
  … // since this would be a fast toFileMode(pattrs.Mode) might as well support it.
}

(I can’t speak to the legacy code doing differently. If I were redoing the FXP_SET_STAT packets, I’d do it quite a bit differently… I mean, I already did it’s in internal/encoding/ssh/filexfer…)

@puellanivis puellanivis mentioned this pull request Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants