Cross-User Security Concerns

[alexbaileyuk]

Based on my testing so far, it appears that this plugin will expose servers to cross-user data leakage by default.

Starting with an empty Redis database:

1 Send a request as User A

PUT http://localhost:3000/profile/:id

authorization: XXXXXXXXXXXX
content-type: application/json
Idempotency-Key: dabfd044-7415-488f-b163-432ccb6eb11f

{
  "firstname": "Jimmy"
}

2 Note successful response and cache key populated in Redis

{"status":"COMPLETE","fingerPrint":"9b76a708ee1e155d0806162ba4ec5778558e13a5567fe31d33e11a575814a000","response":{"additional":{"statusCode":200},"body":{"message":"Profile updated"}}}

3 Send same request as User B

PUT http://localhost:3000/profile/:id

authorization: YYYYYYYYYYYYY
content-type: application/json
Idempotency-Key: dabfd044-7415-488f-b163-432ccb6eb11f

{
  "firstname": "Jimmy"
}

4 Note response from cache

idempotent-replayed true

---

This will almost certainly be exposing servers to fairly significant risks in production. I suspect that #4 would help solve the problem.

Looking at other libraries like https://docs.powertools.aws.dev/lambda/typescript/latest/utilities/idempotency/#choosing-a-payload-subset-for-idempotency, we could offer a the ability to specify a path using something like JMES paths. Simply putting the authorisation token in the fingerprint would solve the problem in this case (though may not support idempotent requests if tokens are refreshed).

It feels like extreme care needs to be taken with this library in production and that it's not suited for any multi-tenant applications.

[mahendraHegde]

hey @alexbaileyuk thanks for raising the concern. Since the client/consumer controls the generation of the idempotency key, it should typically be a UUID, which minimizes the chances of collisions or being easily guessed. It’s also a good practice to perform the idempotency check after authorization. Besides, today the uri path is part if the idempotency key as well, which reduces above possibility further.

node-idempotency/packages/core/src/idempotency.ts

Lines 92 to 96 in 42ce80e

	private getIdempotencyCacheKey(req: IdempotencyParamsWithDefaults): string {
	const idempotencyKey = this.getIdempotencyKey(req);
	const { path, method } = req;
	return `${req.options.cacheKeyPrefix}:${method}:${path}:${idempotencyKey}`;
	}

there is also a possibility to specify key prefix which can provide additional security like you suggested.

node-idempotency/packages/core/tests/e2e/idempotency.spec.ts

Lines 70 to 105 in e6259d6

	it("should allow overriding cacheKeyPrefix", async () => {
	const req: IdempotencyParams = {
	headers: { [HttpHeaderKeysEnum.IDEMPOTENCY_KEY]: "1" },
	path: "/pay",
	body: { a: "a" },
	method: "POST",
	options: {
	cacheKeyPrefix: "tenant-1",
	},
	};
	const res = { body: { success: "true" } };
	const idempotencyRes = await idempotency.onRequest(req);
	expect(idempotencyRes).toBeUndefined();
	await idempotency.onResponse(req, res);
	const cachedResponse = await idempotency.onRequest(req);
	expect(cachedResponse).toEqual(res);
	expect(
	await storage.get(`${req.options?.cacheKeyPrefix}:POST:/pay:1`),
	).toEqual(
	'{"status":"COMPLETE","fingerPrint":"f45aa7a9803525391d546d331b22b3ed4583a11a04797feaeb1027b158c65d10","response":{"body":{"success":"true"}}}',
	);
	// different tenant with same idempotency key
	const cachedResponse2 = await idempotency.onRequest({
	...req,
	options: {
	cacheKeyPrefix: "tenant-2",
	},
	});
	expect(cachedResponse2).toBeUndefined();
	});

[alexbaileyuk]

@mahendraHegde agreed that this can be mitigated in a few different ways. Do you think these concerns should at least be highlighted in the docs?

[mahendraHegde]

Yup totally makes sense to highlight these @alexbaileyuk