Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: centralize validation schemas #2669

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: centralize validation schemas #2669

wants to merge 1 commit into from

Conversation

chronark
Copy link
Collaborator

@chronark chronark commented Nov 18, 2024

Summary by CodeRabbit

Release Notes

  • New Features

    • Added a centralized validation module for improved input validation across various API endpoints and forms.
    • Introduced the @unkey/validation dependency for enhanced validation capabilities.
  • Improvements

    • Refactored validation logic for createPermission, createRole, and other components to use the new validation module, ensuring consistency and maintainability.
  • Documentation

    • Updated the README for the @unkey/validation package to describe its purpose and functionalities.

Copy link

vercel bot commented Nov 18, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
dashboard ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 18, 2024 4:05pm
engineering ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 18, 2024 4:05pm
play ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 18, 2024 4:05pm
workflows ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 18, 2024 4:05pm
www ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 18, 2024 4:05pm

Copy link

changeset-bot bot commented Nov 18, 2024

⚠️ No Changeset found

Latest commit: 8f46c1d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

coderabbitai bot commented Nov 18, 2024

📝 Walkthrough

Walkthrough

The pull request introduces the @unkey/validation package as a new dependency in both the api and dashboard applications. It refactors the validation logic for various API endpoints and components to utilize this new package, replacing inline validation schemas with centralized validation methods. This change affects several files, including those responsible for creating permissions and roles, as well as updating namespace names. Additionally, a new package.json for the @unkey/validation package is created, along with its associated validation schemas.

Changes

File Change Summary
apps/api/package.json Added dependency @unkey/validation with version workspace:^.
apps/api/src/routes/v1_permissions_createPermission.ts Updated validation for name and description fields to use validation.identifier.openapi() and validation.description.optional().openapi().
apps/api/src/routes/v1_permissions_createRole.ts Refactored validation for name and description fields to use validation.name.openapi() and validation.description.optional().openapi().
apps/dashboard/app/(app)/authorization/permissions/create-new-permission.tsx Updated formSchema to use validation.name and validation.description.optional().
apps/dashboard/app/(app)/authorization/roles/[roleId]/update-role.tsx Updated formSchema to use validation.name and validation.description.optional().
apps/dashboard/app/(app)/authorization/roles/create-new-role.tsx Refactored validation for name and description fields to use validation.name and validation.description.optional().
apps/dashboard/package.json Added dependency @unkey/validation with version workspace:^.
internal/validation/README.md Updated to describe the @unkey/validation package and its purpose.
internal/validation/package.json Created new package file for @unkey/validation, specifying metadata and dependencies.
internal/validation/src/index.ts Introduced validation schemas for identifier, name, description, and unkeyId.
internal/validation/tsconfig.json Created new TypeScript configuration file extending @unkey/tsconfig/base.json.

Possibly related PRs

Suggested labels

Feature, Dashboard, 🕹️ oss.gg, :joystick: 300 points, hacktoberfest

Suggested reviewers

  • mcstepp
  • perkinsjr
  • MichaelUnkey

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

github-actions bot commented Nov 18, 2024

Thank you for following the naming conventions for pull request titles! 🙏

const unkeyId = z
.string()
.regex(
/^[a-z]{3,4}_[a-zA-Z0-Z]{8,}$/,

Check warning

Code scanning / CodeQL

Overly permissive regular expression range Medium

Suspicious character range that overlaps with A-Z in the same character class, and is equivalent to \[0-9:;<=>?@A-Z\].

Copilot Autofix AI 12 days ago

To fix the problem, we need to correct the overly permissive regular expression range. The intention is to match only lowercase and uppercase letters, so we should replace the range [a-zA-Z0-Z] with [a-zA-Z0-9]. This change will ensure that only alphanumeric characters are matched, without including unintended characters.

The changes should be made in the file internal/validation/src/index.ts on line 37. No additional methods, imports, or definitions are needed to implement this fix.

Suggested changeset 1
internal/validation/src/index.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/internal/validation/src/index.ts b/internal/validation/src/index.ts
--- a/internal/validation/src/index.ts
+++ b/internal/validation/src/index.ts
@@ -36,3 +36,3 @@
   .regex(
-    /^[a-z]{3,4}_[a-zA-Z0-Z]{8,}$/,
+    /^[a-z]{3,4}_[a-zA-Z0-9]{8,}$/,
     "Unkey IDs must include a prefix, separated by an underscore: key_abcdefg123",
EOF
@@ -36,3 +36,3 @@
.regex(
/^[a-z]{3,4}_[a-zA-Z0-Z]{8,}$/,
/^[a-z]{3,4}_[a-zA-Z0-9]{8,}$/,
"Unkey IDs must include a prefix, separated by an underscore: key_abcdefg123",
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Outside diff range and nitpick comments (4)
internal/validation/src/index.ts (1)

1-46: Consider standardizing error handling across applications.

Since this validation module is used across multiple applications (api and dashboard), consider:

  1. Adding custom error messages that are user-friendly and consistent
  2. Creating a shared error handling utility to standardize how validation errors are processed and displayed
  3. Adding examples in the documentation showing how to properly handle validation errors in both API and UI contexts

Would you like me to provide an example implementation of standardized error handling?

🧰 Tools
🪛 GitHub Check: CodeQL

[warning] 37-37: Overly permissive regular expression range
Suspicious character range that overlaps with A-Z in the same character class, and is equivalent to [0-9:;<=>?@a-z].

apps/api/src/routes/v1_permissions_createRole.ts (1)

Line range hint 67-67: Review the role ID prefix "test"

The role ID is being generated with a "test" prefix: newId("test"). This seems inappropriate for a production route and could lead to confusion or issues with role identification.

Consider using a more appropriate prefix like "role_" instead:

-      id: newId("test"),
+      id: newId("role"),
apps/dashboard/app/(app)/authorization/roles/[roleId]/update-role.tsx (1)

42-43: Good architectural decision: Centralized validation.

Moving validation to a shared package (@unkey/validation) brings several benefits:

  • Ensures consistent validation between frontend and backend
  • Creates a single source of truth for validation rules
  • Simplifies maintenance and updates of validation logic
apps/dashboard/app/(app)/authorization/roles/create-new-role.tsx (1)

Line range hint 147-186: Consider documenting the status of permission management.

There's a commented-out section for permission management that's marked as "Broken". This might affect the completeness of the role creation workflow.

Consider adding a TODO comment with more context about:

  • The current status of this feature
  • Any tracking issues or future plans
  • Whether roles can be properly used without this functionality
-            {/* Broken Have to link permissions on next page after creation */}
+            {/* TODO: Permission selection is currently disabled - permissions must be linked on the next page after creation
+                 Tracking issue: <issue_number>
+                 Impact: Roles can still be created and permissions can be added later
+            */}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 039cf73 and 8f46c1d.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (12)
  • apps/api/package.json (1 hunks)
  • apps/api/src/routes/v1_permissions_createPermission.ts (2 hunks)
  • apps/api/src/routes/v1_permissions_createRole.ts (2 hunks)
  • apps/dashboard/app/(app)/authorization/permissions/create-new-permission.tsx (1 hunks)
  • apps/dashboard/app/(app)/authorization/roles/[roleId]/update-role.tsx (2 hunks)
  • apps/dashboard/app/(app)/authorization/roles/create-new-role.tsx (1 hunks)
  • apps/dashboard/app/(app)/ratelimits/[namespaceId]/settings/update-namespace-name.tsx (1 hunks)
  • apps/dashboard/package.json (1 hunks)
  • internal/validation/README.md (1 hunks)
  • internal/validation/package.json (1 hunks)
  • internal/validation/src/index.ts (1 hunks)
  • internal/validation/tsconfig.json (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • internal/validation/package.json
  • internal/validation/tsconfig.json
🧰 Additional context used
🪛 LanguageTool
internal/validation/README.md

[typographical] ~3-~3: After the expression ‘for example’ a comma is usually used.
Context: ...shared schema or validation utils. For example user-chosen identifiers.

(COMMA_FOR_EXAMPLE)

🪛 GitHub Check: CodeQL
internal/validation/src/index.ts

[warning] 37-37: Overly permissive regular expression range
Suspicious character range that overlaps with A-Z in the same character class, and is equivalent to [0-9:;<=>?@a-z].

🔇 Additional comments (17)
internal/validation/src/index.ts (3)

3-14: Well-structured identifier schema with clear constraints!

The schema effectively enforces URL-safe characters and reasonable length limits, with clear documentation explaining its purpose.


25-32: Apply the same character restrictions to description schema.

Similar to the name schema, consider adding character restrictions for better security.


41-46: LGTM! Clean and well-organized exports.

The validation object provides a clear interface for accessing all schemas.

apps/api/package.json (1)

40-40: LGTM! Workspace dependency correctly configured.

The addition of @unkey/validation as a workspace dependency is properly configured and aligns with the PR's objective of centralizing validation schemas.

Let's verify the workspace setup and package existence:

✅ Verification successful

Workspace dependency setup is correctly configured

The verification confirms:

  • The @unkey/validation package exists in the workspace
  • The package is properly used as a dependency by:
    • @unkey/dashboard
    • api (the current package)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the workspace package setup for @unkey/validation

# Check if the validation package exists in the workspace
fd -t f "package.json" -x jq -r 'select(.name=="@unkey/validation") | .name' {}

# Verify that other packages using @unkey/validation have it in their dependencies
fd -t f "package.json" -x jq -r 'select(.dependencies."@unkey/validation" != null) | .name' {}

Length of output: 217

apps/dashboard/app/(app)/ratelimits/[namespaceId]/settings/update-namespace-name.tsx (2)

19-19: LGTM! Import of centralized validation module

The addition of @unkey/validation import aligns with the PR's objective of centralizing validation schemas.


24-26: Verify validation rules compatibility

The migration to centralized validation schemas looks good, but please verify:

  1. The validation rules in validation.name match the previous requirements
  2. The validation.unkeyId format is compatible with existing namespace and workspace IDs
✅ Verification successful

Validation rules are compatible and more restrictive

The migration to centralized validation rules is safe and actually more restrictive than the previous implementation:

Previous validation:

  • name: 3-50 chars, alphanumeric with _-.
  • namespaceId/workspaceId: just string type

New validation:

  • name: 3-256 chars (more permissive length but still covers min requirement)
  • unkeyId: enforces strict format /^[a-z]{3,4}_[a-zA-Z0-Z]{8,}$/ (more restrictive)

The validation rules are used consistently across the codebase for roles, permissions, and namespaces, ensuring uniform validation behavior.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify validation rules and their usage

# Check the validation rules implementation
echo "Checking validation rules implementation:"
ast-grep --pattern 'export const validation = {
  name: $_, 
  unkeyId: $_
}'

# Check for existing ID formats in the codebase
echo "Checking existing ID formats:"
rg -A 1 'namespace\.id|workspaceId' --type typescript

Length of output: 332


Script:

#!/bin/bash
# Let's try a different approach to find validation rules

# Search for validation schema definitions
echo "Searching for validation schema definitions:"
rg -l "validation.*name.*unkeyId" 

# Look for specific validation rules
echo "Looking for name and ID validation patterns:"
rg "z\.string\(\)|z\.object\(\)" -A 3

# Check for namespace and workspace ID usage patterns
echo "Checking ID usage patterns:"
rg "(namespace|workspace)Id.*=.*" -A 1

# Find any validation-related imports
echo "Checking validation-related imports:"
rg "import.*validation" -A 1

Length of output: 189163


Script:

#!/bin/bash
# Let's check the validation rules in the @unkey/validation package

# Find the validation rules implementation
echo "Checking validation rules in @unkey/validation:"
rg -A 5 "export const validation" internal/validation/src/

# Check the previous validation rules in the file
echo "Checking previous validation rules in the file:"
git log -p apps/dashboard/app/\(app\)/ratelimits/\[namespaceId\]/settings/update-namespace-name.tsx

Length of output: 5950


Script:

#!/bin/bash
# Let's check the specific validation rules in the validation package

echo "Checking name and unkeyId validation rules:"
rg -A 10 "const name =|const unkeyId =" internal/validation/src/

# Check if these validation rules are used consistently across the codebase
echo "Checking usage of validation rules:"
rg "validation\.(name|unkeyId)" -B 2 -A 2

Length of output: 4084

apps/api/src/routes/v1_permissions_createRole.ts (1)

10-10: LGTM: Import of centralized validation package

The addition of @unkey/validation import aligns with the PR objective of centralizing validation schemas.

apps/api/src/routes/v1_permissions_createPermission.ts (4)

4-4: LGTM: Import aligns with centralization objective

The addition of the centralized validation import aligns with the PR's objective of standardizing validation schemas.


Line range hint 4-34: LGTM: Well-structured implementation with proper safeguards

The changes maintain the robust implementation including:

  • Proper error handling through openApiErrorResponses
  • Transaction management for data consistency
  • Comprehensive audit logging

Line range hint 29-34: Verify optional description behavior

The description field is now explicitly marked as optional. Please verify if this is an intentional change in behavior and ensure it's consistently applied across all permission-related endpoints.

✅ Verification successful

The search results show that the description field is consistently marked as optional across all permission and role-related endpoints in both the API and dashboard components. Let me verify if there are any other validation patterns we might have missed.


Optional description field is consistently implemented across the codebase

The verification shows that the description field is consistently marked as optional across all permission and role-related endpoints:

  • API routes for creating permissions and roles
  • Dashboard components for creating and updating permissions and roles
  • Client-side validation schemas
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for consistency in description field validation

# Look for other permission endpoints
rg -A 5 'description.*validation\.description\.optional\(\)'

# Check for any required description fields that might conflict
rg -A 5 'description.*validation\.description[^.]'

Length of output: 3221


Script:

#!/bin/bash
# Search for any other validation patterns for description fields in permissions/roles
rg -A 5 'description:.*validation\.' 

# Search for any schema definitions related to permissions/roles
ast-grep --pattern 'const $_ = z.object({
  $$$
  description: $_,
  $$$
})'

Length of output: 8056


25-28: Verify validation rules for identifier

The change to use validation.identifier needs verification to ensure it maintains or improves upon the previous validation rules for permission names.

apps/dashboard/app/(app)/authorization/permissions/create-new-permission.tsx (3)

40-41: LGTM! Clean refactoring to centralized validation

The changes effectively move validation logic to a centralized location while maintaining type safety and component behavior. The refactoring is clean and focused.

Also applies to: 30-30


40-41: Verify validation rules haven't been weakened

The schema is now more maintainable, but let's verify the validation rules in the centralized schema.

#!/bin/bash
# Description: Verify validation rules in @unkey/validation package

# Find and display the validation schema definition
ast-grep --pattern 'export const validation = {
  name: $$$,
  description: $$$
}'

# Alternatively, search for individual validation rules
rg -A 5 'name:|description:' 

30-30: Verify @unkey/validation package setup

Let's ensure the package is properly configured in the monorepo workspace.

apps/dashboard/app/(app)/authorization/roles/[roleId]/update-role.tsx (1)

30-30: LGTM: Clean import of centralized validation.

The import statement aligns with the PR's objective of centralizing validation schemas.

apps/dashboard/app/(app)/authorization/roles/create-new-role.tsx (2)

31-31: LGTM! Import of centralized validation module.

The addition of the centralized validation import aligns well with the PR's objective of standardizing validation schemas across the application.


42-44: LGTM! Validation rules properly centralized.

The validation rules for name and description fields have been successfully migrated to use the centralized validation schema.

Let's verify the consistency of these validation rules across the application:

✅ Verification successful

Let me analyze the validation rules usage across the codebase to verify consistency.


Validation rules are properly centralized and consistently used

The verification confirms that:

  • The centralized validation rules in internal/validation/src/index.ts define consistent constraints:
    • name: string with min(3) and max(256)
    • description: string with min(3) and max(256)
  • All RBAC-related components (roles and permissions) consistently use these centralized rules through validation.name and validation.description.optional()
  • The optional modifier for description is correctly and consistently applied across the codebase

The changes align with the existing validation patterns in the codebase and maintain consistency in validation rules.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for consistent usage of validation rules across the codebase

# Search for any remaining inline validation for 'name' fields
echo "Checking for potential inconsistent name validations:"
rg "name.*z\.(string|object)" -g "*.{ts,tsx}"

# Search for any remaining inline validation for 'description' fields
echo "Checking for potential inconsistent description validations:"
rg "description.*z\.(string|object)" -g "*.{ts,tsx}"

# Check the actual validation rules being used
echo "Checking current validation usage:"
rg "validation\.(name|description)" -g "*.{ts,tsx}"

Length of output: 12595


Script:

#!/bin/bash
# Let's check the centralized validation rules definition
echo "Checking validation rules definition:"
rg -A 5 "const (name|description) = z\.string" internal/validation/src/index.ts

# Check if there are any other validation rule exports
echo -e "\nChecking validation exports:"
rg "export.*validation" internal/validation/src/index.ts

# Check specific usage in RBAC-related files
echo -e "\nChecking RBAC-specific validation usage:"
rg "name:|description:" apps/dashboard/app/\(app\)/authorization/ -A 1

Length of output: 6100

Comment on lines +34 to +39
const unkeyId = z
.string()
.regex(
/^[a-z]{3,4}_[a-zA-Z0-Z]{8,}$/,
"Unkey IDs must include a prefix, separated by an underscore: key_abcdefg123",
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix regex pattern in unkeyId schema.

The current regex pattern has two issues:

  1. Typo in character class: [a-zA-Z0-Z] should be [a-zA-Z0-9]
  2. The length constraint for the second part is unbounded, which could allow very long IDs
-    /^[a-z]{3,4}_[a-zA-Z0-Z]{8,}$/,
+    /^[a-z]{3,4}_[a-zA-Z0-9]{8,16}$/,

This fix:

  • Corrects the typo in the character class
  • Adds an upper bound of 16 characters for the second part to prevent excessively long IDs
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const unkeyId = z
.string()
.regex(
/^[a-z]{3,4}_[a-zA-Z0-Z]{8,}$/,
"Unkey IDs must include a prefix, separated by an underscore: key_abcdefg123",
);
const unkeyId = z
.string()
.regex(
/^[a-z]{3,4}_[a-zA-Z0-9]{8,16}$/,
"Unkey IDs must include a prefix, separated by an underscore: key_abcdefg123",
);
🧰 Tools
🪛 GitHub Check: CodeQL

[warning] 37-37: Overly permissive regular expression range
Suspicious character range that overlaps with A-Z in the same character class, and is equivalent to [0-9:;<=>?@a-z].

Comment on lines +16 to +23
/**
* A name is a user given human-readable string.
*
* It must not be used in URLs.
*
* @example the name of a key
*/
const name = z.string().min(3).max(256);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding character restrictions for the name schema.

While the documentation states it "must not be used in URLs", there are no character restrictions preventing unsafe characters. Consider adding a regex pattern to enforce safe characters, similar to the identifier schema.

-const name = z.string().min(3).max(256);
+const name = z
+  .string()
+  .min(3)
+  .max(256)
+  .regex(
+    /^[^<>{}[\]\\\/]*$/,
+    "Name cannot contain special characters like <, >, {, }, [, ], \\, or /"
+  );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/**
* A name is a user given human-readable string.
*
* It must not be used in URLs.
*
* @example the name of a key
*/
const name = z.string().min(3).max(256);
/**
* A name is a user given human-readable string.
*
* It must not be used in URLs.
*
* @example the name of a key
*/
const name = z
.string()
.min(3)
.max(256)
.regex(
/^[^<>{}[\]\\\/]*$/,
"Name cannot contain special characters like <, >, {, }, [, ], \\, or /"
);

Comment on lines +24 to +28
name: validation.name.openapi({
description: "The unique name of your role.",
example: "dns.records.manager",
}),
description: validation.description.optional().openapi({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Validation rules have been relaxed and need adjustment

The previous implementation had stricter validation rules for the role name:

  • Old: Required regex pattern /^[a-zA-Z0-9_:\-\.\*]+$/ with specific error message
  • New: Only enforces min length (3) and max length (256) without pattern validation

The centralized validation should be updated to maintain the same level of strictness for role names to prevent potential security or consistency issues.

  • internal/validation/src/index.ts: The name validator should include the regex pattern for role names
🔗 Analysis chain

Verify validation rules consistency

The schema now uses centralized validation methods. Let's ensure the validation rules remain consistent with the previous implementation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Compare the validation rules between old and new implementation

# Check for previous validation rules in git history
git log -p --full-history -S "name:" -- "apps/api/src/routes/v1_permissions_createRole.ts" | grep -A 10 "name:"

# Check the validation implementation in the new package
fd -e ts -e js "validation" -x cat {} \; | grep -A 10 "name ="

Length of output: 1419


Script:

#!/bin/bash
# Let's try a different approach to find the validation rules

# Search for validation rules in the current implementation
ast-grep --pattern 'const validation = {
  $$$
}'

# Also search for any validation-related imports
rg "import.*validation" -A 2

Length of output: 2289


Script:

#!/bin/bash
# Let's examine the validation rules in the centralized validation package

# Check the validation rules for name and description
cat internal/validation/src/index.ts

# Also check if there are any other validation-related files
fd -e ts -e js validation --full-path --exec cat {} \;

Length of output: 2012

@@ -53,6 +53,7 @@
"@unkey/rbac": "workspace:^",
"@unkey/resend": "workspace:^",
"@unkey/schema": "workspace:^",
"@unkey/validation": "workspace:^",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Warning: @unkey/validation package is missing but being imported

The package is being imported in multiple files but appears to be missing from the workspace:

  • No package found at packages/validation
  • Multiple files are already importing from "@unkey/validation":
    • apps/dashboard/app/(app)/authorization/permissions/create-new-permission.tsx
    • apps/dashboard/app/(app)/authorization/roles/create-new-role.tsx
    • apps/dashboard/app/(app)/ratelimits/[namespaceId]/settings/update-namespace-name.tsx
    • apps/dashboard/app/(app)/authorization/roles/[roleId]/update-role.tsx
    • apps/api/src/routes/v1_permissions_createRole.ts
    • apps/api/src/routes/v1_permissions_createPermission.ts

The workspace dependency cannot be added until the package is created and properly set up in the monorepo.

🔗 Analysis chain

LGTM! Verify the @unkey/validation package setup.

The addition of @unkey/validation as a workspace dependency is correctly positioned and properly versioned.

Let's verify the package setup:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the @unkey/validation package existence and configuration

# Check if the package.json exists for @unkey/validation
fd --type f "package.json" | grep -l "validation" | xargs cat

# Check for any existing imports of validation schemas
rg "@unkey/validation" --type ts --type tsx

Length of output: 216


Script:

#!/bin/bash
# Let's try a different approach to verify the package setup

# Check if validation package exists in packages directory
fd "package.json" packages/

# Check for any TypeScript files importing from @unkey/validation
rg -t ts "@unkey/validation"

# Look for validation-related files in packages directory
fd "validation" packages/

Length of output: 968


Script:

#!/bin/bash
# Let's check the root packages directory specifically for validation package
ls -la packages/validation 2>/dev/null || echo "Validation package not found"

# Check the workspace configuration in root package.json
cat package.json | grep -A 5 "workspaces"

# Check if there are any validation schemas in a potential validation package
fd . packages/validation --type f 2>/dev/null || echo "No validation package files found"

Length of output: 391

Comment on lines +42 to +43
name: validation.name,
description: validation.description.optional(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Now I can make a determination. The validation rules are equivalent:

Previous validation:

name: z.string().min(3).regex(/^[a-zA-Z0-9_:\-\.\*]+$/, {
  message: "Must be at least 3 characters long and only contain alphanumeric, colons, periods, dashes and underscores"
}),
description: z.string().optional()

New centralized validation from @unkey/validation:

name: z.string().min(3).max(256),
description: z.string().min(3).max(256)

The new validation rules are actually more strict for the description field (requiring min 3 chars instead of being completely optional) and add a max length constraint of 256 chars for both fields. The name field drops the regex pattern restriction but maintains the min length requirement.

Validation rules have changed and become more restrictive

The new schema:

  1. Adds max length validation (256 chars) for both fields
  2. Makes description field required with min length
  3. Removes special character restrictions on name field
🔗 Analysis chain

Verify validation rules equivalence.

The schema now uses centralized validation rules. Let's verify that these rules maintain the same validation constraints as before.


The previous attempt didn't show us the new validation rules. Let's try to locate the validation module and its contents to compare with the previous implementation we found, which required:

  • name: min 3 chars, alphanumeric with specific special chars
  • description: optional string

The previous attempt showed us the validation module is imported from multiple places. Let's find its implementation to compare with the previous validation rules we found in the git history.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Compare old and new validation rules

# Search for previous validation schema definition
echo "Previous validation rules:"
git log -p -S "formSchema = z.object" --full-history -- "apps/dashboard/app/(app)/authorization/roles/[roleId]/update-role.tsx"

# Check the new validation rules implementation
echo -e "\nNew validation rules from @unkey/validation:"
rg -A 10 "export const name = " --type ts
rg -A 10 "export const description = " --type ts

Length of output: 5722


Script:

#!/bin/bash
# Let's try a different approach to find the validation rules

# Search for validation rules in the entire codebase
echo "Searching for validation rules:"
fd -e ts -e tsx --exec grep -l "validation" {} \; | xargs cat

# Alternatively, let's try to find the specific import path
echo -e "\nSearching for validation import:"
rg "@unkey/validation" --type ts

Length of output: 259433


Script:

#!/bin/bash
# Let's check the validation module's implementation
echo "Searching for validation implementation:"
fd -e ts -e tsx --exec grep -l "export const validation" {} \; | xargs cat

# Also let's check the previous implementation of the form schema
echo -e "\nSearching for previous form schema in the file:"
git log -p -S "formSchema = z.object" -- "apps/dashboard/app/(app)/authorization/roles/[roleId]/update-role.tsx"

Length of output: 6683

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant