Do not let patch object override the id in updateApiKey - #88
Open
bluzername wants to merge 1 commit into
Open
Conversation
The client build the request like this: { id, ...patch }. If someone
pass a patch variable that also have an id field inside (this can
happen because TypeScript only check extra fields on an object
literal, not on a variable), the spread come after and it win. So the
request can go to the wrong key instead of the one you ask for.
Now it build the object as { ...patch, id } so the id argument always
stay the real target, and patch can only touch name, expiresAt and
disabled.
Added a test that create two keys and try to sneak the second key id
inside the patch of the first one. Before the fix this test fail, now
it pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #76.
What was wrong
updateApiKeybuild the request body like this:patchis spread afterid, so if the patch object also carry anidfield, it win over the real target id. TypeScript do not stop this
because excess-property check only apply to object literals, not to a
variable that is passed in — so a caller can pass a
patchvariablethat structurally has an
idand the compiler stay quiet.Effect: an admin who mean to edit key A can end up editing key B
instead, with no error, exactly like the repro in the issue.
Fix
Swap the spread order so
idalways come from the explicit argument,never from
patch:Test
Added
updateApiKey ignores an id smuggled inside the patch objectinpackages/sdk/src/client.test.ts. It create two real keys over thein-memory server, call
updateApiKey(intended.id, { id: victim.id, disabled: true }),and check the intended key get disabled while the victim key stay
untouched. This test fail before the fix and pass after.
Also added a changeset (
@dormice/sdkpatch).Checked locally
pnpm buildpnpm --filter @dormice/sdk test— 32 passed (31 before + 1 new)pnpm test(full monorepo) — all greenpnpm typecheckandpnpm lint— cleanI am not a native English speaker, sorry for any small mistake in the wording, the code and test should speak clear enough.