Backend pg proxy#1719
Conversation
…nnection parameters
- Updated the `createProxyConnectionDto` function to generate unique usernames for each test, allowing for isolated connection pools. - Implemented `expectedCompanyId` and `expectedConnectionId` functions to derive company and connection IDs based on the generated usernames. - Modified the mock API server to handle dynamic usernames and return appropriate connection and company IDs. - Improved usage reporting tests to ensure metrics are accurately reported after queries through the proxy.
… with query object
Artuomka
requested review from
Copilot and
gugu
and removed request for
Copilot
📝 WalkthroughWalkthroughAdds a new POST endpoint to fetch hosted connection credentials, introducing DTO/RO types, a request-scoped use case and interface, controller wiring and module provider registration, updates to E2E tests and test infra (Postgres proxy + mock API), and a small knex configuration change. Changes
Sequence DiagramsequenceDiagram
autonumber
participant Client as Client
participant Controller as SaasController
participant UseCase as GetHostedConnectionCredentialsUseCase
participant Repo as ConnectionRepository
participant DB as GlobalDBContext
Client->>Controller: POST /connection/hosted/credentials\n{ hostedDatabaseId }
Controller->>UseCase: execute(GetHostedConnectionCredentialsDto)
UseCase->>Repo: findOne({ id: hostedDatabaseId })
Repo->>DB: query connection by id
DB-->>Repo: connection record | null
alt connection found
Repo-->>UseCase: connection entity
UseCase->>UseCase: map to HostedConnectionCredentialsRO
UseCase-->>Controller: HostedConnectionCredentialsRO
Controller-->>Client: 200 OK (credentials)
else not found
Repo-->>UseCase: null
UseCase-->>Controller: NotFoundException
Controller-->>Client: 404 Not Found
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (3 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // predict which companyId/connectionId the proxy will see for a username. | ||
| function expectedCompanyId(username: string): string { | ||
| if (username === 'proxy_user') return 'test-company-001'; | ||
| return `test-company-${createHash('sha1').update(username).digest('hex').slice(0, 12)}`; |
|
|
||
| function expectedConnectionId(username: string): string { | ||
| if (username === 'proxy_user') return 'test-connection-001'; | ||
| return `test-connection-${createHash('sha1').update(username).digest('hex').slice(0, 12)}`; |
| if (username === PROXY_USERNAME_PREFIX) { | ||
| return 'test-company-001'; | ||
| } | ||
| const hash = crypto.createHash('sha1').update(username).digest('hex').slice(0, 12); |
| if (username === PROXY_USERNAME_PREFIX) { | ||
| return 'test-connection-001'; | ||
| } | ||
| const hash = crypto.createHash('sha1').update(username).digest('hex').slice(0, 12); |
| ); | ||
| } else { | ||
| console.log( | ||
| `[mock-api] -> 404: username/database mismatch (expected ${PROXY_USERNAME_PREFIX}[_*]/${PROXY_DATABASE})`, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (9)
backend/src/microservices/saas-microservice/data-structures/hosted-connection-credentials.ro.ts (1)
3-24: Inconsistent field naming:is_frozenvsconnectionId.This RO mixes camelCase (
connectionId) with snake_case (is_frozen). Consumers of a single response object typically expect one convention; considerisFrozenfor consistency with the rest of the fields (and with common JSON/TS style in this codebase).✏️ Proposed change
- `@ApiProperty`() - is_frozen: boolean; + `@ApiProperty`() + isFrozen: boolean;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/microservices/saas-microservice/data-structures/hosted-connection-credentials.ro.ts` around lines 3 - 24, The response object HostedConnectionCredentialsRO mixes camelCase and snake_case (e.g., connectionId vs is_frozen); rename the property is_frozen to isFrozen on the HostedConnectionCredentialsRO class and update its `@ApiProperty`() declaration to use the new name, then find and update all references/usages/mappings/serializers, DTOs, tests and any database-to-RO transformation code that read or write the old is_frozen key so they now use isFrozen to keep naming consistent across the response object.docker-compose.tst.yml (1)
35-36: Backend now hard-depends onpostgres-proxybeing healthy.Adding
postgres-proxy: service_healthyto the backend'sdepends_onmeans the entire E2E suite won't start if the proxy build fails or its healthcheck misbehaves — even for tests unrelated to the proxy. If that's acceptable (single suite, single compose), fine; otherwise consider a dedicated compose profile for the proxy E2E so regular E2E runs aren't gated on it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.tst.yml` around lines 35 - 36, The backend service now hard-depends on postgres-proxy being healthy via the depends_on condition "service_healthy", which gates all E2E runs on the proxy; remove the "condition: service_healthy" from the backend's depends_on or move postgres-proxy into a separate docker-compose profile (e.g., profile: "proxy-e2e") and update backend to only depend on the proxy in that profile so regular E2E runs aren't blocked; reference the backend service's depends_on entry and the "postgres-proxy" service/healthcheck when making the change.docker-compose.yml (1)
265-265: Centralize the shared proxy API key.
PROXY_API_KEY: test-proxy-api-keyis duplicated onproxy-mock-api(Line 265) andpostgres-proxy(Line 284). If they ever drift, the proxy's outbound calls will 401 against the mock API and tests will fail opaquely. Consider a YAML anchor or shared.envvar (e.g.${E2E_PROXY_API_KEY}) so the two values can't diverge.Also applies to: 284-284
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` at line 265, The duplicated hard-coded PROXY_API_KEY value should be centralized so the two services (proxy-mock-api and postgres-proxy) cannot drift; replace the literal PROXY_API_KEY: test-proxy-api-key in both service env sections by referencing a single source (either define an env var like ${E2E_PROXY_API_KEY} and use that for both services, or create a YAML anchor/alias for the key and reference the anchor in both places), and ensure the new shared variable/anchor is documented in the compose file or .env so tests use the same value.backend/src/microservices/saas-microservice/saas.controller.ts (1)
353-364: Endpoint wiring looks correct.Constructor injection, DTO binding, and route shape follow the conventions of the other hosted-connection endpoints in this controller. The real behavior (password handling, authorization) lives in the use case; concerns there are raised on
get-hosted-connection-credentials.use.case.ts.One minor note:
@ApiResponsestatus is200but NestJS defaults@Postto201 Created. If the intent is200, add@HttpCode(200); otherwise set the Swagger status to201to match the runtime response (the sibling endpoints in this file consistently document201for@Post).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/microservices/saas-microservice/saas.controller.ts` around lines 353 - 364, The Swagger response status for the POST handler getHostedConnectionCredentials (method getHostedConnectionCredentials using GetHostedConnectionCredentialsDto and returning HostedConnectionCredentialsRO) is inconsistent with Nest's POST default; either add `@HttpCode`(200) to the getHostedConnectionCredentials method to keep the documented 200 status, or change the `@ApiResponse` status to 201 so Swagger matches the runtime POST behavior—update the decorator accordingly to make the doc and runtime consistent.backend/test/ava-tests/saas-tests/hosted-connection-e2e.test.ts (1)
94-94: Remove debugconsole.logstatements before merge.
🚀 ~ createdConnection:/🚀 ~ responseBody:look like leftover debugging aids. They add noise to CI logs and, for the credentials work being introduced in this PR, risk leaking sensitive fields (passwords, tokens) into build output if the response shape ever grows.♻️ Proposed cleanup
- const createdConnection = JSON.parse(createHostedConnectionResult.text); - console.log('🚀 ~ createdConnection:', createdConnection); - - t.is(createHostedConnectionResult.status, 201); + t.is(createHostedConnectionResult.status, 201); + const createdConnection = JSON.parse(createHostedConnectionResult.text);- const responseBody = JSON.parse(result.text); - console.log('🚀 ~ responseBody:', responseBody); t.is(result.status, 500);- const createdConnection = JSON.parse(createResult.text); - console.log('🚀 ~ createdConnection:', createdConnection); - t.is(createResult.status, 201); + t.is(createResult.status, 201); + const createdConnection = JSON.parse(createResult.text);Also applies to: 185-185, 230-230
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/test/ava-tests/saas-tests/hosted-connection-e2e.test.ts` at line 94, Remove the leftover debug console.log statements that print request/response objects to CI (they may leak sensitive data); search for and delete the console.log calls that reference createdConnection and responseBody in the hosted-connection-e2e.test.ts tests (and any other console.log debug prints in the same file) so tests only use assertions and structured test logging instead of dumping full objects.backend/test/ava-tests/saas-tests/saas-postgres-proxy-e2e.test.ts (3)
30-33: Defaults target docker-compose network; won't work for local runs.
PROXY_HOSTandMOCK_API_HOSTdefault topostgres-proxy/proxy-mock-api, which only resolve inside the compose network. A developer runningyarn teston their host machine without these env vars will just hit the TCP probe timeout and see every test silently skip (see earlier comment about silent skips). A README note or a sensiblelocalhostfallback when not running inside the compose network would help onboarding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/test/ava-tests/saas-tests/saas-postgres-proxy-e2e.test.ts` around lines 30 - 33, The test defaults use docker-compose hostnames (PROXY_HOST and MOCK_API_HOST), which fail for local runs; change the defaults in backend/test/ava-tests/saas-tests/saas-postgres-proxy-e2e.test.ts to fall back to 'localhost' when the corresponding env var is not set (keep parseInt logic for PROXY_PORT and MOCK_API_PORT intact), and optionally add a short README note about running tests inside compose versus locally so developers know to override these env vars when needed.
567-623: Budget-exhaustion test: tighten the regex and bound the loop.
/budget|exceeded|plan|53400/iwill match words like "plan" that appear in plenty of unrelated error bodies (e.g. "explain plan", "connection plan", generic NestJS messages mentioning "plan"). Prefer a tighter anchor that specifically ties the assertion to the proxy's 53400 rejection, e.g./53400|budget\s+exceeded|plan\s+limit/i.Also, the 30-call loop will always run to completion when the budget check fails to trigger, adding significant time to CI. Consider
t.timeout(60000)(already set) plus an explicit assertion that the loop terminated beforei === 29, so a silent regression (budget never exhausts) surfaces as a clear failure rather than a post-looprejected === falsewith no diagnostics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/test/ava-tests/saas-tests/saas-postgres-proxy-e2e.test.ts` around lines 567 - 623, The test 'should reject queries once query-time budget is exhausted' uses a too-broad regex and an unbounded 30-iteration loop; tighten the rejection check to a more specific pattern (e.g. /53400|budget\s+exceeded|plan\s+limit/i) and add an explicit assertion that the loop exited early (budget hit) rather than completing all 30 iterations—use the existing loop index or a separate attempts counter (from the for loop variable i or a new attempts variable) and assert it is < 30 (or i < 29) when rejected is true so a silent regression (budget never exhausts) fails fast. Ensure you update the regex reference where lastBody is tested and add the iteration-bound assertion after the loop using lastStatus/ rejected/ i.
302-312: Remove redundantJSON.stringifyaround the request body for consistency.Supertest passes plain objects directly to the body parser when
Content-Type: application/jsonis set. Explicitly wrapping inJSON.stringify()causes the backend to receive a string instead of an object. Other E2E tests in this file (/connection,/saas/connection/hosted) pass plain objects; keep this consistent.♻️ Proposed diff
- .send( - JSON.stringify({ - [testTableColumnName]: newName, - [testTableSecondColumnName]: newEmail, - }), - ) + .send({ + [testTableColumnName]: newName, + [testTableSecondColumnName]: newEmail, + })(same for the update-row call at Lines 361‑366)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/test/ava-tests/saas-tests/saas-postgres-proxy-e2e.test.ts` around lines 302 - 312, The POST to add a row is serializing the body with JSON.stringify which causes the backend to receive a string; update the request in the addRow call (the request built with request(app.getHttpServer()).post(`/table/row/${createConnectionRO.id}?tableName=${testTableName}`) that assigns to addRowResponse) to pass a plain object literal instead of JSON.stringify(...) and do the same for the corresponding update-row request later (the update-row call around lines 361‑366), leaving headers (.set('Content-Type','application/json') and .set('Accept','application/json')) unchanged so supertest/body-parser handle JSON correctly.test/proxy-mock-api/server.js (1)
33-46:TEST_CONNECTION.connectionId/companyIdare dead defaults.Every response path (Lines 112‑119) overrides
connectionIdandcompanyIdviaderiveConnectionId/deriveCompanyId, andcurrentSubscriptionLevelis re-initialized fromTEST_CONNECTION.subscriptionLevelanyway. The'test-connection-001'/'test-company-001'fields onTEST_CONNECTIONare never observed by clients and just duplicate the legacy-case logic insidederiveXxx. Consider dropping them fromTEST_CONNECTIONto make it obvious that the response's IDs always come from the derive functions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/proxy-mock-api/server.js` around lines 33 - 46, Remove the unused dead-default fields connectionId and companyId from the TEST_CONNECTION const and keep only the real defaults (host, port, database, username, password, subscriptionLevel); leave currentSubscriptionLevel initialization as-is using TEST_CONNECTION.subscriptionLevel and ensure all response code continues to use deriveConnectionId and deriveCompanyId (these are the actual sources of IDs). Update any nearby comments to reflect that connectionId/companyId are derived at runtime and no other code should be reading TEST_CONNECTION.connectionId or TEST_CONNECTION.companyId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend/src/microservices/saas-microservice/data-structures/hosted-connection-credentials.ro.ts`:
- Around line 19-20: The password property on HostedConnectionCredentials (the
password field currently annotated only with `@ApiProperty`()) must be either
redacted from serialization or explicitly documented: add the `@Exclude`()
decorator to the password property and ensure the class is serialized via
ClassSerializerInterceptor, or add a clear code comment on the
HostedConnectionCredentials class/property explaining why plaintext password is
exposed and list required safeguards (no caching, response/log redaction,
error-tracking filtering). Update related serialization configuration to
guarantee `@Exclude`() is honored if you choose that approach.
In
`@backend/src/microservices/saas-microservice/use-cases/get-hosted-connection-credentials.use.case.ts`:
- Around line 22-28: The lookup in implementation
(get-hosted-connection-credentials.use.case.ts -> protected async
implementation) uses connectionRepository.findOne({ where: { id:
inputData.hostedDatabaseId } }) without scoping to tenant/company or hosted-only
connections; update the code and DTO to prevent cross-tenant exposure by either
(A) adding companyId to GetHostedConnectionCredentialsDto and changing the query
to connectionRepository.findOne({ where: { id: inputData.hostedDatabaseId,
company: { id: inputData.companyId } } }) with validation that the caller's
company matches, or (B) at minimum filter for hosted connections only by using
connectionRepository.findOne({ where: { id: inputData.hostedDatabaseId, hosted:
true } }) so only system-hosted DBs can be returned; apply the chosen change in
the implementation, adjust any DTO/validator usages, and keep error handling
(NotFoundException) the same if no record matches.
- Around line 22-39: The implementation of GetHostedConnectionCredentialsUseCase
returns the encrypted password; fetch the connection via
_dbContext.connectionRepository.findOne (as already done), then call
decryptConnectionCredentialsAsync(connection) (the same helper used by
UpdateHostedConnectionPasswordUseCase) before constructing and returning the
HostedConnectionCredentialsRO so the password is decrypted; add the appropriate
import for decryptConnectionCredentialsAsync from
entities/connection/utils/decrypt-connection-credentials-async.js and keep the
existing NotFoundException(Messages.CONNECTION_NOT_FOUND) check.
In `@backend/test/ava-tests/saas-tests/hosted-connection-e2e.test.ts`:
- Around line 184-186: The test in hosted-connection-e2e.test.ts is asserting a
500 for an unknown/missing userId (result.status === 500), which should be a
4xx; update the test to expect the correct client error (preferably 404 to match
how get-hosted-connection-credentials.use.case.ts throws NotFoundException) by
changing the assertion to expect 404 (or, if you cannot update upstream yet,
replace the hard 500 assertion with a TODO comment and a looser check that the
response indicates a client error range) so the test no longer locks in a
server-error behavior.
In `@backend/test/ava-tests/saas-tests/saas-postgres-proxy-e2e.test.ts`:
- Around line 662-669: The test's assertion using
t.not(getTablesResponse.status, 200) is too weak; update the assertion to
explicitly check for the expected rejection due to frozen plan by asserting the
exact status code(s) produced by the proxy (e.g., t.is(getTablesResponse.status,
403) or t.true([403,503].includes(getTablesResponse.status))) and also assert
the response body contains the expected error marker string (e.g., that
getTablesResponse.body or getTablesResponse.text includes "CONNECTION_IS_FROZEN"
or "NonAvailableInFreePlanException" as defined in validate-connection.util.ts)
so the test fails for unrelated non-200 responses and only passes when the
frozen-plan path is triggered.
- Around line 143-149: The test suite currently silently sets skipAll = true
when isProxyReachable() returns false, causing AVA to report all tests as passed
even when the proxy is down; change the behavior to require an explicit opt-in
env var (e.g., PROXY_E2E) before allowing silent skips: in the test.before and
the corresponding later guard (references: isProxyReachable, PROXY_HOST,
PROXY_PORT, skipAll), check process.env.PROXY_E2E (or similar) and only set
skipAll=true when the env flag is present, otherwise throw or fail the before
hook (or call t.fail) so CI surfaces a hard failure; additionally emit a clear
console.warn including PROXY_HOST:PROXY_PORT and the env decision so skipped
runs are obviously intentional on developer machines.
In `@docker-compose.tst.yml`:
- Around line 245-259: The Postgres service testPg-proxy-e2e is intentionally
enabling deprecated md5 auth via POSTGRES_HOST_AUTH_METHOD, POSTGRES_INITDB_ARGS
and command (password_encryption=md5) for the proxy E2E tests; add an inline
comment above the service explaining that md5 is required only for this test
because the proxy implementation authenticates using md5, that md5 is deprecated
in Postgres 16 and must never be used in production configs, and ensure these
md5-specific envs/flags remain only in this test compose file.
- Around line 282-284: The docker-compose service "postgres-proxy" uses a
parent-relative build context ("../postgres-proxy") which requires a sibling
checkout; update docker-compose.tst.yml to make the E2E stack reproducible by
either (A) replacing the build context with a pinned pre-built image reference
(e.g. set the service to use image: <registry>/postgres-proxy:<tag>) and
removing the build block, or (B) document the required repo layout and checkout
step in the testing README so CI and contributors know to clone the sibling
"postgres-proxy" repo; modify the "postgres-proxy" service entry accordingly and
ensure any CI scripts pull the pinned image or perform the sibling checkout.
In `@docker-compose.yml`:
- Around line 246-289: The docker-compose services lack healthcheck blocks and
still use short-form depends_on, causing startup races; add healthchecks and
switch to long-form depends_on with condition: service_healthy so ordering is
enforced: add a healthcheck for testPg-proxy-e2e using pg_isready (or a TCP
probe to 5432) that retries until MD5 auth is active, add an HTTP/ TCP
healthcheck for proxy-mock-api (e.g., hit /healthz or TCP 3333) and for
postgres-proxy (HTTP /healthz or TCP 5432), then replace the short depends_on
entries for proxy-mock-api and postgres-proxy with the long-form depends_on
referencing testPg-proxy-e2e and proxy-mock-api and requiring condition:
service_healthy so isProxyReachable in saas-postgres-proxy-e2e.test.ts won’t
skip flaky startups.
In `@test/proxy-mock-api/Dockerfile`:
- Around line 1-8: The Dockerfile currently runs the container as root; update
it to switch to the non-root node user provided by the node base image by adding
a USER directive (e.g., USER node) after copying files and before CMD, and
ensure file permissions are correct by chowning or copying files as that user if
necessary (adjust COPY/ownership steps so server.js is readable/executable by
the node user and avoid running processes as root in CMD).
In `@test/proxy-mock-api/server.js`:
- Around line 62-73: The PUT /api/test/subscription-level handler calls
JSON.parse(body) unguarded which can throw and crash the server; wrap the parse
and subsequent handling in a try/catch around JSON.parse (and the parsed usage
of currentSubscriptionLevel) and on error log the failure and send an HTTP error
response (e.g., 400 with a JSON error body) before calling res.end so the
request always completes; follow the pattern used in the POST /api/proxy/usage
handler (try/catch around JSON.parse) to locate and implement the fix.
---
Nitpick comments:
In
`@backend/src/microservices/saas-microservice/data-structures/hosted-connection-credentials.ro.ts`:
- Around line 3-24: The response object HostedConnectionCredentialsRO mixes
camelCase and snake_case (e.g., connectionId vs is_frozen); rename the property
is_frozen to isFrozen on the HostedConnectionCredentialsRO class and update its
`@ApiProperty`() declaration to use the new name, then find and update all
references/usages/mappings/serializers, DTOs, tests and any database-to-RO
transformation code that read or write the old is_frozen key so they now use
isFrozen to keep naming consistent across the response object.
In `@backend/src/microservices/saas-microservice/saas.controller.ts`:
- Around line 353-364: The Swagger response status for the POST handler
getHostedConnectionCredentials (method getHostedConnectionCredentials using
GetHostedConnectionCredentialsDto and returning HostedConnectionCredentialsRO)
is inconsistent with Nest's POST default; either add `@HttpCode`(200) to the
getHostedConnectionCredentials method to keep the documented 200 status, or
change the `@ApiResponse` status to 201 so Swagger matches the runtime POST
behavior—update the decorator accordingly to make the doc and runtime
consistent.
In `@backend/test/ava-tests/saas-tests/hosted-connection-e2e.test.ts`:
- Line 94: Remove the leftover debug console.log statements that print
request/response objects to CI (they may leak sensitive data); search for and
delete the console.log calls that reference createdConnection and responseBody
in the hosted-connection-e2e.test.ts tests (and any other console.log debug
prints in the same file) so tests only use assertions and structured test
logging instead of dumping full objects.
In `@backend/test/ava-tests/saas-tests/saas-postgres-proxy-e2e.test.ts`:
- Around line 30-33: The test defaults use docker-compose hostnames (PROXY_HOST
and MOCK_API_HOST), which fail for local runs; change the defaults in
backend/test/ava-tests/saas-tests/saas-postgres-proxy-e2e.test.ts to fall back
to 'localhost' when the corresponding env var is not set (keep parseInt logic
for PROXY_PORT and MOCK_API_PORT intact), and optionally add a short README note
about running tests inside compose versus locally so developers know to override
these env vars when needed.
- Around line 567-623: The test 'should reject queries once query-time budget is
exhausted' uses a too-broad regex and an unbounded 30-iteration loop; tighten
the rejection check to a more specific pattern (e.g.
/53400|budget\s+exceeded|plan\s+limit/i) and add an explicit assertion that the
loop exited early (budget hit) rather than completing all 30 iterations—use the
existing loop index or a separate attempts counter (from the for loop variable i
or a new attempts variable) and assert it is < 30 (or i < 29) when rejected is
true so a silent regression (budget never exhausts) fails fast. Ensure you
update the regex reference where lastBody is tested and add the iteration-bound
assertion after the loop using lastStatus/ rejected/ i.
- Around line 302-312: The POST to add a row is serializing the body with
JSON.stringify which causes the backend to receive a string; update the request
in the addRow call (the request built with
request(app.getHttpServer()).post(`/table/row/${createConnectionRO.id}?tableName=${testTableName}`)
that assigns to addRowResponse) to pass a plain object literal instead of
JSON.stringify(...) and do the same for the corresponding update-row request
later (the update-row call around lines 361‑366), leaving headers
(.set('Content-Type','application/json') and .set('Accept','application/json'))
unchanged so supertest/body-parser handle JSON correctly.
In `@docker-compose.tst.yml`:
- Around line 35-36: The backend service now hard-depends on postgres-proxy
being healthy via the depends_on condition "service_healthy", which gates all
E2E runs on the proxy; remove the "condition: service_healthy" from the
backend's depends_on or move postgres-proxy into a separate docker-compose
profile (e.g., profile: "proxy-e2e") and update backend to only depend on the
proxy in that profile so regular E2E runs aren't blocked; reference the backend
service's depends_on entry and the "postgres-proxy" service/healthcheck when
making the change.
In `@docker-compose.yml`:
- Line 265: The duplicated hard-coded PROXY_API_KEY value should be centralized
so the two services (proxy-mock-api and postgres-proxy) cannot drift; replace
the literal PROXY_API_KEY: test-proxy-api-key in both service env sections by
referencing a single source (either define an env var like ${E2E_PROXY_API_KEY}
and use that for both services, or create a YAML anchor/alias for the key and
reference the anchor in both places), and ensure the new shared variable/anchor
is documented in the compose file or .env so tests use the same value.
In `@test/proxy-mock-api/server.js`:
- Around line 33-46: Remove the unused dead-default fields connectionId and
companyId from the TEST_CONNECTION const and keep only the real defaults (host,
port, database, username, password, subscriptionLevel); leave
currentSubscriptionLevel initialization as-is using
TEST_CONNECTION.subscriptionLevel and ensure all response code continues to use
deriveConnectionId and deriveCompanyId (these are the actual sources of IDs).
Update any nearby comments to reflect that connectionId/companyId are derived at
runtime and no other code should be reading TEST_CONNECTION.connectionId or
TEST_CONNECTION.companyId.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 161dcb0d-ab15-4a40-830b-5dcab5d3d7f3
📒 Files selected for processing (14)
backend/src/common/data-injection.tokens.tsbackend/src/microservices/saas-microservice/data-structures/get-hosted-connection-credentials.dto.tsbackend/src/microservices/saas-microservice/data-structures/hosted-connection-credentials.ro.tsbackend/src/microservices/saas-microservice/saas.controller.tsbackend/src/microservices/saas-microservice/saas.module.tsbackend/src/microservices/saas-microservice/use-cases/get-hosted-connection-credentials.use.case.tsbackend/src/microservices/saas-microservice/use-cases/saas-use-cases.interface.tsbackend/test/ava-tests/saas-tests/hosted-connection-e2e.test.tsbackend/test/ava-tests/saas-tests/saas-postgres-proxy-e2e.test.tsdocker-compose.tst.ymldocker-compose.ymlshared-code/src/knex-manager/knex-manager.tstest/proxy-mock-api/Dockerfiletest/proxy-mock-api/server.js
| @ApiProperty() | ||
| password: string; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for logging interceptors / middlewares that might serialize responses or DTOs.
rg -nP --type=ts -C3 '\b(LoggerInterceptor|intercept\s*\(|logResponse|res\.body|response\.body)\b'
# Confirm no toJSON/serializer strips sensitive fields yet.
rg -nP --type=ts -C2 '\bHostedConnectionCredentialsRO\b'Repository: rocket-admin/rocketadmin
Length of output: 28104
🏁 Script executed:
cat -n backend/src/microservices/saas-microservice/data-structures/hosted-connection-credentials.ro.tsRepository: rocket-admin/rocketadmin
Length of output: 579
🏁 Script executed:
# Check for serialization/redaction interceptors or utilities
rg -nP --type=ts -C3 '(ClassSerializerInterceptor|Exclude|Transform|toJSON|serializer)' backend/src/interceptors/Repository: rocket-admin/rocketadmin
Length of output: 50
🏁 Script executed:
# Check SentryInterceptor for password/sensitive field handling
cat -n backend/src/interceptors/sentry.interceptor.tsRepository: rocket-admin/rocketadmin
Length of output: 1424
🏁 Script executed:
# Look for password/sensitive field redaction lists or patterns
rg -nP --type=ts 'password|redact|sensitive|mask' backend/src/interceptors/Repository: rocket-admin/rocketadmin
Length of output: 50
🏁 Script executed:
# Check the use case implementation
cat -n backend/src/microservices/saas-microservice/use-cases/get-hosted-connection-credentials.use.case.tsRepository: rocket-admin/rocketadmin
Length of output: 1958
Add @Exclude() decorator to redact the password field from serialization, or document the security handling.
The password field is returned plaintext from this endpoint without any automatic redaction. While this may be required for the SaaS-to-proxy flow, there are no serialization interceptors (e.g., ClassSerializerInterceptor with @Exclude()) or middleware configured to strip this field from logs, error tracking, or cached responses. Add either:
@Exclude()decorator on thepasswordproperty if it should never be serialized, or- A code comment explaining why the field is exposed and confirming that callers and middleware handle it securely (no caching, request/response logging redaction, etc.).
Without explicit redaction or documentation, future changes risk accidentally leaking credentials.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/microservices/saas-microservice/data-structures/hosted-connection-credentials.ro.ts`
around lines 19 - 20, The password property on HostedConnectionCredentials (the
password field currently annotated only with `@ApiProperty`()) must be either
redacted from serialization or explicitly documented: add the `@Exclude`()
decorator to the password property and ensure the class is serialized via
ClassSerializerInterceptor, or add a clear code comment on the
HostedConnectionCredentials class/property explaining why plaintext password is
exposed and list required safeguards (no caching, response/log redaction,
error-tracking filtering). Update related serialization configuration to
guarantee `@Exclude`() is honored if you choose that approach.
| protected async implementation(inputData: GetHostedConnectionCredentialsDto): Promise<HostedConnectionCredentialsRO> { | ||
| const connection = await this._dbContext.connectionRepository.findOne({ | ||
| where: { id: inputData.hostedDatabaseId }, | ||
| }); | ||
| if (!connection) { | ||
| throw new NotFoundException(Messages.CONNECTION_NOT_FOUND); | ||
| } |
There was a problem hiding this comment.
No tenant/company scoping on lookup.
connectionRepository.findOne({ where: { id: inputData.hostedDatabaseId } }) returns any connection by id, regardless of which company the caller belongs to. The DTO only carries hostedDatabaseId. Even though this route is behind the SaaS-to-SaaS JWT, a compromised or misbehaving caller (or a future non-SaaS caller) could enumerate UUIDs to exfiltrate credentials for any hosted DB in the system.
Recommend either: (a) require and validate a companyId in the DTO and filter where: { id, company: { id: companyId } }, or (b) explicitly filter to connections that are actually hosted (hosted: true / equivalent flag) so this path can't return credentials for customer-owned BYOD connections.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/microservices/saas-microservice/use-cases/get-hosted-connection-credentials.use.case.ts`
around lines 22 - 28, The lookup in implementation
(get-hosted-connection-credentials.use.case.ts -> protected async
implementation) uses connectionRepository.findOne({ where: { id:
inputData.hostedDatabaseId } }) without scoping to tenant/company or hosted-only
connections; update the code and DTO to prevent cross-tenant exposure by either
(A) adding companyId to GetHostedConnectionCredentialsDto and changing the query
to connectionRepository.findOne({ where: { id: inputData.hostedDatabaseId,
company: { id: inputData.companyId } } }) with validation that the caller's
company matches, or (B) at minimum filter for hosted connections only by using
connectionRepository.findOne({ where: { id: inputData.hostedDatabaseId, hosted:
true } }) so only system-hosted DBs can be returned; apply the chosen change in
the implementation, adjust any DTO/validator usages, and keep error handling
(NotFoundException) the same if no record matches.
| protected async implementation(inputData: GetHostedConnectionCredentialsDto): Promise<HostedConnectionCredentialsRO> { | ||
| const connection = await this._dbContext.connectionRepository.findOne({ | ||
| where: { id: inputData.hostedDatabaseId }, | ||
| }); | ||
| if (!connection) { | ||
| throw new NotFoundException(Messages.CONNECTION_NOT_FOUND); | ||
| } | ||
|
|
||
| return { | ||
| connectionId: connection.id, | ||
| host: connection.host, | ||
| port: connection.port, | ||
| database: connection.database, | ||
| username: connection.username, | ||
| password: connection.password, | ||
| is_frozen: connection.is_frozen, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) See how hosted-db connection creation stores the password (encryption?).
fd -t f 'create-connection-for-hosted-db.use.case.ts' | xargs -I{} sed -n '1,220p' {}
# 2) Look for decryption helpers used elsewhere when returning connection credentials.
rg -nP -C2 '\b(decrypt|encrypt)\w*\s*\(' --type=ts -g 'backend/src/**'
# 3) See whether master/azure/connection encryption is applied on hosted connection writes.
rg -nP -C3 '(masterEncryption|azure_encryption|encryptData|decryptData)' --type=ts -g 'backend/src/**'Repository: rocket-admin/rocketadmin
Length of output: 50381
🏁 Script executed:
# Check ConnectionEntity definition for BeforeInsert/AfterLoad hooks
fd -t f 'connection.entity.ts' | xargs -I{} cat -n {} | head -150
# Check if there's connection-level encryption happening at entity level
rg -B5 -A5 'BeforeInsert|BeforeUpdate|AfterLoad' --type=ts -g 'backend/src/**/connection.entity.ts'
# Verify if masterEncryption should be true for hosted connections
rg -C5 'hosted.*masterEncryption|masterEncryption.*hosted' --type=ts -g 'backend/src/**'Repository: rocket-admin/rocketadmin
Length of output: 8372
🏁 Script executed:
# Get the complete BeforeInsert hook implementation
rg -A30 'encryptCredentialsAndGenerateNanoid' --type=ts -g 'backend/src/**/connection.entity.ts'
# Also check how decryptConnectionsCredentialsAsync works
rg -B5 -A15 'decryptConnectionsCredentialsAsync' --type=ts -g 'backend/src/**' | head -100Repository: rocket-admin/rocketadmin
Length of output: 16265
🏁 Script executed:
# Check the actual current implementation of get-hosted-connection-credentials.use.case.ts
cat -n backend/src/microservices/saas-microservice/use-cases/get-hosted-connection-credentials.use.case.tsRepository: rocket-admin/rocketadmin
Length of output: 1958
Add missing decryption step before returning credentials.
The connection password is encrypted by ConnectionEntity's @BeforeInsert hook (via Encryptor.encryptData()), but this use case returns the encrypted value directly without decrypting. The API advertises "decrypted credentials" while returning ciphertext, causing authentication to fail silently.
Follow the pattern in UpdateHostedConnectionPasswordUseCase by calling decryptConnectionsCredentialsAsync() on the fetched connection before returning:
import { decryptConnectionCredentialsAsync } from '../../../entities/connection/utils/decrypt-connection-credentials-async.js';
protected async implementation(inputData: GetHostedConnectionCredentialsDto): Promise<HostedConnectionCredentialsRO> {
const connection = await this._dbContext.connectionRepository.findOne({
where: { id: inputData.hostedDatabaseId },
});
if (!connection) {
throw new NotFoundException(Messages.CONNECTION_NOT_FOUND);
}
await decryptConnectionCredentialsAsync(connection);
return { ... };
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/microservices/saas-microservice/use-cases/get-hosted-connection-credentials.use.case.ts`
around lines 22 - 39, The implementation of
GetHostedConnectionCredentialsUseCase returns the encrypted password; fetch the
connection via _dbContext.connectionRepository.findOne (as already done), then
call decryptConnectionCredentialsAsync(connection) (the same helper used by
UpdateHostedConnectionPasswordUseCase) before constructing and returning the
HostedConnectionCredentialsRO so the password is decrypted; add the appropriate
import for decryptConnectionCredentialsAsync from
entities/connection/utils/decrypt-connection-credentials-async.js and keep the
existing NotFoundException(Messages.CONNECTION_NOT_FOUND) check.
| const responseBody = JSON.parse(result.text); | ||
| console.log('🚀 ~ responseBody:', responseBody); | ||
| t.is(result.status, 500); |
There was a problem hiding this comment.
Asserting HTTP 500 codifies a server-error response for an invalid user id.
A missing/unknown userId is a caller/input problem and should surface as a 4xx (e.g. 404 Not Found or 400 Bad Request), not 500 Internal Server Error. Pinning the test at 500 freezes the buggy behavior in place and will mask a future fix in the use case.
Consider either fixing the upstream use case to throw NotFoundException (consistent with how get-hosted-connection-credentials.use.case.ts handles missing connections) and asserting 404 here, or at minimum leaving a TODO so this doesn't ossify.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/test/ava-tests/saas-tests/hosted-connection-e2e.test.ts` around lines
184 - 186, The test in hosted-connection-e2e.test.ts is asserting a 500 for an
unknown/missing userId (result.status === 500), which should be a 4xx; update
the test to expect the correct client error (preferably 404 to match how
get-hosted-connection-credentials.use.case.ts throws NotFoundException) by
changing the assertion to expect 404 (or, if you cannot update upstream yet,
replace the hard 500 assertion with a TODO comment and a looser check that the
response indicates a client error range) so the test no longer locks in a
server-error behavior.
| test.before(async () => { | ||
| const reachable = await isProxyReachable(); | ||
| if (!reachable) { | ||
| console.log(`[postgres-proxy e2e] Proxy not reachable at ${PROXY_HOST}:${PROXY_PORT}, skipping tests`); | ||
| skipAll = true; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Silent skip of the whole suite when the proxy isn't reachable will hide regressions in CI.
If isProxyReachable() returns false (2s TCP timeout), every test short-circuits to t.pass('skipped: proxy not available') — which AVA reports as a passing test. In CI, that means a broken/misconfigured postgres-proxy service, a DNS change, or a port change all produce a green build even though zero behavior was actually exercised.
Recommend gating on an explicit env flag (e.g. PROXY_E2E=1) so the skip path only fires in developer laptops, while CI treats missing proxy as a hard failure. At minimum, emit a visible warning/counter so the "all tests passed" signal isn't misleading.
Also applies to: 184-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/test/ava-tests/saas-tests/saas-postgres-proxy-e2e.test.ts` around
lines 143 - 149, The test suite currently silently sets skipAll = true when
isProxyReachable() returns false, causing AVA to report all tests as passed even
when the proxy is down; change the behavior to require an explicit opt-in env
var (e.g., PROXY_E2E) before allowing silent skips: in the test.before and the
corresponding later guard (references: isProxyReachable, PROXY_HOST, PROXY_PORT,
skipAll), check process.env.PROXY_E2E (or similar) and only set skipAll=true
when the env flag is present, otherwise throw or fail the before hook (or call
t.fail) so CI surfaces a hard failure; additionally emit a clear console.warn
including PROXY_HOST:PROXY_PORT and the env decision so skipped runs are
obviously intentional on developer machines.
| # === Postgres Proxy E2E Testing === | ||
|
|
||
| testPg-proxy-e2e: | ||
| image: postgres:16 | ||
| ports: | ||
| - 9003:5432 | ||
| environment: | ||
| POSTGRES_PASSWORD: proxy_test_123 | ||
| POSTGRES_HOST_AUTH_METHOD: md5 | ||
| POSTGRES_INITDB_ARGS: "--auth-host=md5" | ||
| command: postgres -c 'max_connections=300' -c 'password_encryption=md5' | ||
|
|
||
| proxy-mock-api: | ||
| build: | ||
| context: ./test/proxy-mock-api | ||
| ports: | ||
| - 3333:3333 | ||
| environment: | ||
| MOCK_API_PORT: 3333 | ||
| PROXY_API_KEY: test-proxy-api-key | ||
| UPSTREAM_PG_HOST: testPg-proxy-e2e | ||
| UPSTREAM_PG_PORT: 5432 | ||
| UPSTREAM_PG_DATABASE: postgres | ||
| UPSTREAM_PG_USERNAME: postgres | ||
| UPSTREAM_PG_PASSWORD: proxy_test_123 | ||
| depends_on: | ||
| - testPg-proxy-e2e | ||
|
|
||
| postgres-proxy: | ||
| build: | ||
| context: ../postgres-proxy | ||
| ports: | ||
| - 9005:5432 | ||
| - 8085:8080 | ||
| environment: | ||
| PROXY_LISTEN_ADDR: ":5432" | ||
| PROXY_HEALTH_ADDR: ":8080" | ||
| PROXY_BACKEND_API_URL: "http://proxy-mock-api:3333" | ||
| PROXY_API_KEY: "test-proxy-api-key" | ||
| PROXY_USAGE_REPORT_INTERVAL: "10s" | ||
| PROXY_LOG_LEVEL: "debug" | ||
| depends_on: | ||
| - proxy-mock-api | ||
| - testPg-proxy-e2e |
There was a problem hiding this comment.
Missing healthchecks and service_healthy gating — ordering is not what it looks like.
The AI summary describes pg_isready / /healthz healthchecks and depends_on … condition: service_healthy for the new proxy topology, but the committed code uses the short-list depends_on: form (Lines 271‑272, 287‑289) with no healthcheck: blocks on testPg-proxy-e2e, proxy-mock-api, or postgres-proxy.
Consequences:
proxy-mock-apican start before Postgres is accepting MD5 auth, andpostgres-proxycan start before the mock API is actually listening on:3333, producing flaky E2E startup.- The E2E test's
isProxyReachable()TCP probe (insaas-postgres-proxy-e2e.test.ts) will silently skip the whole suite if the proxy isn't up yet, which can turn genuine regressions into green builds.
Either (a) update the summary to reflect reality, or (b) add the healthchecks and long-form depends_on so the ordering is real:
🛠️ Proposed healthchecks + gated depends_on
testPg-proxy-e2e:
image: postgres:16
ports:
- 9003:5432
environment:
POSTGRES_PASSWORD: proxy_test_123
POSTGRES_HOST_AUTH_METHOD: md5
POSTGRES_INITDB_ARGS: "--auth-host=md5"
command: postgres -c 'max_connections=300' -c 'password_encryption=md5'
+ healthcheck:
+ test: ["CMD-SHELL", "pg_isready -U postgres -d postgres"]
+ interval: 5s
+ timeout: 5s
+ retries: 20
proxy-mock-api:
build:
context: ./test/proxy-mock-api
ports:
- 3333:3333
environment:
MOCK_API_PORT: 3333
PROXY_API_KEY: test-proxy-api-key
UPSTREAM_PG_HOST: testPg-proxy-e2e
UPSTREAM_PG_PORT: 5432
UPSTREAM_PG_DATABASE: postgres
UPSTREAM_PG_USERNAME: postgres
UPSTREAM_PG_PASSWORD: proxy_test_123
- depends_on:
- - testPg-proxy-e2e
+ healthcheck:
+ test: ["CMD", "wget", "--spider", "-q", "http://localhost:3333/healthz"]
+ interval: 5s
+ timeout: 5s
+ retries: 20
+ depends_on:
+ testPg-proxy-e2e:
+ condition: service_healthy
postgres-proxy:
build:
context: ../postgres-proxy
ports:
- 9005:5432
- 8085:8080
environment:
PROXY_LISTEN_ADDR: ":5432"
PROXY_HEALTH_ADDR: ":8080"
PROXY_BACKEND_API_URL: "http://proxy-mock-api:3333"
PROXY_API_KEY: "test-proxy-api-key"
PROXY_USAGE_REPORT_INTERVAL: "10s"
PROXY_LOG_LEVEL: "debug"
- depends_on:
- - proxy-mock-api
- - testPg-proxy-e2e
+ healthcheck:
+ test: ["CMD", "wget", "--spider", "-q", "http://localhost:8080/healthz"]
+ interval: 5s
+ timeout: 5s
+ retries: 20
+ depends_on:
+ proxy-mock-api:
+ condition: service_healthy
+ testPg-proxy-e2e:
+ condition: service_healthy📝 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.
| # === Postgres Proxy E2E Testing === | |
| testPg-proxy-e2e: | |
| image: postgres:16 | |
| ports: | |
| - 9003:5432 | |
| environment: | |
| POSTGRES_PASSWORD: proxy_test_123 | |
| POSTGRES_HOST_AUTH_METHOD: md5 | |
| POSTGRES_INITDB_ARGS: "--auth-host=md5" | |
| command: postgres -c 'max_connections=300' -c 'password_encryption=md5' | |
| proxy-mock-api: | |
| build: | |
| context: ./test/proxy-mock-api | |
| ports: | |
| - 3333:3333 | |
| environment: | |
| MOCK_API_PORT: 3333 | |
| PROXY_API_KEY: test-proxy-api-key | |
| UPSTREAM_PG_HOST: testPg-proxy-e2e | |
| UPSTREAM_PG_PORT: 5432 | |
| UPSTREAM_PG_DATABASE: postgres | |
| UPSTREAM_PG_USERNAME: postgres | |
| UPSTREAM_PG_PASSWORD: proxy_test_123 | |
| depends_on: | |
| - testPg-proxy-e2e | |
| postgres-proxy: | |
| build: | |
| context: ../postgres-proxy | |
| ports: | |
| - 9005:5432 | |
| - 8085:8080 | |
| environment: | |
| PROXY_LISTEN_ADDR: ":5432" | |
| PROXY_HEALTH_ADDR: ":8080" | |
| PROXY_BACKEND_API_URL: "http://proxy-mock-api:3333" | |
| PROXY_API_KEY: "test-proxy-api-key" | |
| PROXY_USAGE_REPORT_INTERVAL: "10s" | |
| PROXY_LOG_LEVEL: "debug" | |
| depends_on: | |
| - proxy-mock-api | |
| - testPg-proxy-e2e | |
| # === Postgres Proxy E2E Testing === | |
| testPg-proxy-e2e: | |
| image: postgres:16 | |
| ports: | |
| - 9003:5432 | |
| environment: | |
| POSTGRES_PASSWORD: proxy_test_123 | |
| POSTGRES_HOST_AUTH_METHOD: md5 | |
| POSTGRES_INITDB_ARGS: "--auth-host=md5" | |
| command: postgres -c 'max_connections=300' -c 'password_encryption=md5' | |
| healthcheck: | |
| test: ["CMD-SHELL", "pg_isready -U postgres -d postgres"] | |
| interval: 5s | |
| timeout: 5s | |
| retries: 20 | |
| proxy-mock-api: | |
| build: | |
| context: ./test/proxy-mock-api | |
| ports: | |
| - 3333:3333 | |
| environment: | |
| MOCK_API_PORT: 3333 | |
| PROXY_API_KEY: test-proxy-api-key | |
| UPSTREAM_PG_HOST: testPg-proxy-e2e | |
| UPSTREAM_PG_PORT: 5432 | |
| UPSTREAM_PG_DATABASE: postgres | |
| UPSTREAM_PG_USERNAME: postgres | |
| UPSTREAM_PG_PASSWORD: proxy_test_123 | |
| healthcheck: | |
| test: ["CMD", "wget", "--spider", "-q", "http://localhost:3333/healthz"] | |
| interval: 5s | |
| timeout: 5s | |
| retries: 20 | |
| depends_on: | |
| testPg-proxy-e2e: | |
| condition: service_healthy | |
| postgres-proxy: | |
| build: | |
| context: ../postgres-proxy | |
| ports: | |
| - 9005:5432 | |
| - 8085:8080 | |
| environment: | |
| PROXY_LISTEN_ADDR: ":5432" | |
| PROXY_HEALTH_ADDR: ":8080" | |
| PROXY_BACKEND_API_URL: "http://proxy-mock-api:3333" | |
| PROXY_API_KEY: "test-proxy-api-key" | |
| PROXY_USAGE_REPORT_INTERVAL: "10s" | |
| PROXY_LOG_LEVEL: "debug" | |
| healthcheck: | |
| test: ["CMD", "wget", "--spider", "-q", "http://localhost:8080/healthz"] | |
| interval: 5s | |
| timeout: 5s | |
| retries: 20 | |
| depends_on: | |
| proxy-mock-api: | |
| condition: service_healthy | |
| testPg-proxy-e2e: | |
| condition: service_healthy |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker-compose.yml` around lines 246 - 289, The docker-compose services lack
healthcheck blocks and still use short-form depends_on, causing startup races;
add healthchecks and switch to long-form depends_on with condition:
service_healthy so ordering is enforced: add a healthcheck for testPg-proxy-e2e
using pg_isready (or a TCP probe to 5432) that retries until MD5 auth is active,
add an HTTP/ TCP healthcheck for proxy-mock-api (e.g., hit /healthz or TCP 3333)
and for postgres-proxy (HTTP /healthz or TCP 5432), then replace the short
depends_on entries for proxy-mock-api and postgres-proxy with the long-form
depends_on referencing testPg-proxy-e2e and proxy-mock-api and requiring
condition: service_healthy so isProxyReachable in
saas-postgres-proxy-e2e.test.ts won’t skip flaky startups.
| FROM node:24-alpine | ||
|
|
||
| WORKDIR /app | ||
| COPY server.js . | ||
|
|
||
| EXPOSE 3333 | ||
|
|
||
| CMD ["node", "server.js"] |
There was a problem hiding this comment.
Run the container as a non-root user.
Even for a test-only image, running as root is flagged by Trivy (DS-0002) and is trivial to fix — the node base images already ship a node user.
🛡️ Proposed fix
FROM node:24-alpine
WORKDIR /app
COPY server.js .
EXPOSE 3333
+USER node
CMD ["node", "server.js"]📝 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.
| FROM node:24-alpine | |
| WORKDIR /app | |
| COPY server.js . | |
| EXPOSE 3333 | |
| CMD ["node", "server.js"] | |
| FROM node:24-alpine | |
| WORKDIR /app | |
| COPY server.js . | |
| EXPOSE 3333 | |
| USER node | |
| CMD ["node", "server.js"] |
🧰 Tools
🪛 Trivy (0.69.3)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/proxy-mock-api/Dockerfile` around lines 1 - 8, The Dockerfile currently
runs the container as root; update it to switch to the non-root node user
provided by the node base image by adding a USER directive (e.g., USER node)
after copying files and before CMD, and ensure file permissions are correct by
chowning or copying files as that user if necessary (adjust COPY/ownership steps
so server.js is readable/executable by the node user and avoid running processes
as root in CMD).
| if (req.method === 'PUT' && parsedUrl.pathname === '/api/test/subscription-level') { | ||
| let body = ''; | ||
| req.on('data', (chunk) => (body += chunk)); | ||
| req.on('end', () => { | ||
| const parsed = JSON.parse(body); | ||
| currentSubscriptionLevel = parsed.subscriptionLevel; | ||
| console.log(`[mock-api] Subscription level changed to: ${currentSubscriptionLevel}`); | ||
| res.writeHead(200, { 'Content-Type': 'application/json' }); | ||
| res.end(JSON.stringify({ ok: true, subscriptionLevel: currentSubscriptionLevel })); | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
JSON.parse on request body can crash the handler — no response will be sent.
If a test (or a misbehaving caller) posts a malformed body to PUT /api/test/subscription-level, JSON.parse(body) throws inside the 'end' callback, the server never calls res.end, and the Node process emits uncaughtException. In the containerized test topology that means the mock-api may die mid-run and subsequent E2E assertions become flaky for reasons unrelated to the code under test.
Contrast with POST /api/proxy/usage at Lines 136-145, which already wraps its parse in try/catch.
🛡️ Proposed fix
if (req.method === 'PUT' && parsedUrl.pathname === '/api/test/subscription-level') {
let body = '';
req.on('data', (chunk) => (body += chunk));
req.on('end', () => {
- const parsed = JSON.parse(body);
- currentSubscriptionLevel = parsed.subscriptionLevel;
- console.log(`[mock-api] Subscription level changed to: ${currentSubscriptionLevel}`);
- res.writeHead(200, { 'Content-Type': 'application/json' });
- res.end(JSON.stringify({ ok: true, subscriptionLevel: currentSubscriptionLevel }));
+ try {
+ const parsed = JSON.parse(body);
+ currentSubscriptionLevel = parsed.subscriptionLevel;
+ console.log(`[mock-api] Subscription level changed to: ${currentSubscriptionLevel}`);
+ res.writeHead(200, { 'Content-Type': 'application/json' });
+ res.end(JSON.stringify({ ok: true, subscriptionLevel: currentSubscriptionLevel }));
+ } catch (e) {
+ res.writeHead(400, { 'Content-Type': 'application/json' });
+ res.end(JSON.stringify({ error: `Invalid JSON: ${e.message}` }));
+ }
});
return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/proxy-mock-api/server.js` around lines 62 - 73, The PUT
/api/test/subscription-level handler calls JSON.parse(body) unguarded which can
throw and crash the server; wrap the parse and subsequent handling in a
try/catch around JSON.parse (and the parsed usage of currentSubscriptionLevel)
and on error log the failure and send an HTTP error response (e.g., 400 with a
JSON error body) before calling res.end so the request always completes; follow
the pattern used in the POST /api/proxy/usage handler (try/catch around
JSON.parse) to locate and implement the fix.
…s and remove unused postgres proxy service
There was a problem hiding this comment.
Pull request overview
Adds infrastructure and backend API support for exercising a Postgres proxy flow end-to-end, including a mock backend API, Docker Compose wiring, and AVA e2e coverage. Also introduces a SaaS endpoint to fetch hosted connection credentials and tags Postgres connections with an application_name.
Changes:
- Added Postgres proxy E2E environment (proxy + upstream Postgres + mock API) and comprehensive AVA tests for proxy CRUD/usage/budget scenarios.
- Added SaaS endpoint
/saas/connection/hosted/credentialswith DTO/RO + use-case wiring. - Set
application_name: 'rocketadmin'for Postgres connections created via Knex.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
test/proxy-mock-api/server.js |
Implements a mock backend API for the proxy contract + test-only endpoints for subscription level/usage reports. |
test/proxy-mock-api/Dockerfile |
Containerizes the proxy mock API used by compose-based E2E runs. |
shared-code/src/knex-manager/knex-manager.ts |
Adds application_name to Postgres driver connection config for better observability. |
docker-compose.yml |
Wires up upstream Postgres, proxy mock API, and the Go postgres-proxy service for E2E testing. |
docker-compose.tst.yml |
Updates test compose command/formatting and healthchecks to support revised test execution. |
backend/test/ava-tests/saas-tests/saas-postgres-proxy-e2e.test.ts |
New AVA E2E suite validating RocketAdmin functionality through the Postgres proxy plus usage/budget behaviors. |
backend/test/ava-tests/saas-tests/hosted-connection-e2e.test.ts |
Updates hosted connection E2E payload/expectations to include hostedDatabaseId and align response fields. |
backend/src/microservices/saas-microservice/use-cases/saas-use-cases.interface.ts |
Exposes a new use-case interface for hosted connection credential retrieval. |
backend/src/microservices/saas-microservice/use-cases/get-hosted-connection-credentials.use.case.ts |
Implements hosted connection credential lookup for the new SaaS endpoint. |
backend/src/microservices/saas-microservice/saas.module.ts |
Registers the new use-case provider and route in the SaaS module. |
backend/src/microservices/saas-microservice/saas.controller.ts |
Adds the /saas/connection/hosted/credentials controller endpoint with Swagger metadata. |
backend/src/microservices/saas-microservice/data-structures/hosted-connection-credentials.ro.ts |
Defines the response object returned by the new credentials endpoint. |
backend/src/microservices/saas-microservice/data-structures/get-hosted-connection-credentials.dto.ts |
Defines request DTO for the credentials endpoint. |
backend/src/common/data-injection.tokens.ts |
Adds a new DI token for the hosted-credentials use-case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| protected async implementation(inputData: GetHostedConnectionCredentialsDto): Promise<HostedConnectionCredentialsRO> { | ||
| const connection = await this._dbContext.connectionRepository.findOne({ | ||
| where: { id: inputData.hostedDatabaseId }, | ||
| }); |
There was a problem hiding this comment.
connectionRepository.findOne() returns the ConnectionEntity with encrypted credentials (host/username/password/database) because no decrypt helper is called. As a result this endpoint will return encrypted values instead of the advertised decrypted credentials. Use a repository method that decrypts (e.g., connectionRepository.findOneById() / findOneConnection() / findAndDecryptConnection() as appropriate) before building the response, and consider how to handle masterEncryption connections.
| protected async implementation(inputData: GetHostedConnectionCredentialsDto): Promise<HostedConnectionCredentialsRO> { | |
| const connection = await this._dbContext.connectionRepository.findOne({ | |
| where: { id: inputData.hostedDatabaseId }, | |
| }); | |
| private async findHostedConnectionWithDecryptedCredentials(hostedDatabaseId: string) { | |
| const connectionRepository = this._dbContext.connectionRepository as typeof this._dbContext.connectionRepository & { | |
| findOneById?: (id: string) => Promise<any>; | |
| findOneConnection?: (id: string) => Promise<any>; | |
| findAndDecryptConnection?: (id: string) => Promise<any>; | |
| }; | |
| if (typeof connectionRepository.findAndDecryptConnection === 'function') { | |
| return connectionRepository.findAndDecryptConnection(hostedDatabaseId); | |
| } | |
| if (typeof connectionRepository.findOneConnection === 'function') { | |
| return connectionRepository.findOneConnection(hostedDatabaseId); | |
| } | |
| if (typeof connectionRepository.findOneById === 'function') { | |
| return connectionRepository.findOneById(hostedDatabaseId); | |
| } | |
| throw new Error('Connection repository does not expose a decryption-aware lookup method.'); | |
| } | |
| protected async implementation(inputData: GetHostedConnectionCredentialsDto): Promise<HostedConnectionCredentialsRO> { | |
| const connection = await this.findHostedConnectionWithDecryptedCredentials(inputData.hostedDatabaseId); |
| export class GetHostedConnectionCredentialsDto { | ||
| @ApiProperty() | ||
| @IsString() | ||
| @IsNotEmpty() |
There was a problem hiding this comment.
This DTO only accepts hostedDatabaseId. Since the SaaS auth middleware validates only that a JWT is signed (no tenant scoping), an internal caller could fetch credentials for any connection ID. Consider requiring companyId (or other tenant identifier) and validating the connection belongs to that tenant before returning credentials.
| @IsNotEmpty() | |
| @IsNotEmpty() | |
| companyId: string; | |
| @ApiProperty() | |
| @IsString() | |
| @IsNotEmpty() |
| // Create hosted connection via SaaS endpoint | ||
| const createHostedConnectionResult = await request(app.getHttpServer()) | ||
| .post('/saas/connection/hosted') | ||
| .send({ | ||
| companyId: companyId, | ||
| userId: userId, | ||
| hostedDatabaseId: faker.string.uuid(), | ||
| databaseName: 'postgres', | ||
| hostname: 'testPg-e2e-testing', | ||
| port: 5432, | ||
| username: 'postgres', | ||
| password: '123', | ||
| }) | ||
| .set('Authorization', `Bearer ${saasToken}`) | ||
| .set('Content-Type', 'application/json') | ||
| .set('Accept', 'application/json'); | ||
|
|
||
| t.is(createHostedConnectionResult.status, 201); | ||
|
|
||
| const createdConnection = JSON.parse(createHostedConnectionResult.text); | ||
| const connectionId = createdConnection.id; | ||
| console.log('🚀 ~ createdConnection:', createdConnection); | ||
|
|
||
| t.is(createHostedConnectionResult.status, 201); | ||
| const connectionId = createdConnection.connectionId; | ||
|
|
||
| // Verify connection was created | ||
| t.truthy(connectionId); | ||
| t.is(createdConnection.type, 'postgres'); | ||
| t.is(createdConnection.database, 'postgres'); | ||
| t.is(createdConnection.host, 'testPg-e2e-testing'); | ||
| t.is(createdConnection.port, 5432); | ||
|
|
||
| // Verify admin group was created | ||
| t.truthy(createdConnection.groups); | ||
| t.is(createdConnection.groups.length, 1); | ||
| const adminGroup = createdConnection.groups[0]; | ||
| t.truthy(adminGroup.id); | ||
| t.is(adminGroup.isMain, true); | ||
|
|
There was a problem hiding this comment.
The new hosted-connection credentials endpoint (/saas/connection/hosted/credentials) is introduced, but this e2e suite doesn’t exercise it. Adding a test that creates a hosted connection, calls the credentials endpoint, and asserts the returned credentials are decrypted (and that auth is enforced) would prevent regressions.
| "/opt/mssql-tools18/bin/sqlcmd -S localhost -U sa -P | ||
| 'yNuXf@6T#BgoQ%U6knMp' -C -Q 'SELECT 1' || | ||
| /opt/mssql-tools/bin/sqlcmd -S localhost -U sa -P | ||
| 'yNuXf@6T#BgoQ%U6knMp' -Q 'SELECT 1'", |
There was a problem hiding this comment.
This MSSQL healthcheck command is now split across multiple lines inside a double-quoted YAML string. YAML preserves the newlines, so the shell will see them as command separators and sqlcmd will be invoked with a dangling -P (then the next line will be treated as a separate command), breaking the healthcheck. Keep the command on one line, or use a folded scalar (>) / explicit \ line continuations so the shell receives a single valid command string.
| "/opt/mssql-tools18/bin/sqlcmd -S localhost -U sa -P | |
| 'yNuXf@6T#BgoQ%U6knMp' -C -Q 'SELECT 1' || | |
| /opt/mssql-tools/bin/sqlcmd -S localhost -U sa -P | |
| 'yNuXf@6T#BgoQ%U6knMp' -Q 'SELECT 1'", | |
| "/opt/mssql-tools18/bin/sqlcmd -S localhost -U sa -P 'yNuXf@6T#BgoQ%U6knMp' -C -Q 'SELECT 1' || /opt/mssql-tools/bin/sqlcmd -S localhost -U sa -P 'yNuXf@6T#BgoQ%U6knMp' -Q 'SELECT 1'", |
| # === Postgres Proxy E2E Testing === | ||
|
|
||
| testPg-proxy-e2e: | ||
| image: postgres:16 | ||
| ports: | ||
| - 9003:5432 | ||
| environment: | ||
| POSTGRES_PASSWORD: proxy_test_123 | ||
| POSTGRES_HOST_AUTH_METHOD: md5 | ||
| POSTGRES_INITDB_ARGS: "--auth-host=md5" | ||
| command: postgres -c 'max_connections=300' -c 'password_encryption=md5' | ||
|
|
||
| proxy-mock-api: | ||
| build: | ||
| context: ./test/proxy-mock-api | ||
| ports: | ||
| - 3333:3333 | ||
| environment: | ||
| MOCK_API_PORT: 3333 | ||
| PROXY_API_KEY: test-proxy-api-key | ||
| UPSTREAM_PG_HOST: testPg-proxy-e2e | ||
| UPSTREAM_PG_PORT: 5432 | ||
| UPSTREAM_PG_DATABASE: postgres | ||
| UPSTREAM_PG_USERNAME: postgres | ||
| UPSTREAM_PG_PASSWORD: proxy_test_123 | ||
| depends_on: | ||
| - testPg-proxy-e2e | ||
|
|
||
| postgres-proxy: | ||
| build: | ||
| context: ../postgres-proxy | ||
| ports: | ||
| - 9005:5432 | ||
| - 8085:8080 | ||
| environment: | ||
| PROXY_LISTEN_ADDR: ":5432" | ||
| PROXY_HEALTH_ADDR: ":8080" | ||
| PROXY_BACKEND_API_URL: "http://proxy-mock-api:3333" | ||
| PROXY_API_KEY: "test-proxy-api-key" | ||
| PROXY_USAGE_REPORT_INTERVAL: "10s" | ||
| PROXY_LOG_LEVEL: "debug" | ||
| depends_on: | ||
| - proxy-mock-api | ||
| - testPg-proxy-e2e |
There was a problem hiding this comment.
The Postgres proxy E2E services added here will be started/built on a default docker compose up, and postgres-proxy depends on a sibling ../postgres-proxy checkout. To avoid breaking normal dev workflows (and unnecessary resource usage/port conflicts), consider placing these services behind a Compose profiles entry or moving them into a dedicated compose file used only for proxy E2E runs.
| const parsed = JSON.parse(body); | ||
| currentSubscriptionLevel = parsed.subscriptionLevel; | ||
| console.log(`[mock-api] Subscription level changed to: ${currentSubscriptionLevel}`); | ||
| res.writeHead(200, { 'Content-Type': 'application/json' }); | ||
| res.end(JSON.stringify({ ok: true, subscriptionLevel: currentSubscriptionLevel })); | ||
| }); |
There was a problem hiding this comment.
JSON.parse(body) here is not wrapped in a try/catch. If the request body is empty or invalid JSON, the exception will crash the mock API process (and likely the whole e2e run). Handle parse failures and return a 400 with a helpful error message instead of throwing.
| console.log(`[mock-api] POST usage: ${body}`); | ||
| try { | ||
| const reports = JSON.parse(body); | ||
| if (Array.isArray(reports)) { | ||
| usageReports.push(...reports); | ||
| } else { | ||
| usageReports.push(reports); | ||
| } | ||
| } catch (e) { | ||
| console.error(`[mock-api] Failed to parse usage body: ${e.message}`); | ||
| } | ||
| res.writeHead(200, { 'Content-Type': 'application/json' }); | ||
| res.end(JSON.stringify({ ok: true })); | ||
| }); |
There was a problem hiding this comment.
If parsing the usage body fails, the handler logs an error but still returns 200 { ok: true }. That can mask proxy/reporting bugs and make tests falsely pass. Consider returning a 400 on JSON parse errors (or at least ok: false) so failures are surfaced.
| // Wait for the proxy's usage report interval (configured at 10s in docker-compose) | ||
| // plus a small buffer for the HTTP call to complete | ||
| await sleep(12000); | ||
|
|
||
| const reports = await getUsageReports(); |
There was a problem hiding this comment.
This test uses a fixed sleep(12000) to wait for usage reporting. That makes the suite slower and can still be flaky if the report interval drifts. Prefer polling getUsageReports() until a new relevant report arrives (with an overall timeout based on the configured report interval) so the test proceeds as soon as the report is available.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docker-compose.yml (1)
248-289:⚠️ Potential issue | 🟡 MinorAdd readiness healthchecks before relying on this E2E topology.
The new services still use short-form
depends_on, so Compose starts containers in order but does not wait for Postgres auth, the mock API, or the proxy health endpoint to be ready. This can make proxy E2E startup flaky.Suggested direction
testPg-proxy-e2e: image: postgres:16 @@ command: postgres -c 'max_connections=300' -c 'password_encryption=md5' + healthcheck: + test: ["CMD-SHELL", "pg_isready -U postgres -d postgres"] + interval: 5s + timeout: 5s + retries: 20 proxy-mock-api: @@ - depends_on: - - testPg-proxy-e2e + healthcheck: + test: ["CMD-SHELL", "wget --spider -q http://localhost:3333/healthz || nc -z localhost 3333"] + interval: 5s + timeout: 5s + retries: 20 + depends_on: + testPg-proxy-e2e: + condition: service_healthy postgres-proxy: @@ - depends_on: - - proxy-mock-api - - testPg-proxy-e2e + healthcheck: + test: ["CMD-SHELL", "wget --spider -q http://localhost:8080/healthz || nc -z localhost 5432"] + interval: 5s + timeout: 5s + retries: 20 + depends_on: + proxy-mock-api: + condition: service_healthy + testPg-proxy-e2e: + condition: service_healthyAdjust the probe command to tools actually available in each image.
Run this read-only check to confirm whether the proxy topology still lacks health-gated dependencies:
#!/bin/bash # Verifies healthcheck and depends_on shape for the Postgres proxy E2E services. python3 - <<'PY' from pathlib import Path import re text = Path("docker-compose.yml").read_text() def service_block(name: str) -> str: pattern = rf"^ {re.escape(name)}:\n(?P<body>.*?)(?=^ [A-Za-z0-9_.-]+:|\Z)" match = re.search(pattern, text, re.M | re.S) return match.group("body") if match else "" for service in ["testPg-proxy-e2e", "proxy-mock-api", "postgres-proxy"]: body = service_block(service) print(f"\n[{service}]") print("healthcheck:", "present" if re.search(r"^\s+healthcheck:", body, re.M) else "missing") print("service_healthy:", "present" if "condition: service_healthy" in body else "missing") print("short depends_on list:", "present" if re.search(r"^\s+depends_on:\n\s+- ", body, re.M) else "not detected") PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` around lines 248 - 289, The Compose services lack healthchecks so startup order via depends_on is flaky; add healthcheck sections and make postgres-proxy depend on healthy services: for testPg-proxy-e2e add a healthcheck using pg_isready (service name testPg-proxy-e2e), for proxy-mock-api add a healthcheck that curls its health endpoint (proxy-mock-api, use MOCK_API_PORT/3333 and e.g. /health or /ready), and for postgres-proxy add a healthcheck that probes its PROXY_HEALTH_ADDR (port 8080) and change postgres-proxy's depends_on to wait for service_healthy for both testPg-proxy-e2e and proxy-mock-api so the proxy only starts when both backends report healthy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docker-compose.yml`:
- Around line 248-289: The Compose services lack healthchecks so startup order
via depends_on is flaky; add healthcheck sections and make postgres-proxy depend
on healthy services: for testPg-proxy-e2e add a healthcheck using pg_isready
(service name testPg-proxy-e2e), for proxy-mock-api add a healthcheck that curls
its health endpoint (proxy-mock-api, use MOCK_API_PORT/3333 and e.g. /health or
/ready), and for postgres-proxy add a healthcheck that probes its
PROXY_HEALTH_ADDR (port 8080) and change postgres-proxy's depends_on to wait for
service_healthy for both testPg-proxy-e2e and proxy-mock-api so the proxy only
starts when both backends report healthy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a600598d-2146-4509-86d8-0004d01639d5
📒 Files selected for processing (5)
docker-compose.tst.ymldocker-compose.ymlfrontend/src/app/components/connect-db/connect-db.component.cssfrontend/src/app/components/connect-db/connect-db.component.tsfrontend/src/app/validators/connection-string.validator.ts
💤 Files with no reviewable changes (1)
- frontend/src/app/components/connect-db/connect-db.component.css
✅ Files skipped from review due to trivial changes (2)
- frontend/src/app/validators/connection-string.validator.ts
- frontend/src/app/components/connect-db/connect-db.component.ts
Summary by CodeRabbit
New Features
Tests
Chores