Embedded client-only build guards - #1272
ejohnstown wants to merge 3 commits into
Conversation
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1272
Scan targets checked: wolfssh-src, wolfssh-bugs
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.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 1
Open (4)
statictest… · New2479:the). This reads as a typo in… · New
What changed in this PR
This PR improves compatibility for embedded and client-only builds by relaxing server-only compile guards and tightening test conditionals so code compiles cleanly across one-sided configurations.
Changes:
- Remove the hard error that rejected client-only SCP builds.
- Build path helpers (e.g.,
wolfSSH_GetPath) for SCP/SFTP in both client-only and server-enabled configurations. - Gate regress tests so server-only keyboard-interactive tests and SFTP-only attribute tests compile in one-sided builds.
| File | Description |
|---|---|
| wolfssh/settings.h | Removes compile-time rejection for client-only SCP builds. |
| wolfssh/internal.h | Exposes wolfSSH_GetPath for any SCP/SFTP build (not server-only). |
| src/internal.c | Enables path helper compilation for client-only SCP/SFTP and adjusts terminal guard. |
| src/ssh.c | Enables terminal request/resize code without requiring filesystem support. |
| tests/regress.c | Adds build guards around server-only kb tests and SFTP-only attribute tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
7bd3842 to
637782b
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1272
Scan targets checked: wolfssh-src, wolfssh-bugs
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
The terminal code is compiled for any WOLFSSH_TERM build. The window dimensions come from an ioctl or the console API and the terminal modes from termios, each already guarded on its own, so a client on a target without a filesystem can request a PTY and send its modes. - <unistd.h> is included beside <termios.h> and <sys/ioctl.h>, since port.h only pulls it in for a filesystem build
wolfscp.c builds its server half in a client-only build too, and that code calls wolfSSH_CleanPath(). Compile it and wolfSSH_GetPath() whenever SFTP or SCP is enabled, not just for a server build. - SCP's client half is guarded on NO_WOLFSSH_CLIENT, so the settings.h error rejecting a client-only SCP build goes with it
637782b to
05ca3d9
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1272
Scan targets checked: none
Unchanged since last review (not re-run): wolfssh-src, wolfssh-bugs
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
philljj
left a comment
philljj
left a comment
There was a problem hiding this comment.
Looks good, with 3 nits / suggestions:
- should there be a workflow that combines
WOLFSSH_TERM && NO_FILESYSTEM? It looks like they're only tested in separation.
$grep -rl "WOLFSSH_TERM" .github/*
.github/workflows/sbom.yml
$grep -rl "NO_FILESYSTEM" .github/*
.github/workflows/os-check.yml-
I don't see any
NO_WOLFSSH_SERVERordisable-serverworkflows. If(SCP || SFTP) && NO_WOLFSSH_SERVERis valid now, then a workflow combining that could be good. -
I think the copilot nit was correct (see below).
| wolfSSH_free(ssh); | ||
| wolfSSH_CTX_free(ctx); | ||
| } | ||
| #endif /* WOLFSSH_SFTP */ |
There was a problem hiding this comment.
Unless I'm misreading I think copilot was right.
It looks like:
#ifdef WOLFSSH_SFTP
#ifndef NO_WOLFSSH_CLIENT
...blahblah...
#endif /* WOLFSSH_SFTP */
#endif /* !NO_WOLFSSH_CLIENT */ (the comments are reversed)
#ifdef WOLFSSH_SFTP
/* fenrir 2479: the attribute encoder and decoder must agree. Extensions areThe ifdef WOLFSSH_SFTP is the outer scope
There was a problem hiding this comment.
Yeah, that's a good catch. I just needed to reread it again. Fixed.
I also combined the two define checks into a compound check.
regress.c compiles in a client-only build with keyboard-interactive and in a server-only build with SFTP. - the TestKb*() calls are gated on NO_WOLFSSH_SERVER like their definitions - the #endif lines after TestOct2DecRejectsInvalidNonLeadingDigit() close both of its guards, and the attribute test has its own WOLFSSH_SFTP block
cb316fc to
bca8449
Compare
More guard fixes for embedded client-only builds: the terminal code no longer needs a filesystem, SCP builds client-only, and regress.c compiles in both one-sided configurations.