Bug: Payload validation broken for DynamoDBPersistenceLayer

[kevin-secrist]

Expected Behaviour

The IdempotencyHandler should throw a IdempotencyValidationError when the validation hash in DynamoDB does not match the incoming event's validation hash (specified by payloadValidationJmesPath).

Current Behaviour

It does not do that.

Code snippet

I would not expect the example code to work: https://docs.powertools.aws.dev/lambda/typescript/latest/utilities/idempotency/#payload-validation

Steps to Reproduce

1. Specify payloadValidationJmesPath in the IdempotencyConfig
2. Run the same payload twice (except with a difference at the field specified at payloadValidationJmesPath

No IdempotencyValidationError will be thrown.

Possible Solution

Reverting to 1.17 fixes the issue.

The changes done in this PR #1779 does not do payload hash checking on the returned document, because that logic is short circuited when returning the existingRecord on the error.

powertools-lambda-typescript/packages/idempotency/src/IdempotencyHandler.ts

Lines 316 to 320 in 7715df8

	const idempotencyRecord: IdempotencyRecord =
	e.existingRecord ||
	(await this.#persistenceStore.getRecord(
	this.#functionPayloadToBeHashed
	));

The following logic within BasePersistenceLayer.getRecord() is skipped, so the IdempotencyValidationError error is not thrown.

powertools-lambda-typescript/packages/idempotency/src/persistence/BasePersistenceLayer.ts

Lines 106 to 121 in 7715df8

	public async getRecord(data: JSONValue): Promise<IdempotencyRecord> {
	const idempotencyKey = this.getHashedIdempotencyKey(data);
	const cachedRecord = this.getFromCache(idempotencyKey);
	if (cachedRecord) {
	this.validatePayload(data, cachedRecord);
	return cachedRecord;
	}
	const record = await this._getRecord(idempotencyKey);
	this.saveToCache(record);
	this.validatePayload(data, record);
	return record;
	}

Not sure how stable the interface for BaseIdempotencyLayer is but exposing validatePayload (and also saveToCache, because local caching is also being skipped) as protected and calling that on the returned record would solve the immediate problem but it doesn't seem like the most elegant solution.

Powertools for AWS Lambda (TypeScript) version

latest

AWS Lambda function runtime

18.x

Packaging format used

npm

Execution logs

No response

[dreamorosi]

Hi @kevin-secrist thank you for opening this issue.

Could you please share an example of the two payloads you're sending to the function?

Run the same payload twice (except with a difference at the field specified at

I'm not sure I follow the wording here.

That would help us reproduce the bug.

[kevin-secrist]

Hi @dreamorosi, the payload/code I'm using is simple and is similar to the example code. I've removed some extraneous fields and renamed things a bit but it should still illustrate the point.

const persistenceStore = new DynamoDBPersistenceLayer({
  tableName: process.env.IDEMPOTENCY_TABLE_NAME as string,
})
const config = new IdempotencyConfig({
  eventKeyJmesPath: '["rcn", "subscriptionType"]',
  payloadValidationJmesPath: 'customerAccounts',
  throwOnNoIdempotencyKey: true,
  expiresAfterSeconds: 3 * 24 * 60 * 60, // 3 days
})
const createSubscriptionIdempotent = makeIdempotent(
  (
    request: CreateSubscriptionRequest,
    client: IApiClient
  ): Promise<number> => client.createSubscription(request),
  {
    persistenceStore: createSubscriptionPersistenceStore,
    config,
  }
)
const triggerCheckSubscriptionEvent = async (
  event: CreateSubscriptionLambdaEvent
) => {
  // calls eventbridge PutEvents
};
const lamdaHandler = async (
  event: CreateSubscriptionLambdaEvent,
  context: CreateSubscriptionContext
): Promise<number> => {
  config.registerLambdaContext(context);

  const client = NewClient();
  try {
    const createSubscriptionResponse = await createSubscriptionIdempotent(
      event.detail,
      client
    );
    return createSubscriptionResponse;
  } catch (error) {
    if (error instanceof IdempotencyValidationError) {
      // a subscription was already created by another lambda
      // but with a different set of customerAccounts
      await triggerCheckSubscriptionEvent(event);
      // we don't have access to the existing idempotency record
      // so we will just return -1
      return -1;
    } else {
      throw error;
    }
  }
};

export const handler = middy(lamdaHandler).use(
  injectLambdaContext(logger, { logEvent: true })
);

And the payload looks something like this:

{
  "rcn": "RCN-000-000-000-000",
  "customerAccounts": {
    "customerAccount": [
      {
        "accountId": "12345",
        "accountType": "someaccounttype"
      }
    ]
  },
  "subscriptionType": "somesubscriptiontype"
}

and a second one

{
  "rcn": "RCN-000-000-000-000",
  "customerAccounts": {
    "customerAccount": [
      {
        "accountId": "54321",
        "accountType": "someaccounttype"
      }
    ]
  },
  "subscriptionType": "somesubscriptiontype"
}

The use case is that I'm basically turning concurrent creates into a create+updates, so for the same rcn and subscriptionType the first event would translate into a create and the second+future events will append the customerAccounts to the original entity as an update. I'm expecting the IdempotencyValidationError to be thrown because the customerAccounts property is different in the second payload. Reverting to 1.17 fixes the behavior.

[dreamorosi]

Hi @kevin-secrist thanks for sharing the additional info, I'll be looking into this today and try to reproduce the error.

[dreamorosi]

Hi again, I was able to reproduce the issue, indeed there seems to be a regression between versions 1.17.0 and 1.18.0.

I'm going to try implementing a fix.

[dreamorosi]

The PR is ready for review, we should be able to release the fix on Monday.

[github-actions]

⚠️ COMMENT VISIBILITY WARNING ⚠️

This issue is now closed. Please be mindful that future comments are hard for our team to see.

If you need more assistance, please either tag a team member or open a new issue that references this one.

If you wish to keep having a conversation with other community members under this issue feel free to do so.

[github-actions]

This is now released under v1.18.1 version!