Security: file authority for stored paths, linked files, placed documents, font folders, recovery areas and atomic saves

CodeRabbit findings #7, #72, #92, #94, #95, #99, #100, #140, #158, #174:

- Link search hashes at most the size it looked at (a growing file no longer matches).
- Resolving links reads, decodes and previews each linked file once, shared by every key.
- Placed documents read inside one another resolve links at most 8 deep, never one already
  being read.
- A data set's image path and the placed-document rereader follow stored paths through
  `document_path` (no other computer's share).
- The Links panel looks at a linked file within the roots of the automation caller that
  opened the document.
- A fonts folder set by a confined caller is scanned without following links out of it.
- Recovery refuses an area folder that is a link or another account's.
- Atomic saves keep the replaced file's group and Linux access ACL, or make the replacement
  owner-only.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Dylan M. Taylor
2026-10-24 17:19:59 +07:00
committed by bflatastic
co-authored by Claude Opus 5.5
parent 8a73ed9963
commit 03d2f0e83c
14 changed files with 199 additions and 26 deletions
Generated
+1
View File
@@ -4730,6 +4730,7 @@ dependencies = [
"log",
"proptest",
"roxmltree 0.21.1",
"rustix 1.1.5",
"ruzstd",
"serde",
"serde_json",
+18 -4
View File
@@ -86,13 +86,24 @@ pub fn hash_bytes(bytes: &[u8]) -> String {
}
/// [`hash_bytes`] of everything `r` reads, read 64 KB at a time.
pub fn hash_reader(mut r: impl std::io::Read) -> std::io::Result<String> {
pub fn hash_reader(r: impl std::io::Read) -> std::io::Result<String> {
hash_reader_within(r, u64::MAX).map(|(hash, _)| hash)
}
/// [`hash_reader`] of at most `max` bytes, and how many it read: `max` + 1 when there were more (the
/// hash then covers only the first `max` + 1). A file that grew since its size was looked at isn't
/// read to its new end.
pub fn hash_reader_within(r: impl std::io::Read, max: u64) -> std::io::Result<(String, u64)> {
let mut r = r.take(max.saturating_add(1));
let mut buf = vec![0; 1 << 16];
let mut h = FNV_BASIS;
let (mut h, mut read) = (FNV_BASIS, 0u64);
loop {
match r.read(&mut buf) {
Ok(0) => return Ok(format!("img{h:016x}")),
Ok(n) => h = fnv1a(h, buf.get(..n).unwrap_or_default()),
Ok(0) => return Ok((format!("img{h:016x}"), read)),
Ok(n) => {
h = fnv1a(h, buf.get(..n).unwrap_or_default());
read = read.saturating_add(n as u64);
}
Err(e) if e.kind() == std::io::ErrorKind::Interrupted => {}
Err(e) => return Err(e),
}
@@ -405,6 +416,9 @@ mod tests {
let bytes: Vec<u8> = (0..200_000u32).map(|i| (i * 7 % 251) as u8).collect();
assert_eq!(hash_reader(&bytes[..]).unwrap(), hash_bytes(&bytes));
assert_eq!((hash_reader(&b""[..]).unwrap(), hash_bytes(b"")), ("imgcbf29ce484222325".to_string(), "imgcbf29ce484222325".to_string()));
// Within a size: the whole file at that size; one byte past it when it is longer.
assert_eq!(hash_reader_within(&bytes[..], bytes.len() as u64).unwrap(), (hash_bytes(&bytes), bytes.len() as u64));
assert_eq!(hash_reader_within(&bytes[..], 3).unwrap().1, 4);
}
#[test]
+5
View File
@@ -45,6 +45,11 @@ serde_json = { workspace = true }
thiserror = { workspace = true }
log = { workspace = true }
# Whether a recovery area folder is this account's own (`FolderStore::own_area`). Already in
# Cargo.lock (tempfile, ui-egui), so this adds no new crate.
[target.'cfg(unix)'.dependencies]
rustix = { workspace = true, features = ["std", "process"] }
[dev-dependencies]
vectorcraft-affinity = { workspace = true, features = ["synth"] }
crc32fast = { workspace = true }
+9 -1
View File
@@ -96,7 +96,15 @@ impl Visitor for LinkFiles {
if let Some((Some(saved_size), Some(saved_hash))) = self.saved.get(i)
&& *saved_size == size
&& size <= MAX_HASHED
&& hash.get_or_insert_with(|| std::fs::File::open(path).and_then(vectorcraft_doc::links::hash_reader).ok()).as_ref()
&& hash
.get_or_insert_with(|| {
// Only the size looked at is read: a file growing meanwhile no longer
// matches, rather than being hashed to its new end.
let file = std::fs::File::open(path).ok()?;
let (hash, read) = vectorcraft_doc::links::hash_reader_within(file, size).ok()?;
(read == size).then_some(hash)
})
.as_ref()
== Some(saved_hash)
{
items.push(n + i);
+32 -5
View File
@@ -231,7 +231,12 @@ impl Found {
/// `base` pointing at this file as read now (`bytes`).
fn link(&self, bytes: &[u8], base: &LinkInfo) -> LinkInfo {
LinkInfo { path: self.path.clone(), size: Some(self.size), modified: self.modified, hash: Some(hash_bytes(bytes)), ..base.clone() }
self.link_hashed(hash_bytes(bytes), base)
}
/// [`Self::link`] with the bytes' hash already taken.
fn link_hashed(&self, hash: String, base: &LinkInfo) -> LinkInfo {
LinkInfo { path: self.path.clone(), size: Some(self.size), modified: self.modified, hash: Some(hash), ..base.clone() }
}
}
@@ -542,8 +547,11 @@ fn clip_image(d: &mut Document, id: NodeId, (frame, room): Clip) -> Result<()> {
/// Replace image `id` with the raster at `path`, keeping the object's bounds (what an image
/// variable does on `dataset.select`). The file is linked; a missing or unreadable path is an
/// error so the dataset application can skip that binding.
pub(crate) fn set_image_from_path(d: &mut Document, id: NodeId, path: &str) -> Result<()> {
let path = absolute_path(path);
pub(crate) fn set_image_from_path(d: &mut Document, id: NodeId, path: &str, doc_path: Option<&str>) -> Result<()> {
// A path stored in the document (a data set's value) is followed as its other stored paths
// are: on this computer or the document's own share only, so choosing a data set can't make
// the app connect to another computer.
let path = fileio::document_path(path, doc_path).map_err(|r| EngineError::Other(format!("the image `{path}` isn't followed: {r}")))?;
let bytes = read_file(&path)?;
let read = (link_info(&path, &bytes), fileio::raster_image(&bytes)?);
install(d, &[id], read, Some(Preserve::Bounds))
@@ -618,6 +626,10 @@ impl Resolved {
/// image's bounds, which isn't a change to report. Nothing here is an undo step.
pub fn resolve(d: &mut Document, doc_path: Option<&str>, update: bool, low_res_proxy_eps: bool) -> Resolved {
let mut out = Resolved::default();
// The image (with its preview) and hash of each file read for a preview here, by path: images
// of other keys linking to the same file share them, rather than each reading, decoding and
// keeping a copy of its own.
let mut read_images: BTreeMap<String, (ImageBlob, String)> = BTreeMap::new();
for g in groups(d, None) {
let row = rows(std::iter::once(&g), doc_path);
let unread = g.document.is_none() && g.link.hash.is_none() && g.link.size.is_none() && previewing(d, &g.key);
@@ -655,14 +667,24 @@ pub fn resolve(d: &mut Document, doc_path: Option<&str>, update: bool, low_res_p
}
(Status::Ok, Some(mut f)) => {
let preview = previewing(d, &g.key);
if let Some((blob, hash)) = read_images.get(&f.path).filter(|_| preview && !eps_preview).cloned() {
store_image(d, &g.key, blob, true);
let link = f.link_hashed(hash, &g.link);
if link != g.link {
set_links(d, &g.ids, &link);
}
continue;
}
// Read when the images show the preview, or when the file's details changed.
let bytes = if (preview && !eps_preview) || f.bytes.is_some() { f.read().ok() } else { None };
if preview && !eps_preview {
let Some(img) = bytes.as_deref().and_then(|b| fileio::raster_image(b).ok()) else {
let Some((b, img)) = bytes.as_deref().and_then(|b| Some((b, fileio::raster_image(b).ok()?))) else {
out.missing.extend(row);
continue;
};
store_image(d, &g.key, img.blob, true);
let blob = img.blob.with_proxy();
read_images.insert(f.path.clone(), (blob.clone(), hash_bytes(b)));
store_image(d, &g.key, blob, true);
} else if let Some(b) = d.images.get(&g.key).filter(|b| b.proxy.is_none()) {
// Linked before links kept a preview.
let b = b.clone().with_proxy();
@@ -1266,6 +1288,11 @@ fn placement_json(o: PlacementOptions) -> Value {
/// Link Info's fields of the linked file `l` (of object `id` of `st`) into `out`.
fn file_fields(st: &crate::DocState, id: NodeId, l: &LinkInfo, out: &mut Value) {
// Within what the caller that opened the document may read, whoever asks.
crate::file_access::confine(st.opened_by.as_ref(), || file_fields_here(st, id, l, out));
}
fn file_fields_here(st: &crate::DocState, id: NodeId, l: &LinkInfo, out: &mut Value) {
let status = groups(&st.doc, Some(&[id])).first().map_or(Status::Missing, |g| probe(g, st.path.as_deref()).0);
let doc_path = st.path.as_deref();
let path = fileio::document_path(&l.path, doc_path).ok();
+39 -3
View File
@@ -126,8 +126,13 @@ pub(crate) fn document_art(
return super::pdf_art(name, bytes, path, page as usize, o, bounding, cmd);
}
let mut l = fileio::load(name, bytes)?;
// Its linked images show their files; its placed documents stay as they are.
super::super::links::resolve(&mut l.doc, path, false, false);
// Its linked images show their files; its placed documents stay as they are, unless their
// stored bytes are lost, when resolving reads their files, which can place this one again.
// A document already being read further up, or past [`MAX_NESTED_READS`] of them, has its links
// left as stored.
if let Some(_reading) = Reading::enter(bytes) {
super::super::links::resolve(&mut l.doc, path, false, false);
}
l.doc.drop_edit_modes();
let n = l.doc.artboards.len();
let board = (page as usize).checked_sub(1).and_then(|i| l.doc.artboards.get(i)).map(|a| a.rect);
@@ -140,6 +145,33 @@ pub(crate) fn document_art(
Ok(DocumentArt { doc: l.doc, nodes, clip: (!bounding).then_some(board), frame, warnings: l.warnings })
}
/// The most placed documents read inside one another while their links are resolved.
const MAX_NESTED_READS: usize = 8;
std::thread_local! {
/// The hashes of the documents whose links are being resolved on this thread, outermost first.
static READING: std::cell::RefCell<Vec<String>> = const { std::cell::RefCell::new(Vec::new()) };
}
/// A document's links being resolved ([`READING`]): the entry goes when this does.
struct Reading;
impl Reading {
/// `None` when the document of `bytes` is already being read, or [`MAX_NESTED_READS`] are.
fn enter(bytes: &[u8]) -> Option<Self> {
let hash = hash_bytes(bytes);
READING.with_borrow_mut(|r| (r.len() < MAX_NESTED_READS && !r.contains(&hash)).then(|| r.push(hash))).map(|()| Reading)
}
}
impl Drop for Reading {
fn drop(&mut self) {
READING.with_borrow_mut(|r| {
r.pop();
});
}
}
/// `nodes` as one group, clipped to `clip` (its own ids: what drawing reads).
fn group(nodes: Vec<Node>, clip: Option<Rect>) -> Node {
let mut children: Vec<Arc<Node>> = nodes.into_iter().map(Arc::new).collect();
@@ -159,8 +191,12 @@ pub(crate) fn read_document(p: &PlacedDocument, bytes: &[u8]) -> Option<(Documen
/// `p`'s file read again, while it is the file it was read from (how the document model gets it
/// when only the preview is stored, [`vectorcraft_doc::placed_document::set_file_reader`]).
pub(crate) fn read_file_again(p: &PlacedDocument) -> Option<Vec<u8>> {
// The stored path is followed as stored paths are, but with no document to share a network
// folder with: a file on another computer is never read here, even one resolving refused
// (which leaves the stored path in place), so embedding or exporting can't connect to it.
let path = fileio::document_path(&p.link.path, None).ok()?;
// One byte over the limit tells a file that is too big, without reading it all.
let bytes = fileio::read_file_head(&p.link.path, MAX_FILE + 1).ok()?;
let bytes = fileio::read_file_head(&path, MAX_FILE + 1).ok()?;
if bytes.len() > MAX_FILE {
return None;
}
+3 -1
View File
@@ -543,7 +543,9 @@ impl Session {
self.prefs = p;
if fonts_folder_changed {
let folder = self.prefs.fonts_folder.trim();
vectorcraft_text::set_user_font_dirs(if folder.is_empty() { vec![] } else { vec![folder.into()] });
// A folder an automation caller chose is read without following its links out of it.
let confined = crate::file_access::current().is_some();
vectorcraft_text::set_user_font_dirs_confined(if folder.is_empty() { vec![] } else { vec![folder.into()] }, confined);
// Once the fonts were scanned, scan again now: the folder's fonts appear (or go) in the
// font menus, and type in them lays out again. The first scan reads it anyway.
if vectorcraft_text::FontDb::global().installed_fonts_changed() {
+18
View File
@@ -264,6 +264,22 @@ impl FolderStore {
builder.create(dir).map_err(|e| format!("{}: {e}", dir.display()))
}
/// Refuse area folder `dir` unless it is a folder itself, not a link to one, and (on Unix)
/// belongs to this account. Listing areas leaves links out, so a name another account had
/// linked to a folder of its own in a shared recovery folder looked free, and claiming it
/// wrote the copies through the link.
fn own_area(dir: &std::path::Path) -> std::result::Result<(), String> {
let m = std::fs::symlink_metadata(dir).map_err(|e| format!("{}: {e}", dir.display()))?;
if !m.file_type().is_dir() {
return Err(format!("{}: not a folder of its own", dir.display()));
}
#[cfg(unix)]
if std::os::unix::fs::MetadataExt::uid(&m) != rustix::process::geteuid().as_raw() {
return Err(format!("{}: belongs to another account", dir.display()));
}
Ok(())
}
/// The plain names of the files (or, `dirs`, folders) in `dir`; none when it doesn't exist.
fn names(dir: &std::path::Path, dirs: bool) -> std::result::Result<Vec<String>, String> {
let entries = match std::fs::read_dir(dir) {
@@ -298,6 +314,7 @@ impl RecoveryStore for FolderStore {
let f = self.file(name)?;
if let Some(dir) = f.parent() {
Self::create_area(dir)?;
Self::own_area(dir)?;
}
vectorcraft_format::write_atomic(&f, bytes).map_err(|e| format!("{}: {e}", f.display()))
}
@@ -317,6 +334,7 @@ impl RecoveryStore for FolderStore {
fn lock(&self, area: &str) -> std::result::Result<Lock, String> {
let dir = self.area(area)?;
Self::create_area(&dir)?;
Self::own_area(&dir)?;
let path = dir.join(LOCK_FILE);
let file = std::fs::OpenOptions::new()
.read(true)
+2 -1
View File
@@ -469,6 +469,7 @@ fn dataset_list(s: &mut Session, _p: &Value) -> Result<Value> {
fn apply_dataset(s: &mut Session, name: &str) -> Result<Value> {
const C: &str = "dataset.select";
let doc_path = s.doc()?.path.clone();
let (applied, skipped, mut typed, bound) = s.edit("Apply Data Set", |d, sel| {
let ds = d.variables.dataset(name).cloned().ok_or_else(|| bad(C, format!("no dataset named `{name}`")))?;
// One object may be driven by several variables; walk them in definition order, so two
@@ -513,7 +514,7 @@ fn apply_dataset(s: &mut Session, name: &str) -> Result<Value> {
true
}),
// Swap the linked file; keep the object's bounds. A missing path skips.
(VariableKind::Image, DataValue::Image(path)) => links::set_image_from_path(d, id, path).is_ok(),
(VariableKind::Image, DataValue::Image(path)) => links::set_image_from_path(d, id, path, doc_path.as_deref()).is_ok(),
(VariableKind::Graph, DataValue::Graph(csv)) => super::graph::apply_csv(d, id, csv).is_ok(),
_ => false,
};
+5
View File
@@ -401,6 +401,10 @@ pub struct DocState {
/// Transform Again after a perspective move or scale (Perspective Selection tool): the
/// `perspective.transform` params it repeats. `None` once an ordinary transform follows.
pub last_perspective: Option<Value>,
/// The folders a confined (automation) caller that opened or made the document may read: the
/// paths the document links to are looked at within them later too, when the Links panel
/// asks about a link outside any caller's scope.
pub opened_by: Option<Arc<file_access::AutomationRoots>>,
}
static NEXT_DOC_UID: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(1);
@@ -574,6 +578,7 @@ impl DocState {
imported_from: None,
import_losses: vec![],
last_perspective: None,
opened_by: file_access::current(),
}
}
/// Unsaved changes: the document differs from the saved one (selection changes and changes that
+4 -3
View File
@@ -15,9 +15,10 @@ serde_json = { workspace = true }
thiserror = { workspace = true }
flate2 = { workspace = true }
# A plain fsync where a volume refuses the full flush (`sync_file`, atomic.rs). Already in
# Cargo.lock (tempfile), so this adds no new crate.
[target.'cfg(target_vendor = "apple")'.dependencies]
# A plain fsync where a volume refuses the full flush (`sync_file`, atomic.rs), and the replaced
# file's access ACL on Linux (`keep_permissions`). Already in Cargo.lock (tempfile), so this adds
# no new crate.
[target.'cfg(unix)'.dependencies]
rustix = { workspace = true, features = ["std", "fs"] }
[dev-dependencies]
+31 -6
View File
@@ -261,15 +261,40 @@ fn temp_path(target: &Path) -> io::Result<PathBuf> {
Ok(target.with_file_name(format!(".{}.{}-{n}.tmp", name.to_string_lossy(), std::process::id())))
}
/// The replacement keeps the replaced file's permissions (Unix; Windows files have none to keep
/// beyond read-only, which makes the save fail as writing in place would).
/// The replacement keeps the replaced file's permissions, group and (Linux) access ACL: the same
/// mode bits mean other people when the group differs, and an ACL can narrow who reads the file.
/// When the group or the ACL can't be kept, the replacement is readable by its owner alone rather
/// than by people the old file didn't let in. (Unix; Windows files have none to keep beyond
/// read-only, which makes the save fail as writing in place would.)
#[cfg(unix)]
fn keep_permissions(target: &Path, tmp: &Path) -> io::Result<()> {
match std::fs::metadata(target) {
Ok(m) => std::fs::set_permissions(tmp, m.permissions()),
// A new file: the default permissions.
Err(_) => Ok(()),
use std::os::unix::fs::{MetadataExt, PermissionsExt};
// A new file: the default permissions.
let Ok(m) = std::fs::metadata(target) else { return Ok(()) };
let mut mode = m.permissions().mode();
let kept_group = std::fs::metadata(tmp).is_ok_and(|t| t.gid() == m.gid()) || std::os::unix::fs::chown(tmp, None, Some(m.gid())).is_ok();
if !kept_group || !keep_acl(target, tmp) {
mode &= !0o077;
}
// After the ACL: the mode's group bits are its mask, kept as they were.
std::fs::set_permissions(tmp, std::fs::Permissions::from_mode(mode))
}
/// Give `tmp` the access ACL of `target`; `true` when it has none (or its volume keeps none).
#[cfg(target_os = "linux")]
fn keep_acl(target: &Path, tmp: &Path) -> bool {
use rustix::io::Errno;
const ACL: &str = "system.posix_acl_access";
let mut buf = vec![0u8; 64 << 10];
match rustix::fs::getxattr(target, ACL, &mut buf[..]) {
Ok(n) => buf.get(..n).is_some_and(|acl| rustix::fs::setxattr(tmp, ACL, acl, rustix::fs::XattrFlags::empty()).is_ok()),
Err(e) => e == Errno::NODATA || e == Errno::NOTSUP,
}
}
#[cfg(all(unix, not(target_os = "linux")))]
fn keep_acl(_: &Path, _: &Path) -> bool {
true
}
#[cfg(not(unix))]
+30 -1
View File
@@ -1457,12 +1457,34 @@ static USER_FONT_DIRS: std::sync::RwLock<Vec<PathBuf>> = std::sync::RwLock::new(
/// if installed. The next check for installed fonts ([`FontDb::installed_fonts_changed`]) then
/// reports the change, and a scan catalogs them.
pub fn set_user_font_dirs(dirs: Vec<PathBuf>) {
set_user_font_dirs_confined(dirs, false);
}
/// [`set_user_font_dirs`]; `confined` when a caller confined to some folders (automation) chose
/// them: the scan then reads only what is inside them, not files or folders that links in them
/// lead to elsewhere, since the caller could plant such a link.
pub fn set_user_font_dirs_confined(dirs: Vec<PathBuf>, confined: bool) {
#[cfg(not(target_arch = "wasm32"))]
{
*USER_FONT_DIRS.write().unwrap_or_else(std::sync::PoisonError::into_inner) = dirs;
USER_FONT_DIRS_CONFINED.store(confined, std::sync::atomic::Ordering::Relaxed);
}
#[cfg(target_arch = "wasm32")]
let _ = dirs;
let _ = (dirs, confined);
}
/// Whether [`USER_FONT_DIRS`] were chosen by a confined caller ([`set_user_font_dirs_confined`]).
#[cfg(not(target_arch = "wasm32"))]
static USER_FONT_DIRS_CONFINED: std::sync::atomic::AtomicBool = std::sync::atomic::AtomicBool::new(false);
/// The folders, as found on disk, that the scan reads only the inside of
/// ([`set_user_font_dirs_confined`]).
#[cfg(not(target_arch = "wasm32"))]
fn confined_font_dirs() -> Vec<PathBuf> {
if !USER_FONT_DIRS_CONFINED.load(std::sync::atomic::Ordering::Relaxed) {
return vec![];
}
user_font_dirs().iter().filter_map(|d| std::fs::canonicalize(d).ok()).collect()
}
/// The folders [`set_user_font_dirs`] added (none on wasm).
@@ -2031,10 +2053,14 @@ impl FontDb {
let mut stack = paths.clone();
// Each folder once, however links lead back to it.
let mut visited = std::collections::HashSet::new();
let confined = confined_font_dirs();
let inside_confined = |p: &Path| std::fs::canonicalize(p).is_ok_and(|r| confined.iter().any(|root| r.starts_with(root)));
while let Some(d) = stack.pop() {
if !visited.insert(std::fs::canonicalize(&d).unwrap_or_else(|_| d.clone())) {
continue;
}
// In a confined folder, a link is followed only to somewhere inside one.
let in_confined = !confined.is_empty() && inside_confined(&d);
// Before listing it: a font installed meanwhile changes it after this.
modified_at.push((d.clone(), modified(&d)));
// Not a folder: a font file named by itself (or nothing, yet).
@@ -2044,6 +2070,9 @@ impl FontDb {
};
for e in rd.flatten() {
let p = e.path();
if in_confined && e.file_type().is_ok_and(|t| t.is_symlink()) && !inside_confined(&p) {
continue;
}
if p.is_dir() {
stack.push(p);
} else {
+2 -1
View File
@@ -36,7 +36,8 @@ pub use craft_fonts::{CRAFT_FONTS, CraftFont, WEB_FONTS, WebFont};
pub use features::{EXTRA_FEATURES, LIGATURE_TRACKING_LIMITS, OtFeatures, explicit_ligatures, ligatures_suppressed_by};
pub use fontdb::{
FALLBACK_FAMILY, FontClass, FontDb, FontFace, FontMatch, FontTraits, IcfMargins, PlatformFontFiles, WantedFont, WantedFonts, app_font_dir,
is_font_file, is_suitcase, set_app_font_dir, set_platform_font_files, set_user_font_dirs, style_weight, system_font_dirs, user_font_dirs,
is_font_file, is_suitcase, set_app_font_dir, set_platform_font_files, set_user_font_dirs, set_user_font_dirs_confined, style_weight,
system_font_dirs, user_font_dirs,
};
pub use hyphen::{hyphenation_exceptions, set_hyphenation_exceptions};
use std::sync::Arc;