Skip to content

Implement KV attribution#124

Open
corrideat wants to merge 3 commits intomasterfrom
kv-attribution
Open

Implement KV attribution#124
corrideat wants to merge 3 commits intomasterfrom
kv-attribution

Conversation

@corrideat
Copy link
Member

@corrideat corrideat commented Mar 2, 2026

Also implements token hints storage


Open with Devin

Also implements token hints storage
@corrideat corrideat requested a review from taoeffect March 2, 2026 08:08
Copy link

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

View 4 additional findings in Devin Review.

Open in Devin Review

Comment on lines +873 to +875
const existing = await sbp('chelonia.db/get', `_private_kv_${contractID}_${key}`)
// This type of SAK authorization is only allowed for creating new keys
if (existing) return Boom.unauthorized('Invalid shelter auth', 'shelter')
Copy link

@devin-ai-integration devin-ai-integration bot Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 TOCTOU race condition allows SAK-authenticated user to overwrite existing KV entries

The SAK authorization check for the POST /kv/{contractID}/{key} endpoint reads the existing value outside the serialization queue, creating a time-of-check-time-of-use (TOCTOU) vulnerability that bypasses the "SAK auth is only for creating new keys" restriction.

Root Cause and Exploitation

The authorization check at line 873 reads existing and, if it's null, allows the SAK-authenticated request through (since SAK is only meant for creating new keys). However, this check happens before the request enters the queue at line 899.

A race can occur:

  1. Request A (SAK auth) hits line 873 — key doesn't exist, so auth passes; isOwner remains false
  2. Request B (owner/bearer auth) enters the queue first and creates the key
  3. Request A enters the queue at line 899 — key now exists
  4. Line 901: isOwner && !existingfalse && !existing → skipped (no guard)
  5. If if-match: * was sent, the ETag check at line 926 passes through
  6. Request A overwrites the key, bypassing the intended restriction

The isOwner flag at line 901 only guards owners from writing to non-existent keys. There is no corresponding guard for !isOwner (SAK auth) writing to existing keys inside the queue.

Impact: A SAK-authenticated user can overwrite KV entries they should only have been allowed to create, not update. This is an authorization bypass that could lead to data corruption.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration[bot]

This comment was marked as resolved.

Copy link

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 new potential issue.

View 7 additional findings in Devin Review.

Open in Devin Review

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Missing deletion of _private_deletionTokenHint_ in backend/deleteContract

When deleting a contract in backend/deleteContract, line 392 deletes _private_deletionTokenDgst_${cid} but the corresponding _private_deletionTokenHint_${cid} is never deleted. This is inconsistent with backend/deleteFile (src/serve/server.ts:282-283) which correctly deletes both. The result is that the hint data leaks and persists in the database after the contract is deleted.

(Refers to line 392)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant