fix: webhook regex url with underscores #1755
Conversation
sanpj2292
requested review from
lvrach,
cisse21,
debanjan97,
am6010,
ssbeefeater,
AchuthaSourabhC,
ruchiramoitra,
ItsSudip,
shrouti1507,
sandeepdsvs,
krishna2020 and
1abhishekpandey
as code owners
WalkthroughThe changes in this pull request involve updates to the JSON schema and configuration files related to webhook settings and validation rules. Specifically, the regex pattern for the Changes
Possibly related PRs
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1755 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 53 53
Branches 7 7
=========================================
Hits 53 53 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/configurations/destinations/webhook/schema.json (1 hunks)
- src/configurations/destinations/webhook/ui-config.json (1 hunks)
- test/data/validation/destinations/webhook.json (34 hunks)
🧰 Additional context used
🔇 Additional comments (10)
src/configurations/destinations/webhook/schema.json (1)
520-520: Approved: URL pattern updated to allow underscores in domain namesThe change successfully addresses the PR objective by allowing underscores in the domain name part of the webhook URL pattern. This modification enhances flexibility for users setting up webhooks.
However, it's worth noting that while underscores in domain names are technically allowed in some systems, they are not compliant with the original DNS standards (RFC 1034). Many modern systems do support them, but it's important to ensure this change doesn't introduce any compatibility issues with other parts of the system or external services.
To ensure this change doesn't have unintended consequences, please run the following verification:
Please review the output to ensure that all URL-related code in the system is compatible with this new pattern.
test/data/validation/destinations/webhook.json (9)
44-44: LGTM: Updated regex pattern allows underscores in domain namesThe regex pattern for
webhookUrlhas been updated to include underscores ([a-zA-Z0-9-_]). This change aligns with the PR objective of allowing URLs with underscores and is consistently applied across multiple test cases.
68-68: Consistent application of updated regex patternThe updated regex pattern allowing underscores in domain names is consistently applied across different test cases, including this one for an ngrok.io URL. This consistency ensures uniform enforcement of the new rule.
92-92: Security measure maintained while allowing underscoresThe updated regex pattern is consistently applied here as well. It's important to note that while the pattern now allows underscores in domain names, it still correctly blocks localhost URLs, maintaining essential security measures.
116-116: Security against IP address URLs maintainedThe updated regex pattern is consistently applied here as well. It's noteworthy that while allowing underscores in domain names, the pattern still correctly blocks direct IP address URLs, maintaining good security practices.
140-140: Security against potentially vulnerable URLs maintainedThe updated regex pattern continues to be consistently applied. It's crucial to note that while allowing underscores in domain names, the pattern still correctly blocks potentially vulnerable URLs like 0.0.0.0, maintaining strong security measures.
164-164: Consistent security maintained for IPv6 URLsThe updated regex pattern is applied consistently here as well. It's important to highlight that while allowing underscores in domain names, the pattern continues to correctly block IPv6 URLs, ensuring consistent security measures across different IP formats.
188-188: Security against mixed IP format URLs maintainedThe updated regex pattern continues to be consistently applied. It's noteworthy that while allowing underscores in domain names, the pattern still correctly blocks mixed IPv6-IPv4 URLs, effectively preventing potential bypass attempts and maintaining robust security measures.
Line range hint
672-972: Robust security against SSRF attempts maintainedThe updated regex pattern is consistently applied across multiple SSRF (Server-Side Request Forgery) test cases. It's crucial to note that while allowing underscores in domain names, the pattern continues to correctly block various SSRF attempt URLs, including:
- Localhost with port
- IPv4 address with port
- Internal IP addresses
- Private IP addresses
- IPv6 loopback
- URL encoding bypass attempts
- Decimal and octal IP representations
- IPv6 mapped IPv4 addresses
- Hexadecimal IP representations
- Mixed encodings
- Punycode domains
- URLs with credentials
- Data URL schemes
- Localhost with subdomains
- Dotless decimal and hex IPs
- Malformed IPv6 addresses
- Exotic protocols
- URL fragments
This consistent blocking of potential SSRF vectors demonstrates that the security measures have been robustly maintained while accommodating the new requirement for underscores in domain names.
975-981: New test case aligns with PR objectiveThe addition of this new test case, "should not throw error for valid url", directly addresses the PR objective of allowing URLs with underscores. It checks that the URL "http://e2e_some.requestcatcher.com" is considered valid, which is the expected behavior with the updated regex pattern.
This test case serves as a positive validation, ensuring that the changes made to allow underscores in domain names are working as intended. It complements the existing test cases that check for various invalid URL patterns.
| @@ -9,7 +9,7 @@ | |||
| "value": "webhookUrl", | |||
| "required": true, | |||
| "placeholder": "http://www.abcd.com", | |||
| "regex": "^(https?:\\/\\/)(?![a-zA-Z0-9-]*\\.ngrok\\.io)(?!localhost|.*\\.localhost)([a-zA-Z0-9-]{1,63}\\.)+[a-zA-Z]{2,}(\\/.*)?$" | |||
| "regex": "^(https?:\\/\\/)(?![a-zA-Z0-9-]*\\.ngrok\\.io)(?!localhost|.*\\.localhost)([a-zA-Z0-9-_]{1,63}\\.)+[a-zA-Z]{2,}(\\/.*)?$" | |||
There was a problem hiding this comment.
💡 Codebase verification
Issue Identified: Regex Fails to Exclude Base Domain "ngrok.io"
The current regex incorrectly allows the base domain http://ngrok.io, which should be excluded as per the negative lookahead. This occurs because the pattern (?![a-zA-Z0-9-]*\.ngrok\.io) permits zero characters before .ngrok.io.
Recommended Actions:
- Update Regex: Modify the negative lookahead to
(?![a-zA-Z0-9-]+\.)ngrok\.ioto ensure that at least one character precedes.ngrok.io, effectively excluding the base domain. - Re-run Tests: Execute the updated shell script to verify that
http://ngrok.iois correctly excluded.
🔗 Analysis chain
LGTM: Regex updated to allow underscores in domain names.
The change successfully addresses the PR objective by allowing underscores in the domain part of the webhook URL. This modification maintains all existing validations while expanding the range of accepted URLs.
While this change improves flexibility, it's worth noting that technically, according to RFC 1034, underscores are not allowed in domain names. However, they are widely used and accepted in practice. To ensure compatibility, you may want to verify that all systems interacting with these URLs can handle underscores in domain names without issues.
To verify the regex change, you can use the following script:
This script will test the updated regex against various valid and invalid URLs to ensure it behaves as expected.
[/run_scripts]
[/run_scripts]
[/run_scripts]
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Test cases
valid_urls=(
"http://example.com"
"https://sub_domain.example.com"
"http://sub-domain.example_domain.com/path"
)
invalid_urls=(
"http://localhost"
"https://example.localhost"
"http://ngrok.io"
"ftp://example.com"
)
# Function to test regex
test_regex() {
local regex="$1"
local url="$2"
if echo "$url" | grep -qP "$regex"; then
echo "PASS: $url"
else
echo "FAIL: $url"
fi
}
# Extract regex from file
regex=$(grep -oP '(?<="regex": ").*(?=")' src/configurations/destinations/webhook/ui-config.json)
echo "Testing valid URLs:"
for url in "${valid_urls[@]}"; do
test_regex "$regex" "$url"
done
echo -e "\nTesting invalid URLs:"
for url in "${invalid_urls[@]}"; do
test_regex "$regex" "$url"
done
Length of output: 9882
Script:
#!/bin/bash
# Test cases
valid_urls=(
"http://example.com"
"https://sub_domain.example.com"
"http://sub-domain.example_domain.com/path"
)
invalid_urls=(
"http://localhost"
"https://example.localhost"
"http://ngrok.io"
"ftp://example.com"
)
# Function to test regex
test_regex() {
local regex="$1"
local url="$2"
if echo "$url" | grep -P "$regex" >/dev/null 2>&1; then
echo "PASS: $url"
else
echo "FAIL: $url"
fi
}
# Extract regex from file using jq for accurate parsing
regex=$(jq -r '.regex' src/configurations/destinations/webhook/ui-config.json)
echo "Extracted Regex: $regex"
echo "Testing valid URLs:"
for url in "${valid_urls[@]}"; do
test_regex "$regex" "$url"
done
echo -e "\nTesting invalid URLs:"
for url in "${invalid_urls[@]}"; do
test_regex "$regex" "$url"
done
Length of output: 2180
Script:
#!/bin/bash
# Display the entire JSON content for manual inspection
echo "Full JSON Content:"
cat src/configurations/destinations/webhook/ui-config.json
echo -e "\nListing top-level keys:"
jq 'keys' src/configurations/destinations/webhook/ui-config.json
echo -e "\nSearching for 'regex' key at any depth:"
jq '.. | .regex? // empty' src/configurations/destinations/webhook/ui-config.json
Length of output: 6740
Script:
#!/bin/bash
# Test cases
valid_urls=(
"http://example.com"
"https://sub_domain.example.com"
"http://sub-domain.example_domain.com/path"
)
invalid_urls=(
"http://localhost"
"https://example.localhost"
"http://ngrok.io"
"ftp://example.com"
)
# Function to test regex
test_regex() {
local regex="$1"
local url="$2"
if echo "$url" | grep -P "$regex" >/dev/null 2>&1; then
echo "PASS: $url"
else
echo "FAIL: $url"
fi
}
# Extract regex for "Webhook URL" field using jq
regex=$(jq -r '.uiConfig[].fields[] | select(.label == "Webhook URL").regex' src/configurations/destinations/webhook/ui-config.json)
echo "Extracted Regex for 'Webhook URL': $regex"
echo "Testing valid URLs:"
for url in "${valid_urls[@]}"; do
test_regex "$regex" "$url"
done
echo -e "\nTesting invalid URLs:"
for url in "${invalid_urls[@]}"; do
test_regex "$regex" "$url"
done
Length of output: 4938
| @@ -517,7 +517,7 @@ | |||
| }, | |||
| "webhookUrl": { | |||
| "type": "string", | |||
| "pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(https?:\\/\\/)(?![a-zA-Z0-9-]*\\.ngrok\\.io)(?!localhost|.*\\.localhost)([a-zA-Z0-9-]{1,63}\\.)+[a-zA-Z]{2,}(\\/.*)?$" | |||
| "pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(https?:\\/\\/)(?![a-zA-Z0-9-]*\\.ngrok\\.io)(?!localhost|.*\\.localhost)([a-zA-Z0-9-_]{1,63}\\.)+[a-zA-Z]{2,}(\\/.*)?$" | |||
There was a problem hiding this comment.
_ is valid only for subdomains, not for domains
|
Deemed unnecessary |
What are the changes introduced in this PR?
Issue: When URL contains
_, the regex is not matching even for a valid URL.Fix:
_is being included in the match part of the regexWhat is the related Linear task?
Resolves INT-2790
Please explain the objectives of your changes below
Put down any required details on the broader aspect of your changes. If there are any dependent changes, mandatorily mention them here
Any changes to existing capabilities/behaviour, mention the reason & what are the changes ?
N/A
Any new dependencies introduced with this change?
N/A
Any new checks got introduced or modified in test suites. Please explain the changes.
N/A
Developer checklist
My code follows the style guidelines of this project
No breaking changes are being introduced.
All related docs linked with the PR?
All changes manually tested?
Any documentation changes needed with this change?
I have executed schemaGenerator tests and updated schema if needed
Are sensitive fields marked as secret in definition config?
My test cases and placeholders use only masked/sample values for sensitive fields
Is the PR limited to 10 file changes & one task?
Reviewer checklist
Is the type of change in the PR title appropriate as per the changes?
Verified that there are no credentials or confidential data exposed with the changes.
Summary by CodeRabbit
New Features
Bug Fixes
Tests