Warn when the .env holding the API token is readable by others

.env.example

@@ -1,4 +1,6 @@
 # Copy to .env and fill in. Required by markfluence.
+# Then: chmod 600 .env -- it holds your API token, and markfluence warns if
+# anyone else can read or write it.
 CONFLUENCE_URL=https://your-org.atlassian.net
 CONFLUENCE_USERNAME=you@example.com
 CONFLUENCE_TOKEN=your-api-token

README.md

@@ -84,6 +84,19 @@ CONFLUENCE_TOKEN=your-api-token
 > The API token is deliberately not accepted as a command-line flag; it comes
 > only from the environment or `.env`.
 
+Then restrict it, since it holds your API token:
+
+```
+chmod 600 .env
+```
+
+markfluence warns when the `.env` it read is reachable by anyone but you *and*
+contains `CONFLUENCE_TOKEN` — a `.env` holding only the URL and username is
+nobody's secret, so its mode is left alone. The warning names the file, what is
+wrong with its mode, and the `chmod` that fixes it. Under `--json` it is not
+printed but carried in the output document's `warnings` array (and on the
+stderr error object), because stderr in that mode is itself a JSON document.
+
 (Optional): `alias mf=markfluence`
 
 ### Scoped tokens and service accounts
@@ -865,6 +878,7 @@ target (a single element for `info`/`read`); `summary` carries batch counts:
   "markfluence_version": "1.4.0",
   "command": "update",
   "roots": ["/repo/docs"],
+  "warnings": [],
   "results": [
     {
       "ok": true,
@@ -907,6 +921,12 @@ Notes on the schema:
   concept (`find`, `search`, ...) or a pre-flight failure that never reached
   root resolution. `schema` emits no envelope at all, so it has no `roots` key
   to speak of.
+- **`warnings`** carries warnings about the *invocation* rather than about any
+  page or file — currently only the `.env` permission warning below. A result's
+  own warnings live on the result; this is for something that belongs to no
+  result. `[]` when there is nothing to report. It appears on the stderr error
+  object too, since a fatal failure emits no envelope and a credential failure
+  is exactly the run where a warning about your `.env` matters.
 - **Status verbs** are per-command: `published`/`skipped` (`update`),
   `created`/`not_created` (`create`), `changed`/`consistent` (`fix`),
   `clean`/`warnings`/`broken` (`check`),
@@ -956,7 +976,7 @@ Errors and exit codes:
   error object to **stderr** and exit `2`:
 
   ```json
-  { "schema_version": 1, "command": "update", "error": "…", "code": "CONFIG" }
+  { "schema_version": 1, "command": "update", "error": "…", "code": "CONFIG", "warnings": [] }
   ```
 
 - Error `code` values: `CONFIG`, `AUTH`, `NOT_FOUND`, `VALIDATION`, `CONVERT`,

cmd/root.go

@@ -67,6 +67,7 @@ var rootCmd = &cobra.Command{
 		ui.SetDebug(debugFlag)
 		ui.SetJSON(jsonFlag)
 		client.SetRetryLogger(logRetry)
+		client.SetSecurityWarner(reportSecurityWarning)
 		return nil
 	},
 	// Bare `markfluence` prints help; subcommands carry the work.
@@ -79,6 +80,20 @@ var rootCmd = &cobra.Command{
 	SilenceErrors: true,
 }
 
+// reportSecurityWarning delivers a credential-hygiene warning to both output
+// modes: a human sees it immediately on stderr, and --json carries it in the
+// documents rather than printing it, because stderr under --json is itself a
+// schema-validated document (#/$defs/errorObject) -- a stray human line ahead
+// of it would break a consumer that parses stderr, which the schema invites.
+//
+// Both, not either: the warning is raised during credential resolution, before
+// anything knows whether this run will emit an envelope, an error object, or
+// (on a --dry-run of nothing) neither.
+func reportSecurityWarning(msg string) {
+	jsonout.AddWarning(msg)
+	ui.Warn(msg)
+}
+
 // Execute runs the root command, exiting non-zero on error. A failure a command
 // already reported (a silent error) is not printed again and exits with its
 // carried code (1 operational, 2 config/usage). Any other error is

cmd/root_test.go

@@ -2,10 +2,16 @@ package cmd
 
 import (
 	"bytes"
+	"io"
+	"os"
+	"path/filepath"
 	"strings"
 	"testing"
 
+	"github.com/mozilla/markfluence/internal/client"
+	"github.com/mozilla/markfluence/internal/jsonout"
 	"github.com/mozilla/markfluence/internal/schematest"
+	"github.com/mozilla/markfluence/internal/ui"
 )
 
 // TestRootCommandWiring is the step-1 smoke test: it confirms the root command
@@ -139,3 +145,128 @@ func TestSubcommandsCompleteArgs(t *testing.T) {
 		}
 	}
 }
+
+// TestSecurityWarnerIsWired pins the one line that makes the .env permission
+// warning exist at runtime. Everything else about it is tested in
+// internal/client (the predicate) and internal/ui (the output), each against
+// its own double -- so deleting the SetSecurityWarner call in
+// PersistentPreRunE would leave every one of those tests passing and the
+// feature silently gone. A retry log going quiet is a debugging annoyance; a
+// security warning going quiet is the feature not existing.
+func TestSecurityWarnerIsWired(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, ".env")
+	body := "CONFLUENCE_URL=https://wiki\nCONFLUENCE_USERNAME=bot\nCONFLUENCE_TOKEN=secret\n"
+	if err := os.WriteFile(path, []byte(body), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Chmod(path, 0o644); err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { client.SetSecurityWarner(nil) })
+
+	if err := rootCmd.PersistentPreRunE(rootCmd, nil); err != nil {
+		t.Fatalf("PersistentPreRunE: %v", err)
+	}
+
+	r, w, err := os.Pipe()
+	if err != nil {
+		t.Fatal(err)
+	}
+	old := os.Stderr
+	os.Stderr = w
+	_, resolveErr := client.Resolve(client.ResolveOptions{EnvFile: path})
+	os.Stderr = old
+	if err := w.Close(); err != nil {
+		t.Fatal(err)
+	}
+	out, err := io.ReadAll(r)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if resolveErr != nil {
+		t.Fatalf("Resolve: %v", resolveErr)
+	}
+	if !strings.Contains(string(out), "holds your API token") {
+		t.Errorf("stderr = %q, want the .env permission warning: is SetSecurityWarner still wired?", out)
+	}
+}
+
+// TestSecurityWarningUnderJSONStaysOffStderr is the regression this design
+// exists for. Under --json, stderr is itself a schema-validated document
+// (#/$defs/errorObject, asserted in cmd/children's own tests), so a
+// human-readable warning line printed ahead of it would break any consumer
+// that parses stderr -- and the schema invites exactly that. The warning has
+// to travel inside the documents instead.
+func TestSecurityWarningUnderJSONStaysOffStderr(t *testing.T) {
+	jsonout.ResetWarnings()
+	t.Cleanup(jsonout.ResetWarnings)
+	ui.SetJSON(true)
+	t.Cleanup(func() { ui.SetJSON(false) })
+
+	const msg = "/tmp/.env is readable by others (mode 0644) and holds your API token; run: chmod 600 /tmp/.env"
+
+	r, w, err := os.Pipe()
+	if err != nil {
+		t.Fatal(err)
+	}
+	old := os.Stderr
+	os.Stderr = w
+	reportSecurityWarning(msg)
+	os.Stderr = old
+	if err := w.Close(); err != nil {
+		t.Fatal(err)
+	}
+	printed, err := io.ReadAll(r)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(printed) != 0 {
+		t.Errorf("stderr = %q under --json, want nothing: it would precede the error object", printed)
+	}
+
+	// Both documents carry it, and both still validate.
+	var buf bytes.Buffer
+	if err := jsonout.EmitError(&buf, "read", "missing Confluence username", jsonout.CodeConfig); err != nil {
+		t.Fatalf("EmitError: %v", err)
+	}
+	schematest.ValidateError(t, buf.Bytes())
+	if !strings.Contains(buf.String(), "holds your API token") {
+		t.Errorf("error object = %s, want the warning carried in it", buf.String())
+	}
+
+	buf.Reset()
+	env := jsonout.NewEnvelope("read", nil, map[string]int{"total": 0})
+	if err := jsonout.Emit(&buf, env); err != nil {
+		t.Fatalf("Emit: %v", err)
+	}
+	if !strings.Contains(buf.String(), "holds your API token") {
+		t.Errorf("envelope = %s, want the warning carried in it", buf.String())
+	}
+}
+
+// TestSecurityWarningInHumanModeGoesToStderr: the other half. Nothing structured
+// is emitted in human mode, so the line itself is the whole delivery.
+func TestSecurityWarningInHumanModeGoesToStderr(t *testing.T) {
+	jsonout.ResetWarnings()
+	t.Cleanup(jsonout.ResetWarnings)
+
+	r, w, err := os.Pipe()
+	if err != nil {
+		t.Fatal(err)
+	}
+	old := os.Stderr
+	os.Stderr = w
+	reportSecurityWarning("mind the mode")
+	os.Stderr = old
+	if err := w.Close(); err != nil {
+		t.Fatal(err)
+	}
+	printed, err := io.ReadAll(r)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !strings.Contains(string(printed), "mind the mode") {
+		t.Errorf("stderr = %q, want the warning", printed)
+	}
+}

internal/client/config.go

@@ -161,6 +161,75 @@ func loadEnvFile(envFile string, roots *project.Cache) (map[string]string, error
 	return env, nil
 }
 
+// securityWarner receives a credential-hygiene warning. Package-level and set
+// once from the command layer for the same reason SetRetryLogger is
+// (retrylog.go): twelve commands build a client through Resolve with an
+// identical literal, so anything passed per-call is something the thirteenth
+// silently forgets -- and internal/client deliberately produces no output and
+// imports no ui.
+var securityWarner func(string)
+
+// SetSecurityWarner installs fn as the credential-hygiene reporter, replacing
+// any previous one. Pass nil to silence it.
+func SetSecurityWarner(fn func(string)) { securityWarner = fn }
+
+// warnLoosePermissions reports a .env that anyone but its owner can reach,
+// when that file is the one holding the API token.
+//
+// The token gate is what keeps this worth reading. A .env carrying only
+// CONFLUENCE_URL and CONFLUENCE_USERNAME at 0644 leaks nothing -- neither is a
+// secret, and the cloud ID is documented as not one either -- and a warning
+// that fires on a file with no secret in it is how a security warning becomes
+// something people learn to scroll past.
+//
+// os.Stat, not Lstat: a .env symlinked to a 0600 file is perfectly safe, and
+// the link's own 0777 would cry wolf on every run. The user execute bit is
+// ignored for the same reason -- 0700 is odd, but it is not a leak.
+//
+// A stat failure is silent. The file was just read, so a failure here is
+// exotic, and a warning about the inability to warn is noise.
+func warnLoosePermissions(path string, env map[string]string) {
+	if securityWarner == nil || env[tokenEnv] == "" {
+		return
+	}
+	fi, err := os.Stat(path)
+	if err != nil {
+		return
+	}
+	perm := fi.Mode().Perm()
+	if perm&0o077 == 0 {
+		return
+	}
+	securityWarner(fmt.Sprintf(
+		"%s is %s (mode %#o) and holds your API token; run: chmod 600 %s",
+		path, accessDescription(perm), perm, shellArg(path)))
+}
+
+// accessDescription names what is actually wrong with a mode, rather than
+// assuming the readable case: 0622 is a real finding but nobody can read it,
+// and a message that says otherwise is one a reader can check and disbelieve.
+func accessDescription(perm os.FileMode) string {
+	switch {
+	case perm&0o044 != 0:
+		return "readable by others"
+	case perm&0o022 != 0:
+		return "writable by others"
+	default:
+		return "accessible to others"
+	}
+}
+
+// shellArg quotes a path that would not survive being pasted into a shell. The
+// remedy is the point of the warning, so a path with a space in it has to come
+// out runnable; a path without one stays unquoted, since that is every path
+// anyone actually has.
+func shellArg(path string) string {
+	if strings.ContainsAny(path, " \t\n'\"$`\\&;|<>()*?[]#~") {
+		return "'" + strings.ReplaceAll(path, "'", `'\''`) + "'"
+	}
+	return path
+}
+
 // loadDotenv reads a simple .env file into a map: KEY=value lines, with blank
 // lines and # comments skipped, an optional leading "export ", and optional
 // surrounding single or double quotes stripped. Values are taken verbatim (no
@@ -183,6 +252,10 @@ func loadDotenv(path string) (map[string]string, error) {
 		}
 		out[strings.TrimSpace(key)] = unquote(strings.TrimSpace(value))
 	}
+	// Here rather than in loadEnvFile: this is the one function both the
+	// discovered .env and an explicit --env-file go through, and the check
+	// needs the parsed contents to know whether a token is in there.
+	warnLoosePermissions(path, out)
 	return out, nil
 }