From 2285e8130bd8fe1dfbf267d610994de85ba5286b Mon Sep 17 00:00:00 2001 From: Robb Hughes Date: Mon, 12 Oct 2026 11:12:48 -0500 Subject: [PATCH] Close the risks that matter before installing things3 Disable the unauthenticated web server, keep the history key and task bodies out of ordinary logs, and refuse to reuse a saved history key for a different Things account. Short ids no longer resolve to the wrong object, reorder failures are reported, and delete moves a task to Trash instead of removing its history record. TLS crates are bumped to the patched versions. --- Cargo.lock | 62 +++------- Cargo.toml | 1 - src/auth.rs | 47 ++------ src/client.rs | 78 +++++++++++- src/cloud_writer.rs | 50 +++++--- src/commands/delete.rs | 112 +++++++++-------- src/commands/edit.rs | 21 +++- src/commands/new.rs | 10 +- src/commands/projects.rs | 12 +- src/commands/reorder.rs | 21 ++-- src/commands/schedule.rs | 6 + src/commands/tags.rs | 23 ++++ src/commands/webserver.rs | 219 ++-------------------------------- src/dirs.rs | 136 ++++++++++++++++++++- src/ids/matching.rs | 29 +++-- src/ids/things_id.rs | 32 +++++ src/log_cache.rs | 174 +++++++++++++++++++-------- trycmd/delete/multiple.trycmd | 9 +- trycmd/delete/single.trycmd | 9 +- trycmd/run.sh | 3 + 20 files changed, 598 insertions(+), 456 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 17ca305..4afa760 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -106,12 +106,6 @@ version = "0.7.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7c02d123df017efcdfbd739ef81735b36c5ba83ec3c59c80a9d7ecc718f92e50" -[[package]] -name = "ascii" -version = "1.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d92bec98840b8f03a5ff5413de5293bfcd8bf96467cf5452609f939ec6f5de16" - [[package]] name = "atomic" version = "0.6.1" @@ -146,9 +140,9 @@ dependencies = [ [[package]] name = "aws-lc-rs" -version = "1.16.2" +version = "1.18.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a054912289d18629dc78375ba2c3726a3afe3ff71b4edba9dedfca0e3446d1fc" +checksum = "faac5829c2b74c28f830747e7818ccfb684261b5f48a1118b1e2a13d36dfab13" dependencies = [ "aws-lc-sys", "zeroize", @@ -156,14 +150,15 @@ dependencies = [ [[package]] name = "aws-lc-sys" -version = "0.39.1" +version = "0.46.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "83a25cf98105baa966497416dbd42565ce3a8cf8dbfd59803ec9ad46f3126399" +checksum = "f1622d8446a2d4b2ce0c7eefc73dd43a99779028d5ee5c2dd8073a658ba8a2bc" dependencies = [ "cc", "cmake", "dunce", "fs_extra", + "pkg-config", ] [[package]] @@ -259,12 +254,6 @@ dependencies = [ "windows-link", ] -[[package]] -name = "chunked_transfer" -version = "1.5.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6e4de3bc4ea267985becf712dc6d9eed8b04c953b3fcfb339ebc87acd9804901" - [[package]] name = "clap" version = "4.6.0" @@ -416,9 +405,9 @@ dependencies = [ [[package]] name = "crossbeam-epoch" -version = "0.9.18" +version = "0.9.21" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5b82ac4a3c2ca9c3460964f020e1402edd5753411d7737aa39c3714ad1b5420e" +checksum = "dc74980687109a3b14c72fd458107bf0baa1da1a1a805e178d15501ba9b86d9d" dependencies = [ "crossbeam-utils", ] @@ -819,12 +808,6 @@ version = "1.10.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6dbf3de79e51f3d586ab4cb9d5c3e2c14aa28ed23d180cf89b4df0454a69cc87" -[[package]] -name = "httpdate" -version = "1.0.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "df3b46402a9d5adb4c86a0cf463f42e19994e3ee891101b1841f30a545cb49a9" - [[package]] name = "humantime" version = "2.3.0" @@ -1422,6 +1405,12 @@ version = "0.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8b870d8c151b6f2fb93e84a13146138f05d02ed11c7e7c54f8826aaaf7c9f184" +[[package]] +name = "pkg-config" +version = "0.3.34" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f6b464fbc74e149a392436b17d523f769e057cb6877f6a5c4618bc6f11800548" + [[package]] name = "plain" version = "0.2.3" @@ -1509,9 +1498,9 @@ dependencies = [ [[package]] name = "quinn-proto" -version = "0.11.14" +version = "0.11.15" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "434b42fec591c96ef50e21e886936e66d3cc3f737104fdb9b737c40ffb94c098" +checksum = "4fcb935c5bec503c2f0e306bdd3e58bb9029dcb14fa8d9ac76e3a5256ac0763e" dependencies = [ "aws-lc-rs", "bytes", @@ -1784,9 +1773,9 @@ dependencies = [ [[package]] name = "rustls" -version = "0.23.37" +version = "0.23.45" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "758025cb5fccfd3bc2fd74708fd4682be41d99e5dff73c377c0646c6012c73a4" +checksum = "0d41d731c7d2f962d1ccc364cec258de3c0e93b38c2fb3ba97ac74513048d634" dependencies = [ "aws-lc-rs", "once_cell", @@ -1847,9 +1836,9 @@ checksum = "f87165f0995f63a9fbeea62b64d10b4d9d8e78ec6d7d51fb2125fda7bb36788f" [[package]] name = "rustls-webpki" -version = "0.103.10" +version = "0.103.15" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "df33b2b81ac578cabaf06b89b0631153a3f416b0a886e8a7a1707fb51abbd1ef" +checksum = "f3c3cf1d8b1e7d4927e2d154c3fcb02979afb9939629c62cd9048d4f07b60ac2" dependencies = [ "aws-lc-rs", "ring", @@ -2212,7 +2201,6 @@ dependencies = [ "sha1", "strum", "tempfile", - "tiny_http", "tracing", "tracing-subscriber", "trycmd", @@ -2269,18 +2257,6 @@ dependencies = [ "cfg-if", ] -[[package]] -name = "tiny_http" -version = "0.12.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "389915df6413a2e74fb181895f933386023c71110878cd0825588928e64cdc82" -dependencies = [ - "ascii", - "chunked_transfer", - "httpdate", - "log", -] - [[package]] name = "tinystr" version = "0.8.2" diff --git a/Cargo.toml b/Cargo.toml index a7d4a89..783358c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -37,7 +37,6 @@ tracing-subscriber = { version = "0.3", features = ["env-filter", "json"] } iocraft = "0.7.18" urlencoding = "2.1" uuid = { version = "1.22", features = ["v4"] } -tiny_http = "0.12" [dev-dependencies] insta = "1.46" diff --git a/src/auth.rs b/src/auth.rs index 1394340..8f1ceed 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -1,5 +1,3 @@ -use std::{fs, io::Write}; - use anyhow::{Context, Result, anyhow}; use figment::{ Figment, @@ -7,7 +5,7 @@ use figment::{ }; use serde::{Deserialize, Serialize}; -use crate::dirs::{auth_file_path, create_private_dir}; +use crate::dirs::{auth_file_path, write_private_atomic}; #[derive(Debug, Clone, Serialize, Deserialize)] struct AuthPayload { @@ -76,52 +74,21 @@ pub fn write_auth(email: &str, password: &str) -> Result { fn write_auth_at(path: &std::path::Path, email: &str, password: &str) -> Result<()> { let (email, password) = validate_auth(email, password)?; - let parent = path - .parent() - .ok_or_else(|| anyhow!("Invalid auth file path"))?; - create_private_dir(parent).with_context(|| format!("Failed creating {}", parent.display()))?; - let payload = AuthPayload { email, password }; let serialized = serde_json::to_string(&payload)?; - let tmp_path = path.with_extension("tmp"); - - // Create the staging file already private: chmod'ing after the fact leaves - // the plaintext password world-readable in between, and create_new won't - // follow a symlink planted at tmp_path. The rename carries the mode over. - let mut opts = fs::OpenOptions::new(); - opts.write(true).create_new(true); - #[cfg(unix)] - { - use std::os::unix::fs::OpenOptionsExt; - opts.mode(0o600); - } - // Clear a stale staging file from an interrupted run; create_new refuses it. - let _ = fs::remove_file(&tmp_path); - let mut file = opts - .open(&tmp_path) - .with_context(|| format!("Failed writing {}", tmp_path.display()))?; - // open() filters the requested mode through the umask, which can clear - // owner bits too, so restate it before the password goes in. The file is - // already no wider than 0600, so this opens no window. - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - fs::set_permissions(&tmp_path, fs::Permissions::from_mode(0o600)) - .with_context(|| format!("Failed securing {}", tmp_path.display()))?; - } - file.write_all(serialized.as_bytes()) - .with_context(|| format!("Failed writing {}", tmp_path.display()))?; - - fs::rename(&tmp_path, path).with_context(|| format!("Failed finalizing {}", path.display()))?; - + // Mode 0600, and do not follow a symlink at the staging path or the final path. + write_private_atomic(path, serialized.as_bytes()) + .with_context(|| format!("Failed writing {}", path.display()))?; Ok(()) } #[cfg(all(test, unix))] mod tests { - use super::*; + use std::fs; use std::os::unix::fs::PermissionsExt; + use super::*; + #[test] fn auth_file_is_created_private() { let dir = tempfile::tempdir().expect("tempdir"); diff --git a/src/client.rs b/src/client.rs index 426f658..d94f3a2 100644 --- a/src/client.rs +++ b/src/client.rs @@ -1,5 +1,6 @@ use std::{ collections::BTreeMap, + fmt, time::{SystemTime, UNIX_EPOCH}, }; @@ -35,7 +36,7 @@ pub(crate) fn now_timestamp() -> f64 { now_ts() } -#[derive(Debug, Clone)] +#[derive(Clone)] pub struct ThingsCloudClient { pub email: String, pub password: String, @@ -44,6 +45,39 @@ pub struct ThingsCloudClient { http: Client, } +impl fmt::Debug for ThingsCloudClient { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("ThingsCloudClient") + .field("email", &"") + .field("password", &"") + .field( + "history_key", + &self.history_key.as_ref().map(|_| ""), + ) + .field("head_index", &self.head_index) + .finish_non_exhaustive() + } +} + +fn redact_url(url: &str) -> String { + if let Some(index) = url.find("/history/") { + let rest = &url[index + "/history/".len()..]; + let key_end = rest.find('/').unwrap_or(rest.len()); + let mut out = String::with_capacity(url.len()); + out.push_str(&url[..index]); + out.push_str("/history/"); + out.push_str(&rest[key_end..]); + return out; + } + if let Some(index) = url.find("/account/") { + let mut out = String::with_capacity(index + "/account/".len()); + out.push_str(&url[..index]); + out.push_str("/account/"); + return out; + } + url.to_string() +} + impl ThingsCloudClient { pub fn new(email: String, password: String) -> Result { let http = Client::builder().build()?; @@ -85,20 +119,29 @@ impl ThingsCloudClient { .json(&payload); } + let safe_url = redact_url(url); let resp = req .send() - .with_context(|| format!("request failed: {url}"))?; + .with_context(|| format!("request failed: {safe_url}"))?; let status = resp.status(); let text = resp.text().with_context(|| { - format!("failed reading body from {url} (HTTP {})", status.as_u16()) + format!( + "failed reading body from {safe_url} (HTTP {})", + status.as_u16() + ) })?; if !status.is_success() { - return Err(anyhow!("HTTP {} for {}: {}", status.as_u16(), url, text)); + return Err(anyhow!( + "HTTP {} for {}: {}", + status.as_u16(), + safe_url, + text + )); } if text.trim().is_empty() { return Ok(json!({})); } - serde_json::from_str(&text).with_context(|| format!("invalid json from {url}")) + serde_json::from_str(&text).with_context(|| format!("invalid json from {safe_url}")) } pub fn authenticate(&mut self) -> Result { @@ -205,3 +248,28 @@ impl ThingsCloudClient { Ok(new_index) } } + +#[cfg(test)] +mod tests { + use super::redact_url; + + #[test] + fn redacts_history_key_and_account_email() { + assert_eq!( + redact_url( + "https://cloud.culturedcode.com/version/1/history/SECRETKEY/items?start-index=0" + ), + "https://cloud.culturedcode.com/version/1/history//items?start-index=0" + ); + assert_eq!( + redact_url( + "https://cloud.culturedcode.com/version/1/history/SECRETKEY/commit?ancestor-index=3&_cnt=1" + ), + "https://cloud.culturedcode.com/version/1/history//commit?ancestor-index=3&_cnt=1" + ); + assert_eq!( + redact_url("https://cloud.culturedcode.com/version/1/account/user%40example.com"), + "https://cloud.culturedcode.com/version/1/account/" + ); + } +} diff --git a/src/cloud_writer.rs b/src/cloud_writer.rs index 31c591c..8f3c739 100644 --- a/src/cloud_writer.rs +++ b/src/cloud_writer.rs @@ -1,6 +1,6 @@ use std::collections::BTreeMap; -use anyhow::Result; +use anyhow::{Context, Result}; use serde_json::json; use tracing::{debug, error}; @@ -36,21 +36,33 @@ impl CloudWriter for LoggingCloudWriter { ancestor_index: Option, ) -> Result { let uuids = changes.keys().cloned().collect::>(); - let request_value = json!({ - "ancestor_index": ancestor_index.unwrap_or(self.inner.head_index()), - "changes": &changes, - }); - let request_json = - serde_json::to_string(&request_value).unwrap_or_else(|_| "{}".to_string()); - debug!( - target: "things_cli::cloud_commit::request", - event = "cloud.commit.request", - ancestor_index, - change_count = uuids.len(), - uuids = ?uuids, - request_json = %request_json, - "cloud commit request" - ); + // Task titles and notes stay out of logs unless a test harness opts in. + if std::env::var("THINGS3_LOG_COMMIT_BODY").ok().as_deref() == Some("1") { + let request_value = json!({ + "ancestor_index": ancestor_index.unwrap_or(self.inner.head_index()), + "changes": &changes, + }); + let request_json = + serde_json::to_string(&request_value).unwrap_or_else(|_| "{}".to_string()); + debug!( + target: "things_cli::cloud_commit::request", + event = "cloud.commit.request", + ancestor_index, + change_count = uuids.len(), + uuids = ?uuids, + request_json = %request_json, + "cloud commit request" + ); + } else { + debug!( + target: "things_cli::cloud_commit::request", + event = "cloud.commit.request", + ancestor_index, + change_count = uuids.len(), + uuids = ?uuids, + "cloud commit request" + ); + } match self.inner.commit(changes, ancestor_index) { Ok(head_index) => { @@ -104,8 +116,10 @@ impl LiveCloudWriter { pub fn new() -> Result { let (email, password) = load_auth()?; let mut client = ThingsCloudClient::new(email, password)?; - let _ = client.authenticate(); - client.head_index = read_cached_head_index(&append_log_dir()); + client + .authenticate() + .context("Things Cloud authentication failed")?; + client.head_index = read_cached_head_index(&append_log_dir())?; Ok(Self { client }) } } diff --git a/src/commands/delete.rs b/src/commands/delete.rs index 5975131..46730e9 100644 --- a/src/commands/delete.rs +++ b/src/commands/delete.rs @@ -8,7 +8,10 @@ use crate::{ arg_types::IdentifierToken, commands::Command, common::{DIM, GREEN, ICONS, colored}, - wire::wire_object::{EntityType, WireObject}, + wire::{ + task::TaskPatch, + wire_object::{EntityType, WireObject}, + }, }; #[derive(Debug, Args)] @@ -24,56 +27,48 @@ struct DeletePlan { changes: BTreeMap, } -fn build_delete_plan(args: &DeleteArgs, store: &crate::store::ThingsStore) -> DeletePlan { +fn build_delete_plan( + args: &DeleteArgs, + store: &crate::store::ThingsStore, + now: f64, +) -> Result { let mut targets: Vec<(String, EntityType, String)> = Vec::new(); let mut seen = HashSet::new(); for identifier in &args.item_ids { let (task, task_err, task_ambiguous) = store.resolve_task_identifier(identifier.as_str()); - let (area, area_err, area_ambiguous) = store.resolve_area_identifier(identifier.as_str()); - - let task_match = task.is_some(); - let area_match = area.is_some(); - - if task_match && area_match { - eprintln!( - "Ambiguous identifier '{}' (matches task and area).", - identifier.as_str() - ); - continue; - } - - if !task_match && !area_match { - if !task_ambiguous.is_empty() && !area_ambiguous.is_empty() { - eprintln!( - "Ambiguous identifier '{}' (matches multiple tasks and areas).", - identifier.as_str() - ); - } else if !task_ambiguous.is_empty() { - eprintln!("{task_err}"); - } else if !area_ambiguous.is_empty() { - eprintln!("{area_err}"); - } else { - eprintln!("Item not found: {}", identifier.as_str()); + let (area, _area_err, area_ambiguous) = store.resolve_area_identifier(identifier.as_str()); + let task_hits = usize::from(task.is_some()) + task_ambiguous.len(); + let area_hits = usize::from(area.is_some()) + area_ambiguous.len(); + let raw = identifier.as_str(); + + // One namespace must have one hit and the other none. An ambiguous + // prefix in either namespace used to fall through to the unique side. + if !matches!((task_hits, area_hits), (1, 0) | (0, 1)) { + if task_hits == 0 && area_hits == 0 { + return Err(format!("Item not found: {raw}")); } - continue; + let _ = task_err; + return Err(format!( + "Ambiguous identifier '{raw}' ({task_hits} tasks, {area_hits} areas). Refusing to delete." + )); } if let Some(task) = task { if !task.entity.can_upgrade_to_task7() { - eprintln!("Unsupported task entity for deletion: {}", task.entity); - continue; + return Err(format!( + "Unsupported task entity for deletion: {}", + task.entity + )); } if task.trashed { - eprintln!("Item already deleted: {}", task.title); - continue; + return Err(format!("Item already in Trash: {}", task.title)); } - if task.has_repeater() { - eprintln!( - "Task7 repeater tasks are blocked from deletion until repeater bookkeeping is supported: {}", + if task.has_repeater() || task.is_recurrence_template() { + return Err(format!( + "Recurring tasks are blocked from deletion until recurrence bookkeeping is supported: {}", task.title - ); - continue; + )); } if !seen.insert(task.uuid.clone()) { continue; @@ -92,10 +87,24 @@ fn build_delete_plan(args: &DeleteArgs, store: &crate::store::ThingsStore) -> De let mut changes = BTreeMap::new(); for (uuid, entity, _title) in &targets { - changes.insert(uuid.clone(), WireObject::delete(entity.clone())); + // Tasks go to Trash (`tr`) so Things can restore them. `t: 2` removes + // the history object. Areas have no trash flag in this client. + let change = if entity == &EntityType::Task7 { + WireObject::update( + EntityType::Task7, + TaskPatch { + trashed: Some(true), + modification_date: Some(Some(now)), + ..Default::default() + }, + ) + } else { + WireObject::delete(entity.clone()) + }; + changes.insert(uuid.clone(), change); } - DeletePlan { targets, changes } + Ok(DeletePlan { targets, changes }) } impl Command for DeleteArgs { @@ -106,7 +115,8 @@ impl Command for DeleteArgs { ctx: &mut dyn crate::cmd_ctx::CmdCtx, ) -> Result<()> { let store = cli.load_store()?; - let plan = build_delete_plan(self, &store); + let plan = build_delete_plan(self, &store, ctx.now_timestamp()) + .map_err(|err| anyhow::anyhow!(err))?; if plan.targets.is_empty() { return Ok(()); @@ -150,7 +160,7 @@ mod tests { impl crate::cmd_ctx::CmdCtx for FailingCtx { fn now_timestamp(&self) -> f64 { - unreachable!() + 1.0 } fn today_timestamp(&self) -> i64 { @@ -249,10 +259,12 @@ mod tests { item_ids: vec![IdentifierToken::from(TASK_A)], }, &build_store(vec![task(TASK_A, "Alpha", false)]), - ); + 1.0, + ) + .expect("plan"); assert_eq!( serde_json::to_value(single.changes).expect("to value"), - serde_json::json!({ TASK_A: {"t":2,"e":"Task7","p":{}} }) + serde_json::json!({ TASK_A: {"t":1,"e":"Task7","p":{"md":1.0,"tr":true}} }) ); let multi = build_delete_plan( @@ -260,16 +272,18 @@ mod tests { item_ids: vec![IdentifierToken::from(TASK_A), IdentifierToken::from(AREA_A)], }, &build_store(vec![task(TASK_A, "Alpha", false), area(AREA_A, "Work")]), - ); + 1.0, + ) + .expect("plan"); assert_eq!( serde_json::to_value(multi.changes).expect("to value"), serde_json::json!({ - TASK_A: {"t":2,"e":"Task7","p":{}}, + TASK_A: {"t":1,"e":"Task7","p":{"md":1.0,"tr":true}}, AREA_A: {"t":2,"e":"Area3","p":{}} }) ); - let skip_trashed = build_delete_plan( + let already_trashed = build_delete_plan( &DeleteArgs { item_ids: vec![IdentifierToken::from(TASK_A), IdentifierToken::from(TASK_B)], }, @@ -277,10 +291,8 @@ mod tests { task(TASK_A, "Active", false), task(TASK_B, "Trashed", true), ]), + 1.0, ); - assert_eq!( - serde_json::to_value(skip_trashed.changes).expect("to value"), - serde_json::json!({ TASK_A: {"t":2,"e":"Task7","p":{}} }) - ); + assert!(already_trashed.is_err()); } } diff --git a/src/commands/edit.rs b/src/commands/edit.rs index fe58d3f..992c84c 100644 --- a/src/commands/edit.rs +++ b/src/commands/edit.rs @@ -192,6 +192,18 @@ fn build_edit_plan( tasks.push(task); } + let move_raw_early = args.move_target.clone().unwrap_or_default(); + if !move_raw_early.trim().is_empty() { + for task in &tasks { + if task.has_repeater() || task.is_recurrence_template() { + return Err(format!( + "Recurring tasks are blocked from --move until recurrence bookkeeping is supported: {}", + task.title + )); + } + } + } + let mut shared_update = TaskPatch::default(); let mut move_from_inbox_st: Option = None; let mut labels: Vec = Vec::new(); @@ -211,8 +223,13 @@ fn build_edit_plan( } else if move_l == "clear" { labels.push("move=clear".to_string()); } else { - let (project_opt, _, _) = store.resolve_mark_identifier(&move_raw); - let (area_opt, _, _) = store.resolve_area_identifier(&move_raw); + let (project_opt, _, project_ambiguous) = store.resolve_mark_identifier(&move_raw); + let (area_opt, _, area_ambiguous) = store.resolve_area_identifier(&move_raw); + if !project_ambiguous.is_empty() || !area_ambiguous.is_empty() { + return Err(format!( + "Ambiguous --move target '{move_raw}'. Refusing to move." + )); + } let project_uuid = project_opt.as_ref().and_then(|p| { if p.is_project() { diff --git a/src/commands/new.rs b/src/commands/new.rs index 659f778..6f61ecf 100644 --- a/src/commands/new.rs +++ b/src/commands/new.rs @@ -226,8 +226,13 @@ fn build_new_plan( let in_target = args.in_target.trim(); if !in_target.eq_ignore_ascii_case("inbox") { - let (project, _, _) = store.resolve_mark_identifier(in_target); - let (area, _, _) = store.resolve_area_identifier(in_target); + let (project, _, project_ambiguous) = store.resolve_mark_identifier(in_target); + let (area, _, area_ambiguous) = store.resolve_area_identifier(in_target); + if !project_ambiguous.is_empty() || !area_ambiguous.is_empty() { + return Err(format!( + "Ambiguous --in target '{in_target}'. Refusing to create the task." + )); + } let project_uuid = project.as_ref().and_then(|p| { if p.is_project() { Some(p.uuid.clone()) @@ -312,7 +317,6 @@ fn build_new_plan( let target_bucket = props_bucket(&props); if let Some(anchor) = &anchor - && !anchor_is_today && task_bucket(anchor, store) != target_bucket { return Err( diff --git a/src/commands/projects.rs b/src/commands/projects.rs index 93efe0e..da54daf 100644 --- a/src/commands/projects.rs +++ b/src/commands/projects.rs @@ -148,8 +148,16 @@ fn build_projects_edit_plan( update.area_ids = Some(vec![]); labels.push("move=clear".to_string()); } else { - let (resolved_project, _, _) = store.resolve_mark_identifier(move_raw); - let (area, _, _) = store.resolve_area_identifier(move_raw); + let (resolved_project, _, project_ambiguous) = store.resolve_mark_identifier(move_raw); + let (area, _, area_ambiguous) = store.resolve_area_identifier(move_raw); + if !project_ambiguous.is_empty() + || !area_ambiguous.is_empty() + || (resolved_project.is_some() && area.is_some()) + { + return Err(format!( + "Ambiguous --move target '{move_raw}'. Refusing to move." + )); + } let project_uuid = resolved_project.as_ref().and_then(|p| { if p.is_project() { Some(p.uuid.clone()) diff --git a/src/commands/reorder.rs b/src/commands/reorder.rs index 945ac89..f404126 100644 --- a/src/commands/reorder.rs +++ b/src/commands/reorder.rs @@ -288,7 +288,7 @@ fn build_reorder_plan( changes, ancestor_index: ancestor, }); - ancestor = ancestor.map(|v| v + 1).or(Some(1)); + ancestor = ancestor.map(|v| v + 1); } let reorder_label = if args.before_id.is_some() { @@ -312,25 +312,18 @@ impl Command for ReorderArgs { ctx: &mut dyn crate::cmd_ctx::CmdCtx, ) -> Result<()> { let store = cli.load_store()?; - let plan = match build_reorder_plan( + let plan = build_reorder_plan( self, &store, ctx.now_timestamp(), ctx.today_timestamp(), - None, - ) { - Ok(plan) => plan, - Err(err) => { - eprintln!("{err}"); - return Ok(()); - } - }; + Some(ctx.current_head_index()), + ) + .map_err(|err| anyhow::anyhow!(err))?; for commit in plan.commits { - if let Err(e) = ctx.commit_changes(commit.changes, commit.ancestor_index) { - eprintln!("Failed to reorder item: {e}"); - return Ok(()); - } + ctx.commit_changes(commit.changes, commit.ancestor_index) + .map_err(|err| anyhow::anyhow!("Failed to reorder item: {err}"))?; } writeln!( diff --git a/src/commands/schedule.rs b/src/commands/schedule.rs index b7ced6b..e45c005 100644 --- a/src/commands/schedule.rs +++ b/src/commands/schedule.rs @@ -54,6 +54,12 @@ fn build_schedule_plan( .to_string(), ); } + if task.is_recurrence_template() { + return Err( + "Recurring template tasks are blocked from scheduling until recurrence bookkeeping is supported." + .to_string(), + ); + } let mut update = TaskPatch::default(); let mut when_label: Option = None; diff --git a/src/commands/tags.rs b/src/commands/tags.rs index fcb77c2..dcfacb1 100644 --- a/src/commands/tags.rs +++ b/src/commands/tags.rs @@ -72,6 +72,24 @@ struct TagsEditPlan { labels: Vec, } +fn tag_parent_reaches( + store: &crate::store::ThingsStore, + start: &crate::ids::ThingsId, + needle: &crate::ids::ThingsId, +) -> bool { + let mut current = Some(start.clone()); + let mut seen = std::collections::HashSet::new(); + while let Some(id) = current { + if &id == needle || !seen.insert(id.clone()) { + return true; + } + current = store + .get_tag(&id.to_string()) + .and_then(|tag| tag.parent_uuid); + } + false +} + fn build_tags_edit_plan( args: &TagsEditArgs, store: &crate::store::ThingsStore, @@ -107,6 +125,11 @@ fn build_tags_edit_plan( if parent.uuid == tag.uuid { return Err("A tag cannot be its own parent.".to_string()); } + if tag_parent_reaches(store, &parent.uuid, &tag.uuid) { + return Err( + "Refusing to parent a tag to itself or to one of its descendants.".to_string(), + ); + } let parent_id = parent.uuid; update.parent_ids = Some(vec![parent_id]); labels.push(format!("move={move_raw}")); diff --git a/src/commands/webserver.rs b/src/commands/webserver.rs index 569fec5..14e48aa 100644 --- a/src/commands/webserver.rs +++ b/src/commands/webserver.rs @@ -1,66 +1,18 @@ -use std::process::Command as ProcessCommand; - -use anyhow::{Context, Result}; +use anyhow::{Result, bail}; use clap::Args; -use serde::{Deserialize, Serialize}; -use serde_json::Value; -use tiny_http::{Header, Method, Request, Response, Server, StatusCode}; use crate::{app::Cli, commands::Command}; #[derive(Debug, Args)] pub struct WebserverArgs { - /// Host interface to bind to + /// Host interface to bind to. Ignored: the server is disabled. #[arg(long, default_value = "127.0.0.1")] pub host: String, - /// TCP port to listen on + /// TCP port to listen on. Ignored: the server is disabled. #[arg(long, default_value_t = 8765)] pub port: u16, } -#[derive(Debug, Deserialize)] -struct CommandRequest { - args: Option>, -} - -#[derive(Debug, Serialize)] -#[serde(tag = "status", rename_all = "snake_case")] -enum CommandResponse { - Success { - data: Value, - }, - Error { - error: String, - #[serde(flatten)] - details: ErrorDetails, - }, -} - -#[derive(Debug, Serialize, Default)] -struct ErrorDetails { - #[serde(skip_serializing_if = "Option::is_none")] - returncode: Option, - #[serde(skip_serializing_if = "Option::is_none")] - stderr: Option, - #[serde(skip_serializing_if = "Option::is_none")] - stdout: Option, -} - -#[derive(Debug)] -struct RequestError { - status: StatusCode, - message: String, -} - -impl RequestError { - fn new(status: StatusCode, message: impl Into) -> Self { - Self { - status, - message: message.into(), - } - } -} - impl Command for WebserverArgs { fn run_with_ctx( &self, @@ -68,163 +20,12 @@ impl Command for WebserverArgs { _out: &mut dyn std::io::Write, _ctx: &mut dyn crate::cmd_ctx::CmdCtx, ) -> Result<()> { - let addr = format!("{}:{}", self.host, self.port); - let server = - Server::http(&addr).map_err(|err| anyhow::anyhow!("failed to bind {addr}: {err}"))?; - eprintln!("things3 webserver listening on http://{addr}"); - - for request in server.incoming_requests() { - if let Err(err) = handle_request(request) { - eprintln!("webserver request error: {err}"); - } - } - - Ok(()) - } -} - -fn handle_request(mut request: Request) -> Result<()> { - let (status, payload) = match process_request(&mut request) { - Ok(response) => (StatusCode(200), response), - Err(err) => ( - err.status, - CommandResponse::Error { - error: err.message, - details: Default::default(), - }, - ), - }; - - send_json(request, status, &payload) -} - -fn process_request(request: &mut Request) -> std::result::Result { - if request.method() != &Method::Post { - return Err(RequestError::new(StatusCode(405), "method not allowed")); - } - - if request.url() != "/" { - return Err(RequestError::new(StatusCode(404), "not found")); - } - - let mut body = String::new(); - request - .as_reader() - .read_to_string(&mut body) - .map_err(|_| RequestError::new(StatusCode(400), "failed reading request body"))?; - - let parsed = if body.trim().is_empty() { - CommandRequest { args: None } - } else { - serde_json::from_str::(&body).map_err(|err| { - RequestError::new(StatusCode(400), format!("invalid JSON payload: {err}")) - })? - }; - - let args = normalize_args(parsed.args); - - execute_command(&args).map_err(|err| { - RequestError::new(StatusCode(500), format!("failed to execute command: {err}")) - }) -} - -fn normalize_args(args: Option>) -> Vec { - let mut out = args - .unwrap_or_default() - .into_iter() - .map(|s| s.trim().to_string()) - .filter(|s| !s.is_empty()) - .collect::>(); - - if out.is_empty() { - out.push("today".to_string()); - } - - out -} - -fn execute_command(args: &[String]) -> Result { - let exe = std::env::current_exe().context("failed to locate current executable")?; - - let mut argv = args.to_vec(); - if !argv.iter().any(|v| v == "--json") { - argv.insert(0, "--json".to_string()); - } - - let output = ProcessCommand::new(exe) - .args(&argv) - .output() - .context("failed to execute command")?; - - let stdout = String::from_utf8_lossy(&output.stdout).trim().to_string(); - let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); - - if !output.status.success() { - return Ok(command_error_response( - "command failed", - output.status.code(), - stdout, - stderr, - )); - } - - match serde_json::from_str::(&stdout) { - Ok(data) => Ok(CommandResponse::Success { data }), - Err(_) => Ok(command_error_response( - "command did not return JSON output", - output.status.code(), - stdout, - stderr, - )), - } -} - -fn command_error_response( - message: &str, - returncode: Option, - stdout: String, - stderr: String, -) -> CommandResponse { - CommandResponse::Error { - error: message.to_string(), - details: ErrorDetails { - returncode, - stderr: (!stderr.is_empty()).then_some(stderr), - stdout: (!stdout.is_empty()).then_some(stdout), - }, - } -} - -fn send_json(request: Request, status: StatusCode, payload: &T) -> Result<()> { - let body = serde_json::to_string(payload)?; - let content_type = Header::from_bytes(&b"Content-Type"[..], &b"application/json"[..]) - .expect("static content-type header should always be valid"); - - let response = Response::from_string(body) - .with_status_code(status) - .with_header(content_type); - request.respond(response)?; - Ok(()) -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn normalize_args_defaults_to_today() { - assert_eq!(normalize_args(None), vec!["today"]); - assert_eq!( - normalize_args(Some(vec!["".to_string(), " ".to_string()])), - vec!["today"] - ); - } - - #[test] - fn normalize_args_trims_entries() { - assert_eq!( - normalize_args(Some(vec![" today ".to_string(), " ".to_string()])), - vec!["today"] - ); + bail!( + "things3 webserver is disabled (requested {}:{}). It accepted \ + unauthenticated HTTP commands and could be asked to listen on a \ + public address. Run the things3 CLI directly.", + self.host, + self.port + ) } } diff --git a/src/dirs.rs b/src/dirs.rs index ec038ae..e07c5b2 100644 --- a/src/dirs.rs +++ b/src/dirs.rs @@ -1,5 +1,6 @@ use std::{ - fs, + fs::{self, File}, + io::{self, Read, Write}, path::{Path, PathBuf}, }; @@ -61,6 +62,125 @@ pub fn create_private_dir(dir: &Path) -> std::io::Result<()> { Ok(()) } +/// Write `bytes` to `path` as mode 0600 without following a symlink at the +/// staging path or replacing the final path through a symlink. +pub fn write_private_atomic(path: &Path, bytes: &[u8]) -> io::Result<()> { + if let Some(parent) = path + .parent() + .filter(|parent| !parent.as_os_str().is_empty()) + { + create_private_dir(parent)?; + } + refuse_symlink(path)?; + + let tmp = path.with_extension("tmp"); + let _ = fs::remove_file(&tmp); + let mut opts = fs::OpenOptions::new(); + opts.write(true).create_new(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + opts.mode(0o600); + opts.custom_flags(o_nofollow()); + } + let mut file = opts.open(&tmp)?; + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + fs::set_permissions(&tmp, fs::Permissions::from_mode(0o600))?; + } + file.write_all(bytes)?; + file.sync_all()?; + fs::rename(&tmp, path)?; + Ok(()) +} + +/// Append to `path`, creating it mode 0600. A symlink at `path` is refused. +pub fn open_private_append(path: &Path) -> io::Result { + if let Some(parent) = path + .parent() + .filter(|parent| !parent.as_os_str().is_empty()) + { + create_private_dir(parent)?; + } + refuse_symlink(path)?; + + let mut opts = fs::OpenOptions::new(); + opts.create(true).append(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + opts.mode(0o600); + opts.custom_flags(o_nofollow()); + } + let file = opts.open(path)?; + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + fs::set_permissions(path, fs::Permissions::from_mode(0o600))?; + } + Ok(file) +} + +/// Open `path` for reading. A symlink is refused. +pub fn open_private_read(path: &Path) -> io::Result { + refuse_symlink(path)?; + let mut opts = fs::OpenOptions::new(); + opts.read(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + opts.custom_flags(o_nofollow()); + } + opts.open(path) +} + +/// Read `path` to a string. `Ok(None)` means the file is absent. +/// A symlink is an error, so a planted link cannot supply a history key. +pub fn read_private_to_string(path: &Path) -> io::Result> { + match fs::symlink_metadata(path) { + Err(err) if err.kind() == io::ErrorKind::NotFound => return Ok(None), + Err(err) => return Err(err), + Ok(meta) if meta.file_type().is_symlink() => { + return Err(io::Error::new( + io::ErrorKind::AlreadyExists, + format!("refusing to follow symlink at {}", path.display()), + )); + } + Ok(_) => {} + } + let mut file = open_private_read(path)?; + let mut buf = String::new(); + file.read_to_string(&mut buf)?; + Ok(Some(buf)) +} + +fn refuse_symlink(path: &Path) -> io::Result<()> { + match fs::symlink_metadata(path) { + Ok(meta) if meta.file_type().is_symlink() => Err(io::Error::new( + io::ErrorKind::AlreadyExists, + format!("refusing to follow symlink at {}", path.display()), + )), + _ => Ok(()), + } +} + +#[cfg(unix)] +fn o_nofollow() -> i32 { + #[cfg(target_os = "linux")] + { + 0o400000 + } + #[cfg(target_os = "macos")] + { + 0x0100 + } + #[cfg(not(any(target_os = "linux", target_os = "macos")))] + { + 0 + } +} + #[cfg(all(test, unix))] mod tests { use super::*; @@ -78,6 +198,20 @@ mod tests { assert_eq!(mode_of(&dir), 0o700); } + #[test] + fn private_write_does_not_follow_a_symlink() { + let tmp = tempfile::tempdir().expect("tempdir"); + let path = tmp.path().join("cursor.json"); + let victim = tmp.path().join("victim"); + fs::write(&victim, "original").expect("seed victim"); + std::os::unix::fs::symlink(&victim, path.with_extension("tmp")).expect("plant symlink"); + + write_private_atomic(&path, b"{\"history_key\":\"secret\"}").expect("write"); + + assert_eq!(fs::read_to_string(&victim).expect("victim"), "original"); + assert_eq!(mode_of(&path), 0o600); + } + #[test] fn narrows_an_existing_world_readable_directory() { let tmp = tempfile::tempdir().expect("tempdir"); diff --git a/src/ids/matching.rs b/src/ids/matching.rs index 54bf7f5..c07af04 100644 --- a/src/ids/matching.rs +++ b/src/ids/matching.rs @@ -82,25 +82,24 @@ fn lcp_len_bytes(a: &[u8], b: &[u8]) -> usize { max } -pub fn prefix_matches<'a>(sorted_ids: &'a [ThingsId], prefix: &str) -> Vec<&'a ThingsId> { +/// Every id whose base58 form starts with `prefix`. +/// +/// Callers pass ids sorted by raw-byte order. That order is not base58 order, +/// so matches for one prefix are not contiguous. Scanning must not stop at the +/// first miss, or an ambiguous prefix is treated as a single task and a write +/// lands on the wrong item. +pub fn prefix_matches<'a>(ids: &'a [ThingsId], prefix: &str) -> Vec<&'a ThingsId> { let prefix = prefix.as_bytes(); if prefix.is_empty() { - return sorted_ids.iter().collect(); + return ids.iter().collect(); } - let mut matches = Vec::new(); - let mut collecting = false; - for id in sorted_ids { - let (buf, len) = base58_encode_fixed(id.as_bytes()); - let encoded = &buf[..len]; - if encoded.starts_with(prefix) { - matches.push(id); - collecting = true; - } else if collecting { - break; - } - } - matches + ids.iter() + .filter(|id| { + let (buf, len) = base58_encode_fixed(id.as_bytes()); + buf[..len].starts_with(prefix) + }) + .collect() } #[cfg(test)] diff --git a/src/ids/things_id.rs b/src/ids/things_id.rs index 02269bb..2ee6d91 100644 --- a/src/ids/things_id.rs +++ b/src/ids/things_id.rs @@ -382,6 +382,38 @@ mod tests { } } + #[test] + fn prefix_matches_keeps_hits_that_byte_order_splits() { + use crate::ids::matching::prefix_matches; + + // Raw-byte order is not base58 order. These three are sorted by bytes. + // The first and third encode with prefix "8"; the middle one does not. + // Stopping at the first miss would edit only one of the two "8" tasks. + let ids = [ + ThingsId([ + 0x01, 0x06, 0x89, 0x85, 0x30, 0x2f, 0xf7, 0xdb, 0x23, 0xf7, 0x54, 0x7d, 0x17, 0xbf, + 0x9f, 0x5f, + ]), + ThingsId([ + 0x01, 0x1e, 0x80, 0x80, 0x06, 0xfa, 0x1f, 0x11, 0xfa, 0xc9, 0x44, 0x0f, 0x3a, 0x1d, + 0x31, 0xed, + ]), + ThingsId([ + 0x40, 0xc5, 0x80, 0x1c, 0x5c, 0x85, 0xd2, 0x0f, 0x8a, 0xef, 0x09, 0x25, 0x1a, 0x6e, + 0x79, 0x45, + ]), + ]; + assert!(ids[0] < ids[1] && ids[1] < ids[2]); + assert!(ids[0].to_string().starts_with('8')); + assert!(!ids[1].to_string().starts_with('8')); + assert!(ids[2].to_string().starts_with('8')); + + let matches = prefix_matches(&ids, "8"); + assert_eq!(matches.len(), 2); + assert_eq!(matches[0], &ids[0]); + assert_eq!(matches[1], &ids[2]); + } + #[test] fn base58_encode_fixed_preserves_sort_order() { let ids: Vec = (0..50).map(|_| ThingsId::random()).collect(); diff --git a/src/log_cache.rs b/src/log_cache.rs index 2fa9e79..1961026 100644 --- a/src/log_cache.rs +++ b/src/log_cache.rs @@ -1,16 +1,18 @@ use std::{ - fs::{self, File, OpenOptions}, io::{BufRead, BufReader, Seek, SeekFrom, Write}, path::{Path, PathBuf}, }; -use anyhow::{Context, Result, anyhow}; +use anyhow::{Context, Result, anyhow, bail}; use serde::{Deserialize, Serialize}; use serde_json::Value; use crate::{ client::ThingsCloudClient, - dirs::create_private_dir, + dirs::{ + create_private_dir, open_private_append, open_private_read, read_private_to_string, + write_private_atomic, + }, store::{RawState, fold_item}, wire::wire_object::WireItem, }; @@ -27,6 +29,9 @@ struct CursorData { history_key: String, #[serde(default)] head_index: i64, + /// Account that produced `history_key`. Empty on cursors written before this field existed. + #[serde(default)] + account_email: String, } #[derive(Debug, Clone, Serialize, Deserialize, Default)] @@ -39,14 +44,61 @@ struct StateCacheData { const STATE_CACHE_VERSION: u8 = 3; -fn read_cursor(path: &Path) -> CursorData { - if !path.exists() { - return CursorData::default(); - } - let Ok(raw) = fs::read_to_string(path) else { - return CursorData::default(); +fn read_cursor(path: &Path) -> Result { + let Some(raw) = read_private_to_string(path)? else { + return Ok(CursorData::default()); }; - serde_json::from_str(&raw).unwrap_or_default() + if raw.trim().is_empty() { + return Ok(CursorData::default()); + } + serde_json::from_str(&raw) + .with_context(|| format!("cursor at {} is not valid JSON", path.display())) +} + +fn same_account(saved: &str, current: &str) -> bool { + saved.trim().eq_ignore_ascii_case(current.trim()) +} + +/// A saved history key is the capability for one Things database. Do not reuse +/// it for a different email, and do not reuse a key that no longer matches the +/// account we can authenticate. +fn bind_cursor_account( + client: &mut ThingsCloudClient, + cursor: &CursorData, + cache_dir: &Path, +) -> Result<()> { + if cursor.history_key.is_empty() { + if client.history_key.is_none() { + client.authenticate()?; + } + return Ok(()); + } + + if !cursor.account_email.is_empty() && !same_account(&cursor.account_email, &client.email) { + bail!( + "state directory {} belongs to {}, current credentials are for {}. Refusing to sync so the two Things Cloud accounts are not mixed.", + cache_dir.display(), + cursor.account_email, + client.email + ); + } + + if cursor.account_email.is_empty() { + let authed = client + .authenticate() + .context("saved history key has no account email; authenticating before reuse")?; + if authed != cursor.history_key { + bail!( + "saved history key in {} does not match the current Things Cloud account. Refusing to sync so two accounts are not mixed.", + cache_dir.display() + ); + } + } + + if client.history_key.is_none() { + client.history_key = Some(cursor.history_key.clone()); + } + Ok(()) } fn write_cursor( @@ -54,39 +106,33 @@ fn write_cursor( next_start_index: i64, history_key: &str, head_index: i64, + account_email: &str, ) -> Result<()> { let payload = serde_json::to_string(&serde_json::json!({ "next_start_index": next_start_index, "history_key": history_key, "head_index": head_index, + "account_email": account_email, "updated_at": crate::client::now_timestamp(), }))?; - let tmp = path.with_extension("tmp"); - fs::write(&tmp, payload)?; - fs::rename(tmp, path)?; + write_private_atomic(path, payload.as_bytes())?; Ok(()) } pub fn sync_append_log(client: &mut ThingsCloudClient, cache_dir: &Path) -> Result<()> { + let state_dir = crate::dirs::app_state_dir(); + if cache_dir.starts_with(&state_dir) { + create_private_dir(&state_dir)?; + } create_private_dir(cache_dir)?; let log_path = cache_dir.join("things.log"); let cursor_path = cache_dir.join("cursor.json"); - let cursor = read_cursor(&cursor_path); + let cursor = read_cursor(&cursor_path)?; + bind_cursor_account(client, &cursor, cache_dir)?; let mut start_index = cursor.next_start_index; - if client.history_key.is_none() { - if !cursor.history_key.is_empty() { - client.history_key = Some(cursor.history_key.clone()); - } else { - let _ = client.authenticate()?; - } - } - - let mut fp = OpenOptions::new() - .create(true) - .append(true) - .open(&log_path) + let mut fp = open_private_append(&log_path) .with_context(|| format!("failed to open {}", log_path.display()))?; loop { @@ -128,6 +174,7 @@ pub fn sync_append_log(client: &mut ThingsCloudClient, cache_dir: &Path) -> Resu start_index, client.history_key.as_deref().unwrap_or_default(), client.head_index, + &client.email, )?; } @@ -143,29 +190,27 @@ pub fn sync_append_log(client: &mut ThingsCloudClient, cache_dir: &Path) -> Resu start_index, ¤t_history_key, client.head_index, + &client.email, )?; } Ok(()) } -fn read_state_cache(cache_dir: &Path) -> (RawState, u64) { +fn read_state_cache(cache_dir: &Path) -> Result<(RawState, u64)> { let path = cache_dir.join("state_cache.json"); - if !path.exists() { - return (RawState::new(), 0); - } - let Ok(raw) = fs::read_to_string(&path) else { - return (RawState::new(), 0); + let Some(raw) = read_private_to_string(&path)? else { + return Ok((RawState::new(), 0)); }; let Ok(cache) = serde_json::from_str::(&raw) else { - return (RawState::new(), 0); + return Ok((RawState::new(), 0)); }; if cache.version != STATE_CACHE_VERSION { - return (RawState::new(), 0); + return Ok((RawState::new(), 0)); } - (cache.state, cache.log_offset) + Ok((cache.state, cache.log_offset)) } fn write_state_cache(cache_dir: &Path, state: &RawState, log_offset: u64) -> Result<()> { @@ -175,9 +220,7 @@ fn write_state_cache(cache_dir: &Path, state: &RawState, log_offset: u64) -> Res log_offset, state: state.clone(), })?; - let tmp = path.with_extension("tmp"); - fs::write(&tmp, payload)?; - fs::rename(tmp, path)?; + write_private_atomic(&path, payload.as_bytes())?; Ok(()) } @@ -187,11 +230,11 @@ pub fn fold_state_from_append_log(cache_dir: &Path) -> Result { return Ok(RawState::new()); } - let (mut state, byte_offset) = read_state_cache(cache_dir); + let (mut state, byte_offset) = read_state_cache(cache_dir)?; let mut new_lines = 0u64; - let mut file = - File::open(&log_path).with_context(|| format!("failed to open {}", log_path.display()))?; + let mut file = open_private_read(&log_path) + .with_context(|| format!("failed to open {}", log_path.display()))?; file.seek(SeekFrom::Start(byte_offset))?; let mut reader = BufReader::new(file); let mut line = String::new(); @@ -265,8 +308,8 @@ pub fn fold_state_from_append_log_or_empty(cache_dir: &Path) -> RawState { fold_state_from_append_log(cache_dir).unwrap_or_default() } -pub fn read_cached_head_index(cache_dir: &Path) -> i64 { - read_cursor(&cache_dir.join("cursor.json")).head_index +pub fn read_cached_head_index(cache_dir: &Path) -> Result { + Ok(read_cursor(&cache_dir.join("cursor.json"))?.head_index) } pub fn sync_append_log_or_err(client: &mut ThingsCloudClient, cache_dir: &Path) -> Result<()> { @@ -275,6 +318,8 @@ pub fn sync_append_log_or_err(client: &mut ThingsCloudClient, cache_dir: &Path) #[cfg(test)] mod tests { + use std::fs; + use super::*; #[test] @@ -304,7 +349,7 @@ mod tests { let task = store.get_task(task_id).expect("Task7 task after refold"); assert_eq!(task.title, "Current task"); assert_eq!(task.entity, crate::wire::wire_object::EntityType::Task7); - let (cached_state, offset) = read_state_cache(cache_dir); + let (cached_state, offset) = read_state_cache(cache_dir).expect("read state cache"); assert_eq!(cached_state, state); assert_eq!(offset, log.len() as u64); } @@ -363,10 +408,10 @@ mod tests { let first_state = fold_state_from_append_log(cache_dir).expect("first fold"); assert_eq!(first_state.len(), 1); - let (_, first_offset) = read_state_cache(cache_dir); + let (_, first_offset) = read_state_cache(cache_dir).expect("read state cache"); assert_eq!(first_offset, (line_one.len() + 1) as u64); - let mut fp = OpenOptions::new() + let mut fp = fs::OpenOptions::new() .append(true) .open(&log_path) .expect("open log for append"); @@ -376,7 +421,42 @@ mod tests { assert_eq!(second_state.len(), 2); let expected_offset = fs::metadata(&log_path).expect("log metadata").len(); - let (_, second_offset) = read_state_cache(cache_dir); + let (_, second_offset) = read_state_cache(cache_dir).expect("read state cache"); assert_eq!(second_offset, expected_offset); } + + #[test] + fn sync_refuses_cursor_for_a_different_account() { + let temp_dir = tempfile::tempdir().expect("tempdir"); + let cache_dir = temp_dir.path(); + fs::write( + cache_dir.join("cursor.json"), + r#"{"next_start_index":4,"history_key":"old-history-key","head_index":4,"account_email":"old@example.com"}"#, + ) + .expect("seed cursor"); + + let mut client = + ThingsCloudClient::new("new@example.com".to_string(), "not-sent".to_string()) + .expect("client"); + let error = sync_append_log(&mut client, cache_dir) + .expect_err("different account must fail before any network use") + .to_string(); + assert!(error.contains("old@example.com"), "{error}"); + assert!(error.contains("new@example.com"), "{error}"); + assert!(client.history_key.is_none()); + assert!(!cache_dir.join("things.log").exists()); + } + + #[cfg(unix)] + #[test] + fn fold_refuses_a_symlinked_log() { + let temp_dir = tempfile::tempdir().expect("tempdir"); + let cache_dir = temp_dir.path(); + let victim = cache_dir.join("victim.log"); + fs::write(&victim, "secret\n").expect("seed"); + std::os::unix::fs::symlink(&victim, cache_dir.join("things.log")).expect("symlink"); + let error = fold_state_from_append_log(cache_dir).expect_err("symlink log"); + let error = format!("{error:#}"); + assert!(error.contains("symlink"), "{error}"); + } } diff --git a/trycmd/delete/multiple.trycmd b/trycmd/delete/multiple.trycmd index 962bf80..c0cc04a 100644 --- a/trycmd/delete/multiple.trycmd +++ b/trycmd/delete/multiple.trycmd @@ -1,5 +1,5 @@ ```trycmd -$ run.sh things3 --today-ts 1699920000 delete A7h5eCi24RvAWKC3Hv3muf MpkEei6ybkFS2n6SXvwfLf +$ run.sh things3 --today-ts 1699920000 --now-ts 1700000111.0 delete A7h5eCi24RvAWKC3Hv3muf MpkEei6ybkFS2n6SXvwfLf × Deleted Alpha A7h5eCi24RvAWKC3Hv3muf × Deleted Work MpkEei6ybkFS2n6SXvwfLf { @@ -7,8 +7,11 @@ $ run.sh things3 --today-ts 1699920000 delete A7h5eCi24RvAWKC3Hv3muf MpkEei6ybkF "changes": { "A7h5eCi24RvAWKC3Hv3muf": { "e": "Task7", - "p": {}, - "t": 2 + "p": { + "md": 1700000111.0, + "tr": true + }, + "t": 1 }, "MpkEei6ybkFS2n6SXvwfLf": { "e": "Area3", diff --git a/trycmd/delete/single.trycmd b/trycmd/delete/single.trycmd index 32d6b85..871b75a 100644 --- a/trycmd/delete/single.trycmd +++ b/trycmd/delete/single.trycmd @@ -1,13 +1,16 @@ ```trycmd -$ run.sh things3 --today-ts 1699920000 delete A7h5eCi24RvAWKC3Hv3muf +$ run.sh things3 --today-ts 1699920000 --now-ts 1700000111.0 delete A7h5eCi24RvAWKC3Hv3muf × Deleted Alpha A7h5eCi24RvAWKC3Hv3muf { "ancestor_index": 0, "changes": { "A7h5eCi24RvAWKC3Hv3muf": { "e": "Task7", - "p": {}, - "t": 2 + "p": { + "md": 1700000111.0, + "tr": true + }, + "t": 1 } } } diff --git a/trycmd/run.sh b/trycmd/run.sh index c888d7b..e59717f 100755 --- a/trycmd/run.sh +++ b/trycmd/run.sh @@ -2,6 +2,9 @@ set -euo pipefail export TZ=UTC +# Test snapshots read the commit body from the debug log. The binary omits it +# unless this is set, so a normal debug log does not contain task notes. +export THINGS3_LOG_COMMIT_BODY=1 # Wrapper for trycmd cases. #