feat(workspace): expand forbidden subdomain validation #9082
Conversation
Added new forbidden words and regex patterns to subdomain validation in `update-workspace-input`. Enhanced the `ForbiddenWords` validator to support both strings and regex matching. Updated tests to verify regex-based forbidden subdomain validation.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Enhanced the ForbiddenWords validator to support regex pattern matching for subdomain validation, allowing more flexible control over restricted workspace names.
- Added regex pattern support in
packages/twenty-server/src/engine/utils/custom-class-validator/ForbiddenWords.tswith case-insensitive string comparison - Implemented test case in
forbidden-words-custom-class-validator.spec.tsto verify regex pattern validation (e.g., 'api-*' subdomains) - Expanded forbidden subdomain list in
update-workspace-input.tsto include system paths, authentication terms, and country codes - Found duplicate entries 'eu' and 'support' in forbidden words list that should be removed
3 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile
...ages/twenty-server/src/engine/utils/__tests__/forbidden-words-custom-class-validator.spec.ts
Outdated
Show resolved
Hide resolved
...ages/twenty-server/src/engine/utils/__tests__/forbidden-words-custom-class-validator.spec.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/workspace/dtos/update-workspace-input.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/workspace/dtos/update-workspace-input.ts
Outdated
Show resolved
Hide resolved
Added new forbidden words and regex patterns to subdomain validation in `update-workspace-input`. Enhanced the `ForbiddenWords` validator to support both strings and regex matching. Updated tests to verify regex-based forbidden subdomain validation.
packages/twenty-server/src/engine/utils/custom-class-validator/ForbiddenWords.ts
Outdated
Show resolved
Hide resolved
Removed 'support' and 'eu' tags from the workspace input DTO. These tags are no longer needed and their removal simplifies the codebase. Ensures the DTO reflects the current requirements accurately.
Replaced 'ForbiddenWords' custom validation with 'NotIn' for clarity and consistency. Updated all references, including test files, to align with the new naming convention.
|
Hey @AMoreaux sorry one last feedback, do you think it would be possible to add a user-friendly message? I feel like this will create support issues 
|
+1 to that, also as a 500 this will be captured by sentry which we don't want 🤔 |
Is there a guideline or an example to do that ? |
|
Maybe look at |
Updated subdomain validation to disallow prefixes like "api-" and improved error messages for clearer feedback. Adjusted test cases and validation logic to reflect the changes, ensuring consistency across functionality.
Removed unnecessary console logs and commented-out code in the ValidationPipe exceptionFactory. This improves code cleanliness and reduces potential distractions in the production environment.
| @@ -101,7 +101,8 @@ export const SettingsDomain = () => { | |||
| } catch (error) { | |||
| if ( | |||
| error instanceof Error && | |||
| error.message === 'Subdomain already taken' | |||
| (error.message === 'Subdomain already taken' || | |||
| error.message.endsWith('not allowed')) | |||
There was a problem hiding this comment.
this feels very manual! We will need to discuss later if we actually want the backend to return error codes to the FE
packages/twenty-server/src/main.ts
Outdated
| @@ -54,6 +58,18 @@ const bootstrap = async () => { | |||
| app.useGlobalPipes( | |||
| new ValidationPipe({ | |||
| transform: true, | |||
| exceptionFactory: (validationErrors: ValidationErrorType[] = []) => { | |||
There was a problem hiding this comment.
I'm a bit lost, is this a global pipe which is validating our graphql input types !?
Nest is so weird, I would expect it to be passed to graphql yoga directly and yoga to accept a middleware or something like this
@Weiko what do you think?
There was a problem hiding this comment.
@charlesBochet I think the goal of ValidationPipe is to validate class-validator globally, whether it's from typeorm or yoga or any internal part that's using class-validator. Ideally we would want to check if we can make it work with useGraphQLErrorHandlerHook hook for yoga 🤔. I would still keep the ValidationType Globally (without your change) so we still catch those but probably also call it from the hook I mentioned above so we can throw a proper graphql ValidationError
packages/twenty-server/src/engine/core-modules/workspace/dtos/update-workspace-input.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/main.ts
Outdated
| @@ -54,6 +58,18 @@ const bootstrap = async () => { | |||
| app.useGlobalPipes( | |||
| new ValidationPipe({ | |||
| transform: true, | |||
| exceptionFactory: (validationErrors: ValidationErrorType[] = []) => { | |||
There was a problem hiding this comment.
@charlesBochet I think the goal of ValidationPipe is to validate class-validator globally, whether it's from typeorm or yoga or any internal part that's using class-validator. Ideally we would want to check if we can make it work with useGraphQLErrorHandlerHook hook for yoga 🤔. I would still keep the ValidationType Globally (without your change) so we still catch those but probably also call it from the hook I mentioned above so we can throw a proper graphql ValidationError
Removed custom NotIn validator and its tests. Updated references to use the built-in IsNotIn validator from class-validator for improved maintainability and standardization.
|
Thanks @AMoreaux for your contribution! |
Added new forbidden words and regex patterns to subdomain validation in
update-workspace-input. Enhanced theForbiddenWordsvalidator to support both strings and regex matching. Updated tests to verify regex-based forbidden subdomain validation.Fix #9064