Merge pull request #24 from depot/fix-claim-exchange

Fix marking claim as successfully exchanged before success

Author: jacobwgillespie

src/index.ts

@@ -10,9 +10,10 @@ import {claimSchema} from './types'
 import {authenticateAdmin} from './utils/auth'
 import {
   createClaim,
-  exchangeClaim,
   getAllKeys,
+  getClaim,
   getLatestKey,
+  markClaimAsExchanged,
   setGitHubSession,
   setLatestKey,
   storeKey,
@@ -57,8 +58,8 @@ app.post('/claim', async ({req, json}) => {
 app.post('/exchange/:id', async ({req, text}) => {
   const claimId = req.param('id')
 
-  const claim = await exchangeClaim(claimId)
-  if (!claim) throw new Error('challenge already used or not found')
+  const claim = await getClaim(claimId)
+  if (!claim || claim.exchanged) throw new Error('challenge already used or not found')
 
   const {issuer, claimData, challengeCode} = claim
 
@@ -90,6 +91,8 @@ app.post('/exchange/:id', async ({req, text}) => {
     claims: validatedClaims,
   })
 
+  await markClaimAsExchanged(claimId)
+
   return text(token)
 })
 

src/utils/dynamodb.ts

@@ -3,6 +3,7 @@ import {DynamoDBDocumentClient, GetCommand, PutCommand, QueryCommand, UpdateComm
 import {addMinutes, addSeconds, getUnixTime} from 'date-fns'
 import type {webcrypto} from 'node:crypto'
 import type {ClaimSchema} from '../types'
+import {logger} from './logger'
 import type {JsonWebKeyWithKid, Key} from './oidc'
 
 /**
@@ -93,27 +94,21 @@ export async function getClaim(claimId: string): Promise<ClaimRecord | null> {
 }
 
 /**
- * Atomically mark a claim as exchanged and return the claim data
+ * Mark a claim as successfully exchanged
  */
-export async function exchangeClaim(claimId: string): Promise<ClaimRecord | null> {
+export async function markClaimAsExchanged(claimId: string): Promise<void> {
   try {
-    const result = await docClient.send(
+    await docClient.send(
       new UpdateCommand({
         TableName: TABLE_NAME,
         Key: {pk: `CLAIM#${claimId}`, sk: 'CLAIM'},
         UpdateExpression: 'SET exchanged = :true',
-        ConditionExpression: 'attribute_exists(pk) AND exchanged = :false',
-        ExpressionAttributeValues: {':true': true, ':false': false},
-        ReturnValues: 'ALL_OLD',
+        ConditionExpression: 'attribute_exists(pk)',
+        ExpressionAttributeValues: {':true': true},
       }),
     )
-    if (!result.Attributes) return null
-    return result.Attributes as ClaimRecord
   } catch (error) {
-    if (error instanceof Error && error.name === 'ConditionalCheckFailedException') {
-      return null
-    }
-    throw error
+    logger.error('Failed to mark claim as exchanged', {claimId, error})
   }
 }