Refuse plain http in the Nextcloud client, below every URL it sends
NFR-SEC-3 held only for the address a person types: normalise_endpoint upgrades it to https, and nothing else was checked. The login flow's poll endpoint, an account an older build saved and a redirect all come from somewhere else, and any of them naming http:// would send the app password in Basic auth in the clear. http_client now sets https_only. reqwest checks it before connecting and again on each redirect, so a refused request never opens a socket, which the new test checks with a listener that nothing may reach. A refused scheme is reported as a Configuration error, not Network. The request never left the process, and Network puts the app into offline mode over a connection that is working. Other builder errors (a URL that does not parse) go the same way, for the same reason. Part of #65.
This commit is contained in:
@@ -723,6 +723,15 @@ pub fn http_client(user_agent: &str) -> Result<reqwest::Client, RemoteError> {
|
||||
// for months. [`EXTRA_ROOTS`] carries those, and is additive — the
|
||||
// webpki-roots set is still installed alongside.
|
||||
.tls_certs_only(extra_roots())
|
||||
// TRACES: NFR-SEC-3
|
||||
// Refuse `http` here, below every URL this crate builds, rather than
|
||||
// trusting each place a URL comes from. `normalise_endpoint` upgrades
|
||||
// what the user types, but the login flow's poll endpoint, the account
|
||||
// an older build saved and a redirect all arrive from somewhere else,
|
||||
// and any of them naming `http://` would send the app password in
|
||||
// Basic auth in the clear. reqwest checks this before connecting and
|
||||
// again on every redirect, so a refused request never opens a socket.
|
||||
.https_only(true)
|
||||
// A request that hangs forever is indistinguishable from a worker that
|
||||
// died, and cost a long time to tell apart once. These turn that into
|
||||
// an error the UI can show.
|
||||
@@ -780,8 +789,16 @@ fn map_send_error(e: reqwest::Error) -> RemoteError {
|
||||
let mut detail = e.to_string();
|
||||
let mut src: Option<&dyn std::error::Error> = std::error::Error::source(&e);
|
||||
let mut tls = false;
|
||||
// A URL the client refused to send — `http` under `https_only`, on the
|
||||
// first request or a redirect. Nothing left the process, so this is the
|
||||
// account's configuration, not the network: reported as `Network` it
|
||||
// would put the app into offline mode over a connection that is fine.
|
||||
let mut refused = e.is_builder();
|
||||
while let Some(s) = src {
|
||||
let text = s.to_string();
|
||||
if text.contains("URL scheme is not allowed") {
|
||||
refused = true;
|
||||
}
|
||||
// rustls surfaces every verification failure through this wording:
|
||||
// UnknownIssuer, Expired, NotValidForName, BadSignature.
|
||||
if text.contains("invalid peer certificate")
|
||||
@@ -795,7 +812,9 @@ fn map_send_error(e: reqwest::Error) -> RemoteError {
|
||||
src = std::error::Error::source(s);
|
||||
}
|
||||
|
||||
if tls {
|
||||
if refused {
|
||||
RemoteError::Configuration(detail)
|
||||
} else if tls {
|
||||
RemoteError::Tls(detail)
|
||||
} else {
|
||||
RemoteError::Network(detail)
|
||||
@@ -1013,6 +1032,35 @@ mod tests {
|
||||
assert!(c.is_ok(), "client must build without a backend");
|
||||
}
|
||||
|
||||
/// TRACES: NFR-SEC-3
|
||||
#[tokio::test]
|
||||
async fn plain_http_is_refused_before_a_connection_opens() {
|
||||
// A listener that would take the connection if one were made. The
|
||||
// app password travels in a header, so a request that reached the
|
||||
// socket has already leaked it; failing on the response is too late.
|
||||
let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap();
|
||||
listener.set_nonblocking(true).unwrap();
|
||||
let url = format!("http://{}/remote.php/dav/", listener.local_addr().unwrap());
|
||||
|
||||
let err = http_client("test")
|
||||
.unwrap()
|
||||
.get(&url)
|
||||
.basic_auth("duncan", Some("app-password"))
|
||||
.send()
|
||||
.await
|
||||
.expect_err("http must be refused");
|
||||
|
||||
assert!(
|
||||
matches!(map_send_error(err), RemoteError::Configuration(_)),
|
||||
"a refused scheme is configuration, not the network"
|
||||
);
|
||||
assert_eq!(
|
||||
listener.accept().err().map(|e| e.kind()),
|
||||
Some(std::io::ErrorKind::WouldBlock),
|
||||
"nothing may have connected"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn delta_is_unsupported_and_says_why() {
|
||||
// Verified absent in the server; the engine must fall back rather
|
||||
|
||||
Reference in New Issue
Block a user