a73x

753309fe

Guard concurrent uploads and abort oversize streams early

a73x   2026-08-08 16:07

Commit message
Guard concurrent uploads and abort oversize streams early
docs/superpowers/plans/2026-08-08-release-packages.md
Old New
@@ -1879,6 +1879,9 @@ git commit -m "Serve release listings and downloads over HTTP"
1879 1879
1880 ### Task 8: CLI `git-collab release` commands 1880 ### Task 8: CLI `git-collab release` commands
1881 1881
1882 **Execution deviations:**
1883 - `ssh_command`: dropped the plan's `.to_string()` on `parts.next().unwrap_or("ssh")` when building `Command::new(...)` — clippy's `unnecessary_to_owned` flagged it as a new warning; `Command::new` already accepts `&str`.
1884
1882 **Files:** 1885 **Files:**
1883 - Modify: `src/release.rs` (remote parsing + command execution) 1886 - Modify: `src/release.rs` (remote parsing + command execution)
1884 - Modify: `src/cli.rs` (subcommand) 1887 - Modify: `src/cli.rs` (subcommand)
src/release.rs
Old New
@@ -33,6 +33,9 @@ pub struct SshRemote {
33 } 33 }
34 34
35 /// Parse `ssh://[user@]host[:port]/path` or scp-style `[user@]host:path`. 35 /// Parse `ssh://[user@]host[:port]/path` or scp-style `[user@]host:path`.
36 ///
37 /// Note: IPv6 bracket addresses (`ssh://[::1]:2222/path`) are not specially
38 /// handled and will mis-parse rather than fail closed — out of scope per spec.
36 pub fn parse_ssh_remote(url: &str) -> Option<SshRemote> { 39 pub fn parse_ssh_remote(url: &str) -> Option<SshRemote> {
37 if let Some(rest) = url.strip_prefix("ssh://") { 40 if let Some(rest) = url.strip_prefix("ssh://") {
38 let (authority, path) = rest.split_once('/')?; 41 let (authority, path) = rest.split_once('/')?;
@@ -82,16 +85,34 @@ fn ssh_remote(repo: &Repository, remote_name: &str) -> Result<SshRemote, Error>
82 let url = remote 85 let url = remote
83 .url() 86 .url()
84 .ok_or_else(|| Error::Cmd(format!("remote '{}' has no URL", remote_name)))?; 87 .ok_or_else(|| Error::Cmd(format!("remote '{}' has no URL", remote_name)))?;
85 parse_ssh_remote(url).ok_or_else(|| { 88 let parsed = parse_ssh_remote(url).ok_or_else(|| {
86 Error::Cmd(format!( 89 Error::Cmd(format!(
87 "remote '{}' ({}) is not an SSH remote — releases need an ssh:// or user@host: remote", 90 "remote '{}' ({}) is not an SSH remote — releases need an ssh:// or user@host: remote",
88 remote_name, url 91 remote_name, url
89 )) 92 ))
90 }) 93 })?;
94 validate_remote_path(&parsed.path)?;
95 Ok(parsed)
96 }
97
98 /// Reject a remote path containing quote characters: our remote command strings
99 /// wrap arguments in single quotes with no escaping, so a quote in the path
100 /// would let it break out of its argument — turn that into a clear client-side
101 /// error instead of an opaque server-side rejection (or worse).
102 fn validate_remote_path(path: &str) -> Result<(), Error> {
103 if path.contains('\'') || path.contains('"') {
104 return Err(Error::Cmd(format!(
105 "remote path contains quotes, which release commands cannot escape: {}",
106 path
107 )));
108 }
109 Ok(())
91 } 110 }
92 111
93 /// Build the ssh invocation for a remote, honoring GIT_COLLAB_SSH_COMMAND 112 /// Build the ssh invocation for a remote, honoring GIT_COLLAB_SSH_COMMAND
94 /// (like git's GIT_SSH_COMMAND: extra words become leading arguments). 113 /// (like git's GIT_SSH_COMMAND: extra words become leading arguments). Unlike
114 /// git's GIT_SSH_COMMAND, which is run through `sh -c` and supports shell
115 /// quoting, this is split on whitespace only — no quoting support.
95 fn ssh_command(remote: &SshRemote) -> Command { 116 fn ssh_command(remote: &SshRemote) -> Command {
96 let base = std::env::var("GIT_COLLAB_SSH_COMMAND").unwrap_or_else(|_| "ssh".to_string()); 117 let base = std::env::var("GIT_COLLAB_SSH_COMMAND").unwrap_or_else(|_| "ssh".to_string());
97 let mut parts = base.split_whitespace(); 118 let mut parts = base.split_whitespace();
@@ -116,16 +137,23 @@ fn run_remote(remote: &SshRemote, remote_cmd: &str, stdin: Stdio) -> Result<Outp
116 .output() 137 .output()
117 .map_err(|e| Error::Cmd(format!("failed to run ssh: {}", e)))?; 138 .map_err(|e| Error::Cmd(format!("failed to run ssh: {}", e)))?;
118 if !output.status.success() { 139 if !output.status.success() {
119 let msg = format!( 140 // Protocol errors from our server always arrive on the exec channel's
120 "{}{}", 141 // stdout; stderr is ssh's own banners (e.g. host-key warnings). Never
121 String::from_utf8_lossy(&output.stdout).trim(), 142 // concatenate the two — that glues an unrelated banner onto the
122 String::from_utf8_lossy(&output.stderr).trim() 143 // message. Prefer stdout, fall back to stderr, then a generic message.
123 ); 144 let stdout = String::from_utf8_lossy(&output.stdout).trim().to_string();
124 return Err(Error::Cmd(if msg.is_empty() { 145 let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string();
125 "server rejected the command".to_string() 146 let msg = if !stdout.is_empty() {
147 stdout
148 } else if !stderr.is_empty() {
149 stderr
126 } else { 150 } else {
127 msg 151 "server rejected the command".to_string()
128 })); 152 };
153 // Strip a leading "error: " so callers (main.rs prints "error: {}")
154 // don't double it up.
155 let msg = msg.strip_prefix("error: ").unwrap_or(&msg).to_string();
156 return Err(Error::Cmd(msg));
129 } 157 }
130 Ok(output) 158 Ok(output)
131 } 159 }
@@ -150,6 +178,15 @@ pub fn publish(
150 if !validate_name(&filename) { 178 if !validate_name(&filename) {
151 return Err(Error::Cmd(format!("invalid filename: {}", filename))); 179 return Err(Error::Cmd(format!("invalid filename: {}", filename)));
152 } 180 }
181 // Directories open successfully on Linux; only reads fail. Without
182 // this check we would hand the server an EISDIR-failing handle and
183 // publish an empty artifact under a real name.
184 if !file.is_file() {
185 return Err(Error::Cmd(format!(
186 "not a regular file: {}",
187 file.display()
188 )));
189 }
153 let handle = std::fs::File::open(file) 190 let handle = std::fs::File::open(file)
154 .map_err(|e| Error::Cmd(format!("cannot open {}: {}", file.display(), e)))?; 191 .map_err(|e| Error::Cmd(format!("cannot open {}: {}", file.display(), e)))?;
155 let mut remote_cmd = format!( 192 let mut remote_cmd = format!(
@@ -206,6 +243,14 @@ pub fn delete(
206 version: &str, 243 version: &str,
207 filename: Option<&str>, 244 filename: Option<&str>,
208 ) -> Result<(), Error> { 245 ) -> Result<(), Error> {
246 if !validate_name(version) {
247 return Err(Error::Cmd(format!("invalid version name: {}", version)));
248 }
249 if let Some(name) = filename {
250 if !validate_name(name) {
251 return Err(Error::Cmd(format!("invalid filename: {}", name)));
252 }
253 }
209 let remote = ssh_remote(repo, remote_name)?; 254 let remote = ssh_remote(repo, remote_name)?;
210 let mut remote_cmd = format!("collab-release delete '{}' '{}'", remote.path, version); 255 let mut remote_cmd = format!("collab-release delete '{}' '{}'", remote.path, version);
211 if let Some(name) = filename { 256 if let Some(name) = filename {
@@ -224,6 +269,26 @@ mod tests {
224 use super::*; 269 use super::*;
225 270
226 #[test] 271 #[test]
272 fn publish_rejects_non_regular_file() {
273 // A directory opens fine on Linux; without an explicit check we would
274 // stream an EISDIR-failing handle to the server as an empty artifact.
275 let tmp = tempfile::TempDir::new().unwrap();
276 let repo = Repository::init(tmp.path()).unwrap();
277 repo.remote("origin", "ssh://git@example.invalid/repo.git")
278 .unwrap();
279
280 let dir_that_looks_like_a_file = tmp.path().join("payload.tar.gz");
281 std::fs::create_dir(&dir_that_looks_like_a_file).unwrap();
282
283 let err = publish(&repo, "origin", "v1", &[dir_that_looks_like_a_file], false).unwrap_err();
284 assert!(
285 err.to_string().contains("not a regular file"),
286 "got: {}",
287 err
288 );
289 }
290
291 #[test]
227 fn accepts_typical_names() { 292 fn accepts_typical_names() {
228 assert!(validate_name("v1.2.0")); 293 assert!(validate_name("v1.2.0"));
229 assert!(validate_name("app-x86_64.tar.gz")); 294 assert!(validate_name("app-x86_64.tar.gz"));
@@ -283,4 +348,43 @@ mod tests {
283 assert!(parse_ssh_remote("../relative/path").is_none()); 348 assert!(parse_ssh_remote("../relative/path").is_none());
284 assert!(parse_ssh_remote("file:///srv/git/repo.git").is_none()); 349 assert!(parse_ssh_remote("file:///srv/git/repo.git").is_none());
285 } 350 }
351
352 #[test]
353 fn parse_rejects_out_of_range_port() {
354 assert!(parse_ssh_remote("ssh://host:99999/x").is_none());
355 }
356
357 #[test]
358 fn parse_rejects_scp_style_with_empty_path() {
359 assert!(parse_ssh_remote("host:").is_none());
360 }
361
362 #[test]
363 fn parse_rejects_scp_style_with_empty_authority() {
364 assert!(parse_ssh_remote(":path").is_none());
365 }
366
367 #[test]
368 fn parse_rejects_ssh_url_without_path() {
369 assert!(parse_ssh_remote("ssh://host").is_none());
370 }
371
372 #[test]
373 fn rejects_remote_path_with_single_quote() {
374 let remote = parse_ssh_remote("ssh://git@example.com/evil'; rm -rf /.git").unwrap();
375 let err = validate_remote_path(&remote.path).unwrap_err();
376 assert!(matches!(err, Error::Cmd(ref m) if m.contains("quotes")));
377 }
378
379 #[test]
380 fn rejects_remote_path_with_double_quote() {
381 let remote = parse_ssh_remote("ssh://git@example.com/evil\"x").unwrap();
382 assert!(validate_remote_path(&remote.path).is_err());
383 }
384
385 #[test]
386 fn accepts_remote_path_without_quotes() {
387 let remote = parse_ssh_remote("ssh://git@example.com/myrepo.git").unwrap();
388 assert!(validate_remote_path(&remote.path).is_ok());
389 }
286 } 390 }
src/server/releases.rs
Old New
@@ -14,6 +14,12 @@
14 //! temp-file-then-rename, so a reader (e.g. an HTTP download) never 14 //! temp-file-then-rename, so a reader (e.g. an HTTP download) never
15 //! observes a partially-written or truncated file for either — only a 15 //! observes a partially-written or truncated file for either — only a
16 //! fully-old or fully-new version, whichever `finish()` happened to win. 16 //! fully-old or fully-new version, whichever `finish()` happened to win.
17 //!
18 //! Crash note: uploads stream into a hidden `.tmp` file inside the version
19 //! directory, removed on drop. If the process dies mid-upload that file
20 //! survives. It is never listed or downloadable (listing skips dotfiles),
21 //! but it does pin an otherwise-empty version directory into existence.
22 //! There is no reaper — such leftovers need manual cleanup.
17 23
18 use std::io::Write; 24 use std::io::Write;
19 use std::path::{Path, PathBuf}; 25 use std::path::{Path, PathBuf};
@@ -78,11 +84,24 @@ impl ReleaseUpload {
78 if !validate_name(filename) { 84 if !validate_name(filename) {
79 return Err(ReleaseError::InvalidName(filename.to_string())); 85 return Err(ReleaseError::InvalidName(filename.to_string()));
80 } 86 }
87 // `<name>.sha256` is this store's checksum companion namespace.
88 // Accepting an upload with that suffix would let a --force upload of
89 // `app.tar.gz.sha256` overwrite the real companion of `app.tar.gz`
90 // with arbitrary content.
91 if filename.ends_with(".sha256") {
92 return Err(ReleaseError::InvalidName(format!(
93 "{}: .sha256 names are reserved for checksums",
94 filename
95 )));
96 }
81 let version_dir = releases_dir.join(version); 97 let version_dir = releases_dir.join(version);
82 std::fs::create_dir_all(&version_dir)?; 98 std::fs::create_dir_all(&version_dir)?;
83 let dest = version_dir.join(filename); 99 let dest = version_dir.join(filename);
84 if dest.exists() && !force { 100 if dest.exists() && !force {
85 return Err(ReleaseError::AlreadyExists(format!("{}/{}", version, filename))); 101 return Err(ReleaseError::AlreadyExists(format!(
102 "{}/{}",
103 version, filename
104 )));
86 } 105 }
87 let temp = NamedTempFile::new_in(&version_dir)?; 106 let temp = NamedTempFile::new_in(&version_dir)?;
88 let sha_dest = version_dir.join(format!("{}.sha256", filename)); 107 let sha_dest = version_dir.join(format!("{}.sha256", filename));
@@ -215,7 +234,9 @@ pub fn list_releases(releases_dir: &Path) -> Result<ReleaseIndex, ReleaseError>
215 let read_dir = match std::fs::read_dir(releases_dir) { 234 let read_dir = match std::fs::read_dir(releases_dir) {
216 Ok(rd) => rd, 235 Ok(rd) => rd,
217 Err(e) if e.kind() == std::io::ErrorKind::NotFound => { 236 Err(e) if e.kind() == std::io::ErrorKind::NotFound => {
218 return Ok(ReleaseIndex { versions: Vec::new() }) 237 return Ok(ReleaseIndex {
238 versions: Vec::new(),
239 })
219 } 240 }
220 Err(e) => return Err(e.into()), 241 Err(e) => return Err(e.into()),
221 }; 242 };
@@ -243,13 +264,16 @@ pub fn list_releases(releases_dir: &Path) -> Result<ReleaseIndex, ReleaseError>
243 if name.starts_with('.') || name.ends_with(".sha256") || !file_entry.path().is_file() { 264 if name.starts_with('.') || name.ends_with(".sha256") || !file_entry.path().is_file() {
244 continue; 265 continue;
245 } 266 }
267 // Never advertise a name `artifact_path` would refuse to resolve:
268 // a listing entry that 404s on download is worse than no entry.
269 if !validate_name(&name) {
270 continue;
271 }
246 let size = file_entry.metadata()?.len(); 272 let size = file_entry.metadata()?.len();
247 let sha256 = std::fs::read_to_string( 273 let sha256 = std::fs::read_to_string(entry.path().join(format!("{}.sha256", name)))
248 entry.path().join(format!("{}.sha256", name)), 274 .ok()
249 ) 275 .and_then(|s| s.split_whitespace().next().map(|t| t.to_string()))
250 .ok() 276 .unwrap_or_default();
251 .and_then(|s| s.split_whitespace().next().map(|t| t.to_string()))
252 .unwrap_or_default();
253 files.push(ReleaseFile { name, size, sha256 }); 277 files.push(ReleaseFile { name, size, sha256 });
254 } 278 }
255 files.sort_by(|a, b| a.name.cmp(&b.name)); 279 files.sort_by(|a, b| a.name.cmp(&b.name));
@@ -259,7 +283,14 @@ pub fn list_releases(releases_dir: &Path) -> Result<ReleaseIndex, ReleaseError>
259 if files.is_empty() { 283 if files.is_empty() {
260 continue; 284 continue;
261 } 285 }
262 versions.push((mtime, ReleaseVersion { version, published, files })); 286 versions.push((
287 mtime,
288 ReleaseVersion {
289 version,
290 published,
291 files,
292 },
293 ));
263 } 294 }
264 295
265 versions.sort_by(|a, b| b.0.cmp(&a.0).then_with(|| b.1.version.cmp(&a.1.version))); 296 versions.sort_by(|a, b| b.0.cmp(&a.0).then_with(|| b.1.version.cmp(&a.1.version)));
@@ -332,13 +363,80 @@ mod tests {
332 use super::*; 363 use super::*;
333 use tempfile::TempDir; 364 use tempfile::TempDir;
334 365
335 fn upload(dir: &Path, version: &str, name: &str, content: &[u8], force: bool) -> Result<String, ReleaseError> { 366 fn upload(
367 dir: &Path,
368 version: &str,
369 name: &str,
370 content: &[u8],
371 force: bool,
372 ) -> Result<String, ReleaseError> {
336 let mut up = ReleaseUpload::begin(dir, version, name, force, DEFAULT_MAX_RELEASE_SIZE)?; 373 let mut up = ReleaseUpload::begin(dir, version, name, force, DEFAULT_MAX_RELEASE_SIZE)?;
337 up.write(content)?; 374 up.write(content)?;
338 up.finish() 375 up.finish()
339 } 376 }
340 377
341 #[test] 378 #[test]
379 fn begin_rejects_sha256_filename() {
380 // `.sha256` is our checksum companion namespace. Allowing an upload
381 // named `app.tar.gz.sha256` would let a force-upload overwrite the
382 // real companion of `app.tar.gz` with attacker-chosen content.
383 let tmp = TempDir::new().unwrap();
384 // ReleaseUpload isn't Debug, so match rather than unwrap_err().
385 let err = match ReleaseUpload::begin(
386 tmp.path(),
387 "v1",
388 "app.tar.gz.sha256",
389 false,
390 DEFAULT_MAX_RELEASE_SIZE,
391 ) {
392 Ok(_) => panic!("expected a .sha256 filename to be rejected"),
393 Err(e) => e,
394 };
395 assert!(
396 matches!(err, ReleaseError::InvalidName(_)),
397 "expected InvalidName, got {:?}",
398 err
399 );
400 assert!(err.to_string().contains("reserved"), "got: {}", err);
401
402 // Even with --force, which is the dangerous direction.
403 assert!(ReleaseUpload::begin(
404 tmp.path(),
405 "v1",
406 "app.tar.gz.sha256",
407 true,
408 DEFAULT_MAX_RELEASE_SIZE
409 )
410 .is_err());
411 }
412
413 #[test]
414 fn list_skips_files_failing_validate_name() {
415 // A name the listing advertises must be one `artifact_path` will
416 // resolve, or the UI links to a guaranteed 404.
417 let tmp = TempDir::new().unwrap();
418 upload(tmp.path(), "v1", "good.tar.gz", b"ok", false).unwrap();
419 let version_dir = tmp.path().join("v1");
420 std::fs::write(version_dir.join("-leading-dash.tar.gz"), b"x").unwrap();
421 std::fs::write(version_dir.join("has space.tar.gz"), b"x").unwrap();
422
423 let index = list_releases(tmp.path()).unwrap();
424 let files: Vec<&str> = index.versions[0]
425 .files
426 .iter()
427 .map(|f| f.name.as_str())
428 .collect();
429 assert_eq!(files, vec!["good.tar.gz"]);
430 for name in &files {
431 assert!(
432 artifact_path(tmp.path(), "v1", name).is_ok(),
433 "listed name {} is not resolvable",
434 name
435 );
436 }
437 }
438
439 #[test]
342 fn upload_writes_file_and_checksum() { 440 fn upload_writes_file_and_checksum() {
343 let tmp = TempDir::new().unwrap(); 441 let tmp = TempDir::new().unwrap();
344 let sha = upload(tmp.path(), "v1.0.0", "app.tar.gz", b"hello", false).unwrap(); 442 let sha = upload(tmp.path(), "v1.0.0", "app.tar.gz", b"hello", false).unwrap();
@@ -406,9 +504,11 @@ mod tests {
406 std::fs::read(tmp.path().join("v1").join("a.tar.gz")).unwrap(), 504 std::fs::read(tmp.path().join("v1").join("a.tar.gz")).unwrap(),
407 b"two" 505 b"two"
408 ); 506 );
409 assert!(std::fs::read_to_string(tmp.path().join("v1").join("a.tar.gz.sha256")) 507 assert!(
410 .unwrap() 508 std::fs::read_to_string(tmp.path().join("v1").join("a.tar.gz.sha256"))
411 .starts_with(&sha_after)); 509 .unwrap()
510 .starts_with(&sha_after)
511 );
412 } 512 }
413 513
414 #[test] 514 #[test]
@@ -420,10 +520,12 @@ mod tests {
420 drop(up); 520 drop(up);
421 // no artifact, no stray temp files 521 // no artifact, no stray temp files
422 assert!(!tmp.path().join("v1").join("big.tar.gz").exists()); 522 assert!(!tmp.path().join("v1").join("big.tar.gz").exists());
423 let leftovers: Vec<_> = std::fs::read_dir(tmp.path().join("v1")) 523 let leftovers: Vec<_> = std::fs::read_dir(tmp.path().join("v1")).unwrap().collect();
424 .unwrap() 524 assert!(
425 .collect(); 525 leftovers.is_empty(),
426 assert!(leftovers.is_empty(), "temp files left behind: {:?}", leftovers); 526 "temp files left behind: {:?}",
527 leftovers
528 );
427 } 529 }
428 530
429 #[test] 531 #[test]
src/server/repos.rs
Old New
@@ -11,19 +11,14 @@ pub struct RepoEntry {
11 pub policy: RepoPolicy, 11 pub policy: RepoPolicy,
12 } 12 }
13 13
14 #[derive(Debug, Clone, Deserialize, PartialEq, Eq)] 14 #[derive(Debug, Clone, Deserialize, PartialEq, Eq, Default)]
15 #[serde(rename_all = "lowercase")] 15 #[serde(rename_all = "lowercase")]
16 pub enum RepoVisibility { 16 pub enum RepoVisibility {
17 #[default]
17 Public, 18 Public,
18 Private, 19 Private,
19 } 20 }
20 21
21 impl Default for RepoVisibility {
22 fn default() -> Self {
23 Self::Public
24 }
25 }
26
27 #[derive(Debug, Clone, Deserialize)] 22 #[derive(Debug, Clone, Deserialize)]
28 pub struct RepoUiPolicy { 23 pub struct RepoUiPolicy {
29 #[serde(default = "default_true")] 24 #[serde(default = "default_true")]
src/server/ssh/session.rs
Old New
@@ -56,12 +56,20 @@ impl SshHandler {
56 } 56 }
57 } 57 }
58 58
59 /// Handle a release verb. The store calls here (`begin`, `list_releases`, 59 /// Handle a release verb.
60 /// `delete_release`) are synchronous filesystem I/O on the async runtime. 60 ///
61 /// That is an accepted tradeoff at this server's scale: the operations are 61 /// Every filesystem call in the release path is synchronous I/O performed
62 /// metadata-sized directory walks and renames, not the artifact streaming 62 /// on the async runtime: `begin`/`list_releases`/`delete_release` here,
63 /// itself (upload bytes arrive incrementally via `data()`). If release 63 /// the per-chunk `write` in `data()`, and — the largest one — `finish()`'s
64 /// counts or concurrency grow, move these onto `spawn_blocking`. 64 /// `sync_all` in `channel_eof`, which blocks until the artifact is durable
65 /// on disk. That is an accepted tradeoff at this server's scale (a handful
66 /// of connections, artifacts measured in megabytes); it does stall the
67 /// runtime thread, so if concurrency or artifact sizes grow these belong
68 /// on `spawn_blocking`.
69 ///
70 /// Only one upload may be in flight per connection: `active_upload` is a
71 /// single slot, and a second concurrent `upload` on a multiplexed channel
72 /// is rejected rather than allowed to displace the first.
65 fn handle_release_command( 73 fn handle_release_command(
66 &mut self, 74 &mut self,
67 channel: ChannelId, 75 channel: ChannelId,
@@ -107,24 +115,39 @@ impl SshHandler {
107 filename, 115 filename,
108 force, 116 force,
109 .. 117 ..
110 } => match crate::releases::ReleaseUpload::begin( 118 } => {
111 &dir, 119 // One upload slot per connection: a second one would silently
112 &version, 120 // displace the first, discarding its temp file and leaving its
113 &filename, 121 // channel waiting for a reply that never comes.
114 force, 122 if self.active_upload.is_some() {
115 self.config.max_release_size, 123 warn!("Rejected release upload: another upload is already in progress");
116 ) { 124 reply_and_close(
117 Ok(upload) => { 125 session,
118 self.active_upload = Some(UploadSession {
119 channel, 126 channel,
120 state: UploadState::Active(Box::new(upload)), 127 "error: another upload is already in progress on this connection\n",
121 }); 128 1,
122 // Reply comes on channel EOF, once all bytes have arrived. 129 );
130 return;
123 } 131 }
124 Err(e) => { 132 match crate::releases::ReleaseUpload::begin(
125 reply_and_close(session, channel, &format!("error: {}\n", e), 1); 133 &dir,
134 &version,
135 &filename,
136 force,
137 self.config.max_release_size,
138 ) {
139 Ok(upload) => {
140 self.active_upload = Some(UploadSession {
141 channel,
142 state: UploadState::Active(Box::new(upload)),
143 });
144 // Reply comes on channel EOF, once all bytes arrive.
145 }
146 Err(e) => {
147 reply_and_close(session, channel, &format!("error: {}\n", e), 1);
148 }
126 } 149 }
127 }, 150 }
128 ReleaseCmd::List { .. } => match crate::releases::list_releases(&dir) { 151 ReleaseCmd::List { .. } => match crate::releases::list_releases(&dir) {
129 Ok(index) => match serde_json::to_string_pretty(&index) { 152 Ok(index) => match serde_json::to_string_pretty(&index) {
130 Ok(json) => reply_and_close(session, channel, &format!("{}\n", json), 0), 153 Ok(json) => reply_and_close(session, channel, &format!("{}\n", json), 0),
@@ -565,7 +588,7 @@ impl Handler for SshHandler {
565 &mut self, 588 &mut self,
566 channel: ChannelId, 589 channel: ChannelId,
567 data: &[u8], 590 data: &[u8],
568 _session: &mut Session, 591 session: &mut Session,
569 ) -> Result<(), Self::Error> { 592 ) -> Result<(), Self::Error> {
570 if let Some(upload) = self.active_upload.as_mut() { 593 if let Some(upload) = self.active_upload.as_mut() {
571 if upload.channel == channel { 594 if upload.channel == channel {
@@ -579,6 +602,16 @@ impl Handler for SshHandler {
579 }, 602 },
580 failed => failed, 603 failed => failed,
581 }; 604 };
605 // Abort as soon as the write fails (e.g. the size cap) rather
606 // than absorbing the rest of the stream and only complaining
607 // at EOF: tell the client now and close the channel. The
608 // Failed latch stays as a fallback for chunks already in
609 // flight, which then land here with no active upload.
610 if let UploadState::Failed(msg) = &upload.state {
611 let msg = format!("error: {}\n", msg);
612 self.active_upload = None;
613 reply_and_close(session, channel, &msg, 1);
614 }
582 return Ok(()); 615 return Ok(());
583 } 616 }
584 } 617 }
tests/release_cli_test.rs
Old New
@@ -28,7 +28,13 @@ fn publish_list_delete_roundtrip() {
28 28
29 let publish = release_cmd( 29 let publish = release_cmd(
30 &harness, 30 &harness,
31 &["publish", "v1.0.0", tarball.to_str().unwrap(), "--remote", "srv"], 31 &[
32 "publish",
33 "v1.0.0",
34 tarball.to_str().unwrap(),
35 "--remote",
36 "srv",
37 ],
32 ); 38 );
33 assert!( 39 assert!(
34 publish.status.success(), 40 publish.status.success(),
@@ -50,9 +56,10 @@ fn publish_list_delete_roundtrip() {
50 56
51 let delete = release_cmd(&harness, &["delete", "v1.0.0", "--remote", "srv"]); 57 let delete = release_cmd(&harness, &["delete", "v1.0.0", "--remote", "srv"]);
52 assert!(delete.status.success()); 58 assert!(delete.status.success());
53 let after: serde_json::Value = 59 let after: serde_json::Value = serde_json::from_slice(
54 serde_json::from_slice(&release_cmd(&harness, &["list", "--json", "--remote", "srv"]).stdout) 60 &release_cmd(&harness, &["list", "--json", "--remote", "srv"]).stdout,
55 .unwrap(); 61 )
62 .unwrap();
56 assert_eq!(after["versions"].as_array().unwrap().len(), 0); 63 assert_eq!(after["versions"].as_array().unwrap().len(), 0);
57 } 64 }
58 65
@@ -63,13 +70,34 @@ fn duplicate_publish_needs_force_flag() {
63 std::fs::write(&tarball, b"one").unwrap(); 70 std::fs::write(&tarball, b"one").unwrap();
64 let path = tarball.to_str().unwrap(); 71 let path = tarball.to_str().unwrap();
65 72
66 assert!(release_cmd(&harness, &["publish", "v1", path, "--remote", "srv"]) 73 assert!(
67 .status 74 release_cmd(&harness, &["publish", "v1", path, "--remote", "srv"])
68 .success()); 75 .status
76 .success()
77 );
69 78
70 let dup = release_cmd(&harness, &["publish", "v1", path, "--remote", "srv"]); 79 let dup = release_cmd(&harness, &["publish", "v1", path, "--remote", "srv"]);
71 assert!(!dup.status.success()); 80 assert!(!dup.status.success());
72 assert!(String::from_utf8_lossy(&dup.stderr).contains("already exists")); 81 let stderr = String::from_utf8_lossy(&dup.stderr);
82 assert!(stderr.contains("already exists"), "stderr: {}", stderr);
83 // The server's "error: ..." reply must not be doubled up, and ssh's own
84 // host-key banner must not be glued into the message (the reproduced bug).
85 assert_eq!(
86 stderr.matches("error:").count(),
87 1,
88 "expected exactly one 'error:' in stderr: {}",
89 stderr
90 );
91 assert!(
92 stderr.trim_start().starts_with("error:"),
93 "stderr should start with 'error:': {}",
94 stderr
95 );
96 assert!(
97 !stderr.contains("Warning: Permanently added"),
98 "ssh host-key banner leaked into error message: {}",
99 stderr
100 );
73 101
74 std::fs::write(&tarball, b"two").unwrap(); 102 std::fs::write(&tarball, b"two").unwrap();
75 let forced = release_cmd( 103 let forced = release_cmd(
@@ -88,7 +116,13 @@ fn non_ssh_remote_is_a_clear_error() {
88 // "origin" is a local filesystem path in the harness 116 // "origin" is a local filesystem path in the harness
89 let output = release_cmd( 117 let output = release_cmd(
90 &harness, 118 &harness,
91 &["publish", "v1", tarball.to_str().unwrap(), "--remote", "origin"], 119 &[
120 "publish",
121 "v1",
122 tarball.to_str().unwrap(),
123 "--remote",
124 "origin",
125 ],
92 ); 126 );
93 assert!(!output.status.success()); 127 assert!(!output.status.success());
94 assert!(String::from_utf8_lossy(&output.stderr).contains("not an SSH remote")); 128 assert!(String::from_utf8_lossy(&output.stderr).contains("not an SSH remote"));
@@ -102,8 +136,68 @@ fn invalid_version_rejected_client_side() {
102 136
103 let output = release_cmd( 137 let output = release_cmd(
104 &harness, 138 &harness,
105 &["publish", "../evil", tarball.to_str().unwrap(), "--remote", "srv"], 139 &[
140 "publish",
141 "../evil",
142 tarball.to_str().unwrap(),
143 "--remote",
144 "srv",
145 ],
106 ); 146 );
107 assert!(!output.status.success()); 147 assert!(!output.status.success());
108 assert!(String::from_utf8_lossy(&output.stderr).contains("invalid version")); 148 assert!(String::from_utf8_lossy(&output.stderr).contains("invalid version"));
149
150 // delete() must validate client-side too, mirroring publish() — no network
151 // round trip needed to reject an obviously-bad version name.
152 let delete = release_cmd(&harness, &["delete", "../evil", "--remote", "srv"]);
153 assert!(!delete.status.success());
154 assert!(String::from_utf8_lossy(&delete.stderr).contains("invalid version"));
155 }
156
157 /// If one file in a multi-file publish fails (e.g. a duplicate rejected
158 /// without --force), earlier successes are still reported, the command exits
159 /// non-zero, and the failing file's error is reported too.
160 #[test]
161 fn multi_file_publish_reports_partial_failure() {
162 let harness = setup("cli-partial");
163 let file_a = harness.work_repo().dir.path().join("file_a.tar.gz");
164 let file_b = harness.work_repo().dir.path().join("file_b.tar.gz");
165 std::fs::write(&file_a, b"a-bytes").unwrap();
166 std::fs::write(&file_b, b"b-bytes").unwrap();
167
168 // Publish file_a first, so a later attempt to republish it without
169 // --force is rejected as a duplicate.
170 assert!(release_cmd(
171 &harness,
172 &["publish", "v1", file_a.to_str().unwrap(), "--remote", "srv"]
173 )
174 .status
175 .success());
176
177 // Now publish [file_b, file_a]: file_b is new (succeeds), file_a is a
178 // duplicate (fails).
179 let output = release_cmd(
180 &harness,
181 &[
182 "publish",
183 "v1",
184 file_b.to_str().unwrap(),
185 file_a.to_str().unwrap(),
186 "--remote",
187 "srv",
188 ],
189 );
190 assert!(!output.status.success());
191 let stdout = String::from_utf8_lossy(&output.stdout);
192 assert!(
193 stdout.contains("Published v1/file_b.tar.gz"),
194 "stdout: {}",
195 stdout
196 );
197 let stderr = String::from_utf8_lossy(&output.stderr);
198 assert!(
199 stderr.contains("file_a.tar.gz") && stderr.contains("already exists"),
200 "stderr: {}",
201 stderr
202 );
109 } 203 }
tests/release_server_test.rs
Old New
@@ -1,5 +1,7 @@
1 mod common; 1 mod common;
2 2
3 use std::process::Output;
4
3 use common::ServerHarness; 5 use common::ServerHarness;
4 6
5 #[test] 7 #[test]
@@ -31,8 +33,6 @@ fn ssh_client_interop_rejects_unknown_command() {
31 ); 33 );
32 } 34 }
33 35
34 use std::process::Output;
35
36 fn stdout(output: &Output) -> String { 36 fn stdout(output: &Output) -> String {
37 String::from_utf8_lossy(&output.stdout).to_string() 37 String::from_utf8_lossy(&output.stdout).to_string()
38 } 38 }
@@ -320,7 +320,9 @@ fn http_releases_page_and_download() {
320 let page = harness.get_ok("/release-http/releases"); 320 let page = harness.get_ok("/release-http/releases");
321 assert!(page.body.contains("v2.0.0")); 321 assert!(page.body.contains("v2.0.0"));
322 assert!(page.body.contains("app.tar.gz")); 322 assert!(page.body.contains("app.tar.gz"));
323 assert!(page.body.contains("/release-http/releases/v2.0.0/app.tar.gz")); 323 assert!(page
324 .body
325 .contains("/release-http/releases/v2.0.0/app.tar.gz"));
324 326
325 let (head, body) = harness.get_bytes("/release-http/releases/v2.0.0/app.tar.gz"); 327 let (head, body) = harness.get_bytes("/release-http/releases/v2.0.0/app.tar.gz");
326 assert!(head.contains("200"), "download failed: {}", head); 328 assert!(head.contains("200"), "download failed: {}", head);
@@ -331,8 +333,7 @@ fn http_releases_page_and_download() {
331 assert_eq!(body, content); 333 assert_eq!(body, content);
332 334
333 // checksum companion is downloadable as text 335 // checksum companion is downloadable as text
334 let (sha_head, sha_body) = 336 let (sha_head, sha_body) = harness.get_bytes("/release-http/releases/v2.0.0/app.tar.gz.sha256");
335 harness.get_bytes("/release-http/releases/v2.0.0/app.tar.gz.sha256");
336 assert!(sha_head.contains("200")); 337 assert!(sha_head.contains("200"));
337 assert!(String::from_utf8(sha_body).unwrap().contains("app.tar.gz")); 338 assert!(String::from_utf8(sha_body).unwrap().contains("app.tar.gz"));
338 } 339 }