fix(config): require https for a server URL unless --insecure - #5
Merged
Conversation
A cleartext server URL was accepted with no opt-in. Every authenticated request carries a Basic password or a bearer token in an Authorization header, so `ocis server add work http://cloud.example` silently arranged to send the credential in the clear on every later request. The OIDC path was already protected: auth.ValidateTransportSecurity rejects a discovered cleartext endpoint unless the profile passed --insecure. But that check only runs during OIDC discovery, so Basic authentication -- where the account password itself goes on the wire -- had no equivalent gate. This also contradicted AUTHENTICATION.md, which states Basic credentials are "safe in transit only when TLS is used". Require https in ValidateServerURL and add ValidateInsecureServerURL for the explicit opt-in, which still rejects an unusable URL. The application layer selects between them on --insecure: the same flag that already permits cleartext OIDC endpoints and unverified certificates, so a local development server stays a one-flag case rather than needing a second concept. Both entry points that accept a URL are covered, `server add` and `login`. The rejection is a usage error, so it exits 2 and stores no profile. Flag help on both commands now states the widened meaning. Two existing tests logged in to cleartext httptest servers and now declare the opt-in, which is the scenario they were always exercising.
Validating a URL only where one is entered did not establish the invariant for existing installations. A release before this requirement saved cleartext profiles without an opt-in, and such a profile still reached the point of receiving its keyring password or bearer token, refreshing a token, and sending it to its stored http:// URL. A Basic login against that profile skipped validation too, because passing no new --server was the only path checked. A persisted profile is now revalidated when it is selected, before any credential is applied or refreshed, and in the login path after --insecure is applied but before a password is read, a browser opens, discovery runs, or a probe is sent. Validation stays out of config.Load, so server list, status, logout, and server remove remain usable for repairing a rejected profile, and a rejection never migrates a profile to insecure on its own. Redirects are covered as well. Go decides whether to forward the Authorization header across a redirect by comparing hostnames alone, without regard to scheme, so an https endpoint redirecting to http:// on the same host would have carried the credential over cleartext. The shared client now refuses a downgrade, keeps a finite redirect limit, and names only the scheme and host of the rejected target. Existing tests take the explicit opt-in their cleartext test servers always implicitly relied on.
- distinguish keyring loading from credential-bearing network activity - align security documentation and comments with runtime ordering Signed-off-by: Matteo <mzner@pm.me>
mzner
force-pushed
the
fix/require-https-for-credentials
branch
from
August 9, 2026 14:13
ab1e9ee to
f255629
Compare
mzner
enabled auto-merge (squash)
August 9, 2026 14:13
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A cleartext server URL was accepted with no opt-in:
ocis server add work http://cloud.example # accepted before this changeEvery authenticated request carries a Basic password or a bearer token in an
Authorizationheader, so this silently arranged to send the credential in the clear on every later request.Why the OIDC path was already safe
auth.ValidateTransportSecurity(internal/auth/oidc.go:120) rejects a discovered cleartext endpoint unless the profile passed--insecure. But it runs only during OIDC discovery, and has just two call sites, both on the OIDC path. Basic authentication — where the account password itself goes on the wire, not a limited-lifetime token — had no equivalent gate.This also contradicted the repo's own documentation. AUTHENTICATION.md states Basic credentials are "safe in transit only when TLS is used and its certificate is verified", which nothing enforced.
Fix
ValidateServerURLnow requireshttps.ValidateInsecureServerURLpermitshttpfor the explicit opt-in, still rejecting an unusable URL.--insecure— the same flag that already permits cleartext OIDC endpoints and unverified certificates. A local development server stays a one-flag case instead of needing a second concept.server addandlogin.The rejection is a usage error, so it exits 2 per the stable exit-code contract, and no profile is stored. The error names the opt-in rather than just refusing:
Flag help on both commands now states the widened meaning.
Tests
internal/configcovers the required scheme, the opt-in, and that the opt-in does not become a bypass for otherwise-invalid URLs.internal/appcovers both entry points end to end, asserting the usage-error kind and that a rejected server leaves no profile behind.Two existing tests logged in to cleartext
httptestservers and now declareInsecure: true— that is the scenario they were always exercising, and it remains supported.Compatibility
This rejects input that previously worked, which is the point: the previous behavior sent credentials in cleartext. A profile that needs it adds
--insecure, which is already required for a local oCIS server's self-signed certificate. The integration suite is unaffected — it already useshttpswith--insecure.Verification
go test ./...,go test -race,go vet,gofmt,golangci-lint, andmake coverageall clean.Docs: AUTHENTICATION.md and README.md describe
--insecureas governing the cleartext URL as well as certificate verification. ARCHITECTURE.md gains a design rule that transport security is not authentication-mode specific.Follow-up from review
A second commit (
e0f1e93) enforces the requirement where a credential can actually leave the process, not only where a URL is entered.A profile stored before this change still received its credentials. Validating
server addandlogin --serverdoes nothing for ahttp://profile a previous release already persisted: the next command loaded it, attached the saved password or refreshed the token, and sent it in the clear. The stored URL is now revalidated innewClientWithOptions, before any credential is applied or refreshed, and in the login path after--insecureis applied but before a password is read, a browser opens, or discovery runs. The error names the profile and how to repair it. Validation deliberately does not live inconfig.Load, soserver list,status,logout, andserver removestay usable for repairing or removing the profile — and a rejection never silently migrates a profile to insecure.An https endpoint could redirect to cleartext and still carry the credential. Go's default redirect policy decides whether to forward
Authorizationby comparing hostnames only — it never considers the scheme — sohttps://host→http://hostforwards the header over the network in the clear.CheckRedirectnow refuses a downgrade unless the profile opted in, naming only the scheme and host since a redirect target can carry secrets in its path or query. Replacing the default policy also replaces its redirect limit, so the limit is restated (maxRedirects = 10).Four tests added: a downgrade redirect is refused with the credential never reaching the cleartext server while a same-scheme redirect is still followed; the insecure opt-in permits the downgrade; a legacy cleartext profile sends zero requests yet remains listable and removable; and a legacy-profile login is rejected before the password prompt. The 32 existing tests that seed cleartext
httptestprofiles now declare the opt-in they always implicitly relied on.