Skip to content

Implement KV attribution #124

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
corrideat wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Implement KV attribution #124

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e47bf3e
Implement KV attribution
corrideat Mar 2, 2026
549c1d1
Feedback
corrideat Mar 2, 2026
0aaece3
Delete hint / token digest
corrideat Mar 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 56 additions & 4 deletions src/serve/routes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,11 @@ route.POST('/event', {
}
const deletionTokenDgst = request.headers['shelter-deletion-token-digest']
if (deletionTokenDgst) {
const deletionTokenHint = request.headers['shelter-deletion-token-hint']
await sbp('chelonia.db/set', `_private_deletionTokenDgst_${deserializedHEAD.contractID}`, deletionTokenDgst)
if (deletionTokenHint) {
await sbp('chelonia.db/set', `_private_deletionTokenHint_${deserializedHEAD.contractID}`, deletionTokenHint)
}
}
}
// Store size information
Expand Down Expand Up @@ -651,7 +655,11 @@ route.POST('/file', {
// Store deletion token
const deletionTokenDgst = request.headers['shelter-deletion-token-digest']
if (deletionTokenDgst) {
const deletionTokenHint = request.headers['shelter-deletion-token-hint']
await sbp('chelonia.db/set', `_private_deletionTokenDgst_${manifestHash}`, deletionTokenDgst)
if (deletionTokenHint) {
await sbp('chelonia.db/set', `_private_deletionTokenHint_${manifestHash}`, deletionTokenHint)
}
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
}
return h.response(manifestHash)
} catch (err) {
Expand Down Expand Up @@ -826,7 +834,7 @@ route.POST('/deleteContract/{hash}', {

route.POST('/kv/{contractID}/{key}', {
auth: {
strategies: ['chel-shelter'],
strategies: ['chel-shelter', 'chel-bearer'],
mode: 'required'
},
payload: {
Expand All @@ -840,7 +848,7 @@ route.POST('/kv/{contractID}/{key}', {
key: Joi.string().regex(KV_KEY_REGEX).required()
})
}
}, function (request, h) {
}, async function (request, h) {
if (ARCHIVE_MODE) return Boom.notImplemented('Server in archive mode')
const { contractID, key } = request.params

Expand All @@ -849,14 +857,48 @@ route.POST('/kv/{contractID}/{key}', {
return Boom.badRequest()
}

if (!ctEq(request.auth.credentials.billableContractID as string, contractID)) {
return Boom.unauthorized(null, 'shelter')
const strategy = request.auth.strategy
let isOwner = false
switch (strategy) {
case 'chel-shelter': {
if (!ctEq(request.auth.credentials.billableContractID as string, contractID)) {
const ultimateOwner = await lookupUltimateOwner(contractID)
// Check that the user making the request is the ultimate owner (i.e.,
// that they have permission to delete this file)
if (!ctEq(request.auth.credentials.billableContractID as string, ultimateOwner)) {
return Boom.unauthorized('Invalid shelter auth', 'shelter')
}
isOwner = true
} else {
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
Copy Markdown

@devin-ai-integration devin-ai-integration bot Mar 2, 2026 •
edited
Loading

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 && !existing → false && !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.

}
break
}
case 'chel-bearer': {
const expectedTokenDgst = await sbp('chelonia.db/get', `_private_deletionTokenDgst_${contractID}_kv_${key}`)
if (!expectedTokenDgst) {
return Boom.notFound()
}
const tokenDgst = blake32Hash(request.auth.credentials.token as string)
// Constant-time comparison
// Check that the token provided matches the deletion token for this file
if (!ctEq(expectedTokenDgst, tokenDgst)) {
return Boom.unauthorized('Invalid token', 'bearer')
}
isOwner = true
break
}
default:
return Boom.unauthorized('Missing or invalid auth strategy')
}

// Use a queue to prevent race conditions (for example, writing to a contract
// that's being deleted or updated)
return sbp('chelonia/queueInvocation', contractID, async () => {
const existing = await sbp('chelonia.db/get', `_private_kv_${contractID}_${key}`)
if (isOwner && !existing) return Boom.conflict()

// Some protection against accidental overwriting by implementing the if-match
// header
Expand Down Expand Up @@ -916,6 +958,16 @@ route.POST('/kv/{contractID}/{key}', {
await sbp('chelonia.db/set', `_private_kv_${contractID}_${key}`, request.payload)
await sbp('backend/server/updateSize', contractID, (request.payload as Buffer).byteLength - existingSize)
await appendToIndexFactory(`_private_kvIdx_${contractID}`)(key)

const deletionTokenDgst = request.headers['shelter-deletion-token-digest']
if (deletionTokenDgst) {
const deletionTokenHint = request.headers['shelter-deletion-token-hint']
await sbp('chelonia.db/set', `_private_deletionTokenDgst_${contractID}_kv_${key}`, deletionTokenDgst)
if (deletionTokenHint) {
await sbp('chelonia.db/set', `_private_deletionTokenHint_${contractID}_kv_${key}`, deletionTokenHint)
}
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
}

// No await on broadcast for faster responses
sbp('backend/server/broadcastKV', contractID, key, request.payload.toString()).catch((e: Error) => console.error(e, 'Error broadcasting KV update', contractID, key))

Expand Down