fix: set the minimum byte length for the web app to 16 #2584
Conversation
bitbrief
requested review from
perkinsjr,
chronark,
mcstepp and
MichaelUnkey
as code owners
|
📝 WalkthroughWalkthroughThe pull request introduces changes to the validation logic for byte length fields in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@bitbrief is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
apps/dashboard/app/(app)/apis/[apiId]/settings/default-bytes.tsx (2)
Line range hint
89-99: Add HTML5 validation attributes to improve user experience.The Input component could benefit from HTML5 validation attributes to provide immediate feedback to users.
Apply this diff to enhance the input constraints:
<Input className="max-w-sm" {...field} autoComplete="off" + type="number" + min="16" + max="255" onChange={(e) => field.onChange(Number(e.target.value))} />
Line range hint
57-62: Consider moving validation logic to schema level.The validation for unchanged values could be handled at the schema level for better separation of concerns and clearer error messages.
Consider updating the schema validation:
const formSchema = z.object({ keyAuthId: z.string(), workspaceId: z.string(), defaultBytes: z .number() .min(16, "Byte size needs to be at least 16") .max(255, "Byte size cannot exceed 255") + .refine((val) => val !== keyAuth.defaultBytes, { + message: "Please provide a different byte size than the current default" + }) .optional(), });Then simplify the onSubmit function:
async function onSubmit(values: z.infer<typeof formSchema>) { - if (values.defaultBytes === keyAuth.defaultBytes || !values.defaultBytes) { - return toast.error( - "Please provide a different byte-size than already existing one as default", - ); - } await setDefaultBytes.mutateAsync(values); }apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx (1)
Line range hint
350-356: Enhance the form field description to mention the minimum requirement.The description explains that longer keys are more secure, but it would be helpful to explicitly mention the minimum requirement of 16 bytes to users.
Consider updating the description:
- <FormDescription> - How long the key will be. Longer keys are harder to guess and more - secure. - </FormDescription> + <FormDescription> + How long the key will be (minimum 16 bytes). Longer keys are harder to guess and more + secure. + </FormDescription>packages/api/src/openapi.d.ts (6)
Line range hint
1303-1303: Typographical Error: Correct 'Asnyc' to 'Async'In the description for the
asyncproperty, 'Asnyc ratelimiting' should be corrected to 'Async ratelimiting'.Apply this diff to fix the typo:
- * @description Asnyc ratelimiting doesn't add latency, while sync ratelimiting is slightly more accurate. + * @description Async ratelimiting doesn't add latency, while sync ratelimiting is slightly more accurate.
1379-1381: Clarify the comment: Replace 'permissions' with 'roles'The description mentions 'permissions' instead of 'roles'. Since this block is about roles, it should refer to 'roles' for clarity.
Apply this diff to correct the description:
- * @description Set to true to automatically create the permissions they do not exist yet. Only works when specifying `name`. - * Autocreating roles requires your root key to have the `rbac.*.create_permission` permission, otherwise the request will get rejected + * @description Set to true to automatically create the roles if they do not exist yet. Only works when specifying `name`. + * Autocreating roles requires your root key to have the `rbac.*.create_role` permission, otherwise the request will get rejected
1560-1580: Specify data types explicitly in 'verifications' schemaFor better type safety and clarity, explicitly define the data types for numerical fields such as
time,success,rateLimited, andusageExceeded.Ensure TypeScript type annotations are added:
time: number; success: number; rateLimited: number; usageExceeded: number;
2804-3020: Improve documentation for migration endpointsThe additions in
v1.migrations.createKeysandv1.migrations.enqueueKeyscould benefit from clearer descriptions and consistent formatting to enhance understanding.Review the documentation comments to ensure all fields are properly described, and deprecated fields are clearly indicated.
3291-3306: Typographical Error: Correct 'Sucessfully' to 'Successfully'There is a typo in the response description: 'Sucessfully created a permission' should be 'Successfully created a permission'.
Apply this diff to fix the typo:
- /** @description Sucessfully created a permission */ + /** @description Successfully created a permission */
3641-3642: Update documentation formatting for 'externalId'In the description for
externalId, ensure that formatting is consistent and extraneous characters are removed.Apply this diff:
- * `externalId`s are unique across your workspace and therefore a `PRECONDITION_FAILED` error is returned when you try to create duplicates. + * `externalId`s are unique across your workspace, so a `PRECONDITION_FAILED` error is returned when you try to create duplicates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx (1 hunks)
- apps/dashboard/app/(app)/apis/[apiId]/settings/default-bytes.tsx (1 hunks)
- packages/api/src/openapi.d.ts (35 hunks)
🔇 Additional comments (7)
apps/dashboard/app/(app)/apis/[apiId]/settings/default-bytes.tsx (2)
25-25: LGTM: Minimum byte length requirement updated correctly.The validation change aligns with the PR objective to enforce a minimum of 16 bytes for API keys.
Line range hint
16-28: Verify integration with key creation endpoint.Let's ensure that the default byte length is properly applied when creating new keys.
✅ Verification successful
Integration with key creation is properly implemented.
The verification shows that:
- The default bytes setting is correctly stored in the keyAuth configuration
- The default bytes value is properly passed to the key creation form (
defaultBytesprop)- The key creation form uses the default bytes value as its initial value (
defaultValues.bytes: defaultBytes || 16)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for key creation implementations that should use this default value ast-grep --pattern 'createKey($$$)' # Search for any direct references to byte length in key creation rg -l 'byteLength|defaultBytes' --type ts --type tsxLength of output: 1014
Script:
#!/bin/bash # Search for key creation related code with proper file extensions rg -l "byteLength|defaultBytes" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' . # Look for API endpoints or mutations handling key creation rg -A 5 "createKey|mutation.*Key" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' . # Find form submissions or handlers in the settings component fd "settings.*\.(tsx|ts)$" . --exec rg -l "onSubmit|handleSubmit|mutation"Length of output: 115070
Script:
#!/bin/bash # Look for the implementation of the form submission in the default-bytes settings component rg -A 10 "onSubmit|handleSubmit" ./apps/dashboard/app/\(app\)/apis/\[apiId\]/settings/default-bytes.tsx # Look for the API endpoint that handles the default bytes update rg -A 10 "setDefaultBytes" ./apps/dashboard/lib/trpc/routers/api/Length of output: 1044
Script:
#!/bin/bash # Look for the implementation of key creation that uses defaultBytes rg -A 10 "defaultBytes.*createKey|createKey.*defaultBytes" . # Check for the setDefaultBytes implementation in the API router rg -A 10 "export.*setDefaultBytes" ./apps/dashboard/lib/trpc/routers/api/setDefaultBytes.ts # Look for where defaultBytes is used in key creation ast-grep --pattern 'bytes: $defaultBytes' .Length of output: 202
Script:
#!/bin/bash # Look for the key creation implementation in the dashboard rg -A 10 "createKey.*keyAuthId" ./apps/dashboard/lib/trpc/routers/key/create.ts # Check for defaultBytes usage in key creation rg -A 10 "defaultBytes" ./apps/dashboard/app/\(app\)/apis/\[apiId\]/keys/\[keyAuthId\]/new/ # Look for the setDefaultBytes mutation usage rg -A 5 "setDefaultBytes" ./apps/dashboard/app/\(app\)/apis/\[apiId\]/settings/Length of output: 4424
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx (1)
56-57: LGTM: Minimum byte length requirement implemented correctly.The change correctly implements the minimum byte length requirement of 16 bytes, aligning with the API requirements as specified in issue #2564.
packages/api/src/openapi.d.ts (4)
9-10: LGTM!The updated type definitions for
XORandOneOfimprove readability and maintain correct logic.
552-556: Verify the recursive 'PermissionQuery' type definitionEnsure that the recursive type definition for
PermissionQueryusingOneOfcorrectly models the intended permission query logic and does not introduce any TypeScript type errors.To check for potential type errors, consider running a TypeScript type check on this definition in the context of its usage.
601-615: Review optional properties in 'ratelimits' schemaThe properties
limitanddurationare marked as optional, but the example suggests they are important for proper functioning. Verify whether these fields should be optional or required to avoid unintended behaviors.If these fields are required, update their definitions to reflect that by removing the
?to indicate they are mandatory.
2561-2580: Ensure consistency in 'resources' field descriptionsIn the 'limit' operation, verify that the descriptions and types of the
resourcesarray fields are accurate and consistent with their intended use.Review the
resourcesschema to confirm that all fields are documented correctly, and consider adding any missing descriptions or examples.
|
Also I was also trying to fix this on the api, but I was receiving server error running it locally. |
|
can you elaborate? |
This was the error |
|
@chronark any updates? |
What does this PR do?
Set the minimum byte length to 16 for api creation on web app
Fixes #2564
Type of change
How should this be tested?
Open the api dashboard on the web app and try to change the default minimum byte length which is now 16 (8 earlier)
Try to create an api with less than 16 minimum byte length
Checklist
Required
pnpm buildpnpm fmtconsole.logsSummary by CodeRabbit
New Features
CreateKeycomponent to require a minimum byte value of 16.defaultBytesin theDefaultBytescomponent from 8 to 16.Bug Fixes
Documentation