wolfsftp: apply the attributes SETSTAT and FSETSTAT acknowledge - #1197
wolfsftp: apply the attributes SETSTAT and FSETSTAT acknowledge#1197yosuke-wolfssl wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes SFTP SETSTAT/FSETSTAT so the server only acknowledges attributes it actually applies (size/uid-gid/timestamps), returning SSH_FX_OP_UNSUPPORTED when a port lacks the needed wrappers. Also adjusts the built-in SFTP client’s chmod behavior to avoid unintentionally requesting unrelated attribute changes.
Changes:
- Implement
SIZE,UIDGID, andTIMEapplication inSFTP_SetFileAttributes()/SFTP_SetFileAttributesHandle(), withWS_UNIMPLEMENTED_Emapped toSSH_FX_OP_UNSUPPORTED. - Add portable wrappers in
port.hfor truncate/chown and realWSETTIME/WFSETTIMEimplementations when supported. - Add regression and end-to-end tests covering SETSTAT/FSETSTAT attribute application and directory chmod behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
wolfssh/port.h |
Adds WTRUNCATE/WFTRUNCATE, WCHOWN/WFCHOWN, and functional WSETTIME/WFSETTIME wrappers where supported. |
src/wolfsftp.c |
Applies SETSTAT/FSETSTAT attributes (or reports unsupported), and fixes client-side chmod to send only PERM. |
tests/regress.c |
Adds a regression test ensuring SETSTAT/FSETSTAT really change size/time/ownership on disk. |
tests/sftp.c |
Adds an end-to-end chmod-directory test to guard the client fix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1197
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
726ec03 to
c720cab
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1197
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
c720cab to
417aa08
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1197
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
- SFTP_SetFileAttributes() and SFTP_SetFileAttributesHandle() carry out the size, ownership, permission and timestamp requests while ret is WS_SUCCESS, bound the size with wResolveOffset() against WOLFSSH_MAX_FILE_OFFSET, and set WS_UNIMPLEMENTED_E where the port defines no wrapper. - wolfSSH_SFTP_RecvSetSTAT() and wolfSSH_SFTP_RecvFSetSTAT() answer WOLFSSH_FTP_UNSUPPORTED for WS_UNIMPLEMENTED_E; wolfSSH_SFTP_CHMOD() sets the attribute flags to WOLFSSH_FILEATRB_PERM before sending. - port.h adds WTRUNCATE, WFTRUNCATE, WCHOWN and WFCHOWN for the POSIX port, and defines WSETTIME and WFSETTIME over the existing WUTIMES and WFUTIMES helpers in place of their (0) definitions. - SFTP_SetMode() guards on _WIN32_WCE in place of USE_WINDOWS_API, and port.c adds WS_ChmodA(), which trims the SFTP leading root and calls _wchmod(); it and the WCHMOD mapping to it are left out under _WIN32_WCE, which keeps the _chmod mapping. - tests/regress.c adds TestSftpSetStatAttributes(); tests/sftp.c adds a chmod of a directory and installs SFTP_TEST_UMASK in place of the sftpTestUmask static; the STATE_SET_ATR_SEND case in tests/api.c sets atr.flags to WOLFSSH_FILEATRB_PERM. Issue: F-11658
417aa08 to
8c7be35
Compare
Problem
SFTP_SetFileAttributes()andSFTP_SetFileAttributesHandle()left SIZE andUIDGID as
@TODOno-ops yet returnedWS_SUCCESS, soRecvSetSTAT/RecvFSetSTATreplied
SSH_FX_OKfor truncate and chown requests that never happened.WSETTIME/WFSETTIMEwere(0)stubs on every port, so TIME was discarded too.Closes f-11658.
Fix (
src/wolfsftp.c)Every requested attribute is applied or reported. A port with no wrapper yields
WS_UNIMPLEMENTED_E, answered asSSH_FX_OP_UNSUPPORTED; the 64-bit size isbounds-checked by
wResolveOffset()againstWOLFSSH_MAX_FILE_OFFSET.truncate/ftruncatechown/fchownchmod/fchmod_wchmodvia newWS_ChmodA()futimens/utimensatWindows. SFTP paths arrive as
/C:/..., which_chmod()rejects.WS_ChmodA()strips the leading root viaTrimFileName()as the other Windowsroutines do, so
WCHMODworks on Windows for the first time. It is left outunder
_WIN32_WCE, which does not include<io.h>.Client companion fix.
wolfSSH_SFTP_CHMOD()echoed a whole STAT reply back,so once the server honours SIZE a chmod asked it to
truncate()the target —which fails on a directory. It now sends only
WOLFSSH_FILEATRB_PERM.Tests
TestSftpSetStatAttributes()drives both handlers for size, timestamp andownership, asserting the on-disk result via
WSTAT, with separate root andnon-root paths.
tests/sftp.cadds an end-to-end chmod of a directory under afixed umask;
tests/api.c'sSTATE_SET_ATR_SENDcase sends PERM only.Verification
unchanged; without the client fix the directory chmod fails; stubbing
WFCHOWNfails the give-away-chown case; removingWTRUNCATEfails theapi.ccase before the PERM narrowing.regress.testpasses as an ordinary user and undersudo.make check:10 pass, 1 skip, 2 fail — both
scripts/{sftp,scp}.test, which pass onserial re-run (known parallel flake).
with SFTP enabled,
api-test/unit-testpass, andchmod 400/chmod 600over SFTP toggles the read-only attribute. OpenSSH interop:
chmodon adirectory and
put -pboth correct.Not in this PR
wResolveOffset()bound only bites whereSIZEOF_OFF_T != 8.SFTP_SetFileAttributes()stops at the first failing attribute rather thancontinuing best-effort, and the
UNSUPPORTEDmapping has no POSIX test sinceit is reachable only on ports lacking the wrappers.