add URL host validation for action URLs#1564
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds URL host validation for action URLs to prevent Server-Side Request Forgery (SSRF) attacks. The implementation validates that action URLs do not target forbidden internal IP ranges before allowing table action rules to be created or updated.
Changes:
- Added new validator function
isActionUrlHostAllowedthat checks URLs against forbidden IP ranges (private networks and localhost) - Integrated URL host validation into create and update action rule use cases for both URL and SLACK action methods
- Added new error message
ACTION_URL_HOST_NOT_ALLOWEDfor validation failures - Reformatted test file with consistent indentation (spaces to tabs)
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/src/helpers/validators/is-action-url-host-allowed.ts | New validator function that checks if action URL hosts are allowed by validating against forbidden IP ranges |
| backend/src/exceptions/text/messages.ts | Added new error message for action URL host validation failures |
| backend/src/entities/table-actions/table-action-rules-module/use-cases/create-action-rule.use.case.ts | Integrated URL host validation for URL and SLACK action methods in the create flow |
| backend/src/entities/table-actions/table-action-rules-module/use-cases/update-action-rule-with-actions-and-events.use.case.ts | Integrated URL host validation for URL and SLACK action methods in the update flow |
| backend/test/ava-tests/saas-tests/action-rules-e2e.test.ts | Reformatted with consistent indentation (spaces to tabs) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!isSlackUrlAllowed) { | ||
| throw new BadRequestException(Messages.ACTION_URL_HOST_NOT_ALLOWED); | ||
| } | ||
| } |
There was a problem hiding this comment.
The ZAPIER method appears to use a URL (based on test code showing it uses slack_url field) but is not validated. If ZAPIER actions also use URLs that could be exploited for SSRF attacks, they should be validated similarly to URL and SLACK methods. Please verify if ZAPIER actions need host validation and add the appropriate check if needed.
| } | |
| } | |
| if (action.action_method === TableActionMethodEnum.ZAPIER) { | |
| if (!action.action_slack_url) { | |
| throw new BadRequestException(Messages.SLACK_URL_MISSING); | |
| } | |
| if (process.env.NODE_ENV !== 'test' && !ValidationHelper.isValidUrl(action.action_slack_url)) { | |
| throw new BadRequestException(Messages.URL_INVALID); | |
| } | |
| const isZapierUrlAllowed = await isActionUrlHostAllowed(action.action_slack_url); | |
| if (!isZapierUrlAllowed) { | |
| throw new BadRequestException(Messages.ACTION_URL_HOST_NOT_ALLOWED); | |
| } | |
| } |
| if (!isSlackUrlAllowed) { | ||
| throw new BadRequestException(Messages.ACTION_URL_HOST_NOT_ALLOWED); | ||
| } | ||
| } |
There was a problem hiding this comment.
The ZAPIER method appears to use a URL (based on test code showing it uses slack_url field) but is not validated. If ZAPIER actions also use URLs that could be exploited for SSRF attacks, they should be validated similarly to URL and SLACK methods. Please verify if ZAPIER actions need host validation and add the appropriate check if needed.
| } | |
| } | |
| if (action.action_method === TableActionMethodEnum.ZAPIER) { | |
| if (!action.action_slack_url) { | |
| throw new BadRequestException(Messages.SLACK_URL_MISSING); | |
| } | |
| if (process.env.NODE_ENV !== 'test' && !ValidationHelper.isValidUrl(action.action_slack_url)) { | |
| throw new BadRequestException(Messages.URL_INVALID); | |
| } | |
| const isZapierUrlAllowed = await isActionUrlHostAllowed(action.action_slack_url); | |
| if (!isZapierUrlAllowed) { | |
| throw new BadRequestException(Messages.ACTION_URL_HOST_NOT_ALLOWED); | |
| } | |
| } |
| return await new Promise<boolean>((resolve) => { | ||
| dns.lookup(hostname, (err, address) => { | ||
| if (err) { | ||
| console.error('DNS lookup error for action URL:', err.message); | ||
| resolve(false); | ||
| return; | ||
| } | ||
|
|
||
| if (ipRangeCheck(address, Constants.FORBIDDEN_HOSTS)) { | ||
| resolve(false); | ||
| } else { | ||
| resolve(true); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The error handling in the DNS lookup differs from the existing implementation in is-host-allowed.ts. This implementation resolves to false on DNS errors (line 27), while is-host-allowed.ts rejects the promise on DNS errors (lines 26-27, 36-38). This inconsistency could lead to unexpected behavior. Consider aligning the error handling approach: either both should reject on DNS errors, or both should resolve to false. The current approach in isActionUrlHostAllowed seems more defensive (treating DNS errors as disallowed), but the inconsistency should be addressed for maintainability.
| if (ipRangeCheck(hostname, Constants.FORBIDDEN_HOSTS)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The hostname check using ipRangeCheck on line 19 might not work as expected for all cases. The ipRangeCheck function is designed to check IP addresses against CIDR ranges, but hostname might be a domain name string (e.g., "example.com") rather than an IP address. If hostname is a domain name, this check will likely not match the forbidden IP ranges correctly. Consider either removing this check (since the DNS lookup below will resolve the domain to an IP) or add a guard to only perform this check if hostname is already an IP address.
| export async function isActionUrlHostAllowed(url: string): Promise<boolean> { | ||
| if (process.env.NODE_ENV === 'test') { | ||
| return true; | ||
| } | ||
|
|
||
| if (!isSaaS()) { | ||
| return true; | ||
| } | ||
|
|
||
| try { | ||
| const parsedUrl = new URL(url); | ||
| const hostname = parsedUrl.hostname; | ||
|
|
||
| if (ipRangeCheck(hostname, Constants.FORBIDDEN_HOSTS)) { | ||
| return false; | ||
| } | ||
|
|
||
| return await new Promise<boolean>((resolve) => { | ||
| dns.lookup(hostname, (err, address) => { | ||
| if (err) { | ||
| console.error('DNS lookup error for action URL:', err.message); | ||
| resolve(false); | ||
| return; | ||
| } | ||
|
|
||
| if (ipRangeCheck(address, Constants.FORBIDDEN_HOSTS)) { | ||
| resolve(false); | ||
| } else { | ||
| resolve(true); | ||
| } | ||
| }); | ||
| }); | ||
| } catch (e) { | ||
| console.error('Invalid URL format for action URL validation:', e.message); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
The new URL host validation function isActionUrlHostAllowed lacks test coverage. This is a security-critical feature that prevents SSRF attacks, and it should have comprehensive unit tests covering: valid URLs, forbidden IP ranges (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, 127.0.0.0/8, fd00::/8), DNS resolution to forbidden IPs, invalid URL formats, DNS lookup errors, and edge cases. Consider adding dedicated unit tests for this validator, following the pattern established by other test files in the repository.
| }); | ||
| await Knex.schema.dropTableIfExists(tableName); | ||
| await Knex.destroy(); | ||
| const { host, username, password, database, port, type, ssl, cert } = newConnection; |
There was a problem hiding this comment.
Unused variable ssl.
| }); | ||
| await Knex.schema.dropTableIfExists(tableName); | ||
| await Knex.destroy(); | ||
| const { host, username, password, database, port, type, ssl, cert } = newConnection; |
There was a problem hiding this comment.
Unused variable cert.
| } | ||
| } | ||
| await Knex.destroy(); | ||
| const { host, username, password, database, port, type, ssl, cert } = newConnection; |
There was a problem hiding this comment.
Unused variable ssl.
| } | ||
| } | ||
| await Knex.destroy(); | ||
| const { host, username, password, database, port, type, ssl, cert } = newConnection; |
There was a problem hiding this comment.
Unused variable cert.
| .set('Content-Type', 'application/json') | ||
| .set('Accept', 'application/json'); | ||
|
|
||
| const createTableRuleRO: FoundActionRulesWithActionsAndEventsDTO = JSON.parse(createTableRuleResult.text); |
There was a problem hiding this comment.
Unused variable createTableRuleRO.
| const createTableRuleRO: FoundActionRulesWithActionsAndEventsDTO = JSON.parse(createTableRuleResult.text); |
1 participant
No description provided.