refactor: improve host validation logic and expand forbidden hosts list#1825
Merged
Conversation
📝 WalkthroughWalkthroughThis PR refactors host security validation by extracting address-checking logic into a reusable helper, unifying DNS lookup pathways, expanding the forbidden network ranges to cover additional private and loopback addresses, and adding comprehensive test coverage for both the host check and connection validation flows. ChangesHost Security Validation Refactor
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens SaaS host validation by expanding the forbidden IP/CIDR list and refactoring host resolution checks (including multi-A/AAAA DNS results), while adding unit tests to cover the updated validation behavior.
Changes:
- Expanded
Constants.FORBIDDEN_HOSTSto include additional IPv4/IPv6 reserved, link-local, and loopback ranges. - Refactored
isHostAllowed()to resolve all DNS addresses and addedisForbiddenAddress()to centralize normalization + range checks. - Added AVA unit tests for
isHostAllowed()/isForbiddenAddress()andvalidateCreateConnectionData()(incl. host format validation outside test mode).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| backend/test/ava-tests/unit-tests/validate-create-connection-data.test.ts | Adds validation coverage for connection creation data, including host format checks when not in test mode. |
| backend/test/ava-tests/unit-tests/is-host-allowed.test.ts | Adds unit tests for forbidden address detection and SaaS host-allowance behavior (including SSH scenarios). |
| backend/src/helpers/constants/constants.ts | Expands forbidden host CIDR list to cover more internal/reserved ranges. |
| backend/src/entities/connection/utils/is-host-allowed.ts | Refactors host checking to use a single code path, multi-address DNS resolution, and adds isForbiddenAddress(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+26
to
+34
| const hostnameToCheck = connectionData.ssh ? connectionData.sshHost : connectionData.host; | ||
| if (!hostnameToCheck) { | ||
| return true; | ||
| } | ||
|
|
||
| const testHosts = Constants.getTestConnectionsHostNamesArr(); | ||
| if (testHosts.includes(connectionData.host ?? '')) { | ||
| return true; | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (7)
backend/test/ava-tests/unit-tests/validate-create-connection-data.test.ts (4)
23-25: ⚡ Quick winUse arrow function per coding guidelines.
The function declaration should be converted to an arrow function. As per coding guidelines, arrow functions are preferred over function declarations.
♻️ Proposed refactor
-function makeData(params: Record<string, unknown>): CreateConnectionDs { +const makeData = (params: Record<string, unknown>): CreateConnectionDs => { return { connection_parameters: params } as unknown as CreateConnectionDs; -} +};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/test/ava-tests/unit-tests/validate-create-connection-data.test.ts` around lines 23 - 25, Convert the function declaration makeData into an arrow function per coding guidelines: replace the function makeData(params: Record<string, unknown>): CreateConnectionDs { ... } implementation with a const makeData = (params: Record<string, unknown>): CreateConnectionDs => { ... } form, preserving the returned value ({ connection_parameters: params } as unknown as CreateConnectionDs) and any exports or usages of makeData in the tests so references remain valid.
37-40: ⚡ Quick winUse arrow function and consider safer type handling.
The function declaration should be converted to an arrow function per coding guidelines. Additionally, the type assertion
error as HttpExceptionis unsafe—consider adding a type guard to verify the error is actually anHttpExceptioninstance before callinggetResponse().♻️ Proposed refactor
-function thrownMessage(error: unknown): string { - const response = (error as HttpException).getResponse(); +const thrownMessage = (error: unknown): string => { + if (!(error instanceof HttpException)) { + return String(error); + } + const response = error.getResponse(); return typeof response === 'string' ? response : ((response as { message: string }).message ?? ''); -} +};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/test/ava-tests/unit-tests/validate-create-connection-data.test.ts` around lines 37 - 40, The helper thrownMessage should be converted to an arrow function and made resilient to non-HttpException values: add a type guard that verifies the error is an instance of HttpException (or has a callable getResponse method) before calling getResponse(), and return an empty string when the guard fails; keep the existing logic for extracting a string response or response.message when the guard passes. Ensure you reference thrownMessage, HttpException and getResponse in the change so the test behavior is preserved and no unsafe type assertions remain.
7-21: ⚖️ Poor tradeoffExtract duplicate env management utilities to a shared test helper.
The
setEnvhelper and the pattern of storing/restoringNODE_ENVandIS_SAASare duplicated in bothis-host-allowed.test.tsand this file. Consider extracting these to a shared test utility module (e.g.,backend/test/ava-tests/helpers/env-helpers.ts) to follow DRY principles.📦 Example shared helper structure
Create a new file
backend/test/ava-tests/helpers/env-helpers.ts:export const setEnv = (key: 'NODE_ENV' | 'IS_SAAS', value: string | undefined): void => { if (value === undefined) { delete process.env[key]; } else { process.env[key] = value; } }; export const createEnvRestorer = () => { const ORIGINAL_NODE_ENV = process.env.NODE_ENV; const ORIGINAL_IS_SAAS = process.env.IS_SAAS; return () => { setEnv('NODE_ENV', ORIGINAL_NODE_ENV); setEnv('IS_SAAS', ORIGINAL_IS_SAAS); }; };Then use it in both test files:
import { setEnv, createEnvRestorer } from '../helpers/env-helpers.js'; const restoreEnv = createEnvRestorer(); test.afterEach.always(restoreEnv);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/test/ava-tests/unit-tests/validate-create-connection-data.test.ts` around lines 7 - 21, The env setup and restore logic for NODE_ENV and IS_SAAS is duplicated across the affected test files, so extract it into a shared helper module and reuse it. Move the existing setEnv behavior plus the original-value capture/restoration into a helper such as env-helpers, then update validate-create-connection-data.test and is-host-allowed.test to import the shared utility and use a common restore callback with test.afterEach.always.
10-16: ⚡ Quick winUse arrow function and add explicit return type per coding guidelines.
The function declaration should be converted to an arrow function with an explicit return type annotation. As per coding guidelines, arrow functions are preferred over function declarations, and all TypeScript functions should have explicit return type annotations.
♻️ Proposed refactor
-function setEnv(key: 'NODE_ENV' | 'IS_SAAS', value: string | undefined): void { +const setEnv = (key: 'NODE_ENV' | 'IS_SAAS', value: string | undefined): void => { if (value === undefined) { delete process.env[key]; } else { process.env[key] = value; } -} +};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/test/ava-tests/unit-tests/validate-create-connection-data.test.ts` around lines 10 - 16, The helper setEnv should be converted from a function declaration to an arrow function and keep an explicit return type annotation. Update the local test utility in validate-create-connection-data.test.ts so setEnv follows the project’s preferred style, while preserving its current behavior for deleting or assigning process.env values.backend/test/ava-tests/unit-tests/is-host-allowed.test.ts (2)
15-18: ⚡ Quick winUse arrow function and add explicit return type per coding guidelines.
Similar to
setEnv, this helper should use an arrow function with an explicit return type annotation.♻️ Proposed refactor
-function enableSaaSHostCheck(): void { +const enableSaaSHostCheck = (): void => { process.env.NODE_ENV = 'development'; // isTest() => false process.env.IS_SAAS = '1'; // isSaaS() => true -} +};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/test/ava-tests/unit-tests/is-host-allowed.test.ts` around lines 15 - 18, The helper function enableSaaSHostCheck should be converted to an arrow function with an explicit return type; replace the current function declaration for enableSaaSHostCheck with a const arrow form (e.g., const enableSaaSHostCheck = (): void => { ... }) mirroring the style used by setEnv, keeping the same environment assignments (process.env.NODE_ENV and process.env.IS_SAAS) and return type void.
7-13: ⚡ Quick winUse arrow function and add explicit return type per coding guidelines.
The function declaration should be converted to an arrow function, and the return type should be explicitly annotated. As per coding guidelines, arrow functions are preferred over function declarations, and all TypeScript functions should have explicit return type annotations.
♻️ Proposed refactor
-function setEnv(key: 'NODE_ENV' | 'IS_SAAS', value: string | undefined): void { +const setEnv = (key: 'NODE_ENV' | 'IS_SAAS', value: string | undefined): void => { if (value === undefined) { delete process.env[key]; } else { process.env[key] = value; } -} +};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/test/ava-tests/unit-tests/is-host-allowed.test.ts` around lines 7 - 13, Convert the setEnv helper in is-host-allowed.test.ts from a function declaration to a const arrow function, and keep the explicit void return type annotation as required by the TypeScript style guide. Make sure the updated setEnv signature preserves the same key and value behavior while using the arrow-function form preferred alongside the existing test helpers in this file.backend/src/entities/connection/utils/is-host-allowed.ts (1)
26-34: ⚡ Quick winSecurity bypass when
ssh=truebutsshHostis missing — not reachable through current flows
validateCreateConnectionDatathrowsMessages.SSH_HOST_MISSINGwheneversshis enabled butsshHostis missing/empty, and the controller/use-cases call this validation beforeisHostAllowedruns (create/update/test/restore). Thereturn trueinisHostAllowedis therefore not reachable for SSH connections from these entrypoints; defensive hardening here would be optional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/entities/connection/utils/is-host-allowed.ts` around lines 26 - 34, The current isHostAllowed logic uses hostnameToCheck = connectionData.ssh ? connectionData.sshHost : connectionData.host and returns true if hostnameToCheck is falsy, which can allow a bypass when ssh is true but sshHost is missing; update isHostAllowed to deny (return false) when connectionData.ssh is true and hostnameToCheck is missing or empty, and change the test-hosts check to compare testHosts against hostnameToCheck (use Constants.getTestConnectionsHostNamesArr() and testHosts.includes(hostnameToCheck ?? '')). Ensure this behavior is implemented in the isHostAllowed function and reference Messages.SSH_HOST_MISSING handling remains consistent with validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@backend/src/entities/connection/utils/is-host-allowed.ts`:
- Around line 26-34: The current isHostAllowed logic uses hostnameToCheck =
connectionData.ssh ? connectionData.sshHost : connectionData.host and returns
true if hostnameToCheck is falsy, which can allow a bypass when ssh is true but
sshHost is missing; update isHostAllowed to deny (return false) when
connectionData.ssh is true and hostnameToCheck is missing or empty, and change
the test-hosts check to compare testHosts against hostnameToCheck (use
Constants.getTestConnectionsHostNamesArr() and
testHosts.includes(hostnameToCheck ?? '')). Ensure this behavior is implemented
in the isHostAllowed function and reference Messages.SSH_HOST_MISSING handling
remains consistent with validation.
In `@backend/test/ava-tests/unit-tests/is-host-allowed.test.ts`:
- Around line 15-18: The helper function enableSaaSHostCheck should be converted
to an arrow function with an explicit return type; replace the current function
declaration for enableSaaSHostCheck with a const arrow form (e.g., const
enableSaaSHostCheck = (): void => { ... }) mirroring the style used by setEnv,
keeping the same environment assignments (process.env.NODE_ENV and
process.env.IS_SAAS) and return type void.
- Around line 7-13: Convert the setEnv helper in is-host-allowed.test.ts from a
function declaration to a const arrow function, and keep the explicit void
return type annotation as required by the TypeScript style guide. Make sure the
updated setEnv signature preserves the same key and value behavior while using
the arrow-function form preferred alongside the existing test helpers in this
file.
In `@backend/test/ava-tests/unit-tests/validate-create-connection-data.test.ts`:
- Around line 23-25: Convert the function declaration makeData into an arrow
function per coding guidelines: replace the function makeData(params:
Record<string, unknown>): CreateConnectionDs { ... } implementation with a const
makeData = (params: Record<string, unknown>): CreateConnectionDs => { ... }
form, preserving the returned value ({ connection_parameters: params } as
unknown as CreateConnectionDs) and any exports or usages of makeData in the
tests so references remain valid.
- Around line 37-40: The helper thrownMessage should be converted to an arrow
function and made resilient to non-HttpException values: add a type guard that
verifies the error is an instance of HttpException (or has a callable
getResponse method) before calling getResponse(), and return an empty string
when the guard fails; keep the existing logic for extracting a string response
or response.message when the guard passes. Ensure you reference thrownMessage,
HttpException and getResponse in the change so the test behavior is preserved
and no unsafe type assertions remain.
- Around line 7-21: The env setup and restore logic for NODE_ENV and IS_SAAS is
duplicated across the affected test files, so extract it into a shared helper
module and reuse it. Move the existing setEnv behavior plus the original-value
capture/restoration into a helper such as env-helpers, then update
validate-create-connection-data.test and is-host-allowed.test to import the
shared utility and use a common restore callback with test.afterEach.always.
- Around line 10-16: The helper setEnv should be converted from a function
declaration to an arrow function and keep an explicit return type annotation.
Update the local test utility in validate-create-connection-data.test.ts so
setEnv follows the project’s preferred style, while preserving its current
behavior for deleting or assigning process.env values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0faa94f9-6d49-4d2a-8594-166005eecc75
📒 Files selected for processing (4)
backend/src/entities/connection/utils/is-host-allowed.tsbackend/src/helpers/constants/constants.tsbackend/test/ava-tests/unit-tests/is-host-allowed.test.tsbackend/test/ava-tests/unit-tests/validate-create-connection-data.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests