Fix #276: Deploy wallet re-attachment policy - #353
Conversation
|
@Anambraboi-1 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
@Anambraboi-1 is attempting to deploy a commit to the blockchainmaxis-8449's projects Team on Vercel. A member of the Team first needs to authorize it. |
✅ Deploy Preview for stellar-signet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
d60bdd7 to
55845b5
Compare
blockchain-maxis
left a comment
There was a problem hiding this comment.
The policy write-up in SECURITY.md is exactly what #276 asks for — all three cases named, the 409 message deliberately generic so it doesn't leak which profile holds the wallet, and the no-proof case refused with 401 before the attachment check so the endpoint can't be used as an oracle. That part I'd take as-is.
I can't merge it yet, for two reasons.
1. lint • typecheck • test • build fails, and it's this branch, not main. @signet/web#typecheck:
app/(dashboard)/app/cli/approve/page.tsx(4,24): error TS2307: Cannot find module '@/components/button' or its corresponding type declarations.
app/(dashboard)/app/cli/approve/page.tsx(40,31): error TS2339: Property 'id' does not exist on type 'Account'.
lib/cli-auth.ts(22,29): error TS2345: Argument of type 'string | undefined' is not assignable to parameter of type 'string'.
lib/cli-auth.ts(32,14): error TS2322: Type 'string | undefined' is not assignable to type 'string'.
lib/cli-auth.ts(32,22): error TS2322: Type 'string | undefined' is not assignable to type 'string'.
@/components/button doesn't exist in this repo, Account has no id field (check the Prisma model for the field you actually want), and the three cli-auth.ts errors are unchecked process.env / optional-header reads that need a guard rather than a cast. e2e smoke tests fails downstream of the same build. Please run pnpm typecheck locally before pushing again.
2. apps/cli is a contested location. This PR creates a TypeScript CLI at apps/cli (and the lockfile entry that goes with it). There is a Go CLI series landing at cli/ — #251 scaffolds it, #296 wires it into build/format/lint, and #369/#370/#371 are open against it. Only one CLI survives, and #251 settles which. I flagged the same thing on #322. Nothing in your diff is wasted — the re-attachment policy and its enforcement are the substance of #276 and they're portable — but before you spend more on apps/cli, it's worth landing the web-side enforcement (lib/cli-auth.ts, the API route, SECURITY.md) on its own and leaving the CLI half to the cli/ series.
Related: #354 and #355 are stacked on this branch and carry the same two problems, so fixing them here fixes them there.
blockchain-maxis#276 asks for a *documented* policy covering three cases, enforced, logged and tested. The enforcement landed with the pairing flow — completePairing attaches an unbound wallet, no-ops when it is already this profile's, and refuses `wallet-bound-elsewhere` when another profile holds it, with tests at pairing.test.ts:210/262/274 — but nothing in docs/ ever wrote the policy down, which is the half of the acceptance that was actually missing. docs/WALLET_ATTACHMENT.md records the three outcomes and, more importantly, why the contested case is a refusal rather than a transfer: proving control of a key is not proof of which profile should hold it, and moving the row takes a profile's deployment history with it. It also documents the release path (the holder unlinks from Wallets; primary wallets are a registry operation), why neither the refusal text nor unlinkWallet's "Wallet not found" identifies the holder, and the concurrency and logging behaviour. Linked from SECURITY.md's hardening notes, since the reason the policy is shaped this way is the hijack surface. This branch's code changes are dropped — a second TypeScript CLI under apps/cli and a duplicate /api/cli/link route alongside the pairing flow already on main.
804a224 to
b2a7698
Compare
|
Thanks @Anambraboi-1 — reworked rather than closed, and #276 does land with this. The enforcement half of #276 was already on main and I want to give it its due before explaining the change: What was actually missing was the other half of the acceptance: "a documented policy". Nothing in Your code changes are dropped: they added a second TypeScript CLI under |
8992363
into
blockchain-maxis:main
Fixes #276. This PR codifies the policy for deploy wallet re-attachment. If a wallet is re-attached to the same profile, it is treated as a no-op. If it is attached to a different profile, the request is refused with a generic 409 error. This logic is covered by e2e tests and documented in SECURITY.md.