Repository navigation
feat(evault): enforce company signing authority on writes with single-use grants and revoke cascade - #1193
feat(evault): enforce company signing authority on writes with single-use grants and revoke cascade#1193coodos wants to merge 1 commit into
Conversation
…-use grants and revoke cascade
|
@codex review |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2fa180571
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const ticket = await this.delegationGuard.beforeWrite(eName, write); | ||
| let result: R; | ||
| try { | ||
| result = await run(); |
There was a problem hiding this comment.
Because beforeWrite and run execute in separate database transactions, the authority state can change between them. For example, a role creation can validate while its signer is a director, a concurrent board update can remove that director, and then the role creation can still land; similarly, a re-delegation can land after its parent is concurrently revoked. The relevant authority records need to be locked or revalidated in the same transaction that performs the write.
Useful? React with 👍 / 👎.
| `MATCH (g:DelegationGrant { claimToken: $token }) | ||
| WHERE g.metaEnvelopeId IS NULL | ||
| DELETE g`, |
There was a problem hiding this comment.
For updates, claimGrant initializes metaEnvelopeId to previous.id, but this cleanup only deletes claims whose metaEnvelopeId is null. Consequently, if the subsequent database update fails, abortWrite leaves the claim behind and retrying the same signed update returns REPLAYED_GRANT even though the write never landed. Update claims should remain distinguishable as uncommitted until afterWrite binds them.
Useful? React with 👍 / 👎.
| const previous = write.id ? await this.load(write.id, eName) : null; | ||
| const next = { ontology: write.ontology, parsed: write.payload ?? {} }; |
There was a problem hiding this comment.
updateMetaEnvelopeById has PATCH semantics and retains fields omitted from the submitted payload, but the guard treats only write.payload as the complete next record and verifies the authorization against it. A signed partial Role, Delegation, or Shareholding update can therefore be accepted and stored alongside retained fields that are absent from the signed payload, leaving the stored authority record with an authorization that does not match its actual contents. Merge the existing record with the patch before signature and policy validation.
Useful? React with 👍 / 👎.
| const result = await verifySignature({ | ||
| eName, | ||
| signature, | ||
| payload, | ||
| registryBaseUrl, | ||
| }); | ||
| return result.valid; |
There was a problem hiding this comment.
This verifier delegates to signature-validator, whose verifySignature returns { valid: true } when /whois supplies an empty key-binding-certificate list (infrastructure/signature-validator/src/index.ts:338-345). Thus a resolvable signer with no bound keys can authorize a company record using any nonempty signature, defeating the new write guard; this wrapper must fail closed when no key was actually used to verify the signature.
Useful? React with 👍 / 👎.
| if (!parent.mayRedelegate) { | ||
| throw new DelegationWriteError( | ||
| "NOT_AUTHORIZED", | ||
| "the parent does not allow re-delegation", | ||
| ); | ||
| } | ||
| this.requireNarrowing(next, parent, parent.mayRedelegate, "parent"); |
There was a problem hiding this comment.
Authority payloads are arbitrary JSON and are not validated against the ontology schema here, so a parent stored with mayRedelegate: "false" passes this truthiness check and is treated as allowing re-delegation. A delegate can then create child delegations despite the supplied value representing false; validate mayRedelegate as an actual boolean on every Role and Delegation write before using it in authorization decisions.
Useful? React with 👍 / 👎.
| await this.delegationGuard.assertNotGoverned( | ||
| context.eName, | ||
| id, | ||
| "Rolling back", | ||
| ); |
There was a problem hiding this comment.
When this assertion rejects a governed record, the surrounding rollbackMetaEnvelope catch converts the DelegationWriteError to ROLLBACK_FAILED; removeMetaEnvelope similarly converts it to DELETE_FAILED. Clients of these structured mutations therefore cannot distinguish the intentional immutability policy from an operational database failure, unlike the create/update paths that preserve guard codes. Handle DelegationWriteError explicitly in both catches and return its IMMUTABLE code.
Useful? React with 👍 / 👎.
| private async load(id: string, eName: string): Promise<Existing | null> { | ||
| const found = await this.db.findMetaEnvelopeById(id, eName); | ||
| return found | ||
| ? { id: found.id, ontology: found.ontology, parsed: found.parsed } | ||
| : null; |
There was a problem hiding this comment.
assertNotGoverned relies on this loader, but findMetaEnvelopeById only matches live MetaEnvelope nodes. A previously deleted governed Role or Delegation is labeled PrunedMetaEnvelope, so the assertion sees no record and rollbackMetaEnvelope can restore an old active authority version without signature, authorization, or replay checks. Load the pruned record or inspect the rollback target/history before allowing restoration.
Useful? React with 👍 / 👎.
| await createDelegationGrantConstraint(driver); | ||
| } catch (error) { | ||
| console.warn("Failed to create delegation grant constraint:", error); |
There was a problem hiding this comment.
The single-use claim uses MERGE safely under concurrency only when the new composite uniqueness constraint exists, as the migration itself documents. If constraint creation fails because of permissions, incompatible Neo4j configuration, or existing duplicate data, this catch merely warns and starts the service anyway; concurrent uses of one signed payload can then create separate DelegationGrant nodes and both be accepted. Treat failure to establish this security constraint as fatal rather than running without replay protection.
Useful? React with 👍 / 👎.
|
Superseded: enforcement moves to verifiers; the eVault stays a plain store. |
1 participant
Description of change
PR 3 of 5 for company signing delegation: evault-core enforces who may write company authority records, so a delegation found in a company's eVault can be trusted.
Governed records are those in the company's own vault: a Company with
directors, and Role / Delegation / Shareholding whosecompanyENameis the vault. Copies elsewhere carry no authority and pass untouched.w3ds-grant/v1authorization verified against the signer's Registry-bound keys (fails closed without a Registry).mayRedelegateonly if the role allows.evaluateDelegation) and allow re-delegation; narrowing only.DelegationGrant, unique constraint). Copying it onto another record, or writing back an older signed state, isREPLAYED_GRANT. Identical re-sends pass. A failed write frees the claim.revocationReason: cascade), recursively.IMMUTABLE); revoke instead.guardedWritehelper (create, update ×2, bulk create, legacy store) plusassertNotGovernedon remove/delete/rollback/updateEnvelopeValue. Guard error codes surface in the mutationerrors.DelegatedSignature (the audit record) is not guarded here; platforms write it after verifying (PR 4).
Issue Number
Type of change
How the change has been tested
delegation-write-guard.spec.ts(16, Neo4j testcontainer, fake verifier): first board by creator; unsigned / tampered / outsider boards refused; one board per vault and only directors change it; NO_COMPANY; non-director role; core scopes; wider-than-role delegation; re-delegation by delegate vs outsider; copied grant and older-state replay refused; identical re-send; aborted write frees grant; revoke by grantor/director vs others; revocation final; role revoke cascades through a re-delegation; narrowing cascades only what it no longer covers; delete refused; copies for other companies ignored. Plus a GraphQL test that an unsigned Role returnsUNSIGNED. Existing suites (db, acl, manifest, protocol: 200 tests) pass;tscclean.Change checklist