Give the log a mode that the way off the device can open

The log file has been on external storage since it existed, and the
reason is stated at length in two places: /data/data/<pkg>/files needs
run-as against a debuggable build, /sdcard/Android/data/<pkg>/files is a
plain adb pull from any build, and a log nobody can retrieve is not a
diagnostic. The file was then opened 0600, which cancels that decision
out. On the tablet:

    adb pull      -> remote open failed: Permission denied
    adb shell cat -> Permission denied
    run-as        -> package not debuggable

Every route off the device closed at once, on a file whose whole purpose
is to leave the device.

The mode is now per platform, because "who may read this" has two
different answers and the directory above the file is what makes them
differ. On the desktop, 0600 as before: $XDG_STATE_HOME/darkroom is in a
home directory on a machine that may have other accounts, and nothing
about that directory stops another local user reading a world-readable
file. On Android, 0644: /sdcard/Android/data is drwxrws--x
media_rw:ext_data_rw, so no other app can enter this app's subdirectory
and anyone who can traverse it is holding the unlocked tablet, which
already gets them the photographs the log merely names. What the read
bits buy is adb pull, which runs as shell — able to traverse a --x
directory, but then obliged to open the file as other.

The mode is also applied twice, and the second one is the fix rather
than belt and braces. OpenOptions::mode is a request: the kernel ANDs it
with the process umask, and an Android application process inherits
0o077 from the zygote, so asking for 0644 there creates 0600 and reports
nothing. It is ignored outright on a file that already exists, which
every launch after the first has. fchmod is subject to neither, and is
what the second call makes.

The comment claiming the mode was "ignored by the FAT-derived filesystem
Android presents as external storage" is gone with it. The device says
otherwise: the file it produced was 0600 exactly.

Two tests. One pins the literal mode per platform — only the desktop arm
can run under cargo test, and the comment says so rather than implying
the Android number is covered. The other reopens a log left behind with
the wrong mode, which is the one assertion on the host that fails if the
fchmod is deleted, since OpenOptions::mode cannot touch a file that is
already there.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
2026-08-30 17:31:10 +02:00
co-authored by Claude Opus 5
parent 23c3155128
commit c62edd3317
+131 -8
View File
@@ -23,8 +23,15 @@
//! it was, which matters because losing it while debugging would make this a //! it was, which matters because losing it while debugging would make this a
//! downgrade. [`install`] is the whole of the wiring. //! downgrade. [`install`] is the whole of the wiring.
//! //!
//! Three properties, each of which is a test below: //! Four properties, each of which is a test below — the first of them only
//! half-testable off a device, and the test says which half:
//! //!
//! * **Retrievable.** Created with a mode that the way *this* platform gets a
//! file off the machine can actually open: `FILE_MODE`, the one number here
//! that differs by platform, and the only one whose reasoning is about the
//! directory above the file rather than about the file. Get it wrong and the
//! other three properties are worth nothing, because nobody ever reads the
//! log.
//! * **Capped.** Two files of [`MAX_FILE_BYTES`], so the worst case is stated //! * **Capped.** Two files of [`MAX_FILE_BYTES`], so the worst case is stated
//! rather than discovered on a full phone. //! rather than discovered on a full phone.
//! * **Redacted at the sink** ([`redact`]), not at the call sites. A rule //! * **Redacted at the sink** ([`redact`]), not at the call sites. A rule
@@ -284,20 +291,64 @@ impl LogFile {
} }
} }
/// The mode the log is created with, on the desktop.
///
/// `$XDG_STATE_HOME/darkroom` is inside a home directory on a machine that may
/// have other accounts on it, and this file names the user's photographs and
/// the shape of their library. Nothing about the directory above it stops
/// another local user reading a world-readable file, so the owner is the only
/// reader who has any business with it.
#[cfg(all(unix, not(target_os = "android")))]
const FILE_MODE: u32 = 0o600;
/// The mode the log is created with, on Android — deliberately not the
/// desktop's, because "who may read this" has a different answer here.
///
/// **The directory is the access control, and the file being readable does not
/// widen it.** `/sdcard/Android/data` is `drwxrws--x media_rw:ext_data_rw`: no
/// other application can enumerate or enter this application's subdirectory,
/// and anyone who can traverse it is holding the unlocked tablet, which
/// already gets them the photographs the log merely names.
///
/// What the group and other read bits buy is the entire reason
/// [`crate::state`] puts the file on external storage rather than in
/// `/data/data`: **`adb pull` runs as `shell`**, which can traverse a `--x`
/// directory but must then open the file as *other*. At `0o600` it cannot —
/// and `run-as` is refused on a build that is not debuggable, which a release
/// build is not — so every route off the device is closed at once and the log
/// is a diagnostic nobody can be sent. That is the one outcome the choice of
/// directory exists to prevent.
#[cfg(target_os = "android")]
const FILE_MODE: u32 = 0o644;
fn open_append(path: &Path) -> io::Result<File> { fn open_append(path: &Path) -> io::Result<File> {
let mut options = OpenOptions::new(); let mut options = OpenOptions::new();
options.create(true).append(true); options.create(true).append(true);
// The log names the user's photographs and the shape of their library,
// which is nobody else's business on a shared machine. Applied at
// creation only, which is all that is needed, and ignored by the
// FAT-derived filesystem Android presents as external storage — where the
// directory is already scoped to this application.
#[cfg(unix)] #[cfg(unix)]
{ {
use std::os::unix::fs::OpenOptionsExt as _; use std::os::unix::fs::OpenOptionsExt as _;
options.mode(0o600); options.mode(FILE_MODE);
} }
options.open(path) let file = options.open(path)?;
// And then again, explicitly, because the line above is a *request*: the
// kernel ANDs it with the process umask, and an Android application
// process inherits `0o077` from the zygote. Asking for `0o644` there
// creates `0o600`, silently, and `OpenOptions::mode` is ignored outright
// on a file that already exists — between them, that is how a log written
// for `adb pull` ended up unreadable by it. `fchmod` is subject to
// neither.
//
// Ignored on failure rather than refused. A file this process does not
// own, or a filesystem with no modes to set, is a reason to log without
// the mode; it is not a reason to log nowhere.
#[cfg(unix)]
{
use std::os::unix::fs::PermissionsExt as _;
let _ = file.set_permissions(fs::Permissions::from_mode(FILE_MODE));
}
Ok(file)
} }
/// One record as one line. /// One record as one line.
@@ -418,6 +469,78 @@ mod tests {
format!("{n:04} scanning /home/duncan/Pictures/2026/Iceland for images") format!("{n:04} scanning /home/duncan/Pictures/2026/Iceland for images")
} }
/// The permission bits of a file, as an octal number to compare against.
#[cfg(unix)]
fn mode_of(path: &Path) -> u32 {
use std::os::unix::fs::PermissionsExt as _;
fs::metadata(path).expect("a log").permissions().mode() & 0o777
}
#[cfg(unix)]
#[test]
fn the_log_is_created_with_the_mode_its_platform_can_retrieve_it_by() {
let dir = a_log_dir("mode");
let log = LogFile::open_in(&dir, MAX_FILE_BYTES).expect("opens");
log.write_line("a line the user will be asked for");
// Written as a literal rather than compared against `FILE_MODE`: a
// test that reads the same constant the code writes agrees with
// whatever the constant says, which is not a test of anything.
//
// **Only one of these two arms can ever run.** The Android one is the
// one that matters — `0o600` there closes `adb pull` and `run-as` at
// once, which is the failure this test exists for — and it cannot run
// under `cargo test`, because the behaviour is a property of a
// filesystem and a umask that only exist on a device. What the desktop
// arm does prove is the other half, and it is not nothing: the log
// must not become group- or world-readable in a home directory
// because somebody made one mode serve both platforms.
#[cfg(target_os = "android")]
assert_eq!(
mode_of(&dir.join(FILE_NAME)),
0o644,
"a log only the app can read cannot be sent to anyone (NFR-OPS-1)"
);
#[cfg(not(target_os = "android"))]
assert_eq!(
mode_of(&dir.join(FILE_NAME)),
0o600,
"the log names the user's library, and every other account on \
this machine can now read it"
);
}
#[cfg(unix)]
#[test]
fn a_log_left_by_an_older_build_is_reopened_with_this_mode() {
// The regression test for the actual bug, and the only one that can
// fail on the host: `OpenOptions::mode` applies at *creation* and is
// ANDed with the umask even then, so on its own it neither fixes an
// existing file nor delivers what it asked for in a process whose
// umask is `0o077`, which every Android application process inherits
// from the zygote.
//
// The explicit `set_permissions` is what makes the mode the one the
// platform needs rather than the one the umask happened to allow, and
// deleting it makes this assertion fail: the file already exists, so
// nothing else here can change it.
use std::os::unix::fs::PermissionsExt as _;
let dir = a_log_dir("mode-existing");
let path = dir.join(FILE_NAME);
fs::write(&path, "written by a build that chose a different mode\n").expect("writes");
fs::set_permissions(&path, fs::Permissions::from_mode(0o666)).expect("chmods");
let log = LogFile::open_in(&dir, MAX_FILE_BYTES).expect("opens");
log.write_line("this session");
assert_eq!(
mode_of(&path),
FILE_MODE,
"the mode of an existing log is left as whoever created it left it"
);
}
#[test] #[test]
fn the_log_stays_under_its_stated_cap() { fn the_log_stays_under_its_stated_cap() {
// **This is NFR-OPS-1's "size-capped".** Without rotation a phone left // **This is NFR-OPS-1's "size-capped".** Without rotation a phone left