Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 102 additions & 0 deletions crates/engine/src/tests/service_logins.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1278,3 +1278,105 @@ fn notebook_output_keeps_no_login_binding() {
let (saved, _) = s.project.clone().saved_text(None).unwrap();
assert!(!saved.contains(rustgis_data::login_address::PREFIX), "the saved project keeps a binding: {saved}");
}

/// The server has the credential before the lifecycle action, but its refusal arrives after it.
/// Exercise the public HEAD transport and the real session commands, without a credential store.
#[cfg(not(target_arch = "wasm32"))]
#[test]
fn delayed_head_refusal_keeps_secrets_out_of_history_across_login_lifecycle() {
use rustgis_data::diagnostics::{Filter, History};
use std::io::{Read, Write};
use std::sync::mpsc::channel;
use std::time::Duration;

for action in ["active", "logout", "replace", "drop", "anonymous"] {
for basic in [false, true] {
let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap();
let base = format!("http://{}", listener.local_addr().unwrap());
let secret = "delayed-head/sentinel+757";
let value = if basic { logins::basic("head-user", secret) } else { secret.to_string() };
let encoded = value.bytes().map(|b| format!("%{b:02X}")).collect::<String>();
let payload = value.strip_prefix("Basic ").unwrap_or(&value).to_string();
let echo = format!("{value} {payload} {encoded}");
let (received_tx, received_rx) = channel();
let (release_tx, release_rx) = channel();
let expected = value.clone();
let server = std::thread::spawn(move || {
let (mut stream, _) = listener.accept().unwrap();
stream.set_read_timeout(Some(Duration::from_secs(10))).unwrap();
let mut request = Vec::new();
let mut buf = [0u8; 2048];
while request.len() < 32768 && !request.windows(4).any(|w| w == b"\r\n\r\n") {
let n = stream.read(&mut buf).unwrap();
assert_ne!(n, 0);
request.extend_from_slice(&buf[..n]);
}
assert!(request.ends_with(b"\r\n\r\n"));
let request = String::from_utf8(request).unwrap();
assert!(request.starts_with("HEAD /probe HTTP/1.1\r\n"));
if action == "anonymous" {
assert!(!request.to_ascii_lowercase().contains("x-key:") && !request.to_ascii_lowercase().contains("authorization:"));
} else {
let name = if basic { "authorization" } else { "x-key" };
assert!(request.to_ascii_lowercase().contains(&format!("{name}: {}\r\n", expected.to_ascii_lowercase())));
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
}
received_tx.send(()).unwrap();
release_rx.recv_timeout(Duration::from_secs(10)).unwrap();
let id = if action == "anonymous" { "anonymous-request-id" } else { &echo };
write!(stream, "HTTP/1.1 403 Forbidden\r\nx-request-id: {id}\r\nContent-Length: 0\r\nConnection: close\r\n\r\n").unwrap();
});
let mut s = session(None);
let id = new(
&mut s,
json!({"kind":"service", "address":base, "auth":if basic {"basic"} else {"header"}, "user":"head-user", "keyName":"X-Key"}),
);
let credential = if basic { "password" } else { "apiKey" };
let login = json!({"id":id, credential:secret, "allowInsecure":true});
let plain = format!("{base}/probe");
let url = if action == "anonymous" {
plain.clone()
} else {
s.run("connection.login", &login).unwrap();
bound(&mut s, &id, &plain)
};
let history = History::default();
let _scope = history.enter();
let (done_tx, done_rx) = channel();
crate::render::rustgis_tiles::cache::head(&url, Box::new(move |r| done_tx.send(r).unwrap()));
received_rx.recv_timeout(Duration::from_secs(10)).unwrap();
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
match action {
"logout" => {
s.run("connection.logout", &json!({"id":id})).unwrap();
}
"replace" => {
s.run("connection.login", &json!({"id":id,credential:"replacement-head-secret","allowInsecure":true})).unwrap();
}
"drop" => drop(s),
_ => {}
}
if matches!(action, "logout" | "replace" | "drop") {
assert!(logins::apply(&url).is_err(), "{action} must revoke the old generation");
}
release_tx.send(()).unwrap();
assert!(done_rx.recv_timeout(Duration::from_secs(10)).unwrap().is_err());
server.join().unwrap();
let records = history.records();
assert_eq!(records.len(), 1, "{action}: {records:?}");
let network = records[0].network.as_ref().unwrap();
assert_eq!(network.status, Some(403));
assert_eq!(network.method, "HEAD");
assert_eq!(network.url, plain);
let text = format!("{records:?}\n{}", history.report(Filter::All));
assert!(text.contains("403"), "{action}: {text}");
assert!(text.contains(&plain), "{action}: {text}");
for private in [secret, &value, &payload, &encoded] {
assert!(!text.contains(private), "{action}: credential reached History or copied report");
}
if action == "anonymous" {
assert!(text.contains("x-request-id: anonymous-request-id"), "{text}");
} else {
assert!(!text.contains("x-request-id"), "{action}: {text}");
}
}
}
}
72 changes: 70 additions & 2 deletions crates/tiles/src/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1408,8 +1408,14 @@ mod native {
continue;
}
if resp.status().as_u16() >= 400 {
// The selected headers say who refused and what it wants: kept, as a GET's
// refusal keeps them, so the history's details aren't only a status.
// A server can echo credentials in retained headers, even after logout has
// removed their redaction patterns. Remember every hop, including before a
// cross-origin redirect: authenticated refusals keep only the status.
if carried_login {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description and the new comment say this matches authenticated GET's status-only policy, but the rule differs. HEAD keeps a sticky carried_login across every hop, so a later cross-origin anonymous 403 is status-only. GET decides per hop at fetch_answer (cache.rs:1705, login.is_none() on the final hop), so after a logged-in hop redirects cross-origin, GET still keeps the headers of the anonymous refusal.

The practical risk is low, since the credential never reached the other origin. Two options: (a) make GET sticky too, as a follow-up; or (b) correct the wording here and in docs/user-guide/data.md so it doesn't claim parity. Confidence: high that the policies differ, medium on how much it matters.

let status = resp.status().as_u16();
return Err(Failure::without_answer("", format!("http status: {status}"), Reason::Status(status)));
}
// Anonymous refusals retain the selected diagnostic headers.
let headers: Vec<(String, String)> = resp
.headers()
.iter()
Expand Down Expand Up @@ -2442,6 +2448,68 @@ mod tests {
assert!(logins::deactivate(&binding));
}

/// Once any hop carried a login, a later anonymous hop must not regain response details.
#[cfg(not(target_arch = "wasm32"))]
#[test]
fn delayed_head_refusal_after_cross_origin_redirect_is_status_only() {
use crate::logins::{self, Method};
use rustgis_data::diagnostics::{Capture, Filter, History};
use std::io::{Read, Write};
use std::sync::mpsc::channel;
use std::time::Duration;

let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap();
let other = format!("http://{}", listener.local_addr().unwrap());
let secret = "redirect-head-sentinel-757";
let (received_tx, received_rx) = channel();
let (release_tx, release_rx) = channel();
let server = std::thread::spawn(move || {
let (mut stream, _) = listener.accept().unwrap();
stream.set_read_timeout(Some(Duration::from_secs(10))).unwrap();
let mut request = Vec::new();
let mut buf = [0u8; 2048];
while request.len() < 32768 && !request.windows(4).any(|w| w == b"\r\n\r\n") {
let n = stream.read(&mut buf).unwrap();
assert_ne!(n, 0);
request.extend_from_slice(&buf[..n]);
}
assert!(request.ends_with(b"\r\n\r\n"));
let request = String::from_utf8(request).unwrap();
assert!(request.starts_with("HEAD /refused HTTP/1.1\r\n"));
assert!(!request.to_ascii_lowercase().contains("x-key:") && !request.contains(secret));
received_tx.send(()).unwrap();
release_rx.recv_timeout(Duration::from_secs(10)).unwrap();
write!(stream, "HTTP/1.1 403 Forbidden\r\nx-request-id: {secret}\r\nContent-Length: 0\r\nConnection: close\r\n\r\n").unwrap();
});
let (base, asked) = serve_recorded(vec![
format!("HTTP/1.1 302 Found\r\nLocation: {other}/refused\r\nContent-Length: 0\r\nConnection: close\r\n\r\n").into_bytes(),
]);
let binding = logins::activate(&base, "redirect-head", Method::Header("X-Key".into()), secret, true).unwrap();
let foreign = logins::activate(&other, "foreign-head", Method::Header("X-Key".into()), "foreign-head-secret", true).unwrap();
let url = logins::bind(&binding, &format!("{base}/start")).unwrap();
let history = History::default();
let _scope = history.enter();
let capture = Capture::current();
let worker = std::thread::spawn(move || {
let _scope = capture.enter();
native::head_with(&native::agent(), &url)
});
let request = asked.recv_timeout(Duration::from_secs(10)).unwrap();
assert!(request.to_ascii_lowercase().contains(&format!("x-key: {secret}\r\n")));
received_rx.recv_timeout(Duration::from_secs(10)).unwrap();
assert!(logins::deactivate(&binding));
release_tx.send(()).unwrap();
assert_eq!(worker.join().unwrap().unwrap_err(), "http status: 403");
server.join().unwrap();
let records = history.records();
assert_eq!(records.len(), 1);
assert_eq!(records[0].network.as_ref().unwrap().status, Some(403));
let text = format!("{records:?}\n{}", history.report(Filter::All));
assert!(text.contains("403"));
assert!(!text.contains(secret) && !text.contains("x-request-id") && !text.contains("foreign-head-secret"));
assert!(logins::deactivate(&foreign));
}

#[cfg(not(target_arch = "wasm32"))]
#[test]
fn logins_head_and_post_carry_the_login() {
Expand Down
3 changes: 3 additions & 0 deletions docs/user-guide/data.md
Original file line number Diff line number Diff line change
Expand Up @@ -816,6 +816,9 @@ unchanged and reported.
A plain `http://` address sends the login unencrypted, so the form asks for confirmation
and the command needs `"allowInsecure": true`. Error messages, notices, the preferences,
projects, History and what the [AI Assistant](assistant.md) sees never hold the password or key.
On the desktop, a failed HEAD request that carried a web-service login records the HTTP status
without response headers, including after logout or a redirect to another origin: a server could
echo credentials in those headers. Anonymous HEAD failures keep their selected diagnostic headers.
Runtime login bindings are also removed from recorded command parameters, including addresses
embedded in text, when saving, recovering or exporting them. The live session keeps its originals.

Expand Down
Loading