fix(cli): separate transport auth from operation payload for add_user/alter_user

[kriszyp]

What / why

add_user/alter_user reused the same username=/password= CLI args as both the HTTP Basic auth credentials and the operation payload, so the CLI tried to log in as the user being created. That made non-interactive (CI/CD) add_user/alter_user — authenticating as an existing admin while creating/altering a different user — impossible. Env-var auth (HARPER_CLI_USERNAME/HARPER_CLI_PASSWORD) was also silently overridden by the payload params.

Root cause and fix, entirely in bin/cliOperations.ts:

• Added dedicated auth args auth_username=/auth_password= that always win the auth leg, and added them to TRANSPORT_ONLY_FIELDS so they never serialize into the request body.
• Removed username/password from TRANSPORT_ONLY_FIELDS — they're legitimate operation-payload fields for several ops (add_user, alter_user, create_authentication_tokens), not just transport. They remain the auth fallback for ops where they legitimately are the auth (e.g. plain create_table ... username= password=), preserving backward compatibility.
• Flipped auth precedence so env-var auth (HARPER_CLI_USERNAME/HARPER_CLI_PASSWORD, CLI_TARGET_USERNAME/CLI_TARGET_PASSWORD) beats plain username=/password= when no dedicated auth_* arg is set — otherwise a configured CI login is silently shadowed by an operation's own payload fields. Confirmed with @kriszyp before implementing (this is a deliberate behavior change, called out in the issue).
• Fixed the non-deploy JSON body path (body = req), which sent the entire raw, unstripped req object as the wire body — now uses the same operationFields() stripping the deploy paths already used, so auth_username/auth_password (and target/rejectUnauthorized/etc.) never leak into any request body.
• Updated the CLI HELP text (bin/harper.ts) to document the new args and env-var auth for the non-interactive path. Companion docs: HarperFast/documentation#613.

 const TRANSPORT_ONLY_FIELDS = new Set([
 	'target',
-	'username',
-	'password',
+	'auth_username',
+	'auth_password',
 	'rejectUnauthorized',
 	...
 ]);
...
 			username:
-				req.username || target.username || process.env.HARPER_CLI_USERNAME || process.env.CLI_TARGET_USERNAME,
+				req.auth_username ||
+				target.username ||
+				process.env.HARPER_CLI_USERNAME ||
+				process.env.CLI_TARGET_USERNAME ||
+				req.username,
...
 		} else {
-			body = req;
+			body = operationFields(req);
 		}

Test plan

• Added dedicated auth args vs operation payload fields (harper#1872) describe block in unitTests/bin/cliOperations.test.js: auth_username/auth_password win the Basic-auth header while add_user payload username/password stay in the body; backward-compat fallback to plain username/password when nothing else is configured; env-var auth beats payload username/password; auth args stripped from both the non-deploy JSON body and the multipart directory-deploy body.
• Full unitTests/bin/cliOperations.test.js suite passes (30/30).
• Ran the broader unit suite (test:unit:main pattern): 3743 passing, 10 pre-existing failures in unitTests/components/globalIsolation.test.js unrelated to this change (component sandbox path resolution, reproduces identically on main).
• oxlint and prettier --check clean on all three changed files.

Notes for reviewers

• npm run build currently fails on main with a pre-existing, unrelated TypeScript error in resources/DatabaseTransaction.ts (Promise<number | void> not assignable to Promise<void>, from feat(analytics): record write-transaction commit latency #1688) — not touched by this PR, tsc still emits (noEmitOnError is off) so this didn't block testing.
• Docs: Companion documentation: HarperFast/documentation#613.

Follow-up: review round 2 (commit 9473f61ba)

Three issues from review, plus three found by an outside-model pass on the fix itself:

1. Saved harper login token was still unreachable for add_user/alter_user (blocker). The payload fallback was folded into target.username before the Authorization mode was chosen, and if (target?.username) selects Basic ahead of the saved-token branch — so harper login followed by add_user username=newuser password=... authenticated as the user being created and 401'd. Authentication now resolves in this order:

1. explicitly configured credentials (dedicated auth_* args → target-URL userinfo → HARPER_CLI_* → CLI_TARGET_*)
2. the saved harper login operation token
3. the legacy username=/password= payload fallback — now requiring both, so a lone username= (drop_user username=bob) stays payload rather than becoming half a credential

2. Credentials are resolved as an atomic pair, never composed across sources. resolveTransportCredentials() walks the sources and returns the first that supplies anything, so auth_username=admin can no longer be paired with an add_user payload password. A source counts as configured once either half is supplied, empty or not, so a blank CI variable doesn't read as "absent". An incomplete explicit source (args, URL userinfo) is a hard error; an incomplete env pair warns on stderr and is skipped, because a lone HARPER_CLI_USERNAME/HARPER_CLI_PASSWORD is a supported harper login idiom and shouldn't break every later operation in the same shell or CI job.

3. Passwords no longer reach logs or terminal output. The parsed CLI request is redacted before logger.trace, and a password embedded in target=https://admin:secret@host is masked in the Connecting to ... line and anywhere the target is logged. Help text now leads with the env-var form and states that a password passed as an argument is exposed to shell history, process listings and CI logs.

Also fixed in bin/login.ts (found while verifying the above):

• harper login mixed env namespaces — username from CLI_TARGET_USERNAME, password from HARPER_CLI_PASSWORD — which is the same cross-pairing hazard. Whichever namespace supplies anything now owns both halves; a half it doesn't set is prompted for. This also flips login's namespace preference to HARPER_CLI_*-first, matching the rest of the CLI (only observable if both complete pairs are set to different values).
• Login now passes its credentials as dedicated transport auth as well as payload fields. Without that, moving the saved token ahead of the payload fallback meant a re-login after expiry would try to refresh the very token it was replacing and could exit with "Refresh token expired... please run harper login again".

Follow-up test plan

• unitTests/bin/cliOperations.test.js: 42 passing. New coverage — saved token wins for both add_user and alter_user while payload creds stay in the body; lone payload username= is not auth; CLI_TARGET_* pair still authenticates; lone auth_username, lone auth_password, empty auth_password and both-empty auth args all fail with a clear error; incomplete env pair warns and falls through to the saved token; URL password masked in the connection log; redactCredentials() unit tests.
• unitTests/bin/login.test.js: new credential sources block — no cross-namespace pairing, CLI_TARGET_*-only still works, credentials sent as dedicated transport auth.
• npm run test:unit:main: 3758 passing, 10 pre-existing unitTests/components/globalIsolation.test.js failures (verified identical with these changes stashed).
• oxlint and prettier --check clean.

Open question for reviewers

The asymmetry in 2 is a judgment call: incomplete explicit credentials are fatal, incomplete env credentials only warn. The alternative (fatal everywhere) would break export HARPER_CLI_USERNAME=admin; harper login; harper <op> — every operation after the login would fail even though the saved token is perfectly good. Happy to make it uniformly fatal if reviewers prefer that.

Note on the extra commit 676990fff

676990fff is not part of this change. It's a one-line type widening in resources/DatabaseTransaction.ts (recordCommitLatency's parameter to Promise<unknown>) that fixes the pre-existing npm run build failure from #1688, which was turning the downstream Next.js adapter integration check red on this PR for reasons unrelated to it. The same fix is queued as its own PR against main; once that lands, this commit is redundant and can be dropped in the rebase.

Refs #1872

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request introduces dedicated authentication arguments (auth_username and auth_password) to the CLI operations, preventing payload fields (such as those in add_user or alter_user) from being mistakenly used as transport credentials. It also updates the CLI help documentation and adds unit tests to verify the priority of these credentials and that they are stripped from the request bodies. Feedback was provided regarding a potential credential cross-contamination risk where a partially specified higher-priority credential source could cause the fallback logic to incorrectly mix admin usernames with payload passwords, along with a suggested fix to guard the fallback.

[claude]

Reviewed; no blockers found. The only change since the last review pass (commit 6c6bd5b) is documentation-only — HELP text noting the stale-token 401 behavior and a comment clarifying SECRET_FIELDS is a by-name allowlist. Both prior inline findings (credential cross-contamination) remain resolved via resolveTransportCredentials().

[DavidCockerill]

Read through this carefully — the design is sound and the test coverage is genuinely strong (42 assertions across two files). No correctness bugs found. I traced the auth control flow, the atomic credential resolution, and the redaction paths, and confirmed utility/common_utils.ts httpRequest does no logging, so the Basic header can't leak downstream. The only programmatic caller (bin/login.ts) is updated, and the dormant .mjs test is describe.skip. Not requesting changes.

One thing I want to sign off on explicitly rather than let through silently, plus a few notes.

The auth-precedence change is a real behavior change on a stable CLI, and it deserves a conscious yes. Env-var auth and the saved harper login token now outrank payload username=/password=, where the payload previously won (bin/cliOperations.ts:393).

In plain terms: if someone's CI was quietly leaning on username=admin password=… to authenticate — say for create_table — and that same shell also had HARPER_CLI_USERNAME exported for something else, the command now authenticates as the env var instead of the argument. Almost always that's the same admin and nothing visibly changes. But it's exactly the kind of silent "who am I authenticating as" shift that should be a deliberate decision on an auth path rather than a surprise.

It's intended, it's yours, and it's tested — so consider this me blessing it rather than flagging it. Recording it here so the decision is on the record if someone hits it later.

The docs follow-up matters more than usual because of that. New auth_username=/auth_password= args, env-var auth, and new precedence rules are all user-facing. Is the documentation-repo follow-up tracked yet? I'd rather it not lag far behind the merge.

Smaller notes, none blocking:

1. Incomplete-env warning is noisy (cliOperations.ts:171). Exporting a lone HARPER_CLI_USERNAME is a documented harper login idiom (password prompted), but in a non-login op it emits Ignoring incomplete credentials: … on stderr for that command and every later one in the shell. Consider suppressing it when a saved token or other complete source is going to satisfy the request anyway.
2. redactCredentials is field-name-based, not exhaustive (cliOperations.ts:225). SECRET_FIELDS is {auth_password, password} plus target-URL userinfo — right for this PR's scope, but any other secret-bearing arg (a token, a key) would still reach logger.trace. Worth a comment saying so, so nobody assumes the trace log is safe by construction.
3. Stale-token edge worth a line in the help/docs (cliOperations.ts:438). If a saved token exists and refresh fails with a non-401, Authorization stays set to the expired Bearer, so the legacy payload-credential fallback is skipped and the request 401s — where payload creds used to work. That's a coherent consequence of making the token outrank the payload (i.e. the point of the fix), and harper logout / auth_* is the escape hatch — it just deserves to be written down.
4. On the asymmetry you flagged (incomplete explicit creds fatal, incomplete env creds warn-and-skip): agreed with your default. Failing every op after a bare export HARPER_CLI_USERNAME would be worse. Blessing it rather than letting it pass unremarked.
5. Housekeeping: you noted 676990fff (the DatabaseTransaction.ts type widening) isn't part of this change and gets dropped on rebase — worth confirming it's gone at merge so it doesn't ride in on an auth PR.

Note on review coverage: this one got a Claude-only pass — both outside-model legs were unavailable at the time. Codex is working again now, so if you'd like a second opinion on the auth surface before merging, say the word and I'll run it.

— Reviewed by DAIvid (Claude Opus 5)

[kriszyp]

Addressed two of the small, non-blocking notes from @DavidCockerill's review in 6c6bd5b:

• HELP text (bin/harper.ts) now documents the stale-token 401 edge case (a saved harper login token outranks username=/password=, so a refresh failure 401s instead of falling back to payload creds — harper logout or auth_username=/auth_password= is the escape hatch).
• Comment on SECRET_FIELDS (bin/cliOperations.ts) now notes it's a by-name allowlist, not exhaustive, so future secret-bearing args need to be added explicitly.

Left the "noisy incomplete-env warning" suggestion (suppress it when a saved token/fallback ends up authenticating anyway) as-is — deferring/suppressing that warning would contradict the existing warns and falls through to the saved token when only one env var of a pair is set test, which locks in today's "always warn" behavior as intentional. That's a design call, not a mechanical fix, so leaving it for @kriszyp to weigh in on if he wants it changed.

The two inline review threads (Gemini + earlier Claude pass on atomic credential resolution) were already resolved in 9473f61ba.

— Claude Sonnet 5

[DavidCockerill]

Approving at 6c6bd5b. The delta since my last pass is six lines of comment and help text across two files — zero logic change, CI green on Node 22/24/26.

Everything I raised is closed:

• The auth-precedence change is documented, and documentation#613 covers all three user-facing pieces — the auth_* args, env-var auth, and the precedence order — plus stale-token recovery. That was my main ask, since the precedence shift silently changes who a command authenticates as.
• redactCredentials' field-name-based scope is now noted in a comment.
• The stale-token fallback behaviour is written down.
• The stray 676990fff DatabaseTransaction.ts commit is gone; the merge-base is now its standalone fix on main.

Re-verified the security properties at this head: no credential reaches a log, redaction still applied, Basic header still not logged. I also chased and refuted one candidate of my own — a URL parse error potentially leaking target userinfo.

Only open item is the incomplete-env warning noise, which you deliberately deferred; the current test locks in always-warn, so it wants a design call rather than a quick change. Happy to file it as a follow-up.

— Reviewed by DAIvid (Claude Opus 5)