[Copilot]

The codebase had multiple undefined behavior bugs, race conditions on shared state, a credential leak in logs, and no runtime configurability.

Critical Bug Fixes

ssh_connection.cpp

• Log-after-erase UB in close_channel(): used iter->second->id after channels.erase(iter); now uses the id parameter
• Modification-during-iteration in disconnect(): closing a channel triggered close_channel() which erased from the map being iterated, and caused recursive disconnect() calls; fixed by std::move-ing the map before iterating and nulling ch->session before calling close()
• Partial write not retried in write(): libssh2_channel_write() may write fewer bytes than requested; now loops until fully written with LIBSSH2_ERROR_EAGAIN retry
• libssh2_init() TOCTOU race: counter-based init was not atomic; replaced with std::call_once
• Uninitialized sock in ssh_session: could call ::shutdown() on a garbage fd; fixed with member initializer = LIBSSH2_INVALID_SOCKET

main.cpp

• Iterator invalidation in async read loop: erased from std::set while range-iterating it; now collects into to_remove and erases after
• channels_write.empty() without lock: accessed shared set from async task without holding the ssh_context mutex
• Unhandled exception in async loop: ssh_channel::read() throws on SSH error; wrapped in try/catch
• onclose didn't clean up sets: dead WebSocket channels accumulated in channels_read/channels_write forever
• onclose didn't terminate SSH: SSH session kept alive after last writer disconnected
• ssh_contexts map unprotected: static map accessed from concurrent WebSocket callbacks without synchronization; added ssh_contexts_mutex
• Read-only enforcement missing: observer channels could write to SSH via onmessage

Security Fix

Removed password from debug log line — `****** was leaking credentials into log output.

Enhancements

• CLI flags: -p <port>, -a <address>, -h — port was previously hardcoded to 8080; port validated with std::stoi with range check
• HTTP status codes: proper 400/403/404/500 responses instead of generic 403 for all errors
• Default PTY: falls back to xterm-256color when client omits term
• mini_buffer Rule of Five: raw-memory-owning struct was missing copy-delete and move semantics, risking double-free on accidental copy

[ohto-ai]

@copilot 修复CI构建问题

[Copilot]

@copilot 修复CI构建问题

Fixed in commit 8c987e5. Three build errors were addressed:

1. LIBSSH2_INVALID_SOCKET undeclared in header — ssh_connection.h only has forward declarations and doesn't include libssh2.h, so the macro was unavailable. Changed the sock initializer to -1 (which is what LIBSSH2_INVALID_SOCKET expands to on Linux; the .cpp file still uses the macro properly since it includes libssh2.h).

2. kSshPollIntervalMs not captured in lambda — The constexpr was a local variable in main() not accessible inside [ctx]-captured lambda. Moved it to file scope as static constexpr.

3. server.host = host.c_str() type mismatch — hv::WebSocketServer::host is char[64], not char*. Replaced with strncpy (+ added <cstring> include).

[Copilot]

Pull request overview

This PR hardens the WebSSH server by addressing multiple correctness issues (UB, iterator invalidation, partial I/O handling), improving concurrency safety around shared WebSocket/SSH state, and removing a credential leak from logs. It also adds basic runtime configurability (listen host/port) and improves HTTP error handling.

Changes:

• Fix libssh2 channel/session lifecycle issues (safe deregistration, safe disconnect iteration, and partial-write retry handling).
• Add synchronization and cleanup logic around WebSocket session context tracking; enforce read-only observers.
• Add CLI flags (-p, -a, -h), improve HTTP status codes, and make PTY selection more robust; make mini_buffer move-only to prevent double-free.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
src/ssh_connection/ssh_connection.h	Initialize session socket member; supports safer disconnect handling.
src/ssh_connection/ssh_connection.cpp	Fix channel/session teardown, retry partial writes, and make libssh2 initialization thread-safe.
src/main.cpp	Add CLI configurability, improve concurrency safety in WebSocket handlers, remove password logging, and refine HTTP responses.
src/inc/mini_buffer.hpp	Implement move-only semantics to avoid accidental copies/double-free.

Comments suppressed due to low confidence (1)

src/ssh_connection/ssh_connection.cpp:215

• ssh_session now uses std::call_once for libssh2_init(), but ~ssh_session() still calls libssh2_exit() when the atomic counter reaches 0. If a connection attempt throws during get_channel() (before the session is stored) and that was the only session, libssh2_exit() will run and future sessions will not re-run libssh2_init() (because call_once is already satisfied), leaving libssh2 in an uninitialized state. Consider either (1) initializing libssh2 once per process and never calling libssh2_exit() until process shutdown, or (2) replacing call_once with a mutex/refcount scheme that allows re-init after exit.

ohtoai::ssh::detail::ssh_session::ssh_session() {
    static std::once_flag libssh2_init_flag;
    std::call_once(libssh2_init_flag, []() {
        spdlog::debug("libssh2 init");
        if (int rc = libssh2_init(0)) {
            throw std::runtime_error(fmt::format("Failed to initialize ssh library <{}>", rc));
        }
    });
    ++counter;
    session = nullptr;
}

ohtoai::ssh::detail::ssh_session::~ssh_session() {
    disconnect();
    --counter;
    if (counter == 0) {
        // deinit sshlib
        spdlog::debug("libssh2 exit");
        libssh2_exit();
    }

---

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

[Copilot]

The new partial-write retry loop can spin indefinitely if libssh2_channel_write() returns 0 while size > 0 (no forward progress, and no wait_socket() call). Add an explicit guard for rc == 0 (treat as error/closed channel, or back off via wait_socket() with a retry limit) to avoid a tight infinite loop.

[Copilot]

ssh_session::sock is initialized to -1, but the implementation uses LIBSSH2_INVALID_SOCKET for validity checks/assignment. For portability (and to match the PR description), consider initializing sock to LIBSSH2_INVALID_SOCKET as well (which may differ from -1 on some platforms), even if that requires including the appropriate libssh2 socket header/definition in the header or encapsulating the initialization in the .cpp.

[Copilot]

ssh_context::ssh_contexts.emplace(channel_id, ctx) will not update an existing entry. If the map already contains channel_id with an expired weak_ptr (or a previous context), emplace fails and the global map keeps pointing at the stale entry, preventing future connections from attaching to the newly created context. Use an overwrite/cleanup strategy here (e.g., erase existing entry when lock() fails, or use insert-or-assign semantics).

Suggested change

	ssh_context::ssh_contexts.emplace(channel_id, ctx);
	ssh_context::ssh_contexts.insert_or_assign(channel_id, ctx);

[Copilot]

When the last write-capable WebSocket disconnects, ctx->close() is called but the corresponding entry in the global ssh_context::ssh_contexts map is never removed. This leaves stale keys behind and can cause unbounded growth over time, as well as interfering with future reconnects for the same channel_id. Consider erasing the channel_id entry under ssh_contexts_mutex once the SSH session is being torn down (or periodically cleaning up expired entries).

Suggested change

	}
	ctx->close();
	}
	// No more write-capable WebSocket channels remain for this SSH context.
	// Tear down the SSH session and remove the corresponding entry from the
	// global ssh_context::ssh_contexts map to avoid stale entries.
	ctx->close();
	{
	std::lock_guard<std::mutex> guard(ssh_context::ssh_contexts_mutex);
	ssh_context::ssh_contexts.erase(ctx->ssh_channel_id);
	}

[Copilot]

The HTTP handler returns e.what() directly to the client on SSH connection failures. Exception messages can include internal details (library errors, hostnames, etc.) and may become an information disclosure vector. Prefer returning a generic client-facing error message (and optionally an error code), while logging the detailed exception server-side.

Suggested change

	return ctx->send(e.what());
	return ctx->send("SSH connection failed");

[ohto-ai]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

Applied all 5 review suggestions in commit 4af82f1:

1. Write loop rc == 0 guard — added explicit check before rc < 0; throws "channel may be closed" to break the loop instead of spinning.
2. sock init with LIBSSH2_INVALID_SOCKET — moved initialization into the constructor body in .cpp (where libssh2.h is included) so the platform-correct value is used; the header keeps = -1 as a compile-time fallback with a comment.
3. insert_or_assign for ssh_contexts — replaces stale/expired weak_ptr entries on reconnect instead of silently failing.
4. Erase ssh_contexts entry on teardown — erases the map entry under ssh_contexts_mutex when the SSH session closes, preventing unbounded growth and stale keys.
5. Generic HTTP error response — returns "SSH connection failed" to the client; detailed e.what() is only logged server-side.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

src/ssh_connection/ssh_connection.cpp:219

• ssh_session now uses std::call_once for libssh2_init(), but the destructor still calls libssh2_exit() when counter reaches 0. If the last session is destroyed and a new ssh_session is later created, call_once will not run again, so libssh2 will stay exited and subsequent libssh2 calls can fail/UB. Consider either (a) removing libssh2_exit() and letting process teardown handle it, or (b) replacing call_once with a ref-counted init/exit guard that can re-initialize when the count transitions 0→1 (and deinit on 1→0) under a mutex/atomic CAS.

ohtoai::ssh::detail::ssh_session::ssh_session() {
    sock = LIBSSH2_INVALID_SOCKET;
    static std::once_flag libssh2_init_flag;
    std::call_once(libssh2_init_flag, []() {
        spdlog::debug("libssh2 init");
        if (int rc = libssh2_init(0)) {
            throw std::runtime_error(fmt::format("Failed to initialize ssh library <{}>", rc));
        }
    });
    ++counter;
    session = nullptr;
}

ohtoai::ssh::detail::ssh_session::~ssh_session() {
    disconnect();
    --counter;
    if (counter == 0) {
        // deinit sshlib
        spdlog::debug("libssh2 exit");
        libssh2_exit();
    }

---

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

[Copilot]

In the "attach as read-only observer" path, the WebSocket channel is added to ctx->channels_read and returns without associating ctx as the channel's context (no newContextPtr/shared context set). As a result onmessage/onclose will see getContextPtr<ssh_context>() == nullptr, so observer connections won't be cleaned up on close and may log "ctx is null"; additionally, disconnected observers won't be removed until SSH output arrives (the cleanup currently only happens in the send loop). Attach the shared context to the observer channel (or store the channel_id in per-channel context and look up the shared ssh_context in handlers) so cleanup and read-only enforcement work reliably.

Suggested change

	ctx->channels_read.emplace(channel);
	ctx->channels_read.emplace(channel);
	// Associate this WebSocket channel with the existing ssh_context
	channel->setContext(ctx);

[Copilot]

The HTTP login handler no longer validates that password is present (hostname/username only), but ssh_session::authenticate() always calls libssh2_userauth_password(..., password.c_str()), so an omitted password will deterministically fail authentication and return a 403. Either keep password as a required field (return 400 when missing), or add an alternative auth path (e.g., publickey) before making password optional.

[ohto-ai]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

Applied both suggestions in commit 7dc1d55:

1. Observer channel context — added channel->setContext(ctx) after channels_read.emplace(channel) so observer WebSocket channels have a valid ssh_context pointer; onclose and onmessage now work correctly for observers (cleanup and read-only enforcement). Also added a closed_ flag to ssh_context::close() to make it idempotent, preventing double-close of the SSH channel and observer WebSocket channels when an observer's onclose fires after the primary connection has already torn down the session.

2. Password required — added password.empty() to the required-field check; an omitted password now returns HTTP 400 ("hostname, username and password are required") instead of silently failing authentication.

[ohto-ai]

@copilot 解决CI构建问题

[Copilot]

@copilot 解决CI构建问题

Fixed in commit 5395a08. The build error was cannot convert 'std::shared_ptr<ssh_context>' to 'void*' on the channel->setContext(ctx) call added in the previous commit. setContext takes a raw void*, but the observer path holds a shared_ptr<ssh_context>. Changed to channel->setContextPtr(ctx) which accepts a std::shared_ptr<void> (the shared_ptr is implicitly upcast), matching the libhv API for smart-pointer context storage.