docs(cli): token credentials in the canonical auth precedence (v5.2.0) - #630
Conversation
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-630 This preview will update automatically when you push new commits. |
Restacks this branch on #630, which carries the `harper login --for-ci` / `HARPER_CLI_REFRESH_TOKEN` content that used to live here. That content documents behavior shipped in v5.2.0, so it no longer belongs behind this branch's four unmerged upstream dependencies. Three conflicts, resolved as follows. `reference/cli/authentication.md` and `reference/cli/commands.md` — took #630's side wholesale. Everything this branch had added to those two files was the token content that moved, and #630 states it against main's current structure rather than the structure this branch forked from. That also settles the duplicate-precedence hazard @Ethan-Arrowood raised: main grew a canonical numbered Authentication Precedence section after this branch forked, and the token rules are now folded into that list instead of restated beside it. Merging this branch no longer gives the precedence order twice, differently. `reference/cli/operations-api-commands.md` — hand-merged, because neither side was correct alone. Taking ours would have silently dropped the seven backup rows main added since the fork (`create_backup`, `list_backups`, `verify_backup`, `delete_backup`, `purge_backups`, `restore_backup`, `get_backup`); taking theirs would have dropped this branch's four deploy rows and its recategorization. The merged table has all three sets: - main's seven backup rows, unchanged. - This branch's `stage` / `activate` / `revert_component` / `revert` rows. - This branch's recategorization: the six user/role rows point at `#users--roles` and the seven SSH rows at `#components`, instead of the `#certificate-management` anchor that documented neither. The five genuine certificate rows still point there. Compared both sides row by row before resolving: no description text differed, so the column-width churn is only Prettier re-padding. `operations.md` and `applications.md` auto-merged; main's new `host` badge survives. Build clean. The two broken anchors it reports are pre-existing on main (confirmed by building main with these changes stashed) and are in files this branch does not touch.
The v5.2.0 placeholder this branch has carried since July is wrong, and now demonstrably so rather than provisionally. harper `main` is at **v5.2.2** (tagged 2026-08-13) and by_ref merged 2026-08-14, after that tag and in no release. The v5.3.0 upgrade directive on harper#2173 confirms which release is next. So none of this surface can ship in a 5.2.x patch: | Surface | Upstream | Ships in | | --- | --- | --- | | two-phase deploy, `revert_component`, staging retention | harper#1849 | v5.3.0 | | `deploy by_ref=true` | harper#1850 (merged, untagged) | v5.3.0 | | `deploy setup=true` | harper#1851 | v5.3.0 | | `add_ssh_key generate: true` | harper-pro#594 | v5.3.0 | The one piece that genuinely shipped in v5.2.0 — `login --for-ci` and the token env vars (harper#1876) — left this branch for #630, so nothing here is v5.2.0 any more. Addresses @kriszyp's two versioning findings. Both plain-text `Available since` lines are now `<VersionBadge>`, standalone under their headings per CONTRIBUTING.md, and the API reference no longer reads as though these behaviors exist throughout v5: - `deploy_component` — changed badge; it is existing surface whose cluster behavior changed. - `revert_component` — added badge, and the row it was missing from the Components summary table. - `add_ssh_key`'s `generate` subsection — added badge. Release-notes coverage, the other half of those findings, follows in a separate commit once the OIDC surface is in.
@kriszyp's eight findings, re-checked against the current state of harper#1849 and harper-pro#594 rather than the state they were filed against. Four had been fixed at the source in the meantime; four needed the docs changed. **"All-or-nothing at go-live" was wrong, and self-contradictory.** The page claimed it in the opening paragraph and then correctly described a version split twenty lines later. Reframed around what the two phases actually buy: an all-nodes staging barrier, which eliminates the common failure (a node that can't fetch or install) but not a per-node activation failure. The activation-failure paragraph is now its own `#### Activation failures` section instead of sitting mid-parameter-list, and the opening paragraph links to it. **The deploy modes are now documented as a matrix**, which core made possible by adding the validation @kriszyp asked for. `activate: false` and `deployment_id` are rejected outright when combined with `two_phase: false`, `replicated: false`, or a cluster whose `system` database isn't replicated, so a request that says stage-only can no longer activate. `two_phase: true` is likewise rejected without the replication to support it. `revert_on_failure` is refused rather than accepted and ignored. **The `system`-replication precondition is stated** — @Ethan-Arrowood's second question. A plain deploy on a cluster with `system` excluded from replication still silently takes the one-shot path, and that is the case worth warning about, because it also means no rollback target. **`revert_component`'s guarantees are scoped to the two-phase path.** The retained `.deploy-previous` copy is created by activation, so a component deployed one-shot has nothing to revert to however many times it has been deployed — the docs implied repeated deploys build up a rollback target. Also documents that `restart` is optional (a reverted component keeps serving the rolled-away-from code until something restarts it) and adds the `force` parameter the validator accepts. **The SSH-key encryption guarantee is qualified.** `sealSSHKey` in harper-pro still falls back to storing and replicating the private key in plaintext with only a WARN when no secret custody is registered, and that fallback is deliberate — SSH keys predate custody. So encryption at rest is a property of the deployment's configuration, not of the operation, and `generate: true` in particular reads as though the key could never be exposed. Custody is present by default, so the warning says that too rather than implying the risk is likely. Fixed at the source, no doc change needed: - **Explicit refs now resolve to a SHA** (harper 5c85714b8, 26cce986d, 21043f994) — the guarantee the page claimed is now real. The fuller treatment, including remote resolution and failing closed, is in #617. - **`auth_*` vs. ordinary `username=` precedence** — `main` grew a canonical numbered precedence section covering exactly this, and #630 folds the token rules into it. - **Version badges** — previous commit.
…rface Completes @kriszyp's discoverability findings. The reference pages carry version badges now, but a user who only reads release notes had no way to find any of this — and there was no 5.3 page at all. The directory is autogenerated into the sidebar, so the new page needs no registration. Covers, in the house style of 5.2.md: two-phase cluster deploys, stage-now/activate-later, component rollback, deploying by git reference, sealed deploy credentials, OIDC trusted publishing, and server-side SSH key generation. Each entry states the limits alongside the capability rather than leaving them to the reference page, because these are the ones that change whether a reader should adopt the feature: the two-phase barrier is not an all-or-nothing go-live, it silently degrades on a cluster whose `system` database isn't replicated, rollback reaches back exactly one two-phase activation, and the GitHub Actions ref-gate rule will refuse policy shapes that npm accepts. `login --for-ci` and the token env vars are not here — they shipped in v5.2.0 and belong in 5.2.md, which is #630's.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-630 This preview will update automatically when you push new commits. |
`HARPER_CLI_REFRESH_TOKEN` / `HARPER_CLI_OPERATION_TOKEN` shipped in v5.2.0 (harper#1876) but never reached the reference docs. The Authentication Precedence list stopped at the saved `harper login` token, so the two variables a CI pipeline is supposed to use were absent from the one place that states which credential wins. Folded into main's existing numbered list rather than stated separately, so the order is given once: bearer tokens sit at position 5, above the saved login token and above the legacy `username=`/`password=` fallback, which is what cliOperations.ts does. Also documents the parts that are only discoverable by reading the source: - Token variables apply to remote targets only. A local operation goes over the domain socket and is already trusted, so attaching a bearer token there would opt out of that trust and 401 — hence the deliberate ignore. - A token namespace owns both halves. `HARPER_CLI_OPERATION_TOKEN` cannot pair with `CLI_TARGET_REFRESH_TOKEN`, which would otherwise run as one identity until expiry and then silently continue as another. - A set-but-blank namespace reports and falls back rather than silently running as whoever last logged in locally. - An env-sourced refreshed token stays in memory; there is no file entry to persist it to. - One refresh-token hash per user, so `--for-ci` as your own account revokes your own token. Hence the dedicated-CI-user warning. `harper login --for-ci` is documented in commands.md with the stdout/stderr split that makes `| gh secret set --env-file -` work without displaying the token. Verified against harper v5.2.2: bin/cliOperations.ts (precedence and refresh), bin/login.ts (--for-ci output), config/configUtils.ts (1d / 30d defaults). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The other half of @kriszyp's discoverability finding, for the piece that actually shipped in v5.2.0. A user who only reads release notes had no way to discover that a pipeline can authenticate with a token instead of an admin password. States the two properties that change how someone sets this up: the token variables outrank a saved login (so a runner with both uses the CI identity), and one refresh-token hash per user means issuing a token for CI revokes your own if you run it as yourself. The 5.3 surface goes in 5.3.md, added on #599.
888a5fd to
50292cb
Compare
The paragraph contradicted itself: "an error, not a fallback", then described the fallback, then denied the behavior it had just described. The CLI warns and continues down the precedence list, which does mean running as whoever last logged in on that machine — the warning is the only thing that makes it non-silent. Caught by a review pass against #599, where this text also lives. Fixing it only there would have left this PR publishing the wrong version, since this is the PR that owns the paragraph and the one meant to merge first.
#630 was rebased onto main (12 new commits, mostly analytics docs), so this branch picks up the rewritten base. Three conflicts in `authentication.md`, all resolved to this branch's side — the precedence list with OIDC at 7, the corrected blank-token paragraph, and the workload-identity section #630 does not have. Resolved hunk by hunk rather than with `checkout --ours`, which is what silently dropped content the last time. One of those conflicts is worth noting: #630 still carries the original "an error, not a fallback" wording for a blank token namespace, because that paragraph is #630's content and the correction was only ever applied here. Being backported to #630 separately — otherwise #630 publishes the self-contradictory version if it merges first, which it is meant to.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-630 This preview will update automatically when you push new commits. |
Cross-model review at 613c2e8 — codex and gemini, the first run where the gemini leg produced output. **A rejected refresh token does not reliably stop the command.** The page said it "reports that and exits non-zero rather than falling back to another identity". Only a 401 does that. `refreshExpiredOperationToken` logs any other failure — a 5xx, a timeout, a connection error — and returns, leaving no bearer token set, so execution reaches the legacy `username=`/`password=` fallback and a command carrying that pair authenticates as it. Someone treating a refresh failure as a pipeline stop would be wrong in exactly the cases that matter: a flaky network or a degraded server. **A blank preferred namespace shadows a complete legacy one.** The namespace is picked with `.find()` on `!== undefined`, so `HARPER_CLI_REFRESH_TOKEN=` claims the choice while supplying nothing, and a valid `CLI_TARGET_REFRESH_TOKEN` is never read — the run drops to the saved login token instead. The owns-both-halves rule was documented; that it is decided by presence rather than by usable value was not. Advice added: unset the variable rather than blanking it. Both verified in `bin/cliOperations.ts` at v5.2.4 before changing the text.
Second review round (codex + gemini + cursor-composer, at 5bd6ec0) sharpened the fix from the first. My correction said any non-401 refresh failure leaves the command with no bearer token. That is only true for the refresh-token-only shape. `refreshExpiredOperationToken` is entered when the operation token is absent *or* expired, and on a non-401 failure it returns without clearing what is already there. So: - Refresh token only — what `--for-ci` provisions, and the CI case — no bearer token is attached, and a payload `username=`/`password=` pair can authenticate the command as a different identity. - An expired operation token also present — that expired token is still attached, the server rejects it, and you get a 401 rather than an identity switch. Also documents that a `200` carrying no `operation_token` is not reported at all, so a clean exit is not evidence the intended identity ran. The 5.2 release note claimed a configured CI identity is authoritative on a runner that also has a developer's login. True for precedence, not for this failure mode; qualified accordingly. Both legs converged on the same gap from different angles, which is why it is worth the extra paragraph rather than a parenthetical.
…ntial shape The 5.2 note applied the no-bearer-token fallback to every non-401 refresh failure, which over-generalizes in the same way the reference page did before it was split: with an expired operation token also present, that token is still attached and the server 401s instead. Scoped to the refresh-token-only shape, which is what `--for-ci` provisions and what the note is about. Third review round; the findings have narrowed each time (major, then two minors, then one), which is convergence rather than a moving target.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-630 This preview will update automatically when you push new commits. |
Domain adjudication ran for the first time this session — the `claude` CLI's OAuth token had expired, which is why five earlier rounds reported an opaque `exit-1`. Its first pass found a major the four codex/gemini rounds had missed. **An expired refresh token does not stop the command.** `validateRefreshToken` maps `TokenExpiredError` to `FORBIDDEN` (403) and everything else invalid to `UNAUTHORIZED` (401); the CLI halts on 401 only. So expiry — the guaranteed end state of every `--for-ci` token once `refreshTokenTimeout` elapses — takes the continue path, and in the refresh-token-only shape that means no bearer token and a payload `username=`/`password=` pair authenticating instead. The page promised a non-zero exit for exactly the failure a pipeline is most likely to hit. Now a warning, with the advice not to build a day-31 runbook around an exit code. Three more, all verified before changing the text: - **`--for-ci`'s "never appears on screen" was unconditional and is not.** The guard is on `process.stdin.isTTY` (an interactive confirmation before rotating that user's token); there is none on stdout, so a bare invocation prints the refresh token into scrollback. Qualified to the piped case and made "pipe it" explicit. - **The local-socket carve-out said what is ignored, never what authenticates.** On a self-hosted runner an unset or blank `target` does not fail — it resolves to the local instance and runs as superuser on the socket's ambient trust. A job that loses `HARPER_CLI_TARGET` deploys to the runner's own node. - **Precedence rank 5 taught the wrong fallback model.** Ranks 3 and 4 are a real two-step fallback; the token namespaces are not, since presence alone shadows. An operator generalizing from 3/4 would blank the preferred variable expecting the legacy one to be read. Also corrects `reference/configuration/options.md`, which gave `refreshTokenTimeout` a `1d` default while its own YAML example above said `30d` and `config/configUtils.ts` defaults to `30d`. Pre-existing and self-contradictory within one file; fixed while CI token-rotation planning depends on the number.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-630 This preview will update automatically when you push new commits. |
Filed as harper#2297 (Bug, P2, v5.3). Two changes follow from that: The warning now says the behavior is a defect rather than a property of the product, and names the issue — so a reader knows it is expected to change, and whoever fixes it has a breadcrumb back to the doc that has to be removed. It also drops the internal symbol names (`validateRefreshToken`, `TokenExpiredError`). The adjudicator flagged that as trading durability for reviewer confidence, and it was right: they were there to show the claim was source-derived, which the issue link now does better. Observable status codes are what a reader can act on.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-630 This preview will update automatically when you push new commits. |
Final round, all three findings verified in source before editing. **major — an expired token against a loopback target does not fail, it succeeds as superuser.** `authentication.authorizeLocal` defaults to `true` (config-root.schema.json), and `security/auth.ts` grants `getSuperUser()` to any request from `127.0.0.` or `::1` with no credential checked. So on a self-hosted runner pointed at its own node, a refresh-token-only expiry produces a green, fully privileged run rather than an error — the failure mode least likely to be noticed, and the one the page did not cover. My earlier local-trust note was also too narrow: it credited the domain socket, when loopback TCP is trusted the same way by default. **minor — an expired operation token answers 403, not 401.** `validateOperationToken` and `validateRefreshToken` both delegate to the shared `validateToken`, whose catch maps `TokenExpiredError` to `FORBIDDEN`. I had carried the 401 over from the malformed-token case, which would have sent someone alarming on the wrong status. **minor — `pbcopy` was offered as equivalent to a secret store and is not.** The "off your screen and out of your shell history" reasoning holds for `gh secret set`; a clipboard-history tool keeps a copy on disk, and since a user holds exactly one refresh token, that copy is the pipeline's live credential. The example is removed from both pages that carried it (it came from the CLI's own docstring, so it is worth not propagating) and the claim is scoped to piping into something that will store it.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-630 This preview will update automatically when you push new commits. |
The adjudicator caught that my previous commit fixed the failure list but not the containment promise above it, so the page said two opposite things about one failure. Line 227 claimed that dropping payload credentials makes an expired token "fail visibly", while the new bullet says a loopback target returns a green, fully privileged run under exactly those conditions. The callout is where a hurried reader stops, so the qualifier belongs there rather than three paragraphs down. The one-style tip had the same overreach: it now says the rule bounds *which* credential is used, not whether one is required, because against a loopback target Harper authenticates by address and no choice of style helps. Worth noting the adjudicator reversed its own prior ruling here — it had dropped this finding last round on the reasoning that loopback HTTP authenticates normally, then verified `authorizeLocal` defaults to `true` and said so. Two more from the same round: - **"Revoke its access" named no procedure, and none exists.** There is no revoke operation: `harper logout` only deletes the local copy and leaves the server-side hash valid. The remediation is superseding the hash by minting again as that user, or deactivating the user with `alter_user`. A responder to a leaked token would otherwise have gone looking for a command that isn't there. - **`CLI_TARGET_OPERATION_TOKEN` was implied but never named.** Entry 5 says "legacy `CLI_TARGET_` equivalents" plural while the variable list gave only the refresh one, and the owns-both-halves rule makes the answer load-bearing.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-630 This preview will update automatically when you push new commits. |
Both outside lenses converged on this and were right: the remediation I gave was disproved by the sentence before it. The paragraph established that a loopback run "comes back green and fully privileged", then told the reader to check the operation's result — but a superuser-authorized run produces a successful result, so the check cannot distinguish it from a correctly authenticated one. An operator following that line would have added an assertion, called the pipeline monitored, and learned nothing at day 31. There is no monitoring answer for this case, because nothing fails. The advice is configuration: do not point CI at loopback, or set `authentication.authorizeLocal: false` on a node a runner can reach. Verified that recommendation is safe before giving it — the domain-socket clause in `security/auth.ts` carries no `AUTHORIZE_LOCAL` guard, so turning the flag off removes loopback-TCP superuser without breaking local `harper` commands. Said so, since an operator will otherwise assume it does.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-630 This preview will update automatically when you push new commits. |
…d it needs a restart
Two majors, both invalidating the remediation in the previous commit. Fixing
these despite having declared a stopping rule, because wrong security-remediation
advice is worse than one more edit: a reader following it would believe an
exposure was closed when it was not.
**"Do not point CI at a loopback address" is not sufficient.** A same-host
reverse proxy makes every forwarded request arrive from `127.0.0.1`, so
`https://prod:9925` fronted by nginx on the Harper node is still a loopback peer
for authorization purposes. This repo already documents that hazard at
`reference/security/configuration.md` under `authorizeLocal` ("for example, when
using a local proxy"), so the page I wrote contradicted our own security docs.
That branch is dropped; `authorizeLocal: false` is the only sufficient answer, and
the two pages are now cross-linked.
**The flag needs a restart.** `AUTHORIZE_LOCAL` is a module-scope binding in
`security/auth.ts`, evaluated once at load, so a live config change does not
affect a running node. Omitting that would have left someone believing they had
closed the hole the moment they saved the config.
Also states that turning it off does not break local `harper` commands, since the
domain-socket clause is a separate rule the flag does not gate — otherwise the
advice reads as "break your own CLI to fix CI".
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-630 This preview will update automatically when you push new commits. |
Two majors from the receipt round at 366b09c, both verified in `security/auth.ts` first. **The reassurance and the residual hole are the same fact.** I wrote that turning `authorizeLocal` off does not break local `harper` commands because the domain socket is trusted by a separate rule the flag does not gate — true, and I framed it purely as comfort. That separate rule has no `AUTHORIZE_LOCAL` guard, so a reverse proxy whose upstream is the operations socket rather than a TCP port is still superuser on every request with the flag off. The remediation I gave closes the loopback-TCP path and nothing else. Now stated as a `:::danger`, with the actual boundary: do not proxy the operations socket, and treat filesystem permissions on it as the access control. **The page contradicted itself about a missing target.** Line 27, inherited from `main`, says omitting `target` "defaults to using the local domain socket connection" — absolute. The saved-target correction I added later says it falls back to the previous `harper login` target first. Both were on the page. Line 27 is the wrong one; qualified, and pointed at the precedence section. Recording plainly: this is the thirteenth round on this branch, and it is the third consecutive one to find that a remediation I wrote was insufficient in a case I had not considered. I am not confident a fourteenth converges. The security-adjacent guidance on this page wants a human who owns the deployment model, which is the ask in the PR description rather than something I can settle by re-reviewing.
Three consecutive rounds found the remediation I wrote insufficient in a case I had not considered: first that a result check cannot detect it, then that avoiding a loopback target does not help behind a same-host proxy, then that `authorizeLocal: false` does not cover a proxy whose upstream is the operations socket. Each fix was correct and each was incomplete, which is the signal that this content is in the wrong place rather than merely underspecified. It is server security configuration. It surfaced here because a token-expiry failure exposed it, but `reference/security/configuration.md` owns `authorizeLocal` and already carries the local-proxy warning. So this page now does what a CLI page should: states the hazard a pipeline author needs — an expired token against a node with local authorization enabled produces a successful, fully privileged run, and no exit code or result check detects it — names the flag, notes that neither the flag alone nor a non-loopback target is sufficient, and sends the reader to the page that owns the full remediation. That removes the un-converged material from this PR without hiding the risk, and puts the detailed guidance where someone who owns the deployment model can get it right. Getting that page right is a follow-up, noted in the PR description.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-630 This preview will update automatically when you push new commits. |
The previous commit scoped the loopback remediation down to a pointer at `reference/security/configuration.md#authorizelocal`, promising "the full picture, and the restart requirement" there. That page did not have either — a two-sentence option description with a local-proxy aside. So the deferral was a dangling promise, which is worse than either keeping the detail on the CLI page or not promising it. Fixed at the destination rather than by rewording the pointer again. The option now carries the three facts an operator needs to act on it, all verified in `security/auth.ts`: - It is read once at startup, so a change needs a restart. - A same-host reverse proxy makes remote traffic arrive from `127.0.0.1`, so a non-loopback target URL does not avoid the exposure. - It does not gate the operations API domain socket, which is why local `harper` commands work without credentials — and why disabling the option does not protect a proxy whose upstream is the socket. Cross-linked back to the CI section, since the pipeline case is what makes this concrete. This breaks the "no further content edits" line I set one commit ago. The reason: the finding was that my own deferral pointed at content that did not exist, and shipping a broken security cross-reference to file a follow-up later is the wrong trade. It is also a different file — the CLI page's wording is unchanged, so this is not another iteration of the loop I was trying to leave.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-630 This preview will update automatically when you push new commits. |
kriszyp
left a comment
There was a problem hiding this comment.
Deploys are getting so much better! Thank you!
🤖 Reviewed with Codex
|
|
||
| The namespace is chosen by which one is **set**, not by which one has a usable value — so `HARPER_CLI_REFRESH_TOKEN=` (present but empty) claims the choice and shadows a perfectly good `CLI_TARGET_REFRESH_TOKEN`, which is never consulted. The run then falls through to the saved login token. Unset the preferred variable rather than blanking it. | ||
|
|
||
| For a pipeline, a token is the right style: it is scoped to authentication, it can be revoked without changing the account password, and it cannot be used to log in interactively. Use it _instead of_ a password or payload credentials, not alongside them — see [Token credentials for CI/CD](#token-credentials-for-cicd). |
There was a problem hiding this comment.
reference/cli/overview.md:160-168 still tells CI users to use HARPER_CLI_USERNAME/HARPER_CLI_PASSWORD, omits both token variables, and says the displayed sources are resolved "in the order shown." That leaves the overview inconsistent with this recommendation and the canonical precedence list. Please add token credentials (ideally including the --for-ci path) there, or reword the overview as an intentionally abbreviated list without the ordering claim.
There was a problem hiding this comment.
Fixed in 1a8dde1 — added token credentials with the --for-ci pointer, moved the CI/CD recommendation onto them, and replaced the ordering claim with an explicit note that the list is abbreviated and not in precedence order (pointing at the canonical list for the real order).
Thank you for catching this — it is a symmetry miss on my part. I updated authentication.md and commands.md and never grepped for other pages making precedence claims, which is exactly the check I should have run when the same pattern spans several surfaces. Worth noting the overview also still recommended username/password for CI, which now contradicts the one-style guidance this PR adds, so it was two inconsistencies rather than one.
🤖 Addressed by Claude Code
|
|
||
| - **Refresh token only** (what `--for-ci` provisions): no bearer token is attached. Against a remote target the command fails as unauthenticated — unless it also carries `username=` and `password=` operation parameters, in which case it authenticates as that pair instead, a different identity than the one you configured. | ||
| - **Refresh token only, against a loopback target**: it does not fail at all. `authentication.authorizeLocal` defaults to `true`, so a request from `127.0.0.1` or `::1` is authenticated as **superuser** with no credential checked. On a self-hosted runner pointed at its own node, an expired token therefore produces a green, fully privileged run rather than an error. This is the failure mode least likely to be noticed. | ||
| - **An expired operation token as well**: that expired token is still attached, so the request goes out carrying it and the server rejects it with `403` — token expiry answers `403` on the operations path too, since both token types are checked by the same validator. You get a rejection rather than a silent identity switch, but do not alarm on `401` for it. |
There was a problem hiding this comment.
The preceding loopback guidance says authorizeLocal authenticates any loopback request as superuser without checking credentials, but this unqualified bullet says attaching an expired operation token makes the server reject the request with 403. Both cannot be true for a loopback target. Please either scope this bullet to non-loopback targets or revise the loopback guidance to explain when an Authorization header bypasses local authorization; otherwise self-hosted runners can get the opposite outcome from the documented one.
There was a problem hiding this comment.
You were right that both could not be true, and tracing it produced the fact the whole section was missing. Fixed in 1a8dde1.
authentication() in security/auth.ts is an else if chain: the local-authorization branch is only reached when a request arrives with no Authorization header. A request carrying a token — even an expired one — is validated in the first branch and rejected on its merits, loopback or not. So both statements were true, for different cases, and the page never supplied the hinge between them.
Stated explicitly now, and it makes the section coherent rather than just non-contradictory: the dangerous shape is precisely refresh-token-only, because a failed refresh leaves nothing to validate and the request arrives bare; an expired operation token is comparatively safe because it still produces a header.
That also forced a third change you did not ask for. The one-style tip said "against a loopback target Harper authenticates by address, so no choice of style protects you there" — which overstates once the real distinction is whether a credential is attached rather than which style it is. Left alone it would have reintroduced your contradiction one paragraph earlier.
Fourteen rounds of automated review circled this distinction without landing it, so: genuinely useful catch.
🤖 Addressed by Claude Code
…ached Both findings from @kriszyp's review, and the second supplies a hinge the page was missing rather than just a correction. **The loopback guidance and the expired-operation-token bullet did contradict each other**, exactly as flagged. The resolution is in the control flow: `security/auth.ts` is an `else if` chain, so the local-authorization branch is reached only when a request arrives with no `Authorization` header. A request carrying a token — even an expired one — is validated in the first branch and rejected on its merits, loopback or not. That makes the whole section coherent for the first time: the dangerous shape is precisely refresh-token-only, because a failed refresh leaves no header to validate; an expired operation token is comparatively safe because it still produces a header. Stated explicitly, and the one-style tip's caveat is reconciled with it — it previously said no credential style protects you on loopback, which overstates now that the distinction is "is a credential attached" rather than "which style". **`reference/cli/overview.md` was left inconsistent.** It still pointed CI users at `HARPER_CLI_USERNAME`/`HARPER_CLI_PASSWORD`, omitted both token variables, and claimed its abbreviated list resolved "in the order shown". Token credentials added with the `--for-ci` pointer, CI/CD recommendation moved to them, and the ordering claim replaced with an explicit note that the list is abbreviated and not in precedence order. This is a symmetry miss on my part — I updated authentication.md and commands.md and never grepped for other pages making precedence claims.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-630 This preview will update automatically when you push new commits. |
🧹 Preview CleanupThe preview deployment for this PR has been removed. |
#630 merged, so this branch retargeted to `main` and conflicted on the three files it shares. The conflict was lopsided rather than symmetric: this branch carried the token content as it stood when #630 was first merged in, while `main` now has that same content after fourteen review rounds — the one-style rule, the 403-on-expiry warning, the loopback/`authorizeLocal` material, the corrected saved-target fallback, `CLI_TARGET_OPERATION_TOKEN`, and the trimmed rationale. So resolving hunk-by-hunk would have been reconciling an obsolete copy against a corrected one. Instead: took `main`'s version of all three files wholesale, verified each was byte-identical to `main`, then re-applied this branch's only real contribution to them — the OIDC additions — on top. `authentication.md` is now +53 against `main` rather than a full-file conflict. Two things the re-application had to reconcile, because `main` gained content that interacts with OIDC: - The rejected-exchange paragraph said the operation "returns 401" when nothing else is configured. That is now only true off-loopback: per the hinge @kriszyp established, a credential-less request to a loopback node is authorized as superuser, so a policy mismatch there succeeds with no identity check. Stated, and cross-linked to the refresh-behavior note rather than repeating it. - Workload identity added to the one-style table's CI/CD row, since it is now a second valid choice for that context. Also caught myself re-introducing an inline `<VersionBadge>` in the precedence list — the same CONTRIBUTING violation I fixed on #630 an hour ago. Plain text.
Documents the CI/CD token credentials that shipped in v5.2.0 via harper#1876 and never reached the reference docs —
HARPER_CLI_REFRESH_TOKEN,HARPER_CLI_OPERATION_TOKEN, andharper login --for-ci. Split out of #599, which is gated on unmerged upstream work; this has no such gate.The gap mattered because these are the credentials the CLI is meant to use in a pipeline, and the Authentication Precedence list — the one place that states which credential wins — stopped at the saved
harper logintoken.Reviewing this found more than a missing variable list. Two shipped behaviors do not match what an operator would reasonably assume, and one is filed as a core bug: harper#2297.
For the human reviewer
1. Should this page still recommend tokens as the CI credential style? It does. It also now documents that a refresh token's expiry — guaranteed at 30 days — answers
403, which the CLI's401-only halt does not catch, so the token stops authenticating without stopping the run. We are recommending a pattern with a scheduled fail-open. The alternatives are steering CI to Method 3 (auth_username=/auth_password=) until harper#2297 lands, or keeping the recommendation and relying on the warning. Cheap to change now, expensive once pipelines are provisioned against it. This is the entry I most want ruled on.2. Is a
:::warninginreference/the right home for a core defect?reference/publishes to/reference/v5, so this box is live until someone removes it. I've named harper#2297 in it and said it should come out when fixed, which is the best I can do from the docs side — but it does mean the live site carries a defect description with a removal dependency on another repo.3. Does a CLI page belong telling operators to set a server config flag, and restart? The loopback finding has no monitoring answer — a superuser-authorized run returns success, so nothing is observable — and the only sufficient remediation is
authentication.authorizeLocal: falseplus a restart. That is server configuration and an availability action appearing in CLI docs. I judged the alternative, documenting a hazard with no fix, worse. It also means this page andreference/security/configuration.mdnow both carry loopback guidance; consolidating them is a reasonable ask.4.
options.mdfix bundled into a CLI-auth branch.refreshTokenTimeoutwas documented with a1ddefault against that file's own30dexample three lines above and30dinconfig/configUtils.ts. Real bug, and CI rotation planning depends on the number — but it widens the diff past this branch's subject. Reversible by split.5. Naming internal symbols. I removed
validateRefreshToken/TokenExpiredErrorfrom the prose once harper#2297 existed to carry provenance, keeping only observable status codes. Flagging in case you'd rather the docs never cite core internals even transiently.Open, not fixed — the one-refresh-token-per-user invariant is a property of
refresh_operation_token, but appears only on this CLI page.reference/security/jwt-authentication.mdwalks a caller through the refresh flow without it, so a team driving the Operations API directly never learns that minting revokes the previous token. Different page than this PR touches; wants its own change.Verification
Docs-only, and this repo has no test suite — per
AGENTS.mdthe build is the end-to-end check. At59691d4a:npm run format:checkclean;npm run buildsucceeds. The build reports two broken anchors, both pre-existing onmain(confirmed by buildingmainwith these changes stashed) and neither in a file this PR touches.Every behavioral claim was checked against harper v5.2.4 source, not against the PR that introduced the feature:
bin/cliOperations.ts401halts,403(expiry) does not; env-sourced refresh stays in memoryrefreshExpiredOperationToken403for operation and refresh tokens (shared validator)security/tokenAuthentication.ts—validateTokenauthorizeLocaldefaultstrue; socket clause is independentsecurity/auth.ts,config-root.schema.jsonresolveTargetauthorizeLocalis read once at startup, so changing it needs a restartsecurity/auth.tsmodule scope--for-cistdout contract;stdinTTY guard with none onstdoutbin/login.ts1d/30dlifetime defaultsconfig/configUtils.tssecurity/tokenAuthentication.ts,utility/hdbTerms.tsReview coverage
Authored by Claude Opus 5. Outside lenses: codex
gpt-5.6-sol, gemini viaagy(default model, version not pinned), cursorcomposer-2.5[fast=false](one round, pruned as a narrow delta thereafter), Harper domain adjudication.cursor-groknever ran —cursor-agentis not installed on this machine.Fourteen rounds, and the count is not a merit badge — it needs explaining.
Four early rounds ran with no adjudication because the
claudeCLI's OAuth token had expired. That surfaced asexit-1against a zero-byte log, because the CLI classifies leg failures from stderr while Claude reports API errors on stdout; I read the empty log as "unfixable infrastructure" for four rounds instead of checking the other stream. The first working adjudication immediately found amajorall four earlier rounds had missed.The rest of the count is the subject matter. Precedence, local trust, and token expiry interact, so several rounds found that fixing one claim exposed the next — including one contradiction I authored while fixing a finding, where the callout still promised containment the new bullet disproved. The last two rounds were specifically probing whether the page hangs together, which is the question I'd most like a human to answer.
What the rounds actually produced, all confirmed in source before editing: the
403/401halt mismatch (now harper#2297), loopback superuser viaauthorizeLocal, an expired operation token also answering403, apbcopyexample that persists a live credential to clipboard history, a revocation instruction pointing at a command that does not exist, a lost target falling back to a saved remote rather than local, the self-defeating "check the result" remediation, and finally that avoiding a loopback target does not help when a same-host proxy makes every request arrive from127.0.0.1— which our ownreference/security/configuration.mdalready warned about, so the page I wrote contradicted our security docs until the last commit. Three findings across those rounds were false positives I refuted the same way — including two the reviewer raised by reading harpermainrather than the feature branches.The domain adjudicator also reversed one of its own rulings, having dismissed the loopback finding in an earlier round before verifying
authorizeLocal's default.Where coverage stops, and three findings I did not fix. Head
59691d4awas reviewed by codex and the Harper domain adjudicator. It returned three findings, recorded here rather than fixed — I had told the reviewers no further edits would follow, and I had already overridden that stopping rule three times earlier in this branch, so it needs to mean something. Please treat the first as a correctness item before merge:reference/security/configuration.md: my enumeration of trusted loopback addresses is narrower than the implementation. I wrote "127.0.0.1or::1".security/auth.tstestsrequest.ip?.includes('127.0.0.'), which matches any127.0.0.*, and its own comment notes IPv4-mapped::ffff:127.xforms are covered too. Someone hardening a node could bind a service to127.0.0.2believing it sits outside the trust boundary. Worth widening, and worth checking my other address enumerations at the same time.reference/cli/authentication.md: "create a dedicated CI user … that scopes the pipeline's permissions" overstates. Creating a user does not scope anything; the role assigned to it does. The sentence reads as though isolation is automatic.Also open, and the reason the loopback material moved: the remediation detail now lives in
reference/security/configuration.md, so that page and this one both discuss loopback trust. Whether that split is right, and whetherauthorizeLocal: falseshould be advised for all runner-reachable nodes, are calls for someone who owns the deployment model.Notes
v5.2.0badges are a real release. The rest of the deploy stack splits across v5.2.3, v5.2.4, and v5.3.0 — see docs(deploy): two-phase deploy, revert, by-reference deploys, and OIDC trusted publishing #599.Description drafted by Claude Code (Opus 5).
Review-Coverage: authored=claude; ran=codex; adjudicated=domain; declined=gemini,cursor-grok,cursor-composer; rounds=14 @ 59691d4
Human-Review-Need: 4 (decisions: publish-known-defect, token-style-prescription, heading-depth-h5, authorizelocal-split, hazard-repetition, v4-default-left-stale) @ 59691d4