feat(cli): accept the deployment's known origins as one deployment - #199
Conversation
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
vraspar
left a comment
There was a problem hiding this comment.
Review: comments-only — no majors; 5 minors, 2 nits; aliasing design holds under adversarial review
Reviewed against the diff and the money-path threat model; verified at 63c51c9 in a clean worktree. Three-panel review (security/adversarial, backend+testing, hygiene+CI+agent-usability). The core property survives attack enumeration: comparisons are exact string equality on canonical URL.origin everywhere (CLI and inline hook), the set is a build-time literal with no config/env influence, both-sides membership is symmetric between CLI and hook, self-hosted bases gain no aliasing, lookalike hosts are refused, SIWX binds only to origins within the set, redirects fail closed, and every downgrade window fails closed. 265 tests pass across the touched files locally; lint/typecheck/prettier clean.
What's solid:
- One shared helper decides the money question (
isSameDeploymentconsumed byassertOnBaseOriginand the pay lane); the hook's duplication is forced (self-contained .mjs), named back to the source of truth, and generation-time-inlined from the same constant — no second definition exists. - Refusal envelope is bit-identical before/after for anything outside the set: same USAGE code, message naming both origins, fix line that never coaches re-pointing.
- Both migration directions tested with real artifacts (flipped-server candidates accepted into storage, stored candidates re-resolved against either base, pay lane kept cross-origin).
Minor
- testing The hook's alias rule is pinned as a string literal, never executed: the only pin asserts
KNOWN_ORIGINSis inlined verbatim; every behavioral hook test runs against a localhost base outside the set, so only the exact-compare branch executes. A regression dropping the request-side half — one-sidedKNOWN_ORIGINS.includes(origin)— would let a non-deployment listener launder deployment-origin candidate URLs into payable pointers with the suite green (mitigated downstream byresolveResourceRefre-asserting before send). Fix: strengthen the containment assertion to require the full rule text, or better, onerunScriptcase exercising the sibling-origin branch.tenjin-agent/src/lib/hook-scripts.ts
Line 383 in 63c51c9
- security The trust-anchor set has no exact-membership pin and no revocation runbook:
KNOWN_DEPLOYMENT_ORIGINSis baked into every shipped CLI and install-time hook; from cutover until clients update, whoever controls a member origin receives wallet-signed credentials from CLIs configured on the sibling. Tests pin properties of the set but not its contents, so adding/failing-to-remove a member is never a forced deliberate diff. Fix:expect(knownDeploymentOrigins()).toEqual([PRODUCTION_ORIGIN, 'https://tenjin.sh'])plus a recorded operator runbook for removing a member relative to the #402 flip-back. Severity minor because exploitation requires losing control of a member origin — which is what membership asserts.tenjin-agent/src/lib/production-origin.ts
Lines 42 to 45 in 63c51c9
- ci CI has never run on this PR:
gh pr checksreports no checks on any commit ofA1igator/dual-origin-accept— the stacked-branch artifact the PR body predicts. Local runs are not CI. Fix: land the base (or trigger workflows against this branch) so a real green run exists at the merged SHA before merge. - ci HOOK_SCRIPT_VERSION bump has no pin: nothing asserts the value, so a future body change with a forgotten bump stays green and the header silently misreports the writer version (regeneration itself is safe — it's byte-drift based). Fix:
expect(HOOK_SCRIPT_VERSION).toBe(19)or pin the emitted(v19)header.tenjin-agent/src/lib/hook-scripts.ts
Line 44 in 63c51c9
- agent-usability command-reference.md now lies about the pay lane split: "Any other https origin is the Bazaar lane" — after this PR a deployment-member origin routes to the tenjin lane without
bazaarPay. An agent reading the reference gets the wrong lane model at cutover. Fix: amend to "any other origin outside the deployment's known origins…".tenjin-agent/docs/command-reference.md
Lines 160 to 161 in 63c51c9
Nits (2), none blocking
- search.test.ts / pay.test.ts —
const [base, sibling] = knownDeploymentOrigins()leans on Set insertion order (index 0 differing from the ctx base); an explicit.filter(o => o !== …)like sibling tests use is robust to reordering. Resolved for free by finding 2's exact-membership pin. - src/lib/production-origin.ts:23-41 — 19-line comment block over a 4-line const, well past the ≤12–15% guideline; consistent with this repo's house style, so informational unless the standard binds here.
Verified, not issues
No arbitrary-origin spoofing (exact URL.origin equality everywhere; userinfo tricks yield attacker origin and fail; port/scheme/http-downgrade/lookalike-suffix/dash-prefix all refused and tested); set is build-time-safe, hook-inlined via JSON.stringify with version bumped so installers rewrite; consumer enumeration complete (aliasing applies only at assertOnBaseOrigin ingest paths, resolveLane, hook sameOrigin — fund untouched, bazaar identity unchanged); pay-lane mid-flow swap blocked (redirects pinned on probe and paid leg, SIWX binds target origin in pay / configured base in buy, payTo from decoded challenge unmodified); old-client/new-server CONTRACT_MISMATCH, new-client/old-server exact match, stale v18 hook drops candidates — all fail closed; session-key matching and http.ts redirect block deliberately strict and untouched; refusal envelope regression-tested including fix-line shape.
Verdict: comments-only. No majors; findings 1–2 are the ones worth landing before merge since they guard the trust boundary this PR widens. What ran locally: touched vitest files (265 passed), pnpm typecheck, pnpm lint, prettier --check, gh pr checks.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
All five minors and both nits are in Finding 1, the alias rule is now executed. Two Finding 2, the trust anchor. Finding 4, pinned by digest rather than by value. Finding 5 is corrected in Nits: Finding 3 stands as you wrote it. No checks have run on this branch, and none will while the base is unmerged, so a real green run at the merged SHA needs #150 to land first. Gates: format, lint, typecheck, build clean; 2349 tests pass; pack-smoke PASS; |
vraspar
left a comment
There was a problem hiding this comment.
Re-review (R2) at 8960fff: five of seven findings resolved with genuine execution tests — one blocker-class item remains: CI still has never run
Delta reviewed: 63c51c9..8960fff. Local tests at this head: 273/273 across the five touched suites, typecheck clean. Merge-forward of #150 resolved cleanly (no duplicated tests; two-file-anchor literals coexist correctly with constant-derived pins).
Resolved since round 1
- Hook alias rule now executed: two real
runScriptcases — sibling candidate kept when base is a member (hook-scripts.test.ts:1797), deployment candidate dropped when base is a non-member stub using shipped bytes (:1776); the one rewritten line is asserted present in shipped bytes first. - Trust-anchor pin + runbook: exact-membership
toEqual([PRODUCTION_ORIGIN, 'https://tenjin.sh'])with independently written expected values, revocation runbook atdocs/safety-model.md. HOOK_SCRIPT_VERSIONpinned to 19 with sha256 digests of all four generated scripts.- Pay-lane doc amended to the membership-conditioned rule.
- Both nits: insertion-order reliance replaced with named-origin
.find(); comments trimmed to invariants.
Still open
- CI has never run on any head of this branch —
gh pr checksreports no checks at8960fff, including after the merge-forward. Everything above was verified locally; a green CI run at the merged SHA should exist before merge. This is the one item I'd hold the merge on.
New (nits, non-blocking): the runbook's set-removal step should also name the digest test at hook-scripts.test.ts:203, or the fixer ships stale digests; odd-but-harmless http://127.0.0.1:9 fixture port.
Verdict: approve-once-CI-runs-green-at-head. The code itself is done from my side — every round-1 finding landed as specified.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
vraspar
left a comment
There was a problem hiding this comment.
Re-review delta at 45f98d7: runbook nit fixed; one trivial nit remains and CI still cannot run on this branch
Delta reviewed: 8960fff..45f98d7 — docs-only, adds the digest-pin step to the member-revocation runbook as suggested. The remaining nit from R2 (the http://127.0.0.1:9 fixture port in hook-scripts.test.ts:1799) is untouched — harmless, cosmetic, non-blocking.
On the CI hold from R2: verified the cause is structural, not neglect — ci.yml triggers only on pull_request targeting main, so a stacked PR cannot produce a check run until its base lands. The practical sequence: merge #150, retarget this PR to main, confirm green at the merged SHA. Everything else stands resolved from R2; local verification at 8960fff was 273/273 across the five touched suites plus typecheck clean.
Verdict: unchanged — approve once a green CI run exists at head. Say the word if you'd rather I approve on the strength of the local runs alone.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cip8Fc8VvouqatPNkXxcr3
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cip8Fc8VvouqatPNkXxcr3
Review follow-ups on the deployment-alias PR. The hook's alias rule was pinned as a string and never run: every behavioural hook test uses a localhost base outside KNOWN_ORIGINS, so only the exact-compare branch executed. Two runScript cases now drive the real generated bytes through it. The one that matters is the two-sided half: a localhost listener that hands back a candidate on the deployment origin must have it dropped, because a one-sided KNOWN_ORIGINS.includes(candidateOrigin) would let anything the operator ever configured launder a payable pointer at the marketplace into the store. Verified by reverting the rule to one-sided, which reds it. KNOWN_DEPLOYMENT_ORIGINS gets an exact-membership pin, written out independently of the module, so adding a member or failing to remove one is a deliberate line in a reviewed diff. The set ships baked into every released CLI and every install-time hook, so a member origin receives wallet-signed credentials from CLIs configured on the sibling until each client updates. docs/safety-model.md carries the removal runbook. HOOK_SCRIPT_VERSION gets a digest pin over all four generated scripts. A body change with a forgotten bump was doubly quiet: installs keep the old script, and the new header names a version whose bytes it is not. docs/command-reference.md no longer says any other https origin is the Bazaar lane; a deployment-member origin takes the tenjin lane without bazaarPay. search.test.ts names its origins instead of indexing the set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cip8Fc8VvouqatPNkXxcr3
18 comment lines over 4 lines of code, with the aliasing rule stated twice. The invariants stay (what membership means, why the set exists, both sides must be members, widening it is a security change); the restatement goes, and the removal runbook is named rather than retold. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cip8Fc8VvouqatPNkXxcr3
Following the runbook without updating the generated-script digest pin in hook-scripts.test.ts leaves a failing suite after the HOOK_SCRIPT_VERSION bump.
45f98d7 to
eb50018
Compare
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Owed you a reply on two rounds. Both are answered here, and the CI hold is resolved. R2's runbook nit is done (
The R3's CI hold is resolved, and your diagnosis was exactly right. #150 squash-merged as Rebuilt at CI is running on this PR for the first time. No need to approve on local runs alone. |
|
CI finding is now closed by evidence: run 32541978214 is green at Nothing else moved. The only other event since my last comment is a greptile-apps[bot] review whose body reads "Your trial has ended", with zero inline comments. |
vraspar
left a comment
There was a problem hiding this comment.
Approve at eb50018. Branch rebased onto post-#150/#181 main — diff verified scoped to exactly this feature's 12 files (the notify-registry-pin content visible in earlier range diffs came from main, not this PR). Both R2 holds are cleared: CI now runs and passes at exactly this head (retarget to main fixed the structural trigger gap), and the runbook digest-pin step landed. All round-1 minors and both round-2 nits resolved. One cosmetic item remains — the never-dereferenced http://127.0.0.1:9 fixture constant at hook-scripts.test.ts:1799 — waived here as below the bar, recorded so it isn't lost.
Scope
Teaches the CLI that the deployment's known origins are one deployment, so the
tenjin.shflip (BackTrackCo/tenjin#402) does not break an installed client. Closes the CLI half of BackTrackCo/tenjin#738.src/lib/production-origin.tsgains the deployment origin set andisSameDeployment. Membership means one server, one set of publishers, one SIWX domain acceptance. It is not trust: aliasing applies only when the configured base is itself a member, so a self-hosted, preview, or localhostbaseUrlkeeps the exact comparison it has today.src/lib/resource-ref.tsroutesassertOnBaseOriginthrough it. This is the money-path pin, so the refusal branch is unchanged for everything outside the set: sameUSAGEcode, same message naming both origins, same fix line that reads the configured value and never coaches re-pointing the CLI at the URL that just failed. Risk is the widened surface itself, bounded by a two-member set both sides must belong to.src/commands/pay.tsand the generated hook insrc/lib/hook-scripts.tsbreak identically at the flip and get the same rule.paywould drop a Tenjin URL into the Bazaar lane, which refuses it as a third-party endpoint; the hook would silently drop every candidate.HOOK_SCRIPT_VERSIONmoves to 19 because the generated body changed.PRODUCTION_ORIGINdoes not move; the shipped default flips in a later release. Stored config is not rewritten.Stacked
Stacked on #150, base
A1igator/centralize-production-origin. Required checks may not run while the base is an unmerged branch.Deliberately left
src/lib/session-present.ts,src/commands/doctor.ts). A session is a delegation whose SIWXdomainis one host, and the server compares host-with-port, so presenting atenjin.blogsession attenjin.shwould be rejected server-side. Aliasing it would send a credential to be refused instead of refusing locally. Areadon the other origin falls back to the 402 path;buysigns fresh against the target and works.src/lib/http.ts. It compares nothing, so the alias set does not reach it, but if the deployment ever 301stenjin.blogtotenjin.shinstead of dual-serving, every signed request failsblocked-redirectand no client-side change helps. Dual-serve has to hold.Testing
pnpm format:check,pnpm lint,pnpm typecheck,pnpm build: clean.pnpm test: 2341 passed, 10 skipped, across 74 files (1 skipped). Existing refusal tests pass unchanged.CI=true bash scripts/pack-smoke.sh: PASS.New tests pin the property in both directions: a candidate on the other known origin is accepted when the base is one of the deployment's origins (
search,resolveResourceRef,paylane), an origin outside the set is refused with the same error from either base, a self-hosted base gets no aliasing, scheme and port still separate origins, and a lookalike host (tenjin.blog.evil.example) does not ride in.