Trim the name before matching an API key in disable/enable/revoke - #89
Open
bluzername wants to merge 1 commit into
Open
Trim the name before matching an API key in disable/enable/revoke#89bluzername wants to merge 1 commit into
bluzername wants to merge 1 commit into
Conversation
Creating a key trim the name (apiKeyNameSchema use z.string().trim()), so " incident key " get stored as "incident key". But resolveApiKeyId, the one function used by disable, enable and revoke, compare the raw argument with === and never trim it. So if an operator or a script reuse the exact spelling they typed at creation (with the spaces), disable/enable/revoke say "no API key named ..." even if the key is right there in `dor apikey ls`. Same problem for revoke, which is worse because it silently do nothing instead of throwing. Fix is one line: trim the name inside resolveApiKeyId before the lookup, so it always compare against the same normalized spelling creation already stored. Added a test that create a key with extra spaces, disable it with the same untrimmed spelling, then revoke it with the trimmed spelling, and check both calls succeed. This test fail before the fix and pass after.
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
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 #77.
What was wrong
apiKeyNameSchema(z.string().trim().min(1).max(64)) trim the nameat creation time, so
dor apikey create ' incident key 'storeincident key, anddor apikey lsshow it that way too.But
resolveApiKeyId, the one resolver every management verb use, doa plain
===on the raw argument:So an operator or a script that reuse the exact spelling they typed at
creation (with the surrounding spaces) get "no API key named ...",
even if the key is listed right there.
revokeis the worse case: itanswers
revoked: falseinstead of throwing, so a credential meant tobe killed can silently stay alive.
Fix
Trim the name inside
resolveApiKeyIdbefore the lookup, so it alwayscompare against the same normalized spelling creation already stored:
One place fixes it for disable, enable and revoke together, matching
the doctrine already written above that function.
Test
Added
disable and revoke resolve a name by the same trimmed spelling creation storedinpackages/cli/src/commands.test.ts, following theissue's own repro: create with extra spaces, disable with the same
untrimmed spelling, then revoke with the trimmed one. Fails before the
fix, passes after.
Also added a changeset (
@dormice/clipatch).Checked locally
pnpm buildpnpm --filter @dormice/cli test— 63 passedpnpm test(full monorepo) — all greenpnpm typecheckandpnpm lint— cleanSorry in advance for any small English mistake, I am not a native
speaker, but the fix itself is small and the test should make it easy
to check.