internal: commit a session only once accepted - #1235
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
wolfSSH_AGENT_ChannelOpen() currently contradicts its “safe to poll” contract by returning WS_BAD_ARGUMENT in the pre-request state (and wolfSSH_SFTP_accept()’s appChannels gating can advance SFTP without a properly established subsystem), which can lead to incorrect/fragile runtime behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR refines server-side channel/session handling so a shell/exec/subsystem request only “commits” the session (and advances accept/session state) after the relevant callback accepts it, preventing wolfSSH_accept() (and related entry points like SFTP) from reporting an established session after sending CHANNEL_FAILURE.
Changes:
- Consolidates shell/exec/subsystem request handling into
DoChannelRequestSession()and defers committingsessionType/command+CLIENT_DONEuntil acceptance. - Introduces “application-driven channels” mode (
wolfSSH_{CTX_,}SetAppChannels) that stopswolfSSH_accept()after userauth. - Adds
wolfSSH_AGENT_ChannelOpen()so app-driven servers can open the agent-forwarding channel afteraccept()returns early; adds unit/regression coverage for the new behaviors.
File summaries
| File | Description |
|---|---|
| wolfssh/ssh.h | Documents and exposes application-driven channel mode APIs. |
| wolfssh/internal.h | Adds appChannels flag to context/session internal structs. |
| wolfssh/agent.h | Exposes wolfSSH_AGENT_ChannelOpen() for app-driven servers. |
| tests/unit.c | Adds unit coverage for rejected session requests and appChannels “no callback” behavior. |
| tests/regress.c | Adds regression coverage for accept stopping at userauth, callback/no-callback behavior, and rejected session requests not advancing accept state. |
| src/wolfsftp.c | Adjusts SFTP accept preconditions when appChannels is enabled. |
| src/ssh.c | Updates wolfSSH_accept() to stop at userauth in appChannels mode and factors agent channel opening into a helper. |
| src/internal.c | Adds DoChannelRequestSession() and updates channel request processing to only commit on acceptance. |
| src/agent.c | Implements wolfSSH_AGENT_ChannelOpen() helper. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Lite
💡 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 #1235
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.
cf9a26b to
5bdfaf9
Compare
A shell, exec or subsystem request changes the channel only once the callback accepts it. The session type and command are set for the callback to read and put back if it refuses, and CLIENT_DONE follows acceptance alone, so wolfSSH_accept() stays where it is rather than reporting a session it answered CHANNEL_FAILURE as established. - DoChannelRequestSession() carries the three arms, which differed only in the type and the callback consulted - a refusal puts the type and command back, so a grant an earlier request won still stands - FreeChannelCommand() wipes and releases a command line for both ChannelDelete() and the refusal path - unit.c drives a refused shell, exec and subsystem request through DoChannelRequest() and checks nothing was committed, and that a refusal after a grant puts the earlier command back whole - regress.c checks accept() stays at ACCEPT_SERVER_CHANNEL_ACCEPT_SENT on a refused shell, and that the sftp gate and both diverts ask for the grant alone Issue: F-8852
5bdfaf9 to
ea346b0
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1235
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.
A shell, exec or subsystem request changes the channel only once the callback accepts it, so wolfSSH_accept() no longer reports an established session, or starts SFTP, on a request it answered CHANNEL_FAILURE.
Issue: F-8852