feat(s3): remember saved connection keys

crates/assistant/src/audit.rs

@@ -375,6 +375,10 @@ mod tests {
             "password",
             "api_key",
             "accessKey",
+            "accessKeyId",
+            "access_key_id",
+            "secretAccessKey",
+            "sessionToken",
             "tokens",
             "authToken",
             "client_secret",

crates/assistant/src/chatgpt.rs

@@ -1597,8 +1597,13 @@ mod native {
                     std::thread::sleep(Duration::from_millis(30));
                     *cancel.lock().unwrap() = true;
                 });
-                let e = cancellable(&cancel, Instant::now() + Duration::from_secs(10), || {
-                    std::thread::sleep(Duration::from_millis(150));
+                // The work outlasts the cancel however late the cancelling thread runs.
+                let flag = cancel.clone();
+                let e = cancellable(&cancel, Instant::now() + Duration::from_secs(10), move || {
+                    let end = Instant::now() + Duration::from_secs(5);
+                    while !*flag.lock().unwrap() && Instant::now() < end {
+                        std::thread::sleep(Duration::from_millis(5));
+                    }
                     Ok(())
                 })
                 .unwrap_err();

crates/assistant/src/tests.rs

@@ -268,6 +268,11 @@ fn gate_rules() {
     assert!(g("run_command", json!({"command": "plugin.install", "params": {"path": "x.rgplugin"}})).is_some());
     assert!(g("run_command", json!({"command": "map.addTiles", "params": {"url": "https://tile.example/{z}/{x}/{y}.png"}})).is_some());
     assert!(g("run_command", json!({"command": "plugin:x/y"})).is_some());
+    // Saved S3 keys and the choice of a connection are the user's: asked for, every time.
+    for command in ["connection.forgetSavedKeys", "connection.resolveS3"] {
+        assert!(g("run_command", json!({"command": command, "params": {"id": "c1", "source": "s3://bucket/key"}})).is_some(), "{command}");
+    }
+    assert!(g("run_command", json!({"command": "connection.login", "params": {"id": "c1", "remember": true}})).is_some());
     let sql = g("run_command", json!({"command": "layer.addSql", "params": {"sql": "SELECT ST_Point(0, 0) AS g"}}));
     assert!(sql.as_deref().is_some_and(|w| w.contains("SQL") && w.contains("internet")), "{sql:?}");
     let sql = g("run_command", json!({"command": "layer.addSql", "params": {"sql": "FROM t", "database": "/data/x.duckdb"}}));

crates/data/src/lib.rs

@@ -24,6 +24,7 @@ pub mod osm;
 pub mod photo;
 pub mod query;
 pub mod raster;
+pub mod s3_address;
 pub mod secrets;
 pub mod sheets;
 pub mod source;
@@ -76,6 +77,8 @@ impl DataError {
     /// The error with `secret` (a password the driver sent) taken out of its message, whatever a
     /// server or driver echoes, keeping the kind: a refused login stays [`DataError::Login`].
     /// An empty secret leaves the error as it is, and so does text without it (nothing is copied).
+    /// Even one-character passwords must be redacted: diagnostics may lose readability,
+    /// but an untrusted server can echo a secret of any length.
     pub fn redact_secret(self, secret: &str) -> Self {
         if secret.is_empty() {
             return self;

crates/data/src/s3_address.rs

@@ -0,0 +1,199 @@
+//! The address of a web or `s3://` object read through one saved S3 connection.
+//!
+//! A saved connection's login is kept for the session under its own runtime *binding* (see
+//! `rustgis_tiles::s3`), and an address that must be read with that connection's keys (and no
+//! other's) names it: `rustgis-s3:<binding>:<endpoint-byte-length>:<endpoint>:<inner-url>`. The
+//! endpoint is the connection's canonical one (empty for AWS), the inner address is a web address
+//! or an `s3://` one, kept verbatim. An empty binding is an address no connection has been chosen
+//! for yet. The reserved [`LEGACY`] marker additionally retains an unknown endpoint while a choice
+//! is pending. Neither can be a live login or fetch data. Nothing here is secret, so a saved
+//! project may keep these addresses; `rustgis_tiles::s3` authorizes live bindings, not this parser.
+
+/// Starts an address read through a saved S3 connection.
+pub const PREFIX: &str = "rustgis-s3:";
+/// Reserved unresolved marker for legacy sources whose endpoint is unknown. Never a live login.
+pub const LEGACY: &str = "0000000000000000";
+/// The longest address, in bytes.
+pub const MAX_ADDRESS: usize = 8192;
+/// The longest endpoint, in bytes.
+pub const MAX_ENDPOINT: usize = 2048;
+/// How many hexadecimal digits a binding has.
+pub const BINDING_LEN: usize = 16;
+
+const BAD: &str = "not a saved S3 address";
+const TOO_LONG: &str = "the saved S3 address is too long";
+
+/// The parts of an address read through a saved connection.
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+pub struct BoundS3<'a> {
+    /// The login's runtime token; empty: no login chosen, [`LEGACY`]: unknown endpoint.
+    pub binding: &'a str,
+    /// The connection's canonical endpoint (empty: AWS).
+    pub endpoint: &'a str,
+    /// The address read: `http://`, `https://` or `s3://`.
+    pub url: &'a str,
+}
+
+/// Whether an address starts as one read through a saved connection, well formed or not. A
+/// malformed one is never a file on this computer: callers keep it blocked.
+pub fn is_wrapper(address: &str) -> bool {
+    address.starts_with(PREFIX)
+}
+
+/// Whether `binding` is empty (unresolved) or [`BINDING_LEN`] lower-case hexadecimal digits.
+pub fn valid_binding(binding: &str) -> bool {
+    binding.is_empty() || (binding.len() == BINDING_LEN && binding.bytes().all(|b| matches!(b, b'0'..=b'9' | b'a'..=b'f')))
+}
+
+fn web_or_s3(url: &str) -> bool {
+    ["https://", "http://", "s3://"].iter().any(|p| url.get(..p.len()).is_some_and(|s| s.eq_ignore_ascii_case(p)))
+}
+
+/// The parts of `address` when it is one read through a saved connection, `None` when it isn't
+/// (it doesn't start with [`PREFIX`]), and a fixed error (never part of the input) when it starts
+/// so but is malformed: a bad binding, endpoint length or boundary, controls, a nested wrapper, or
+/// an inner address that isn't a web or `s3://` one. Borrows; never allocates.
+pub fn parse(address: &str) -> Result<Option<BoundS3<'_>>, &'static str> {
+    let Some(rest) = address.strip_prefix(PREFIX) else { return Ok(None) };
+    if address.len() > MAX_ADDRESS {
+        return Err(TOO_LONG);
+    }
+    if address.chars().any(char::is_control) {
+        return Err(BAD);
+    }
+    let (binding, rest) = rest.split_once(':').ok_or(BAD)?;
+    if !valid_binding(binding) {
+        return Err(BAD);
+    }
+    let (length, rest) = rest.split_once(':').ok_or(BAD)?;
+    if length.is_empty() || length.len() > 4 || !length.bytes().all(|b| b.is_ascii_digit()) {
+        return Err(BAD);
+    }
+    let length: usize = length.parse().map_err(|_| BAD)?;
+    if length > MAX_ENDPOINT {
+        return Err(BAD);
+    }
+    // Slicing at a byte offset that isn't a character boundary is `None`, not a panic.
+    let endpoint = rest.get(..length).ok_or(BAD)?;
+    let url = rest.get(length..).and_then(|r| r.strip_prefix(':')).ok_or(BAD)?;
+    if binding == LEGACY && (!endpoint.is_empty() || !url.get(..5).is_some_and(|p| p.eq_ignore_ascii_case("s3://"))) {
+        return Err(BAD);
+    }
+    if is_wrapper(url) || !web_or_s3(url) {
+        return Err(BAD);
+    }
+    Ok(Some(BoundS3 { binding, endpoint, url }))
+}
+
+/// The address read through a saved connection with the given parts, checked as [`parse`] does.
+pub fn bind(binding: &str, endpoint: &str, url: &str) -> Result<String, &'static str> {
+    if !valid_binding(binding) || endpoint.len() > MAX_ENDPOINT {
+        return Err(BAD);
+    }
+    let out = format!("{PREFIX}{binding}:{}:{endpoint}:{url}", endpoint.len());
+    match parse(&out)? {
+        Some(_) => Ok(out),
+        None => Err(BAD),
+    }
+}
+
+/// The address that is read: a valid wrapper's inner address, or any other address as it is
+/// (including a malformed wrapper, which no caller may treat as a file).
+pub fn inner(address: &str) -> &str {
+    match parse(address) {
+        Ok(Some(b)) => b.url,
+        _ => address,
+    }
+}
+
+/// `address` with its binding replaced (a wrapper), or `None` when it isn't a valid wrapper or
+/// the new binding isn't one. The endpoint and inner address stay.
+pub fn rebound(address: &str, binding: &str) -> Option<String> {
+    let b = parse(address).ok().flatten()?;
+    bind(binding, b.endpoint, b.url).ok()
+}
+
+/// The address a server is reached at, for an address that may be read through a saved connection
+/// (as it is, or as the archive after a tile template's `@`): the wrapper's inner address, else
+/// `address` itself.
+pub fn reached(address: &str) -> &str {
+    let wrapped = if is_wrapper(address) { Some(address) } else { address.split_once('@').map(|(_, a)| a).filter(|a| is_wrapper(a)) };
+    wrapped.and_then(|a| parse(a).ok().flatten()).map_or(address, |b| b.url)
+}
+
+#[cfg(test)]
+#[allow(clippy::unwrap_used)]
+mod tests {
+    use super::*;
+
+    const B: &str = "0123456789abcdef";
+
+    #[test]
+    fn s3_address_round_trips_every_part() {
+        let url = "https://minio.example:9000/bucket/a b+c%20d.fgb?list-type=2&prefix=x%2Fy";
+        let a = bind(B, "https://minio.example:9000", url).unwrap();
+        assert_eq!(a, format!("rustgis-s3:{B}:26:https://minio.example:9000:{url}"));
+        let p = parse(&a).unwrap().unwrap();
+        assert_eq!((p.binding, p.endpoint, p.url), (B, "https://minio.example:9000", url));
+        assert_eq!(inner(&a), url);
+        // AWS: an empty endpoint; unresolved: an empty binding.
+        let aws = bind("", "", "s3://my-bucket/k.tif").unwrap();
+        assert_eq!(aws, "rustgis-s3::0::s3://my-bucket/k.tif");
+        let p = parse(&aws).unwrap().unwrap();
+        assert_eq!((p.binding, p.endpoint, p.url), ("", "", "s3://my-bucket/k.tif"));
+        assert_eq!(rebound(&aws, B).unwrap(), format!("rustgis-s3:{B}:0::s3://my-bucket/k.tif"));
+        // Not a wrapper: left alone.
+        assert_eq!(parse("https://x.org/a").unwrap(), None);
+        assert_eq!(inner("https://x.org/a"), "https://x.org/a");
+        assert!(is_wrapper("rustgis-s3:junk") && !is_wrapper("s3://b/k"));
+    }
+
+    #[test]
+    fn s3_address_rejects_malformed_values_without_echoing_them() {
+        let endpoint = "https://h.example";
+        let good = format!("rustgis-s3:{B}:{}:{endpoint}:https://h.example/b/k", endpoint.len());
+        assert!(parse(&good).unwrap().is_some());
+        let bad = [
+            // binding: upper case, short, long, non-hex
+            format!("rustgis-s3:{}:17:{endpoint}:https://h.example/k", B.to_uppercase()),
+            "rustgis-s3:abc:17:https://h.example:https://h.example/k".to_string(),
+            format!("rustgis-s3:{B}0:17:{endpoint}:https://h.example/k"),
+            format!("rustgis-s3:{}:17:{endpoint}:https://h.example/k", "g".repeat(16)),
+            // Unknown origin is only meaningful for a bare s3 object, never a known endpoint.
+            format!("rustgis-s3:{LEGACY}:{}:{endpoint}:s3://b/k", endpoint.len()),
+            format!("rustgis-s3:{LEGACY}:0::https://h.example/b/k"),
+            // length: missing, signed, too long, past the end, off by one either way
+            format!("rustgis-s3:{B}::{endpoint}:https://h.example/k"),
+            format!("rustgis-s3:{B}:+17:{endpoint}:https://h.example/k"),
+            format!("rustgis-s3:{B}:99999:{endpoint}:https://h.example/k"),
+            format!("rustgis-s3:{B}:2049:{endpoint}:https://h.example/k"),
+            format!("rustgis-s3:{B}:16:{endpoint}:https://h.example/k"),
+            format!("rustgis-s3:{B}:18:{endpoint}:https://h.example/k"),
+            // nothing after the endpoint, or no separator
+            format!("rustgis-s3:{B}:17:{endpoint}"),
+            format!("rustgis-s3:{B}:17:{endpoint}https://h.example/k"),
+            // inner: not a web or s3 address, nested wrappers, anonymous marker
+            format!("rustgis-s3:{B}:17:{endpoint}:ftp://h.example/k"),
+            format!("rustgis-s3:{B}:17:{endpoint}:/etc/passwd"),
+            format!("rustgis-s3:{B}:17:{endpoint}:rustgis-s3:{B}:17:{endpoint}:https://h.example/k"),
+            format!("rustgis-s3:{B}:17:{endpoint}:rustgis-anonymous:https://h.example/k"),
+            // controls
+            format!("rustgis-s3:{B}:17:{endpoint}:https://h.example/k\n"),
+            format!("rustgis-s3:{B}:17:{endpoint}:https://h.example/\u{7}k"),
+        ];
+        for a in &bad {
+            let e = parse(a).unwrap_err();
+            assert!(!e.contains("h.example") && !e.contains("passwd"), "{e}");
+        }
+        // A length that cuts a multi-byte character is an error, not a panic.
+        assert!(parse(&format!("rustgis-s3:{B}:2:é:https://h.example/k")).is_ok());
+        assert!(parse(&format!("rustgis-s3:{B}:1:é:https://h.example/k")).is_err());
+        // The overall bound, and the endpoint's.
+        let long = format!("https://h.example/{}", "k".repeat(MAX_ADDRESS));
+        assert!(bind(B, endpoint, &long).is_err() && parse(&format!("rustgis-s3:{B}:17:{endpoint}:{long}")).is_err());
+        assert!(bind(B, &"e".repeat(MAX_ENDPOINT + 1), "https://h.example/k").is_err());
+        assert!(bind("nope", endpoint, "https://h.example/k").is_err());
+        // A malformed wrapper is never read as its own inner address.
+        assert_eq!(inner("rustgis-s3:junk:https://h.example/k"), "rustgis-s3:junk:https://h.example/k");
+    }
+}

crates/data/src/source/credentials.rs

@@ -20,6 +20,8 @@ fn secret_key_lower(k: &str) -> bool {
             | "key"
             | "access_key"
             | "accesskey"
+            | "access_key_id"
+            | "accesskeyid"
             | "secret"
             | "client_secret"
             | "password"
@@ -74,3 +76,22 @@ pub fn redact_query(address: &mut String) {
         address.push_str(&kept);
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn access_key_ids_and_their_secrets_are_secret_names() {
+        for k in ["accessKeyId", "access_key_id", "AccessKeyID", "secretAccessKey", "sessionToken", "X-Amz-Credential"] {
+            assert!(secret_key(k), "{k}");
+        }
+        for k in ["remember", "region", "buckets", "endpoint"] {
+            assert!(!secret_key(k), "{k}");
+        }
+        // In a query, the key id goes with the signature.
+        let mut a = "https://s3.example.org/b/k?accessKeyId=AKIDEXAMPLE&prefix=x".to_string();
+        redact_query(&mut a);
+        assert_eq!(a, "https://s3.example.org/b/k?prefix=x");
+    }
+}

crates/data/src/source/duckdb.rs

@@ -182,10 +182,13 @@ pub fn try_connection_password<T>(dsn: &str, id: &str, password: &str, run: impl
     let result = run();
     if refused(&result) {
         let mut all = lock();
-        if let Some(previous) = previous {
-            all.insert(dsn.to_string(), previous);
-        } else {
-            all.remove(dsn);
+        // A concurrent login or Forget supersedes this attempt, even if the server refused it.
+        if all.get(dsn).is_some_and(|entry| entry.connection_id.as_deref() == Some(id) && entry.password.expose() == password) {
+            if let Some(previous) = previous {
+                all.insert(dsn.to_string(), previous);
+            } else {
+                all.remove(dsn);
+            }
         }
     }
     result
@@ -865,6 +868,32 @@ fn ours(name: &str) -> bool {
 mod tests {
     use super::*;
 
+    #[test]
+    fn refused_password_does_not_restore_over_a_newer_login_or_forget() {
+        let dsn = "host=refused_password_newer_slot";
+        remember_password(dsn, "original");
+        try_connection_password(
+            dsn,
+            "refused-attempt",
+            "bad",
+            || {
+                remember_password(dsn, "newer");
+            },
+            |_| true,
+        );
+        assert_eq!(remembered_password(dsn).as_deref(), Some("newer"));
+        try_connection_password(
+            dsn,
+            "refused-attempt",
+            "bad",
+            || {
+                forget_connection_password("refused-attempt");
+            },
+            |_| true,
+        );
+        assert!(!has_password(dsn));
+    }
+
     #[test]
     fn remembered_database_cache_ownership_and_explicit_preservation() {
         let a = "host=remembered_cache_a user=one";

crates/data/src/source/mod.rs

@@ -321,9 +321,11 @@ pub fn open_shared(file_name: &str, bytes: std::sync::Arc<Vec<u8>>, layer: Optio
     }
 }
 
-/// Whether a web address names a file read in place by range: a FlatGeobuf or a GeoParquet.
+/// Whether a web address names a file read in place by range: a FlatGeobuf or a GeoParquet. An
+/// address read through a saved S3 connection ([`crate::s3_address`]) is judged by the address it
+/// wraps.
 pub fn is_remote_name(url: &str) -> bool {
-    let lower = url.to_ascii_lowercase();
+    let lower = crate::s3_address::inner(url).to_ascii_lowercase();
     let path = lower.split(['?', '#']).next().unwrap_or(&lower);
     // `s3://` too: the app's fetchers check it and read it from its bucket's web address (signed
     // only for a connection that serves the bucket, see rustgis_tiles::s3).

crates/data/src/source/remote.rs

@@ -400,6 +400,11 @@ pub fn forget() {
     files().lock().unwrap_or_else(|e| e.into_inner()).clear();
 }
 
+/// Forgets the fetched parts of one file (after the address's login changed).
+pub fn forget_file(url: &str) {
+    files().lock().unwrap_or_else(|e| e.into_inner()).retain(|r| r.url != url);
+}
+
 /// The error a read that needs bytes gives (it runs again once they are fetched).
 pub fn need() -> DataError {
     DataError::Format(NEED.into())