fix: reject duplicate completed upload names
A user could select another local file with the same name as one that already exists in completed storage. The upload would be allowed to start and only hit an existing-file conflict late in the flow, which made the UI look like the file was uploadable. Reject duplicate sanitized names during upload creation so no staging record or chunk transfer starts for a file that cannot be completed. Keep the completion path non-replacing as a second guard by promoting through a no-overwrite file creation path, with a hard-link fast path and copy fallback for custom temp locations. The browser now treats the server's duplicate-name conflict as a terminal row: it disables the action, marks the item visually, and tells the user to rename the file if they want to upload that copy. Test Plan: - just check Refs: none
This commit is contained in:
@@ -7,8 +7,8 @@ browser -> nginx -> upl Rust server -> local filesystem
|
|||||||
```
|
```
|
||||||
|
|
||||||
The server writes upload chunks directly into an inaccessible temp file at
|
The server writes upload chunks directly into an inaccessible temp file at
|
||||||
their final offsets. Once every chunk is present, completion atomically renames
|
their final offsets. Once every chunk is present, completion promotes that temp
|
||||||
that temp file into the completed upload directory.
|
file into the completed upload directory without replacing an existing file.
|
||||||
|
|
||||||
## Project Structure
|
## Project Structure
|
||||||
|
|
||||||
@@ -19,7 +19,7 @@ upl
|
|||||||
src/app.rs Axum router, shared state, static file service
|
src/app.rs Axum router, shared state, static file service
|
||||||
src/api.rs HTTP handlers and API error responses
|
src/api.rs HTTP handlers and API error responses
|
||||||
src/model.rs JSON request, response, and metadata shapes
|
src/model.rs JSON request, response, and metadata shapes
|
||||||
src/storage.rs local filesystem layout, offset writes, and final rename
|
src/storage.rs local filesystem layout, offset writes, and final promotion
|
||||||
src/lib.rs library surface used by integration tests
|
src/lib.rs library surface used by integration tests
|
||||||
Browser UI
|
Browser UI
|
||||||
static/index.html upload tool markup
|
static/index.html upload tool markup
|
||||||
@@ -48,8 +48,9 @@ upl
|
|||||||
- `upl --help` prints the full argument help text.
|
- `upl --help` prints the full argument help text.
|
||||||
- The server accepts request bodies up to 64 MiB, which leaves room for the
|
- The server accepts request bodies up to 64 MiB, which leaves room for the
|
||||||
planned 16 MiB upload chunks and matches the nginx example in `PLAN.md`.
|
planned 16 MiB upload chunks and matches the nginx example in `PLAN.md`.
|
||||||
- Keep `UPL_TEMP_DIR` on the same filesystem as `<data-dir>/complete` so
|
- Keep `UPL_TEMP_DIR` on the same filesystem as `<data-dir>/complete` for the
|
||||||
completion can promote files with an atomic rename.
|
cheapest final promotion. Cross-filesystem temp directories still work, but
|
||||||
|
completion falls back to copying into a newly created final file.
|
||||||
|
|
||||||
## Common Commands
|
## Common Commands
|
||||||
|
|
||||||
@@ -69,6 +70,11 @@ file uploads at the same time, and each file still uploads up to three chunks
|
|||||||
concurrently. Every selected file keeps its own upload id, progress markers,
|
concurrently. Every selected file keeps its own upload id, progress markers,
|
||||||
abort controller, retry state, and saved IndexedDB resume record.
|
abort controller, retry state, and saved IndexedDB resume record.
|
||||||
|
|
||||||
|
If a completed file with the same sanitized name already exists, the server
|
||||||
|
rejects the upload before staging begins. The selected row is marked
|
||||||
|
unavailable and tells the user to rename the file if they want to upload that
|
||||||
|
copy.
|
||||||
|
|
||||||
## nginx
|
## nginx
|
||||||
|
|
||||||
Run `upl` on localhost and put nginx in front of it for TLS and access control:
|
Run `upl` on localhost and put nginx in front of it for TLS and access control:
|
||||||
|
|||||||
@@ -13,17 +13,21 @@ Keep this file as the reusable verification checklist while implementing
|
|||||||
- `POST /api/uploads` creates `meta.json`, a temp upload file, and a
|
- `POST /api/uploads` creates `meta.json`, a temp upload file, and a
|
||||||
completion-marker directory.
|
completion-marker directory.
|
||||||
- `POST /api/uploads` rejects an empty file name.
|
- `POST /api/uploads` rejects an empty file name.
|
||||||
|
- `POST /api/uploads` rejects a name that already exists in completed
|
||||||
|
storage before staging begins.
|
||||||
- `PUT /api/uploads/:id/chunks/:index` writes validated chunks into the
|
- `PUT /api/uploads/:id/chunks/:index` writes validated chunks into the
|
||||||
temp upload file and records completion markers.
|
temp upload file and records completion markers.
|
||||||
- `PUT /api/uploads/:id/chunks/:index` rejects wrong-size chunks.
|
- `PUT /api/uploads/:id/chunks/:index` rejects wrong-size chunks.
|
||||||
- `PUT /api/uploads/:id/chunks/:index` rejects out-of-range indexes.
|
- `PUT /api/uploads/:id/chunks/:index` rejects out-of-range indexes.
|
||||||
- `PUT /api/uploads/:id/chunks/:index` accepts duplicate chunks.
|
- `PUT /api/uploads/:id/chunks/:index` accepts duplicate chunks.
|
||||||
- `GET /api/uploads/:id` reports completed chunks from disk markers.
|
- `GET /api/uploads/:id` reports completed chunks from disk markers.
|
||||||
- `POST /api/uploads/:id/complete` renames the verified temp upload file
|
- `POST /api/uploads/:id/complete` promotes the verified temp upload file
|
||||||
and removes staging data.
|
and removes staging data.
|
||||||
- Parallel upload requests for separate files complete without crossing
|
- Parallel upload requests for separate files complete without crossing
|
||||||
bytes between temp upload files.
|
bytes between temp upload files.
|
||||||
- `POST /api/uploads/:id/complete` rejects incomplete uploads.
|
- `POST /api/uploads/:id/complete` rejects incomplete uploads.
|
||||||
|
- `POST /api/uploads/:id/complete` refuses to replace a completed file that
|
||||||
|
appears after the upload was created.
|
||||||
- `POST /api/uploads/:id/complete` rejects tampered temp upload files.
|
- `POST /api/uploads/:id/complete` rejects tampered temp upload files.
|
||||||
- `static/app.js` passes `node --check`.
|
- `static/app.js` passes `node --check`.
|
||||||
- `just nginx-smoke`
|
- `just nginx-smoke`
|
||||||
|
|||||||
+52
-5
@@ -20,6 +20,8 @@ use crate::model::{
|
|||||||
UploadProgressResponse,
|
UploadProgressResponse,
|
||||||
};
|
};
|
||||||
|
|
||||||
|
const FILE_EXISTS_MESSAGE: &str = "file already exists";
|
||||||
|
|
||||||
#[derive(Clone, Debug)]
|
#[derive(Clone, Debug)]
|
||||||
pub struct Storage {
|
pub struct Storage {
|
||||||
data_dir: PathBuf,
|
data_dir: PathBuf,
|
||||||
@@ -54,6 +56,10 @@ impl Storage {
|
|||||||
self.ensure_layout().await?;
|
self.ensure_layout().await?;
|
||||||
|
|
||||||
let safe_name = safe_file_name(original_name);
|
let safe_name = safe_file_name(original_name);
|
||||||
|
if fs::try_exists(self.final_path(&safe_name)).await? {
|
||||||
|
return Err(StorageError::Conflict(FILE_EXISTS_MESSAGE));
|
||||||
|
}
|
||||||
|
|
||||||
let created_at = OffsetDateTime::now_utc().format(&Rfc3339)?;
|
let created_at = OffsetDateTime::now_utc().format(&Rfc3339)?;
|
||||||
|
|
||||||
for _ in 0..8 {
|
for _ in 0..8 {
|
||||||
@@ -158,12 +164,9 @@ impl Storage {
|
|||||||
|
|
||||||
self.verify_all_chunks(&meta).await?;
|
self.verify_all_chunks(&meta).await?;
|
||||||
|
|
||||||
let final_path = self.final_dir().join(&meta.safe_name);
|
let final_path = self.final_path(&meta.safe_name);
|
||||||
if fs::try_exists(&final_path).await? {
|
|
||||||
return Err(StorageError::Conflict("complete file already exists"));
|
|
||||||
}
|
|
||||||
|
|
||||||
fs::rename(self.upload_file_path(upload_id), &final_path).await?;
|
promote_file_without_overwrite(&self.upload_file_path(upload_id), &final_path).await?;
|
||||||
self.remove_upload_dir(upload_id).await?;
|
self.remove_upload_dir(upload_id).await?;
|
||||||
|
|
||||||
Ok(meta.complete_response(final_path.display().to_string()))
|
Ok(meta.complete_response(final_path.display().to_string()))
|
||||||
@@ -187,6 +190,10 @@ impl Storage {
|
|||||||
self.data_dir.join("complete")
|
self.data_dir.join("complete")
|
||||||
}
|
}
|
||||||
|
|
||||||
|
fn final_path(&self, safe_name: &str) -> PathBuf {
|
||||||
|
self.final_dir().join(safe_name)
|
||||||
|
}
|
||||||
|
|
||||||
fn upload_dir(&self, upload_id: &str) -> PathBuf {
|
fn upload_dir(&self, upload_id: &str) -> PathBuf {
|
||||||
self.staging_dir().join(upload_id)
|
self.staging_dir().join(upload_id)
|
||||||
}
|
}
|
||||||
@@ -419,6 +426,46 @@ async fn file_len(path: &Path) -> Result<Option<u64>, StorageError> {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
async fn promote_file_without_overwrite(
|
||||||
|
source: &Path,
|
||||||
|
destination: &Path,
|
||||||
|
) -> Result<(), StorageError> {
|
||||||
|
match fs::hard_link(source, destination).await {
|
||||||
|
Ok(()) => return Ok(()),
|
||||||
|
Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => {
|
||||||
|
return Err(StorageError::Conflict(FILE_EXISTS_MESSAGE));
|
||||||
|
}
|
||||||
|
Err(_) => {}
|
||||||
|
}
|
||||||
|
|
||||||
|
let mut output = match fs::OpenOptions::new()
|
||||||
|
.write(true)
|
||||||
|
.create_new(true)
|
||||||
|
.open(destination)
|
||||||
|
.await
|
||||||
|
{
|
||||||
|
Ok(output) => output,
|
||||||
|
Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => {
|
||||||
|
return Err(StorageError::Conflict(FILE_EXISTS_MESSAGE));
|
||||||
|
}
|
||||||
|
Err(error) => return Err(error.into()),
|
||||||
|
};
|
||||||
|
|
||||||
|
let copy_result = async {
|
||||||
|
let mut input = fs::File::open(source).await?;
|
||||||
|
tokio::io::copy(&mut input, &mut output).await?;
|
||||||
|
output.flush().await
|
||||||
|
}
|
||||||
|
.await;
|
||||||
|
|
||||||
|
if let Err(error) = copy_result {
|
||||||
|
let _ = fs::remove_file(destination).await;
|
||||||
|
return Err(error.into());
|
||||||
|
}
|
||||||
|
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
|
|
||||||
fn validate_upload_id(upload_id: &str) -> Result<(), StorageError> {
|
fn validate_upload_id(upload_id: &str) -> Result<(), StorageError> {
|
||||||
let is_valid = !upload_id.is_empty()
|
let is_valid = !upload_id.is_empty()
|
||||||
&& upload_id
|
&& upload_id
|
||||||
|
|||||||
+32
-10
@@ -5,7 +5,7 @@ const CHUNK_CONCURRENCY = 3;
|
|||||||
const FILE_CONCURRENCY = 3;
|
const FILE_CONCURRENCY = 3;
|
||||||
const MAX_RETRIES = 5;
|
const MAX_RETRIES = 5;
|
||||||
const BASE_RETRY_DELAY_MS = 500;
|
const BASE_RETRY_DELAY_MS = 500;
|
||||||
const COMPLETE_EXISTS_MESSAGE = "complete file already exists";
|
const FILE_EXISTS_MESSAGE = "file already exists";
|
||||||
|
|
||||||
const fileInput = document.querySelector("#file-input");
|
const fileInput = document.querySelector("#file-input");
|
||||||
const pickButton = document.querySelector("#pick-button");
|
const pickButton = document.querySelector("#pick-button");
|
||||||
@@ -191,6 +191,10 @@ function renderUploadItems() {
|
|||||||
for (const item of state.uploadItems) {
|
for (const item of state.uploadItems) {
|
||||||
const row = document.createElement("li");
|
const row = document.createElement("li");
|
||||||
row.className = "upload-item";
|
row.className = "upload-item";
|
||||||
|
if (item.terminal) {
|
||||||
|
row.classList.add("upload-item-blocked");
|
||||||
|
row.setAttribute("aria-invalid", "true");
|
||||||
|
}
|
||||||
|
|
||||||
const header = document.createElement("div");
|
const header = document.createElement("div");
|
||||||
header.className = "upload-item-header";
|
header.className = "upload-item-header";
|
||||||
@@ -363,6 +367,10 @@ function uploadItemDetail(item) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
function uploadActionLabel(item) {
|
function uploadActionLabel(item) {
|
||||||
|
if (item.terminal) {
|
||||||
|
return "Unavailable";
|
||||||
|
}
|
||||||
|
|
||||||
if (item.finished) {
|
if (item.finished) {
|
||||||
return "Done";
|
return "Done";
|
||||||
}
|
}
|
||||||
@@ -736,7 +744,25 @@ async function runUploadItem(item) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
async function handleTerminalUploadError(item, error) {
|
async function handleTerminalUploadError(item, error) {
|
||||||
if (!item.record || typeof error.status !== "number") {
|
if (typeof error.status !== "number") {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
if (isFileExistsConflict(error)) {
|
||||||
|
if (item.record) {
|
||||||
|
await deleteRecord(item.record.upload_id);
|
||||||
|
}
|
||||||
|
item.record = null;
|
||||||
|
item.completedChunks = new Set();
|
||||||
|
setItemProgress(item, 0, 0);
|
||||||
|
item.terminal = true;
|
||||||
|
item.statusText =
|
||||||
|
"File already exists on the server. Rename the file to upload this copy.";
|
||||||
|
log(`${item.file.name}: file already exists. Rename it to upload this copy.`);
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
|
||||||
|
if (!item.record) {
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -752,17 +778,13 @@ async function handleTerminalUploadError(item, error) {
|
|||||||
return true;
|
return true;
|
||||||
}
|
}
|
||||||
|
|
||||||
if (error.status === 409 && error.message === COMPLETE_EXISTS_MESSAGE) {
|
|
||||||
await deleteRecord(uploadId);
|
|
||||||
item.terminal = true;
|
|
||||||
item.statusText = "Complete file already exists on the server.";
|
|
||||||
log(`${item.file.name}: complete file already exists on the server.`);
|
|
||||||
return true;
|
|
||||||
}
|
|
||||||
|
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function isFileExistsConflict(error) {
|
||||||
|
return error.status === 409 && error.message === FILE_EXISTS_MESSAGE;
|
||||||
|
}
|
||||||
|
|
||||||
async function createUploadRecord(item, signal) {
|
async function createUploadRecord(item, signal) {
|
||||||
const response = await fetchJson("/api/uploads", {
|
const response = await fetchJson("/api/uploads", {
|
||||||
method: "POST",
|
method: "POST",
|
||||||
|
|||||||
@@ -109,6 +109,15 @@ h1 {
|
|||||||
border-radius: 8px;
|
border-radius: 8px;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
.upload-item-blocked {
|
||||||
|
border-color: #f04438;
|
||||||
|
background: rgb(240 68 56 / 8%);
|
||||||
|
}
|
||||||
|
|
||||||
|
.upload-item-blocked .upload-meta span {
|
||||||
|
color: #b42318;
|
||||||
|
}
|
||||||
|
|
||||||
.upload-item-header {
|
.upload-item-header {
|
||||||
display: grid;
|
display: grid;
|
||||||
grid-template-columns: minmax(0, 1fr) auto;
|
grid-template-columns: minmax(0, 1fr) auto;
|
||||||
|
|||||||
@@ -171,6 +171,46 @@ async fn rejects_incomplete_upload() -> Result<(), Box<dyn std::error::Error>> {
|
|||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn rejects_completion_that_would_replace_file() -> Result<(), Box<dyn std::error::Error>> {
|
||||||
|
let temp_dir = TempDir::new()?;
|
||||||
|
let app = test_app(temp_dir.path());
|
||||||
|
let upload = create_upload(&app, "clash.bin", 8).await?;
|
||||||
|
tokio::fs::write(
|
||||||
|
temp_dir.path().join("complete").join("clash.bin"),
|
||||||
|
b"original",
|
||||||
|
)
|
||||||
|
.await?;
|
||||||
|
|
||||||
|
let response = app
|
||||||
|
.clone()
|
||||||
|
.oneshot(chunk_request(&upload.upload_id, 0, b"incoming".to_vec())?)
|
||||||
|
.await?;
|
||||||
|
assert_eq!(response.status(), StatusCode::NO_CONTENT);
|
||||||
|
|
||||||
|
let response = app
|
||||||
|
.oneshot(empty_request(
|
||||||
|
Method::POST,
|
||||||
|
&format!("/api/uploads/{}/complete", upload.upload_id),
|
||||||
|
)?)
|
||||||
|
.await?;
|
||||||
|
|
||||||
|
assert_eq!(response.status(), StatusCode::CONFLICT);
|
||||||
|
assert_eq!(
|
||||||
|
tokio::fs::read(temp_dir.path().join("complete").join("clash.bin")).await?,
|
||||||
|
b"original"
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
temp_dir
|
||||||
|
.path()
|
||||||
|
.join("staging")
|
||||||
|
.join(&upload.upload_id)
|
||||||
|
.exists()
|
||||||
|
);
|
||||||
|
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
|
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn rejects_tampered_temp_upload_file() -> Result<(), Box<dyn std::error::Error>> {
|
async fn rejects_tampered_temp_upload_file() -> Result<(), Box<dyn std::error::Error>> {
|
||||||
let temp_dir = TempDir::new()?;
|
let temp_dir = TempDir::new()?;
|
||||||
|
|||||||
@@ -77,6 +77,41 @@ async fn rejects_empty_upload_name() -> Result<(), Box<dyn std::error::Error>> {
|
|||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn rejects_upload_name_that_already_exists() -> Result<(), Box<dyn std::error::Error>> {
|
||||||
|
let temp_dir = TempDir::new()?;
|
||||||
|
let app = test_app(temp_dir.path());
|
||||||
|
let complete_dir = temp_dir.path().join("complete");
|
||||||
|
tokio::fs::create_dir_all(&complete_dir).await?;
|
||||||
|
tokio::fs::write(complete_dir.join("xyz.foo"), b"original").await?;
|
||||||
|
|
||||||
|
let response = app
|
||||||
|
.oneshot(json_request(
|
||||||
|
"/api/uploads",
|
||||||
|
&json!({
|
||||||
|
"name": "xyz.foo",
|
||||||
|
"size": 10,
|
||||||
|
"last_modified": 1_760_000_000_000_i64
|
||||||
|
}),
|
||||||
|
)?)
|
||||||
|
.await?;
|
||||||
|
|
||||||
|
assert_eq!(response.status(), StatusCode::CONFLICT);
|
||||||
|
|
||||||
|
let body = response.into_body().collect().await?.to_bytes();
|
||||||
|
let body: serde_json::Value = serde_json::from_slice(&body)?;
|
||||||
|
assert_eq!(body["error"], "file already exists");
|
||||||
|
assert_eq!(
|
||||||
|
tokio::fs::read(complete_dir.join("xyz.foo")).await?,
|
||||||
|
b"original"
|
||||||
|
);
|
||||||
|
|
||||||
|
let mut staging_entries = tokio::fs::read_dir(temp_dir.path().join("staging")).await?;
|
||||||
|
assert!(staging_entries.next_entry().await?.is_none());
|
||||||
|
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
|
|
||||||
fn test_app(data_dir: &Path) -> axum::Router {
|
fn test_app(data_dir: &Path) -> axum::Router {
|
||||||
build_router(&AppConfig::new(
|
build_router(&AppConfig::new(
|
||||||
SocketAddr::from((Ipv4Addr::LOCALHOST, 0)),
|
SocketAddr::from((Ipv4Addr::LOCALHOST, 0)),
|
||||||
|
|||||||
Reference in New Issue
Block a user