fix(security): enforce HTTPS for JMAP/CalDAV/CardDAV URLs by mickenordin · Pull Request #63 · SUNET/chithi

mickenordin · 2026-04-17T10:56:36Z

Summary

Closes #58 (Copilot security audit finding A, Medium).

Adds mail::url_validation::require_https helper that rejects non-HTTPS URLs, with a loopback exemption (localhost, 127/8, ::1) for local dev.
Wires the check into every path that makes auth-bearing remote calls: JmapConnection::connect, CalDavClient::connect / connect_with_token, CardDavClient::connect.
Also validates the post-redirect URL discovered during CalDAV/CardDAV auto-discovery, so a server cannot downgrade the scheme via redirect.
8 unit tests on the helper + 1 wiring test per connector.
Drive-by: fixes a pre-existing clippy warning in calendar/timezone.rs (split into its own commit).

Test plan

cargo test — 135 passing (8 new + 3 wiring tests added)
cargo clippy -- -D warnings — clean
Manual: configure a JMAP/CalDAV/CardDAV account with http://some-host and verify it's rejected with a clear error
Manual: existing https:// configured accounts keep working

Fixes empty-line-after-doc-comments warning.

User-configured server URLs were accepted without scheme validation. If the user (or a tampered config) specified http://, credentials would be transmitted in cleartext. Adds a shared require_https() helper in mail::url_validation that rejects non-https schemes. Loopback hosts (localhost, 127/8, ::1) are allowed over http for local development. The helper also validates the post-redirect URL returned by CalDAV/CardDAV auto-discovery so a server can't downgrade the scheme via a redirect. Wired into: - JmapConnection::connect (user-provided jmap_url) - CalDavClient::connect (user-provided caldav_url + discovered URL) - CalDavClient::connect_with_token (OAuth bearer flow) - CardDavClient::connect (user-provided carddav_url + discovered URL) Closes #58.

Copilot

Pull request overview

Adds centralized URL scheme validation to prevent sending credentials over cleartext connections for JMAP/CalDAV/CardDAV, and wires it into connection + auto-discovery paths.

Changes:

Introduces mail::url_validation::require_https (HTTPS-only, with loopback HTTP exception) plus unit tests.
Enforces HTTPS for configured URLs and validates discovered post-redirect URLs in JMAP/CalDAV/CardDAV connectors.
Adjusts calendar timezone module docs to address a clippy warning.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src-tauri/src/mail/url_validation.rs	Adds HTTPS enforcement helper and unit tests
src-tauri/src/mail/mod.rs	Exposes the new `url_validation` module
src-tauri/src/mail/jmap.rs	Adds HTTPS validation in `JmapConnection::connect` + wiring test
src-tauri/src/mail/caldav.rs	Adds HTTPS validation in connect/token connect and validates discovery result + wiring tests
src-tauri/src/mail/carddav.rs	Adds HTTPS validation in connect and validates discovery result + wiring test
src-tauri/src/calendar/timezone.rs	Converts top-of-file docs to inner docs to satisfy clippy

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-17T11:01:09Z

+    match parsed.scheme() {
+        "https" => Ok(()),
+        "http" if is_loopback_host(parsed.host_str()) => Ok(()),
+        scheme => Err(Error::Other(format!(


Allowing cleartext http:// for loopback hosts is currently enabled unconditionally. If this is meant only for local development, consider gating it behind cfg!(debug_assertions), a feature flag, or an explicit opt-in setting/env var to avoid sending real credentials over HTTP to a local process in production.

Fixed in 63a3299 — loopback http now gated behind cfg!(debug_assertions), so release builds reject cleartext URLs unconditionally.

Copilot · 2026-04-17T11:01:10Z

        let base_url = if config.caldav_url.is_empty() {
            log::info!("caldav: no URL configured, attempting auto-discovery");
            Self::auto_discover(&http, &auth, &config.email).await?
        } else {
+            crate::mail::url_validation::require_https(&config.caldav_url)?;


require_https validates the configured base URL, but later requests can still send credentials to cleartext endpoints because resolve_url() returns any absolute http://... href from server responses unchanged (see caldav.rs:447-450). This allows a server to downgrade subsequent auth-bearing requests via absolute hrefs. Consider enforcing require_https (or rewriting/denying http://) on resolved URLs before issuing requests, or disallowing http:// in resolve_url() altogether.

Fixed in 8938221 — resolve_url now returns Result<String> and runs require_https on the resolved URL, so absolute http:// hrefs from the server are rejected before any auth-bearing request.

Copilot · 2026-04-17T11:01:10Z

                            final_url.scheme(),
                            final_url.host_str().unwrap_or(domain),
                            final_url.path().trim_end_matches('/')
                        );
+                        crate::mail::url_validation::require_https(&discovered)?;


Auto-discovery builds discovered from final_url.scheme()/host_str()/path() but drops any non-default port present in the redirect target. If the server redirects to e.g. https://host:8443/..., the resulting base URL will incorrectly omit :8443 and subsequent requests will fail. Consider including final_url.port() when formatting, or deriving discovered directly from final_url (preserving authority) before trimming the path.

Fixed in 15a6faa — auto-discovery now includes final_url.port() in the reconstructed base URL.

Copilot · 2026-04-17T11:01:10Z

+            crate::mail::url_validation::require_https(carddav_url)?;
            carddav_url.to_string()
        };



require_https validates the configured carddav_url, but later requests can still be downgraded because resolve_url() returns any absolute http://... href from server responses unchanged (see carddav.rs:135-138). That can cause auth-bearing REPORT/PUT/DELETE requests to go over cleartext if the server emits absolute http hrefs. Consider enforcing require_https (or rewriting/denying http://) on resolved URLs before issuing requests, or disallowing http:// in resolve_url().

Suggested change

crate::mail::url_validation::require_https(carddav_url)?;

carddav_url.to_string()

};

carddav_url.to_string()

};

crate::mail::url_validation::require_https(&base_url)?;

Fixed in 8938221 — CardDAV resolve_url now returns Result<String> and validates the resolved URL via require_https.

Copilot · 2026-04-17T11:01:11Z

                        final_url.scheme(),
                        final_url.host_str().unwrap_or(domain),
                        final_url.path().trim_end_matches('/')
                    );
+                    crate::mail::url_validation::require_https(&discovered)?;


Auto-discovery’s discovered URL is reconstructed from final_url.scheme()/host_str()/path() and currently omits any non-default port from the redirect target. If .well-known/carddav redirects to https://host:8443/..., this will produce a base URL without :8443 and break subsequent calls. Consider preserving the full authority (including port) when building discovered.

Fixed in 15a6faa — port preserved in CardDAV auto-discovery too.

Copilot · 2026-04-17T11:01:11Z

+    let parsed = url::Url::parse(url)
+        .map_err(|e| Error::Other(format!("Invalid URL '{}': {}", url, e)))?;
+
+    match parsed.scheme() {
+        "https" => Ok(()),
+        "http" if is_loopback_host(parsed.host_str()) => Ok(()),
+        scheme => Err(Error::Other(format!(
+            "URL must use https:// (got '{}'): {}",
+            scheme, url
+        ))),


The error paths here include the full, raw URL in the error message. If a user configures a URL containing userinfo (e.g. https://user:password@host/...), this will leak credentials into logs/UI via Error::Other. Consider redacting userinfo (or omitting the URL entirely) when formatting these errors.

Fixed in 25e3e02 — added a redact_url helper that strips userinfo. Unparseable URLs render as <invalid URL> to avoid echoing raw input that may also contain secrets. Two new tests cover both paths.

Release builds now reject http:// URLs unconditionally. Loopback (localhost, 127/8, ::1) over cleartext is only allowed in debug builds for local dev/test servers. Addresses Copilot review on #63.

Previous construction of the discovered base URL used only scheme, host and path — any non-default port from the redirect target was dropped, causing subsequent requests to hit the wrong port. Addresses Copilot review on #63.

A malicious server could return absolute http:// hrefs in PROPFIND / REPORT responses. Previously those were passed through unchanged, so subsequent auth-bearing requests (REPORT / PUT / DELETE) could be downgraded to cleartext even though the base URL was validated. resolve_url now returns Result<String> and rejects non-HTTPS resolved URLs via require_https. All callers propagate the Result with ?. Addresses Copilot review on #63.

A URL configured with embedded credentials (e.g. https://user:pw@host/) would be echoed verbatim into error messages and logs on rejection, leaking the password. Strip userinfo before formatting; for unparseable URLs, don't echo the raw input at all. Addresses Copilot review on #63.

mickenordin added 2 commits April 17, 2026 12:24

chore(clippy): use inner doc comments for timezone module

1b48658

Fixes empty-line-after-doc-comments warning.

mickenordin requested a review from Copilot April 17, 2026 10:57

Copilot started reviewing on behalf of mickenordin April 17, 2026 10:57 View session

Copilot AI reviewed Apr 17, 2026

View reviewed changes

mickenordin added 4 commits April 17, 2026 13:16

fix(security): gate loopback HTTP exception behind debug_assertions

63a3299

Release builds now reject http:// URLs unconditionally. Loopback (localhost, 127/8, ::1) over cleartext is only allowed in debug builds for local dev/test servers. Addresses Copilot review on #63.

mickenordin requested a review from kushaldas April 17, 2026 12:08

mickenordin merged commit f7eb5fa into main Apr 18, 2026
1 check passed

Conversation

mickenordin commented Apr 17, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

mickenordin Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

mickenordin Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

mickenordin Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

mickenordin Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

mickenordin Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

mickenordin Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants