tests: cover the channel open, close and req callbacks - #1227
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new tests’ comments/intent imply validation of callback-observed state ordering and channel discoverability during callbacks, but the current implementation does not actually assert those properties.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR expands tests/regress.c to exercise previously untested channel lifecycle and request callbacks in the wolfSSH server harness, aiming to ensure these hooks are invoked and their observable effects are validated.
Changes:
- Adds regression tests for
SSH_MSG_CHANNEL_OPEN_CONFIRMATION/SSH_MSG_CHANNEL_OPEN_FAILUREopen-response callbacks. - Adds regression tests for the channel close callback behavior during peer close handling.
- Adds regression tests for session
"exec"and"subsystem"request callbacks and their resultingCHANNEL_SUCCESS/CHANNEL_FAILUREreplies.
File summaries
| File | Description |
|---|---|
| tests/regress.c | Adds new regression harness helpers and test cases covering channel open-response, close, and request callbacks. |
Review details
Suppressed comments (3)
tests/regress.c:2979
- The open-confirm test comment/PR description implies the peer parameters are already populated when the confirm callback runs, but the callback currently doesn't record them. Likewise, the open-fail path is supposed to invoke the callback while the channel is still on the session's list, but that is not captured. Record peer params and channelListSz inside the callbacks so the tests can assert what the callback could actually observe.
static int RecordingChannelOpenConfCb(WOLFSSH_CHANNEL* channel, void* ctx)
{
AssertNotNull(channel);
openConfCbCalls++;
openRespCbChannel = channel->channel;
tests/regress.c:3059
- To validate that the peer parameters are populated before the open-confirm callback runs (as the comment indicates), assert against the values captured inside the callback rather than only checking the final channel fields after DoReceive().
/* The peer's numbers are in place before the callback can use them. */
AssertIntEQ(channel->peerChannel, 7);
AssertIntEQ(channel->peerWindowSz, 0x4000);
AssertIntEQ(channel->peerMaxPacketSz, 0x8000);
tests/regress.c:3094
- The open-failure test is meant to ensure the failure callback runs while the channel is still on the session list (so it remains discoverable during the callback). Add an assertion for the list size captured inside the callback.
AssertIntEQ(openFailCbCalls, 1);
AssertIntEQ(openConfCbCalls, 0);
AssertIntEQ(openRespCbChannel, selfChannelId);
AssertTrue(openRespCbCtx == &cbCtx);
AssertIntEQ(harness.ssh->channelListSz, 0);
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
561c9ab to
dbb834b
Compare
The confirmation and failure hooks reach an application only through DoChannelOpenConf() and DoChannelOpenFail(), and nothing in tests/ or apps/ registered either, so both arms shipped unexercised. - assert the confirm callback runs with the peer's channel id, window and packet size already recorded - assert the failure callback runs while the channel is still findable, before DoChannelOpenFail() removes it - count each hook separately, so a test can tell which one ran - a rejecting confirm hook fails the receive and leaves the open unfinished; a rejecting failure hook fails it and leaves the channel on the list - seed the unconfirmed channel through ChannelNew() and ChannelAppend(), the state an outstanding open leaves behind
wolfSSH_CTX_SetChannelCloseCb() had no caller in tests/, examples/ or apps/, so nothing held DoChannelClose() to running the hook before it retires the channel. - assert the callback runs with the closing channel's id and the ctx set on the session - assert the channel is still on the list inside the callback and gone by the time the caller is told - a rejecting callback changes nothing: the return is discarded and the close completes
Only the shell hook had coverage, so nothing held DoChannelRequest() to handing the command and session type to the exec and subsystem callbacks or to answering with what they return. - assert each callback sees the session type and the command string the request carried, and the ctx set on the session - assert an accepting callback draws CHANNEL_SUCCESS and a rejecting one CHANNEL_FAILURE, whether it rejects with a WS_ error or a bare nonzero
dbb834b to
37573ce
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1227
Scan targets checked: wolfssh-bugs, wolfssh-src
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
The channel open confirm and fail hooks, the close hook, and the exec and
subsystem request hooks had no caller in tests/, examples/ or apps/, so
each shipped unexercised.
packet size are recorded, the channel is still findable in the fail
arm, and a rejecting hook fails the receive.
rejecting one changing nothing.
command string, and their answer drawing CHANNEL_SUCCESS or
CHANNEL_FAILURE.