-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Corrige validação de URL para permitir localhost e endereços IP #1290
Conversation
Reviewer's Guide by SourceryThe pull request replaces the Sequence diagram for webhook URL validationsequenceDiagram
participant User
participant WebhookController
participant CustomRegex
User->>WebhookController: Sends webhook data with URL
WebhookController->>CustomRegex: Validates URL using regex
alt URL is valid
WebhookController->>WebhookController: Processes webhook
else URL is invalid
WebhookController-->>User: Returns error
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jrCleber - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider defining the regex in a single place and reusing it to avoid inconsistencies.
- The new regex only checks for
https?
at the beginning, which is less strict than the originalisURL
validation.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -18,7 +17,7 @@ export class WebhookController extends EventController implements EventControlle | |||
} | |||
|
|||
override async set(instanceName: string, data: EventDto): Promise<wa.LocalWebHook> { | |||
if (!isURL(data.webhook.url, { require_tld: false })) { | |||
if (!/^(https?:\/\/)/.test(data.webhook.url)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question (bug_risk): Review the new URL validation logic using regex.
The previous implementation used isURL with specific options, which may have been more comprehensive. This regex only checks for the presence of a protocol prefix, so please ensure that this simplified check meets all your validation requirements.
@@ -78,6 +77,7 @@ export class WebhookController extends EventController implements EventControlle | |||
const we = event.replace(/[.-]/gm, '_').toUpperCase(); | |||
const transformedWe = we.replace(/_/gm, '-').toLowerCase(); | |||
const enabledLog = configService.get<Log>('LOG').LEVEL.includes('WEBHOOKS'); | |||
const regex = /^(https?:\/\/)/; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Consolidate URL validation logic.
Consider declaring and using a shared, well-documented constant for the URL validation regex across all methods. This will help avoid inconsistent usage, as the set method uses a literal regex while other parts rely on the declared variable.
Suggested implementation:
/**
* Regex pattern for validating URLs starting with "http://" or "https://"
*/
const URL_VALIDATION_REGEX = /^(https?:\/\/)/;
// (Existing imports remain unchanged)
if (!URL_VALIDATION_REGEX.test(data.webhook.url)) {
// Removed local regex constant in favor of shared URL_VALIDATION_REGEX
if (instance?.enabled && URL_VALIDATION_REGEX.test(instance.url)) {
Make sure that the new constant declaration is positioned appropriately (e.g. at the top of the file after the imports) and that no other part of the file uses a hard-coded regex. Adjust the location if your project's conventions require constants to be declared in a separate file.
O isUrl do class-validator não valida URLs com localhost porque ele segue a especificação da WHATWG URL, que considera localhost um nome de host especial e pode não tratá-lo como um domínio válido.
Se você quiser uma validação mais rigorosa, que permita localhost, IPs e domínios válidos, pode usar esta regex aprimorada:
Este PR substitui a validação de URL que utilizava
isUrl
doclass-validator
por uma regex personalizada. A motivação para essa mudança é queisUrl
, mesmo com a opçãoprotocols: ['http', 'https']
, não validava URLs que usamlocalhost
.A nova regex implementada agora permite:
http
ehttps
localhost
com ou sem porta (http://localhost
,http://localhost:3000
)http://192.168.1.1
,https://10.0.0.1:8080
)https://example.com
,http://sub.example.org
)https://example.com/api
)Essa alteração garante maior flexibilidade na configuração de webhooks, permitindo URLs locais e ambientes de desenvolvimento sem comprometer a segurança ou funcionalidade.
Alterações principais:
isUrl
pela regex/^(https?:\/\/)(localhost|(\d{1,3}\.){3}\d{1,3}|([a-zA-Z0-9-]+\.)+[a-zA-Z]{2,})(:\d+)?(\/.*)?$/
Testes realizados:
✅ Testado com URLs locais (
http://localhost
,http://localhost:3000
)✅ Testado com IPs (
http://127.0.0.1
,https://192.168.0.1:8000
)✅ Testado com domínios (
https://example.com
,http://api.test.net
)✅ Testado com portas e caminhos opcionais
Impacto:
Esta mudança melhora a compatibilidade com ambientes locais e configurações personalizadas de webhook, garantindo uma validação mais robusta sem restringir casos de uso comuns
Summary by Sourcery
Replaces the isUrl validation with a custom regex to allow localhost and IP addresses in webhook URLs.
Bug Fixes: