Relax monitor URL validation #2632
Conversation
Yasir-Rafique
requested review from
ajhollid,
mohicody,
Br0wnHammer,
Owaiseimdad,
karenvicent and
shanikauwu1
as code owners
WalkthroughThe URL validation logic in the monitor creation form was replaced with a new approach that distinguishes URLs starting with "http://" or "https://" and validates them using Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Form
participant Validation
User->>Form: Enter URL in monitor form
Form->>Validation: Validate URL
alt URL starts with http:// or https://
Validation->>Validation: Use URL.canParse() to validate
alt Valid URL
Validation->>Form: Accept value
else
Validation->>Form: Return "string.invalidUrl" error
end
else URL without protocol
Validation->>Validation: Validate as Docker/internal hostname with label checks
alt Valid hostname
Validation->>Form: Accept value
else
Validation->>Form: Return "string.invalidUrl" error
end
end
Why did the Canadian developer apologize to the American developer after changing the validation logic? Because he didn’t want to cause any regex border issues, eh! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![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
Plan: Pro
📒 Files selected for processing (1)
client/src/Validation/validation.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ceskyDJ
PR: bluewave-labs/Checkmate#2410
File: client/src/Pages/Settings/SettingsAbout.jsx:23-25
Timestamp: 2025-06-08T14:56:37.007Z
Learning: User ceskyDJ appropriately manages PR scope by focusing on i18n changes and not addressing unrelated pre-existing technical debt in the same PR, demonstrating good software development practices.
🔇 Additional comments (1)
client/src/Validation/validation.js (1)
160-165: LGTM! The fallback validation logic is well-implemented.The fallback to the original complex regex and proper error handling maintains backward compatibility while supporting the new Docker container name validation. This two-step approach is like having both a friendly Canadian greeting and a formal American handshake - it covers all the bases!
client/src/Validation/validation.js
Outdated
| const noProtocol = value.replace(/^(https?:\/\/)/, ""); | ||
|
|
||
| if (/^[a-zA-Z0-9-]+(:\d{1,5})?$/.test(noProtocol)) { | ||
| return value; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix port range validation and improve hostname pattern.
The current simple validation has a couple of issues that need addressing:
-
Port range validation: The regex
:\d{1,5}allows ports up to 99999, but valid ports are 1-65535. Like a Canadian apologizing for American healthcare costs, this is way too generous! -
Hostname validation: The pattern doesn't prevent hostnames from starting/ending with hyphens or having consecutive hyphens, which violates RFC standards.
Apply this diff to fix the validation:
- if (/^[a-zA-Z0-9-]+(:\d{1,5})?$/.test(noProtocol)) {
+ if (/^[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])*(:(6553[0-5]|655[0-2]\d|65[0-4]\d{2}|6[0-4]\d{3}|[1-5]\d{4}|[1-9]\d{0,3}))?$/.test(noProtocol)) {
return value;
}This ensures:
- Hostnames don't start/end with hyphens
- Port numbers are within the valid range (1-65535)
- Maintains support for Docker container names
📝 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.
| const noProtocol = value.replace(/^(https?:\/\/)/, ""); | |
| if (/^[a-zA-Z0-9-]+(:\d{1,5})?$/.test(noProtocol)) { | |
| return value; | |
| } | |
| const noProtocol = value.replace(/^(https?:\/\/)/, ""); | |
| if (/^[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])*(:(6553[0-5]|655[0-2]\d|65[0-4]\d{2}|6[0-4]\d{3}|[1-5]\d{4}|[1-9]\d{0,3}))?$/.test(noProtocol)) { | |
| return value; | |
| } |
🤖 Prompt for AI Agents
In client/src/Validation/validation.js around lines 131 to 135, the current
regex for validating hostnames and ports is too permissive, allowing invalid
port numbers up to 99999 and hostnames starting or ending with hyphens or
containing consecutive hyphens. Update the regex to enforce that hostnames do
not start or end with hyphens and do not have consecutive hyphens, and refine
the port validation to only allow numbers from 1 to 65535. This will ensure
compliance with RFC standards and proper port range validation while still
supporting Docker container names.
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
Auto Pull Request Review from LlamaPReview
1. Overview
1.1 Core Changes
- Primary purpose and scope: Relax URL validation for monitor URLs to allow Docker internal hostnames (like
containername) in addition to FQDNs and IP addresses - Key components modified: Client-side validation logic in
client/src/Validation/validation.js - Cross-component impacts: Requires corresponding server-side validation updates for security consistency
- Business value alignment: Improves developer experience in local/Docker environments per user request (#2605)
1.2 Technical Architecture
- System design modifications: Validation logic now supports two distinct URL patterns
- Component interaction changes: Client validation no longer rejects Docker internal names
- Integration points impact: Monitor creation API needs matching validation rules
- Dependency changes and implications: Server must resolve Docker internal hostnames
2. Critical Findings
2.1 Must Fix (P0🔴)
Issue: Flawed regex for Docker hostnames blocks valid patterns like containername.localhost
- Analysis Confidence: High
- Impact: Fails to solve original issue #2605 as common Docker patterns remain invalid
- Resolution: Update regex to
^[a-zA-Z0-9.-]+(:\d{1,5})?$to allow dots in hostnames
Issue: Missing server-side validation for relaxed URL format
- Analysis Confidence: High
- Impact: Security vulnerability (SSRF risk) if client validation bypassed via direct API calls
- Resolution: Implement matching server-side validation with protocol/IP restrictions
Issue: No unit tests for new validation logic
- Analysis Confidence: High
- Impact: High risk of regressions and undetected edge case failures
- Resolution: Add comprehensive test cases covering Docker, FQDN, IP, and invalid URLs
2.2 Should Fix (P1🟡)
Issue: Generic error message for URL validation failures
- Analysis Confidence: Medium
- Impact: Poor UX when users can't distinguish between hostname/port/format issues
- Suggested Solution: Implement specific error messages for different failure types
2.3 Consider (P2🟢)
Area: Documentation of validation logic
- Analysis Confidence: Medium
- Improvement Opportunity: Better maintainability through comments explaining two-step validation approach
2.4 Summary of Action Items
- Immediately fix Docker hostname regex and add server-side validation (P0)
- Implement unit tests before merge (P0)
- Improve error messaging (P1 - can be post-merge)
- Add code comments (P2 - optional)
3. Technical Analysis
3.1 Code Logic Analysis
📁 client/src/Validation/validation.js - URL validation logic
- Submitted PR Code:
custom((value, helpers) => {
const noProtocol = value.replace(/^(https?:\/\/)/, "");
if (/^[a-zA-Z0-9-]+(:\d{1,5})?$/.test(noProtocol)) {
return value;
}
// Existing complex URL validation
if (urlRegex.test(value)) {
return value;
}
return helpers.error("string.invalidUrl");
})- Analysis:
- Current logic incorrectly blocks valid Docker hostnames with dots (e.g.,
service.network) - Error message doesn't help users diagnose specific validation failures
- Lack of tests makes regression likely during future changes
- Server-side validation missing creates security gap
- Current logic incorrectly blocks valid Docker hostnames with dots (e.g.,
- LlamaPReview Suggested Improvements:
custom((value, helpers) => {
// Strip protocol prefix if present
const noProtocol = value.replace(/^(https?:\/\/)/, "");
// Updated regex allows dots in hostnames (e.g., 'service.network:8080')
if (/^[a-zA-Z0-9.-]+(:\d{1,5})?$/.test(noProtocol)) {
return value;
}
// Existing complex URL validation as fallback
if (urlRegex.test(value)) {
return value;
}
// Provide more specific error (implementation details in 2.2)
return helpers.error("string.invalidUrl");
})- Improvement rationale:
- Allows Docker-compatible hostnames with dots per original requirement
- Maintains backward compatibility with existing URL formats
- Reduces false negatives while preserving security
3.2 Key Quality Aspects
- Testing strategy and coverage: Must add unit tests covering:
- Valid:
containername,service.network,192.168.1.1:8080,https://example.com - Invalid:
http:///invalid,bad$hostname,:portonly
- Valid:
- Security considerations: Server must validate URLs before use to prevent SSRF attacks
4. Overall Evaluation
- Technical assessment: Solution partially addresses requirement but introduces security gaps and incomplete validation
- Business impact: Critical for Docker-based deployments once validation issues resolved
- Risk evaluation: High risk in current state (security + incomplete solution)
- Notable positive aspects: Good two-step validation approach, clean implementation
- Implementation quality: Requires improvements to validation logic and testing
- Final recommendation: Request Changes - Must fix regex, add server validation, and implement tests
💡 LlamaPReview Community
Have feedback on this AI Code review tool? Join our GitHub Discussions to share your thoughts and help shape the future of LlamaPReview.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/validation/joi.js (1)
171-216: Great implementation of relaxed URL validation!The two-step validation approach effectively addresses the PR objective of allowing Docker container names while maintaining backward compatibility. The logic is sound and the custom error messages provide clear guidance to users.
However, there are a few areas for improvement:
Simple pattern could be more restrictive: The current pattern
/^[a-zA-Z0-9.-]+(:\d{1,5})?$/allows consecutive dots (e.g.,container..name), which aren't valid in hostnames.Complex regex maintainability: That regex is longer than a Canadian winter! Consider extracting it to a constant or using a well-tested URL validation library.
Consider tightening the simple pattern:
- if (/^[a-zA-Z0-9.-]+(:\d{1,5})?$/.test(noProtocol)) { + if (/^[a-zA-Z0-9]([a-zA-Z0-9.-]*[a-zA-Z0-9])?(:\d{1,5})?$/.test(noProtocol)) {This prevents consecutive dots and ensures hostnames start/end with alphanumeric characters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/src/Validation/validation.js(1 hunks)server/validation/joi.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/src/Validation/validation.js
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ceskyDJ
PR: bluewave-labs/Checkmate#2410
File: client/src/Pages/Settings/SettingsAbout.jsx:23-25
Timestamp: 2025-06-08T14:56:37.007Z
Learning: User ceskyDJ appropriately manages PR scope by focusing on i18n changes and not addressing unrelated pre-existing technical debt in the same PR, demonstrating good software development practices.
There was a problem hiding this comment.
I think this is a bit to permissive, this will allow many invalid URLs to be entered.
Why don't we consider using URL.canParse to validate here insteaed of regex?
Using that as a first pass with some regex for things like protocol after that should be sufficient.
|
I’ve updated the validation to use URL.canParse() as the primary check for all standard URLs (as suggested), and a strict, label-wise regex for Docker/internal hostnames (including checks for consecutive dots and leading/trailing dashes or empty labels). |
|
@Yasir-Rafique this validation is disallowing URLs like |
Describe your changes
Relaxed the monitor URL validation logic to allow Docker internal hostnames (such as containername) in addition to fully qualified domains and IP addresses. This makes it possible to monitor Docker containers by name in local/dev environments, as requested.
Write your issue number after "Fixes "
Fixes #2605
Please ensure all items are checked off before requesting a review. "Checked off" means you need to add an "x" character between brackets so they turn into checkmarks.
<div>Add</div>, use):npm run formatin server and client directories, which automatically formats your code.Summary by CodeRabbit