Implement kd#self

[corrideat]

Partially closes #41 (permissionsContext not included, planned for later)

---

[taoeffect]

/review

[github-actions]

Advanced AI Review

• Type: Combined
• Model: glm-5

Click to expand review

🔴 Security: Key name comparison allows unauthorized key deletion

Location: src/utils.ts:336-338

Problem: The validation for kd#self permission compares key names instead of key IDs to determine if a key can be deleted. Key names are not guaranteed to be unique - multiple different keys can have the same name.

if (state._vm.authorizedKeys[k._addedByKeyId].name !== localSigningKey.name) {
  throw new Error('Key was added by a different key')
}

This means if User A has a key named "member" with kd#self permission, and User B also has a key named "member" (but a different key ID) with kd#self permission, User B could delete keys that User A created, even though they are completely different keys.

Impact: An attacker with kd#self permission could delete other users' invites/keys if they happen to have a key with the same name. This allows for escalation of privilege: if a malicious actor creates a key with the same name as a victim's key, and the malicious actor has kd#self permission, they can delete keys created by the victim (provided they know the key IDs), effectively bypassing the "self" restriction.

Recommended fix: Compare the unique Key IDs instead of the names. Since _addedByKeyId stores the ID of the creator, you should compare it directly to the ID of the current signer:

if (selfDeleteOnly) {
  if (!k._addedByKeyId || !state._vm.authorizedKeys[k._addedByKeyId]) {
    throw new Error('Missing or invalid _addedByKeyId')
  }
  // Compare IDs, not names
  if (k._addedByKeyId !== localSigningKey.id) {
    throw new Error('Key was added by a different key')
  }
}

If key rotation support is needed (where a rotated key should be able to delete keys created by its predecessor), track key lineage properly rather than relying on name matching.

• Addressed
• Dismissed

---

🔴 Type Safety: _addedByKeyId is required but should be optional for backwards compatibility

Location: src/types.ts:244

Problem: The _addedByKeyId field is defined as a required string:

export type ChelContractKey = {
  // ...
  _addedByKeyId: string;
  // ...
}

However, existing contracts created before this change will not have this field. When processing historical events or syncing with existing contracts, this will cause issues because:

1. The TypeScript type suggests the field is always present
2. Runtime code at src/utils.ts:363 checks for missing _addedByKeyId

Recommended fix: Make the field optional:

export type ChelContractKey = {
  // ...
  _addedByKeyId?: string;
  // ...
}

Then update the validation at src/utils.ts:363 to handle the optional case appropriately:

if (selfDeleteOnly) {
  // For contracts/keys without _addedByKeyId, deny deletion with kd#self
  if (!k._addedByKeyId) {
    throw new Error('Cannot delete key: _addedByKeyId not available (legacy key)')
  }
  // ...rest of validation
}

• Addressed
• Dismissed

---

🟡 Medium: Overly Restrictive Permission Check in validateKeyAddPermissions

Location: src/utils.ts:278-283

Problem: The logic for adding keys normalizes kd#self to kd (OP_KEY_DEL) before checking if the signing key has permission. This implies that to grant kd#self, the signing key must have full kd permissions.

This prevents a key with kd#self (which allows deleting own keys) from creating a new key that also has kd#self. This restricts the ability to delegate or chain "self-managed" keys. If kd#self is designed to be a safe, limited permission, a key holding it should likely be able to grant it to keys it creates.

k.permissions.some((cv) => {
  if (cv === `${SPMessage.OP_KEY_DEL}#self`) {
    // Granting `kd#self` requires `kd`
    cv = SPMessage.OP_KEY_DEL
  }
  return !signingKeyPermissions.has(cv)
})

Recommended fix: Check for the specific permission directly. If the signing key has kd#self, it should satisfy the requirement for granting kd#self:

k.permissions.some((cv) => {
  if (cv === `${SPMessage.OP_KEY_DEL}#self`) {
    // To grant `kd#self`, signer needs either `kd` or `kd#self`
    return !signingKeyPermissions.has(SPMessage.OP_KEY_DEL) && 
           !signingKeyPermissions.has(`${SPMessage.OP_KEY_DEL}#self`)
  }
  return !signingKeyPermissions.has(cv)
})

• Addressed
• Dismissed

---

🟡 Medium: keysToMap overrides _addedByKeyId for existing keys

Location: src/internals.ts:147

Problem: In keysToMap, the code sets key._addedByKeyId = signingKeyId unconditionally. However, just two lines later, there is logic to handle authorizedKeys?.[key.id] (re-adding existing keys).

If a key is being re-added (e.g., rotating an existing key or updating it), _addedByKeyId will be overwritten by the current signer. This might break the audit trail or ownership logic if the "adder" is supposed to be the original creator. If a key is merely being updated/rotated by a different admin key, _addedByKeyId should likely remain the original creator's ID.

key._addedByKeyId = signingKeyId
if (authorizedKeys?.[key.id]) {
  if (authorizedKeys[key.id]._notAfterHeight == null) {

Recommended fix: Only set _addedByKeyId if the key is new (not found in authorizedKeys):

// Only set addedByKeyId if this is a new key entry
if (!authorizedKeys?.[key.id]) {
  key._addedByKeyId = signingKeyId
} else {
  // Preserve existing metadata if key already exists
  key._addedByKeyId = authorizedKeys[key.id]._addedByKeyId
}

if (authorizedKeys?.[key.id]) {
  // ... existing logic

• Addressed
• Dismissed

---

🟡 Missing Implementation: permissionsContext field is defined but never used

Location: src/types.ts:241, src/SPMessage.ts:31, src/SPMessage.ts:100

Problem: The permissionsContext field has been added to the type definitions but there is no validation logic that actually uses it. According to the issue description:

"We solve this with a context that applies to all invites, let's call it the "invites" context"

The field exists in:

• SPKey.permissionsContext
• SPKeyUpdate.permissionsContext
• ChelContractKey.permissionsContext

But it's never checked in validateKeyDelPermissions or any other validation function. The context-based restriction described in the issue (where "Revoke Invite permission" can only delete keys in the "invites" context) is not implemented.

Recommended action: Either:

1. Implement the permissionsContext validation logic, or
2. Remove the field if this feature is not ready for this PR

• Addressed
• Dismissed

---

🟡 Logic Issue: selfDeleteOnly determination doesn't check for kd#self permission

Location: src/utils.ts:339

Problem: The code determines selfDeleteOnly by checking if the key lacks the full kd permission:

const selfDeleteOnly = localSigningKey.permissions !== '*' && !localSigningKey.permissions.includes(SPMessage.OP_KEY_DEL)

However, this doesn't verify that the key actually HAS kd#self permission. A key with no delete permissions at all would be treated the same as a key with only kd#self permission. The initial permission check in validateKeyPermissions at src/utils.ts:218-225 does allow kd#self as an alternative, but the selfDeleteOnly logic could be clearer.

Recommended improvement: Add an explicit check and better error message:

const selfDeleteOnly = localSigningKey.permissions !== '*' && 
  !localSigningKey.permissions.includes(SPMessage.OP_KEY_DEL)

if (selfDeleteOnly) {
  // Verify the key has kd#self permission
  if (!localSigningKey.permissions.includes(`${SPMessage.OP_KEY_DEL}#self`)) {
    throw new Error('Signing key lacks both kd and kd#self permissions')
  }
  // ... existing selfDeleteOnly validation
}

• Addressed
• Dismissed

---

⚪️ Edge Case: Self-referential _addedByKeyId for initial contract keys

Location: src/internals.ts:1094, src/internals.ts:1795-1797

Problem: For OP_CONTRACT, all keys get _addedByKeyId set to the signingKeyId, including the signing key itself. This means the initial signing key is marked as being "added by itself."

const keys = keysToMap.call(self, v.keys, height, signingKeyId)

This is semantically unusual but appears intentional. The validateKeyDelPermissions logic should handle this case correctly since a key with kd#self would have k._addedByKeyId === localSigningKey.id.

Note: This is informational, not necessarily a bug, but worth being aware of when testing.

• Addressed
• Dismissed

---

Review generated using glm-5 via Z.AI. Comment /review to re-run.

[taoeffect]

/review

[github-actions]

Advanced AI Review

• Type: Combined
• Model: glm-5

Click to expand review

1. 🔴 Security Issue: Name-based comparison allows unauthorized key deletion

• Addressed
• Dismissed

Location: src/utils.ts:366-367 (also referenced at src/utils.ts:334-336)

Problem:
The validateKeyDelPermissions function uses key name comparison instead of key ID comparison to verify deletion authorization:

if (state._vm.authorizedKeys[k._addedByKeyId].name !== localSigningKey.name) {
  throw new Error('Key was added by a different key')
}

Attack scenario:

1. Key A (id="keyA", name="admin") creates Key B (_addedByKeyId="keyA")
2. Key A is deleted (revoked with _notAfterHeight set)
3. An attacker with kd#self permission creates a new Key A' (id="keyA2", name="admin")
4. Key A' attempts to delete Key B using kd#self
5. Check passes: "admin" === "admin" → Key B is deleted despite not being created by Key A'

Additional concerns:

• Ambiguity: name is a metadata string, not a unique identifier. If the system allows two different keys to have the same name, logic breaks.
• Renaming: If a key changes its name, it loses/gains permissions unexpectedly.

Recommended fix:

if (selfDeleteOnly) {
  if (!k._addedByKeyId || !state._vm.authorizedKeys[k._addedByKeyId]) {
    throw new Error('Missing or invalid _addedByKeyId')
  }
  if (k._addedByKeyId !== localSigningKey.id) {
    throw new Error('Key was added by a different key')
  }
}

This enforces that only the exact key that created another key can delete it with kd#self, matching the intent of non-transitivity and direct creation.

---

2. 🔴 Key rotation may break kd#self functionality

• Addressed
• Dismissed

Issue:
The implementation introduces _addedByKeyId to track the creator of a key. This field is populated in keysToMap (src/internals.ts). However, keysToMap is only called during OP_CONTRACT (contract creation) and OP_KEY_ADD (adding new keys). It is not called during OP_KEY_UPDATE (key rotation).

When a key is rotated (updated), a new key entry replaces or supplements the old one. This new key entry may lack the _addedByKeyId property because the update logic does not invoke keysToMap.

Consequently, if a user rotates a key they created, they may lose the ability to delete it using kd#self. The check in validateKeyDelPermissions (src/utils.ts:331-333) will throw "Missing or invalid _addedByKeyId", breaking the deletion workflow for any rotated keys.

Location:

• src/internals.ts:131-133 (keysToMap assignment)
• src/utils.ts:331-333 (Deletion check)

Suggestion:
Ensure that when a key is updated (rotated), the _addedByKeyId is preserved from the old key to the new key:

// In the OP_KEY_UPDATE handler logic:
const newKey = ... // construction of new key
const oldKey = state._vm.authorizedKeys[uk.oldKeyId]
if (oldKey._addedByKeyId) {
  newKey._addedByKeyId = oldKey._addedByKeyId
} else {
  // If the old key didn't have it (legacy), assume current signer
  newKey._addedByKeyId = signingKeyId
}

---

3. 🟡 Dead Code: permissionsContext field is defined but never used

• Addressed
• Dismissed

Locations:

• src/SPMessage.ts:41
• src/SPMessage.ts:111
• src/types.ts:241

Problem:
The permissionsContext field is added to three type definitions (SPKey, SPKeyUpdate, and ChelContractKey) but is never referenced or used anywhere in the codebase:

permissionsContext?: '*' | string[];

Recommendation:
Either:

1. Remove this field if it's not part of the current feature scope
2. Add documentation explaining it's reserved for future use
3. Implement the functionality that uses this field

---

4. 🟡 Type Definition: _addedByKeyId should be optional for backward compatibility

• Addressed
• Dismissed

Location: src/types.ts:244

Problem:
The _addedByKeyId field is defined as required:

export type ChelContractKey = {
  ...
  _addedByKeyId: string;
  ...
}

For contracts created before this feature was implemented, this field won't exist. While the validation in validateKeyDelPermissions handles this gracefully by throwing an error, the type definition should reflect reality.

Recommended fix:

export type ChelContractKey = {
  ...
  _addedByKeyId?: string;  // Optional for backward compatibility with existing contracts
  ...
}

Alternatively, add a JSDoc comment explaining this is required for new contracts but may be absent in legacy data.

---

5. ⚪️ Potential Behavioral Consideration: Key rotation and _addedByKeyId implications

• Addressed
• Dismissed

Location: src/utils.ts:414-417

Observation:
During key rotation via OP_KEY_UPDATE, the _addedByKeyId is preserved from the old key:

const updatedKey = omit(existingKey, [
  '_notAfterHeight',
  '_notBeforeHeight'
]) as ChelContractKey

This means:

• If Key A creates Key B, and Key A later rotates to Key A', then Key A' cannot delete Key B using kd#self (since _addedByKeyId comparison would fail against Key A's ID)

This aligns with the issue summary's non-transitivity requirement but may be surprising behavior. Consider documenting this explicitly or adding a comment in the code.

---

Review generated using glm-5 via Z.AI. Comment /review to re-run.

[taoeffect]

/crush_fast {{openrouter, google/gemini-3.1-pro-preview, google/gemini-3-flash-preview}}

---

AI review started.

[github-actions]

Advanced AI Review

• Type: Agentic (crush)
• Model: google/gemini-3.1-pro-preview

Click to expand review

Here is my review of the changes. I have identified a critical logic error regarding authorization, as well as a few smaller improvements and edge cases to consider.

🔴 1. Broken authorization logic and privilege escalation in kd#self validation

• Addressed
• Dismissed

In utils.ts, the logic inside validateKeyDelPermissions that enforces kd#self contains a significant security vulnerability:

    if (selfDeleteOnly) {
      if (!k._addedByKeyId || !state._vm.authorizedKeys[k._addedByKeyId]) {
        throw new Error('Missing or invalid _addedByKeyId')
      }
      if (state._vm.authorizedKeys[k._addedByKeyId].name !== localSigningKey.name) {
        throw new Error('Key was added by a different key')
      }
    }

1. Privilege Escalation: Because the code compares the .name attribute rather than the ID, any key with kd#self permissions implicitly gains the ability to delete keys created by any other key that happens to share the same name (e.g., overlapping "invite" or "manager" keys). This violates the "strict, conservative implementation" rule explicitly requesting isolated "direct creations only".
2. Broken implicitly assumed Rotation Support: If the name check was a clever attempt to allow keys to delete their descendants across key rotations (where the key ID changes but the name remains the same), it fails before the .name comparison is ever executed. If the creator key rotated, its old ID is permanently deleted from the authorizedKeys map. Thus, !state._vm.authorizedKeys[k._addedByKeyId] will unexpectedly evaluate to true and immediately trigger the Missing or invalid _addedByKeyId error in the block above it.

To tightly enforce strict direct creations and remove the vulnerability (and redundant key lookup), simplify the logic to strictly require identical Key IDs:

    if (selfDeleteOnly) {
      if (!k._addedByKeyId || k._addedByKeyId !== localSigningKey.id) {
        throw new Error('Key was added by a different key')
      }
    }

🟡 2. Ensure _addedByKeyId survives OP_KEY_UPDATE

• Addressed
• Dismissed

While not explicitly visible inside this diff, the property _addedByKeyId acts just like _notBeforeHeight and needs to be cautiously preserved when a key gets updated (OP_KEY_UPDATE).

If your update functionality creates a fresh object for the key or strips properties starting with _ from the patched key, the target key will lose its connection to its parent. Any key updated by an administrator would inadvertently—and permanently—disallow its original creator from later deleting it under kd#self. Be sure that the update logic guarantees the transfer of the _addedByKeyId property to the new key representation.

⚪️ 3. Unused permissionsContext type addition

• Addressed
• Dismissed

In SPMessage.ts:35 and SPMessage.ts:107, the property permissionsContext?: '*' | string[]; is added securely to SPKey and SPKeyUpdate. However, it sits entirely unused in your validation functions in internals.ts and utils.ts.

If this property was preemptively placed and the explicit context logic is reserved for a future PR, this is fine. If not, this is dead code remaining from the "Contexts vs. Ring Levels" debate mentioned in the issue and should be cleaned up.

⚪️ 4. StandardJS max line length violation

• Addressed
• Dismissed

In utils.ts (around line 337), the newly added constant exceeds the 100 character restriction for this project codebase:

  const selfDeleteOnly = localSigningKey.permissions !== '*' && !localSigningKey.permissions.includes(SPMessage.OP_KEY_DEL)

Improvement suggestion:

  const selfDeleteOnly = localSigningKey.permissions !== '*' &&
    !localSigningKey.permissions.includes(SPMessage.OP_KEY_DEL)

⚪️ 5. Simplification of nested permissions condition

• Addressed
• Dismissed

In utils.ts (around lines 214-220), the boolean nesting creates heavily indented logic spanning multiple negations that is a bit tough to read:

    (signingKey.permissions !== '*' &&
      (!Array.isArray(signingKey.permissions) || (
        !signingKey.permissions.includes(opT) &&
        !(
          opT === SPMessage.OP_KEY_DEL &&
          signingKey.permissions.includes(`${opT}#self`)
        )
      )))

Improvement suggestion:
Flatten the end of the statement by exchanging the !(A && B) logic for !A || !B:

    (signingKey.permissions !== '*' &&
      (!Array.isArray(signingKey.permissions) ||
        (!signingKey.permissions.includes(opT) &&
          (opT !== SPMessage.OP_KEY_DEL || !signingKey.permissions.includes(`${opT}#self`)))))

---

Review generated using google/gemini-3.1-pro-preview via OpenRouter. Comment /review to re-run.