Fix SendUserAuthKeyboardResponse() and add regress tests

[yosuke-wolfssl]

This PR introduces 2 changes:

• Fix the SendUserAuthKeyboardResponse() in src/internal.c so that this validates invalid ssh or missing userAuthCb. And also this adds the success check of PreparePacket().
• Add the regress test case for SendUserAuthKeyboardResponse()

[Copilot]

Pull request overview

Fixes robustness issues in keyboard-interactive auth response sending and adds regression coverage to prevent crashes/state corruption.

Changes:

• Add ssh/ctx/userAuthCb validation to SendUserAuthKeyboardResponse() and gate buffer writes on PreparePacket() success.
• Add regression tests covering PreparePacket() failure and missing user-auth callback behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/internal.c	Makes SendUserAuthKeyboardResponse() safer by validating inputs/callback and only building the packet when packet preparation succeeds.
tests/regress.c	Adds regression tests to exercise error paths in SendUserAuthKeyboardResponse().

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[dgarske]

🐺 Skoll Code Review

Overall recommendation: APPROVE
Findings: 3 total — 3 posted, 0 skipped

Posted findings

• [Low] authData not zero-initialized unlike peer function SendUserAuthRequest — src/internal.c:15427
• [Low] authRet initialized to WS_SUCCESS conflates two error-code domains — src/internal.c:15422
• [Info] Redundant post-call cleanup in test TestKeyboardResponsePreparePacketFailure — tests/regress.c:641-643

Review generated by Skoll via openclaw

[dgarske]

🔵 [Low] authData not zero-initialized unlike peer function SendUserAuthRequest
💡 SUGGEST convention

The stack-local WS_UserAuthData authData; is declared but not zero-initialized. On early-error paths (e.g., WS_BAD_ARGUMENT when ssh == NULL), the struct contains indeterminate stack values until ForceZero at line 15525. While currently safe (no code reads authData on those paths), the peer function SendUserAuthRequest at line 15561 unconditionally calls WMEMSET(&authData, 0, sizeof(authData)); right after declaration. Adding the same pattern here would be more defensive and consistent.

Suggestion:

Suggested change

	word32 prompt;
	WS_UserAuthData authData;
	WLOG(WS_LOG_DEBUG, "Entering SendUserAuthKeyboardResponse()");
	WMEMSET(&authData, 0, sizeof(authData));

[yosuke-wolfssl]

Fixed

[dgarske]

🔵 [Low] authRet initialized to WS_SUCCESS conflates two error-code domains
💡 SUGGEST style

authRet is initialized to WS_SUCCESS (value 0, from wolfssh/error.h) but is later compared against WOLFSSH_USERAUTH_SUCCESS (value 0, from the WS_UserAuthResults enum in wolfssh/ssh.h). These two constants are in different semantic domains and only happen to share the same value today. The code is currently safe because authRet is only checked inside if (ret == WS_SUCCESS), which guarantees the callback was called. However, initializing to a failure sentinel from the correct domain (e.g., WOLFSSH_USERAUTH_FAILURE) would be more self-documenting and robust against future refactoring.

Suggestion:

Suggested change

	int authRet = WS_SUCCESS;
	int authRet = WOLFSSH_USERAUTH_FAILURE;
	int ret = WS_SUCCESS;

[yosuke-wolfssl]

Fixed

[dgarske]

⚪ [Info] Redundant post-call cleanup in test TestKeyboardResponsePreparePacketFailure
🔧 NIT test

After calling SendUserAuthKeyboardResponse(ssh), the test manually sets ssh->kbAuth.promptCount = 0, ssh->kbAuth.prompts = NULL, and ssh->kbAuth.promptEcho = NULL. However, the new cleanup block in SendUserAuthKeyboardResponse (lines 15488-15494) already NULLs these fields and sets promptCount to 0. The test assignments are redundant. This isn't a bug — it's just dead code in the test that might confuse future readers into thinking the function doesn't clean up after itself.

Suggestion:

Suggested change

	/* Ownership was transferred and freed by SendUserAuthKeyboardResponse(). */
	/* Ownership was transferred, freed, and NULLed by
	* SendUserAuthKeyboardResponse(). Verify cleanup. */
	AssertNull(ssh->kbAuth.prompts);
	AssertNull(ssh->kbAuth.promptEcho);
	AssertIntEQ(ssh->kbAuth.promptCount, 0);

[yosuke-wolfssl]

Fixed

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Setting ssh->outputBuffer.buffer = NULL introduces a risk of a null dereference inside PreparePacket() (or any helper it calls) if those functions assume the buffer pointer is valid even when they are about to return an error. To keep the regression deterministic and memory-safe, prefer forcing WS_OVERFLOW_E using only the idx/length invariant (or by using a deliberately too-small but non-NULL buffer, if needed) without nulling the buffer pointer.

[Copilot]

The PR description mentions validating an invalid ssh (or missing ctx), but the added regressions only cover missing userAuthCb and PreparePacket() failure. Add a small regression for SendUserAuthKeyboardResponse(NULL) (and/or a case with ssh->ctx == NULL if constructible in the harness) to lock in the new behavior and prevent future regressions.