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

Fixed restrictive URL sanity check #6570 #6575

Merged
merged 2 commits into from
Aug 9, 2024

Conversation

AnanteshG
Copy link
Contributor

@AnanteshG AnanteshG commented Aug 7, 2024

Fixes #6570

Changes

  • Replaced the old regex with a new, more inclusive regex pattern.
  • Updated the isURL function to use the new pattern.

Changes
- Replaced the old regex with a new, more inclusive regex pattern.
- Updated the isURL function to use the new pattern.
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

The pull request updates the regex pattern in the isURL function to be more inclusive, enhancing URL validation robustness.

  • Regex Update: Enhanced regex pattern in packages/twenty-front/src/utils/is-url.ts for broader URL validation.
  • Function Integrity: Maintained isDefined check to ensure URL is neither null nor undefined before validation.
  • Testing Recommendation: Thorough testing is advised to confirm no valid URLs are rejected and no invalid URLs are accepted.

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Copy link

github-actions bot commented Aug 7, 2024

Welcome!

Hello there, congrats on your first PR! We're excited to have you contributing to this project.
By submitting your Pull Request, you acknowledge that you agree with the terms of our Contributor License Agreement.

Generated by 🚫 dangerJS against 3da8ef1

Copy link
Member

@Weiko Weiko left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. We decided to remove the check, at least for now, until we find a better check. Removing those restrictions would have made isUrl confusing imho because it would have been too broad. The next goal is to better handle failing webhooks

@Weiko Weiko merged commit 13d05d8 into twentyhq:main Aug 9, 2024
8 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Webhook URL sanity check is too restrictive
3 participants