Many imorovments
Build and test / Desktop (Linux) (push) Failing after 1m8s
Build and test / Android (aarch64) (push) Failing after 2s
Build and test / Layer separation (push) Canceled after 23s
Traceability / Requirement traces (push) Failing after 59s

This commit is contained in:
2026-08-12 22:16:15 +02:00
parent fa12afed18
commit 4d78041d1d
24 changed files with 711 additions and 78 deletions
@@ -0,0 +1,21 @@
-----BEGIN CERTIFICATE-----
MIICpjCCAiugAwIBAgIRAIchZfw0tuX7qK3Vs3BftTowCgYIKoZIzj0EAwMwTzEL
MAkGA1UEBhMCVVMxKTAnBgNVBAoTIEludGVybmV0IFNlY3VyaXR5IFJlc2VhcmNo
IEdyb3VwMRUwEwYDVQQDEwxJU1JHIFJvb3QgWDIwHhcNMjYwNTEzMDAwMDAwWhcN
MzIwOTAyMjM1OTU5WjAuMQswCQYDVQQGEwJVUzENMAsGA1UEChMESVNSRzEQMA4G
A1UEAxMHUm9vdCBZRTB2MBAGByqGSM49AgEGBSuBBAAiA2IABDwS/6vhrcVqcbBo
+wgdI3fwn9x7DNJJOY/lTOti0vkwuRN87RhEhTH17E7XyFjWsPYhIPt/wzOqxTd2
b+4ZJNy9ID04YywF9U5zasDVyGSNErVNtz8uSGh5izW87j77GaOB6zCB6DAOBgNV
HQ8BAf8EBAMCAQYwEwYDVR0lBAwwCgYIKwYBBQUHAwEwDwYDVR0TAQH/BAUwAwEB
/zAdBgNVHQ4EFgQUo8gmWo6hTNA1Y/ybI8g6rlbzT1YwHwYDVR0jBBgwFoAUfEKW
rt5LSDv6kviejM9ti6lyN5UwMgYIKwYBBQUHAQEEJjAkMCIGCCsGAQUFBzAChhZo
dHRwOi8veDIuaS5sZW5jci5vcmcvMBMGA1UdIAQMMAowCAYGZ4EMAQIBMCcGA1Ud
HwQgMB4wHKAaoBiGFmh0dHA6Ly94Mi5jLmxlbmNyLm9yZy8wCgYIKoZIzj0EAwMD
aQAwZgIxAMU19WCtmxVND8UHBZRoma49Z7jPs64Dma0eTu1OChVbB/2J7GV3nvYK
Ax54uk1G9QIxAO0miLVJu8PLNiXXXkiE/gsK3CTRTF/aeo4bMX42Zw40csRU6AC2
6hSW1/IWaas6dg==
-----END CERTIFICATE-----
3 s:C=US, O=Internet Security Research Group, CN=ISRG Root X2
i:C=US, O=Internet Security Research Group, CN=ISRG Root X1
a:PKEY: EC, (secp384r1); sigalg: sha256WithRSAEncryption
v:NotBefore: May 13 00:00:00 2026 GMT; NotAfter: Sep 2 23:59:59 2032 GMT
+3 -3
View File
@@ -57,7 +57,7 @@ pub async fn begin(
.header(reqwest::header::USER_AGENT, user_agent)
.send()
.await
.map_err(|e| RemoteError::Network(e.to_string()))?;
.map_err(super::map_send_error)?;
if !resp.status().is_success() {
return Err(RemoteError::Server {
@@ -94,7 +94,7 @@ pub async fn poll(
.body(format!("token={}", urlencode(&flow.poll.token)))
.send()
.await
.map_err(|e| RemoteError::Network(e.to_string()))?;
.map_err(super::map_send_error)?;
match resp.status().as_u16() {
200 => {
@@ -147,7 +147,7 @@ pub async fn revoke(
.header("OCS-APIRequest", "true")
.send()
.await
.map_err(|e| RemoteError::Network(e.to_string()))?;
.map_err(super::map_send_error)?;
Ok(())
}
+156 -16
View File
@@ -145,7 +145,7 @@ impl NextcloudBackend {
.body(chunk.to_vec())
.send()
.await
.map_err(|e| RemoteError::Network(e.to_string()))?;
.map_err(map_send_error)?;
map_status(resp.status(), path.as_str())?;
}
@@ -164,7 +164,7 @@ impl NextcloudBackend {
.header("OC-Total-Length", total.to_string())
.send()
.await
.map_err(|e| RemoteError::Network(e.to_string()))?;
.map_err(map_send_error)?;
map_status(resp.status(), path.as_str())?;
// The MOVE response carries the assembled file's ETag on Nextcloud,
@@ -191,7 +191,7 @@ impl NextcloudBackend {
.basic_auth(&self.login, Some(&self.password))
.send()
.await
.map_err(|e| RemoteError::Network(e.to_string()))?;
.map_err(map_send_error)?;
// 405 is "already exists", which is exactly what we want.
if resp.status() == 405 {
@@ -218,12 +218,12 @@ impl NextcloudBackend {
.body(body)
.send()
.await
.map_err(|e| RemoteError::Network(e.to_string()))?;
.map_err(map_send_error)?;
map_status(resp.status(), path.as_str())?;
resp.text()
.await
.map_err(|e| RemoteError::Network(e.to_string()))
.map_err(map_send_error)
}
}
@@ -284,14 +284,14 @@ impl RemoteBackend for NextcloudBackend {
let resp = req
.send()
.await
.map_err(|e| RemoteError::Network(e.to_string()))?;
.map_err(map_send_error)?;
let status = resp.status();
map_status(status, &url)?;
let body = resp
.bytes()
.await
.map_err(|e| RemoteError::Network(e.to_string()))?
.map_err(map_send_error)?
.to_vec();
// Nextcloud does not advertise Accept-Ranges, so support is detected
@@ -346,7 +346,7 @@ impl RemoteBackend for NextcloudBackend {
.body(body)
.send()
.await
.map_err(|e| RemoteError::Network(e.to_string()))?;
.map_err(map_send_error)?;
map_status(resp.status(), path.as_str())?;
resp.headers()
@@ -374,7 +374,7 @@ impl RemoteBackend for NextcloudBackend {
let resp = req
.send()
.await
.map_err(|e| RemoteError::Network(e.to_string()))?;
.map_err(map_send_error)?;
map_status(resp.status(), &url)
}
@@ -413,7 +413,7 @@ impl RemoteBackend for NextcloudBackend {
.header("Overwrite", "F")
.send()
.await
.map_err(|e| RemoteError::Network(e.to_string()))?;
.map_err(map_send_error)?;
map_status(resp.status(), &src)
}
@@ -451,7 +451,7 @@ impl RemoteBackend for NextcloudBackend {
.basic_auth(&self.login, Some(&self.password))
.send()
.await
.map_err(|e| RemoteError::Network(e.to_string()))?;
.map_err(map_send_error)?;
// 405 is "already a collection here", which is exactly what the
// caller wanted. Anything else is reported.
@@ -482,7 +482,7 @@ impl RemoteBackend for NextcloudBackend {
.basic_auth(&self.login, Some(&self.password))
.send()
.await
.map_err(|e| RemoteError::Network(e.to_string()))?;
.map_err(map_send_error)?;
// 404 means no preview for this file — expected for RAW, and a
// fall-through rather than an error.
@@ -494,12 +494,56 @@ impl RemoteBackend for NextcloudBackend {
Ok(Some(
resp.bytes()
.await
.map_err(|e| RemoteError::Network(e.to_string()))?
.map_err(map_send_error)?
.to_vec(),
))
}
}
/// Roots that are publicly trusted but not yet in the compiled-in set.
///
/// # Why this exists
///
/// `webpki-roots` is generated from Mozilla's CA program, and lags it. A CA
/// that has begun issuing from a new root is therefore trusted by every
/// browser and by the platform store while still being unknown to this binary
/// — and because [`http_client`] deliberately bypasses the platform store to
/// keep Android working, there is nothing to fall back to. The handshake fails
/// with `UnknownIssuer` against a server that is entirely healthy.
///
/// Observed 2026-08-12: a Let's Encrypt chain anchored at ISRG Root YE. The
/// server offered the full cross-signed path up to ISRG Root X1, which *is*
/// bundled, but rustls anchors on the first certificate it recognises as
/// self-issued and stops rather than continuing to the cross-sign — so the
/// complete chain did not help.
///
/// Each entry is a root that Mozilla already trusts. Removing one once
/// `webpki-roots` catches up is safe; the set is additive and duplicates are
/// harmless.
const EXTRA_ROOTS: &[(&str, &[u8])] = &[(
// ISRG Root YE — cross-signed by ISRG Root X2, valid to 2032-09-02.
// SHA-256 0FC0901CCA2BAE9E9FDBB02D50D02F1094F7B366720869 91B9E897626DC485F0
"ISRG Root YE",
include_bytes!("../certs/isrg-root-ye.pem"),
)];
/// Parse [`EXTRA_ROOTS`] into reqwest certificates.
///
/// A malformed entry is skipped rather than fatal: it costs one root, and
/// failing here would take down every connection including the ones that never
/// needed the extra anchor.
fn extra_roots() -> impl Iterator<Item = reqwest::Certificate> {
EXTRA_ROOTS.iter().filter_map(|(name, pem)| {
match reqwest::Certificate::from_pem(pem) {
Ok(c) => Some(c),
Err(e) => {
log::warn!("bundled root {name} unusable: {e}");
None
}
}
})
}
/// Build an HTTP client with the crypto provider already installed.
///
/// Every entry point that creates a client must go through here. The auth
@@ -525,9 +569,18 @@ pub fn http_client(user_agent: &str) -> Result<reqwest::Client, RemoteError> {
// verifier entirely (see its ClientBuilder TLS setup); the webpki-roots
// feature supplies the roots it then uses. Spike S3 revisits this to
// honour user-installed and enterprise CAs, which bundled roots cannot.
// Empty: the flag is what matters, and the roots come from the
// webpki-roots feature rather than from certificates passed here.
.tls_certs_only(std::iter::empty())
//
// Not empty: webpki-roots trails Mozilla's store, so a root that is
// already issuing publicly-trusted certificates can be missing from it
// for months. [`EXTRA_ROOTS`] carries those, and is additive — the
// webpki-roots set is still installed alongside.
.tls_certs_only(extra_roots())
// A request that hangs forever is indistinguishable from a worker that
// died, and cost a long time to tell apart once. Connect and total
// timeouts turn that into an error the UI can show. Generous enough for
// a slow phone on mobile data; the login poll has its own deadline.
.connect_timeout(std::time::Duration::from_secs(15))
.timeout(std::time::Duration::from_secs(60))
.build()
.map_err(|e| RemoteError::Network(e.to_string()))
}
@@ -550,6 +603,45 @@ fn install_crypto_provider() {
});
}
/// Translate a transport failure into a typed error.
///
/// `reqwest::Error`'s own `Display` is uninformative for exactly the failures
/// that matter — every one of them renders as "error sending request for url
/// (…)" regardless of cause. The reason lives in the `source()` chain, so this
/// walks it: without that, a rejected certificate and an unplugged cable
/// produce identical text, and the app cannot tell "you are offline" from
/// "I do not trust this server".
///
/// TLS is separated because it is the one transport failure that waiting never
/// fixes, and the one that must not trigger offline mode (FR-CAT-9).
fn map_send_error(e: reqwest::Error) -> RemoteError {
// The full chain, which is what makes the log line actionable: rustls
// reports "invalid peer certificate: UnknownIssuer" only at depth 1.
let mut detail = e.to_string();
let mut src: Option<&dyn std::error::Error> = std::error::Error::source(&e);
let mut tls = false;
while let Some(s) = src {
let text = s.to_string();
// rustls surfaces every verification failure through this wording:
// UnknownIssuer, Expired, NotValidForName, BadSignature.
if text.contains("invalid peer certificate")
|| text.contains("certificate")
|| text.contains("CertificateError")
{
tls = true;
}
detail.push_str(": ");
detail.push_str(&text);
src = std::error::Error::source(s);
}
if tls {
RemoteError::Tls(detail)
} else {
RemoteError::Network(detail)
}
}
/// Translate an HTTP status into a typed error.
fn map_status(status: reqwest::StatusCode, what: &str) -> Result<(), RemoteError> {
match status.as_u16() {
@@ -713,4 +805,52 @@ mod tests {
Err(RemoteError::Unsupported(_))
));
}
#[test]
fn every_bundled_root_parses() {
// A typo'd or truncated PEM would otherwise be discovered only as a
// silently missing anchor on the one server that needs it.
assert_eq!(
extra_roots().count(),
EXTRA_ROOTS.len(),
"every bundled root must parse"
);
}
#[test]
fn isrg_root_ye_is_bundled_until_webpki_roots_carries_it() {
// The anchor this crate had to supply itself. Delete this and the
// entry it guards once webpki-roots ships Root YE.
assert!(EXTRA_ROOTS.iter().any(|(n, _)| *n == "ISRG Root YE"));
}
}
/// Tests that need the real server. Run with `--ignored`.
#[cfg(test)]
mod live {
/// TRACES: FR-NC-12
/// The regression: a Root YE chain must complete the handshake.
///
/// Ignored because it needs the network, but kept because the unit tests
/// cannot catch this — a bundled root that parses is not the same as a
/// bundled root that verifies, and the gap between those two is exactly
/// what reported a healthy server as offline for a day.
#[tokio::test]
#[ignore = "needs network"]
async fn a_lets_encrypt_root_ye_server_verifies() {
let client = super::http_client("DarkRoom-test").expect("client");
// Any host on the newer ISRG anchor exercises this.
let r = client
.get("https://nextcloud.tourolle.paris/remote.php/dav/files/dtourolle/")
.send()
.await;
match r {
// 401 is a completed TLS handshake; auth is not what is under test.
Ok(resp) => assert_eq!(resp.status().as_u16(), 401, "handshake completed"),
Err(e) => panic!("handshake failed: {e}"),
}
}
}
+41 -7
View File
@@ -20,6 +20,41 @@ use serde::{Deserialize, Serialize};
use crate::AppCredentials;
/// Where configuration is written, when the platform has told us.
///
/// Android has no `$HOME` and no XDG directories, so the guess below resolves
/// to a path the app cannot write. Nothing failed loudly: the session list went
/// to a doomed path, so credentials survived only as long as the process did and
/// backgrounding the app lost the account (ARCH §6.9 — no core API may assume a
/// filesystem path on Android).
///
/// The platform layer sets this once at startup, before any store is opened.
static DATA_DIR: std::sync::OnceLock<PathBuf> = std::sync::OnceLock::new();
/// TRACES: FR-NC-2
/// Declare the per-app directory configuration belongs in.
///
/// Call before opening any store; later calls are ignored rather than racing.
/// On Android this is `AndroidApp::internal_data_path`, which is private to the
/// app and survives being backgrounded. Desktop needs no call — the XDG
/// fallback is correct there.
pub fn set_data_dir(dir: PathBuf) {
let _ = DATA_DIR.set(dir);
}
/// The directory configuration lives in.
fn config_dir() -> PathBuf {
if let Some(d) = DATA_DIR.get() {
return d.clone();
}
std::env::var_os("XDG_CONFIG_HOME")
.map(PathBuf::from)
.unwrap_or_else(|| {
PathBuf::from(std::env::var("HOME").unwrap_or_default()).join(".config")
})
.join("darkroom")
}
/// A configured account, minus its credential.
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
pub struct Session {
@@ -119,17 +154,16 @@ impl SessionStore {
/// Linux: `$XDG_CONFIG_HOME/darkroom/sessions.json`, falling back to
/// `~/.config` (FR-PLAT-LIN-1).
pub fn open(secrets: Box<dyn SecretStore>) -> Self {
let dir = std::env::var_os("XDG_CONFIG_HOME")
.map(PathBuf::from)
.unwrap_or_else(|| {
PathBuf::from(std::env::var("HOME").unwrap_or_default()).join(".config")
})
.join("darkroom");
Self::open_at(dir.join("sessions.json"), secrets)
Self::open_at(config_dir().join("sessions.json"), secrets)
}
/// Open at an explicit path — used by tests, and by anything wanting a
/// non-default config location.
/// Where configuration lives, for callers that need to sit files beside it.
pub fn data_dir() -> PathBuf {
config_dir()
}
pub fn open_at(config_path: PathBuf, secrets: Box<dyn SecretStore>) -> Self {
Self {
config_path,
+44
View File
@@ -37,6 +37,20 @@ pub enum RemoteError {
#[error("network error: {0}")]
Network(String),
/// The TLS handshake was refused: an untrusted issuer, an expired
/// certificate, a hostname mismatch.
///
/// **Distinct from [`Network`](Self::Network) on purpose.** The server is
/// there and answering — the connection is refused on trust grounds, which
/// no amount of waiting repairs. Folding it into `Network` puts the app
/// into offline mode and tells the user their connection is down, sending
/// them to inspect a network that is working perfectly (observed
/// 2026-08-12: a Let's Encrypt chain anchored at ISRG Root YE, absent from
/// the compiled-in root store, reported as "you are offline" on a server
/// answering in 19 ms).
#[error("TLS error: {0}")]
Tls(String),
#[error("unexpected server response: {status} {detail}")]
Server { status: u16, detail: String },
@@ -52,6 +66,9 @@ impl RemoteError {
pub fn is_transient(&self) -> bool {
match self {
RemoteError::Network(_) => true,
// Not transient: a rejected certificate is rejected identically on
// every retry. Retrying one buys nothing and hides the cause.
RemoteError::Tls(_) => false,
RemoteError::Server { status, .. } => {
// 5xx and 429 are worth retrying; other 4xx are not.
//
@@ -80,6 +97,10 @@ impl RemoteError {
/// is just failing, and a retry is the right response rather than a
/// mode change. 429 and 423 likewise — those are the server working
/// correctly under load.
///
/// [`Tls`](Self::Tls) is likewise not offline. The host resolved, the
/// socket connected, and the peer answered; only trust failed. Reporting
/// that as offline is what made a root-store gap look like a dead network.
pub fn indicates_offline(&self) -> bool {
matches!(self, RemoteError::Network(_))
}
@@ -89,6 +110,29 @@ impl RemoteError {
mod tests {
use super::*;
#[test]
fn a_tls_failure_is_not_offline() {
// The regression this guards: a rejected certificate was reported as
// `Network`, which put the whole app into offline mode and told the
// user their connection was down — while the server answered in 19 ms.
let e = RemoteError::Tls("invalid peer certificate: UnknownIssuer".into());
assert!(
!e.indicates_offline(),
"trust failure is not unreachability"
);
assert!(
!e.is_transient(),
"a rejected certificate is rejected identically on every retry"
);
}
#[test]
fn a_dead_connection_is_still_offline() {
// The other side of the split: separating TLS out must not stop a
// genuine transport failure from reaching offline mode.
assert!(RemoteError::Network("dns error".into()).indicates_offline());
}
#[test]
fn transient_errors_are_retryable() {
assert!(RemoteError::Network("timeout".into()).is_transient());
+32
View File
@@ -505,6 +505,38 @@ mod tests {
assert_eq!(parsed.cache, CacheSettings::default());
}
#[test]
fn the_cache_defaults_to_a_bounded_budget_that_keeps_what_it_opens() {
// A default of "unlimited" would let the passive cache fill a disk
// before the user knew there was a setting; a default of "keep
// nothing" would re-download every image they reopened. Both budgets
// must therefore be `Some`, and keeping opened originals must be on.
let cache = CacheSettings::default();
assert_eq!(
cache.original_budget_bytes,
Some(DEFAULT_ORIGINAL_BUDGET_BYTES)
);
assert_eq!(
cache.thumbnail_budget_bytes,
Some(DEFAULT_THUMBNAIL_BUDGET_BYTES)
);
assert!(cache.keep_opened_originals);
}
#[test]
fn the_two_cache_budgets_are_independent() {
// Sharing them would let a day of browsing originals evict the
// thumbnails that make the library navigable.
let mut s = Settings::default();
s.cache.original_budget_bytes = None;
s.sanitise();
assert_eq!(
s.cache.thumbnail_budget_bytes,
Some(DEFAULT_THUMBNAIL_BUDGET_BYTES),
"clearing one budget must not disturb the other"
);
}
#[test]
fn upscaling_is_off_by_default() {
// FR-EXP-3 requires an explicit opt-in.