From a1eaa933577b6a3c15fd412ebc895f19a8b94ac1 Mon Sep 17 00:00:00 2001 From: TapTap Date: Sun, 13 Sep 2026 12:57:26 +0200 Subject: [PATCH] fix(client): route explicit remote dry-run targets, fail closed on stray status --dry-run --server-host=H (or TLS / source-bind --address) silently ran the client-side manifest even though a real run contacts the server. Add a client-only, never-serialized server_host_set bit (alongside the existing server_port_set) and extend dry_run_targets_server so every explicit remote target contacts the receiver. Also make incremental_check return the dry-run code (4) only when the session actually requested dry-run; a stray STATUS_DRY_RUN_TRANSFER from a hostile/buggy peer is now a logged protocol error (STATUS_ERROR) instead of falling through to send file data and desync. Both normal send_single_file callers handle rc == 4 explicitly as an abort. --- src/client/client_cli.c | 5 ++++ src/client/client_send.c | 52 ++++++++++++++++++++++++++++++++-------- src/shared/config.c | 1 + src/shared/config.h | 6 +++++ 4 files changed, 54 insertions(+), 10 deletions(-) diff --git a/src/client/client_cli.c b/src/client/client_cli.c index dd5f2ad..4014e8b 100644 --- a/src/client/client_cli.c +++ b/src/client/client_cli.c @@ -1106,6 +1106,11 @@ static bool cli_handle_table_option(CliParseCtx* ctx) { } config->use_metadata = true; } + /* Remember that --server-host was explicitly given (the field itself + defaults to 127.0.0.1, so a value check cannot distinguish it). Used + by --dry-run to route an explicit remote target to the server. */ + if (entry->offset == offsetof(Config, server_host)) + config->server_host_set = true; } } else if (apply_table_option(config, entry, NULL) != 0) { ctx->exit_code = -1; diff --git a/src/client/client_send.c b/src/client/client_send.c index 9168ed1..b4779fc 100644 --- a/src/client/client_send.c +++ b/src/client/client_send.c @@ -482,10 +482,11 @@ static void disconnect_transfer_client(Client* client) { } /* True when --dry-run should contact a receiver rather than running the - * client-side local manifest. A remote (SSH host:path), daemon - * (host::module/path), or an explicit --server-port/--port selects the - * server-contacting path; a plain local destination keeps the original - * client-side behavior (which never dials the default 127.0.0.1:8080). */ + * client-side local manifest. Any target a real run would reach over the wire + * selects the server-contacting path: a remote (SSH host:path), a daemon + * (host::module/path), an explicit --server-host, --server-port/--port, TLS, or + * a source-bind --address. A plain local destination (none of these) keeps the + * original client-side behavior, which never dials the default 127.0.0.1:8080. */ static bool dry_run_targets_server(const Config* config) { if (!config) return false; @@ -493,7 +494,13 @@ static bool dry_run_targets_server(const Config* config) { return true; if (config->module && config->module[0] != '\0') return true; - return config->server_port_set; + if (config->server_host_set || config->server_port_set) + return true; + if (config->use_tls) + return true; + if (config->address != NULL) + return true; + return false; } static bool add_chunk_to_manifest(ArrayList* manifest, const Chunk* chunk) { @@ -1040,11 +1047,18 @@ static int incremental_check(Client* client, File* file, const Config* config, return 3; } /* Server-contacting --dry-run: the receiver decided the file is not up to - date and answered "would transfer" WITHOUT expecting any data. The caller - only uses this in the dry-run path; a non-dry-run sender never receives it - because the receiver only emits it when the wire config sets dry_run. */ - if (s == STATUS_DRY_RUN_TRANSFER) - return 4; + date and answered "would transfer" WITHOUT expecting any data. Treat it as + the dry-run code ONLY when this session actually requested dry-run. A + hostile/buggy peer that emits it outside dry-run is a protocol error: fail + closed (and send STATUS_ERROR) rather than fall through to the normal path, + which would transmit file data the receiver is not reading and desync. */ + if (s == STATUS_DRY_RUN_TRANSFER) { + if (config->dry_run) + return 4; + log_message(LOG_LEVEL_ERROR, "Unexpected DRY_RUN_TRANSFER status outside a --dry-run session"); + send_status(client->file_descriptor, STATUS_ERROR); + return -1; + } if (s != STATUS_NEXT) { log_message(LOG_LEVEL_ERROR, "Unexpected server status"); send_status(client->file_descriptor, STATUS_ERROR); @@ -1478,6 +1492,16 @@ static int send_single_file(Client* client, File* file, Config* config, bool use } return arc == 0 ? 0 : -1; } + // rc == 4: the receiver answered DRY_RUN_TRANSFER, which is only valid in + // incremental_check's dedicated dry-run consumer. send_single_file never + // runs a dry-run session, so this is a protocol error: abort instead of + // falling through and sending data the receiver is not reading. + if (rc == 4) { + log_message(LOG_LEVEL_ERROR, "Receiver answered DRY_RUN_TRANSFER in a non-dry-run transfer"); + delta_signature_destroy(sig); + send_status(client->file_descriptor, STATUS_ERROR); + return -1; + } // rc == 0: unchanged file, skip // rc == 2: server sent delta signature but sendfile doesn't support delta delta_signature_destroy(sig); @@ -1519,6 +1543,14 @@ static int send_single_file(Client* client, File* file, Config* config, bool use } return arc == 0 ? 0 : -1; } + if (rc == 4) { + /* See the sendfile branch above: DRY_RUN_TRANSFER is only valid in the + dedicated dry-run consumer, never in the normal per-file send path. */ + log_message(LOG_LEVEL_ERROR, "Receiver answered DRY_RUN_TRANSFER in a non-dry-run transfer"); + delta_signature_destroy(sig); + send_status(client->file_descriptor, STATUS_ERROR); + return -1; + } if (rc == 2 && config->use_delta && !config->whole_file) { int drc = send_delta(client, file, sig, config); delta_signature_destroy(sig); diff --git a/src/shared/config.c b/src/shared/config.c index 3f9d3a2..9e83e10 100644 --- a/src/shared/config.c +++ b/src/shared/config.c @@ -42,6 +42,7 @@ static void config_set_defaults(Config* config) { config->server_host = str_dup("127.0.0.1"); config->server_port = 8080; config->server_port_set = false; + config->server_host_set = false; /* 0 means "--timeout not given": the transport keeps its own built-in 30 s * socket timeout (tcp_set_timeouts ignores non-positive values) and the * protocol layer keeps its built-in 60 s per-message deadline. A positive diff --git a/src/shared/config.h b/src/shared/config.h index 6180598..890fd10 100644 --- a/src/shared/config.h +++ b/src/shared/config.h @@ -292,6 +292,12 @@ typedef struct Config { * existing client-side dry-run behavior instead of dialing the default * 127.0.0.1:8080. */ bool server_port_set; + /* True when --server-host was explicitly given. CLIENT-ONLY (never + * serialized), and distinct from the "127.0.0.1" default: --dry-run uses it + * to route an explicit remote target to the server so it reports receiver + * state exactly like a real run, instead of silently running the client-side + * manifest. */ + bool server_host_set; char* tls_cert; char* tls_key; char* tls_ca;