control and data plane decoupling and refactor - #11
Merged
Conversation
Contributor
|
Warning Review limit reached
Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (19)
Note
|
| Layer / File(s) | Summary |
|---|---|
Config and dependency updates *.env.example, Makefile, pyproject.toml, services/api/pyproject.toml, libs/pipeline/pyproject.toml, libs/patches/pyproject.toml |
Adds DB_ENCRYPTION_KEY, a dev-worker target, new workspace/package dependencies, and updated lint/coverage settings. |
Paramiko patch package libs/patches/* |
Adds the patches package and a Paramiko monkey-patch module that applies legacy SSH algorithm support on import. |
Backend database contracts
| Layer / File(s) | Summary |
|---|---|
Encryption and schema models libs/database/src/database/encryption.py, libs/database/src/database/migrations/*, libs/database/src/database/models/* |
Adds Fernet-based database encryption and updates global/tenant migrations plus ORM models for SFTP partners, webhooks, and routes. |
Domain and repository contracts services/api/src/api/domain/models.py, services/api/src/api/ports/repository.py, services/api/src/api/ports/sftp_tester.py |
Updates SFTP/route command models, expands control-plane repository ports, empties the data-plane port, and adds the SFTP tester protocol. |
Backend API and processing
| Layer / File(s) | Summary |
|---|---|
Control-plane repository and provisioning services/api/src/api/adapters/repository.py, services/api/src/api/core/provisioning.py, services/api/src/api/adapters/vault.py, libs/pipeline/src/pipeline/adapters/repository.py |
Moves SFTP, webhook, and route persistence into the control plane, encrypts stored passwords, and rewires provisioning and webhook lookup around the new names and fields. |
DTOs, tester, and dependency wiring services/api/src/api/adapters/http/dtos.py, services/api/src/api/adapters/paramiko_sftp_tester.py, services/api/src/api/dependencies.py |
Adds SFTP connection testing DTOs and adapter wiring, and expands partner/route request and response shapes. |
Trading-partners, webhooks, routes, and main wiring services/api/src/api/main.py, services/api/src/api/routers/trading_partners/*, services/api/src/api/routers/webhooks/*, services/api/src/api/routers/routes.py |
Replaces the legacy partners router with trading-partners and webhooks routers, adds route update/delete endpoints, and updates app wiring and validation handling. |
API fakes and tests services/api/tests/* |
Updates fakes and tests for renamed methods, new routes, and additional route and partner fields. |
Pipeline and worker
| Layer / File(s) | Summary |
|---|---|
Pipeline delivery and webhook lookup libs/pipeline/src/pipeline/adapters/{repository,sftp}.py, libs/pipeline/src/pipeline/core/deliver.py, libs/pipeline/src/pipeline/ports/*, libs/pipeline/tests/* |
Adds SSH client helper logic, renames webhook lookup to get_webhook, and updates SFTP delivery to use client keys and outbound remote paths. |
Worker replication and sync deletes services/worker/src/worker/provision/main.py, services/worker/tests/test_provision_worker.py |
Replicates SFTP, webhook, and route rows into tenant shards, adds delete synchronization, and expands the worker test flow. |
Frontend
| Layer / File(s) | Summary |
|---|---|
Shared UI primitives frontend/web/src/components/ui/*, frontend/web/src/hooks/*, frontend/web/src/index.css |
Adds shared table, modal, searchable select, radio group, skeleton, toast, and mutation helpers, and updates popover/combobox behavior. |
Endpoints feature frontend/web/src/features/endpoints/*, frontend/web/src/routes/tenant/endpoints.tsx |
Adds endpoint types, repository/hooks, create modal, table, and the tenant endpoints route. |
Partners feature frontend/web/src/features/partners/*, frontend/web/src/routes/{platform,tenant}/partners.tsx, frontend/web/src/routes/tenant.tsx |
Splits partner types into AS2 and SFTP variants, adds SFTP create/test UI, and rewires the partner pages around the new table/modal components. |
Routes feature frontend/web/src/features/routes/*, frontend/web/src/routes/tenant/routes.tsx |
Adds route types, repository/hooks, context, create/detail/table UI, and rewires the tenant routes page around the new routes flow. |
Estimated code review effort: 5 (Critical) | ~120 minutes
Possibly related PRs
- pramodnarayana/soopaedi#4: Both PRs change
services/worker/src/worker/provision/main.pyand its tenant-config replication/outbox flow. - pramodnarayana/soopaedi#5: This PR removes/replaces the dashboard route components that were introduced there with the new routes context/table flow.
- pramodnarayana/soopaedi#9: Both PRs touch the trading-partners feature contracts, hooks, and API wiring for AS2/SFTP partner flows.
Poem
I hop through routes with joyful ears,
Webhooks ping and tables cheer,
Fernet keys and SFTP seams,
Dance like carrots in my dreams,
🐇 A tidy PR! Hooray for beams!
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title matches the main change: a broad control/data-plane decoupling and refactor across API, frontend, and database layers. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
control-data-plane-refactor
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 @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 40
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
libs/pipeline/src/pipeline/adapters/repository.py (1)
179-190: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftThread the SFTP host key through the delivery flow.
SFTPPartnerhas no host-key field, butDeliveryService._deliver_sftpalways passeshost_key=NoneandParamikoSftpDeliveryAdapterrejects that path, so outbound SFTP delivery cannot succeed as written.🤖 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 `@libs/pipeline/src/pipeline/adapters/repository.py` around lines 179 - 190, Thread the SFTP host key through the partner/repository delivery flow so outbound SFTP can authenticate successfully. Update the SFTP partner mapping in the repository adapter to include a host-key field from the stored record, then propagate that value through `DeliveryService._deliver_sftp` instead of always passing None. Make sure `ParamikoSftpDeliveryAdapter` receives the real host key and the `SFTPPartner` model exposes the corresponding property used by the delivery path.libs/pipeline/tests/test_delivery_service.py (1)
88-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test coverage for the vault-backed SFTP credential path.
credentials_vault_refis set on the partner fixture, butmake_service(...)doesn't pass avault, soself.vaultisNoneand the new vault-fetch/client_keybranch inDeliveryService._deliver_sftpis never actually exercised — the test still passes via the plain password/""path. Adding a variant with a fake vault would have caught theclient_keybytes/str type bug indeliver.py.🤖 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 `@libs/pipeline/tests/test_delivery_service.py` around lines 88 - 123, The SFTP delivery test does not exercise the vault-backed credential path, so the new `DeliveryService._deliver_sftp` logic for `credentials_vault_ref` and `client_key` is untested. Update `test_delivery_service_outbound_sftp` (or add a companion test) to pass a fake vault into `make_service(...)`, configure `FakeSftpDeliveryAdapter` to assert the vault-fetched secret is used, and verify the `client_key` branch is taken instead of falling back to the plain password path.services/api/src/api/adapters/repository.py (1)
233-251: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPersist
cmd.passwordinupdate_sftp_partner. The router passespassword, but this method never encrypts or stores it inpassword_encrypted, so SFTP password updates are dropped and the old secret keeps being used.🤖 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 `@services/api/src/api/adapters/repository.py` around lines 233 - 251, The update_sftp_partner flow in Repository currently ignores cmd.password, so SFTP password changes are not persisted. Update the existing partner-mutation block to handle cmd.password alongside the other fields by encrypting/storing it into password_encrypted, using the same update pattern as the other attributes before session.flush() is called.services/api/src/api/routers/trading_partners/platform/as2_partnerships.py (2)
156-168: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDelete handler swallows all errors into a generic 500, unlike its create/update siblings.
create_platform_as2_partnership/update_platform_as2_partnershipmapValueError→400 andIntegrityError→400, butdelete_platform_as2_partnershipcatches bareExceptionand always returns 500. A delete of a non-existent partnership or one still referenced by a route (FK violation) would incorrectly surface as "internal server error" instead of 404/400, unlikedelete_sftp_partnerinsftp.pywhich correctly mapsIntegrityError→400.🛠️ Proposed fix
+ from sqlalchemy.exc import IntegrityError + try: async with uow: await uow.control_plane.delete_as2_partnership( tenant_id=0, partnership_id=partnership_id ) await uow.commit() - except Exception as err: + except ValueError as err: + raise HTTPException(status_code=404, detail=str(err)) from err + except IntegrityError as err: + raise HTTPException( + status_code=400, detail="Partnership is in use and cannot be deleted." + ) from err + except Exception as err: logger.exception("Internal error deleting platform AS2 partnership") raise HTTPException(status_code=500, detail="An internal server error occurred.") from err🤖 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 `@services/api/src/api/routers/trading_partners/platform/as2_partnerships.py` around lines 156 - 168, The delete handler in delete_platform_as2_partnership is over-catching errors and always returning a generic 500, unlike create_platform_as2_partnership, update_platform_as2_partnership, and delete_sftp_partner. Update its exception handling to map expected domain/database failures like ValueError and IntegrityError to appropriate client responses (for example 400/404 as used elsewhere), and reserve the broad Exception catch for truly unexpected failures. Keep the existing flow around uow, uow.control_plane.delete_as2_partnership, and uow.commit, but align the response mapping with the sibling handlers.
172-200: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winBypasses the control-plane repository abstraction with a direct ORM query.
Every other handler in this file goes through
uow.control_plane, butlist_platform_as2_partnershipsqueriesAS2Partnershipdirectly viauow.global_session. This breaks the hexagonal/repository boundary this PR is establishing elsewhere, and makes the endpoint untestable againstFakeControlPlaneRepositorylike the rest of the suite.🤖 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 `@services/api/src/api/routers/trading_partners/platform/as2_partnerships.py` around lines 172 - 200, The list_platform_as2_partnerships handler is bypassing the control-plane repository abstraction by querying AS2Partnership directly through uow.global_session. Update this endpoint to use uow.control_plane in the same style as the other handlers in this module, and move the lookup into the repository layer so it can be exercised with FakeControlPlaneRepository. Keep the response mapping in list_platform_as2_partnerships unchanged, but fetch the partnerships through a repository method instead of a direct ORM select.services/api/tests/api_fakes.py (1)
247-299: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused SFTP/webhook helpers from
FakeDataPlaneRepository
DataPlaneRepositoryPortis empty, and SFTP/webhook creation and lookup go throughservice.global_repoinstead. These methods only duplicateFakeControlPlaneRepositoryand can be dropped.🤖 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 `@services/api/tests/api_fakes.py` around lines 247 - 299, Remove the unused SFTP/webhook helper methods from FakeDataPlaneRepository: the create_sftp_partner, get_sftp_partner, create_webhook, get_webhook, list_sftp_partners, get_sftp_partners_by_ids, list_webhooks, and get_webhooks_by_ids methods are duplicating FakeControlPlaneRepository and are not part of DataPlaneRepositoryPort. Update the FakeDataPlaneRepository class so it only contains the data-plane behavior it actually needs, and leave SFTP/webhook CRUD in the control-plane fake or service.global_repo path.
🤖 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.
Inline comments:
In @.env.example:
- Line 29: The .env.example entry for DB_ENCRYPTION_KEY is a real Fernet key,
which should not be committed. Replace the hardcoded value with a non-functional
placeholder in the .env.example template and add a short note near
DB_ENCRYPTION_KEY explaining that users must generate their own key; use the
DB_ENCRYPTION_KEY symbol to locate the secret placeholder.
In `@frontend/web/src/components/ui/data-table.tsx`:
- Line 13: The `renderExpandedRow` prop on the data table component is using
`any`, which drops the generic type safety for downstream consumers. Update the
`DataTable` component signature to use `TData` for `renderExpandedRow` instead
of `any`, and ensure the callback type matches the existing generic row type
used by the table so callers like `As2PartnerDetails` and `RouteDetails` receive
a properly typed row.
- Around line 70-79: The expandable row behavior in data-table is mouse-only;
update the table row in the data-table component to be keyboard accessible when
renderExpandedRow is enabled. Add appropriate accessibility semantics on the
<tr> used for row toggling, including a focusable tabIndex, a suitable role, and
an onKeyDown handler that triggers row.toggleExpanded() for Enter/Space, while
keeping the existing onClick behavior and preserving the expanded state styling.
In `@frontend/web/src/components/ui/toast.tsx`:
- Around line 10-14: The `toast` className in `ToastProvider` is overriding
Sonner’s `richColors` styling with base background and foreground utilities,
which prevents the variant colors from showing. Update the
`toastOptions.classNames.toast` styling so the default `bg-background` and
`text-foreground` only apply to the base state and do not override `richColors`
variants, keeping the existing `ToastProvider` setup and `classNames.toast`
selector intact.
In `@frontend/web/src/features/endpoints/api/endpointsApi.ts`:
- Around line 15-40: The fetchApi method is losing useful FastAPI validation
details and has no timeout handling. Update fetchApi to safely extract a
readable message from errData.detail even when it is an array or object, and
format it into a string before throwing the Error so 422 responses do not become
[object Object]. Also align its behavior with partnersApi.ts's request() by
adding an AbortController-based request timeout and ensuring the fetch call is
canceled and surfaced as a request timeout error.
In `@frontend/web/src/features/endpoints/api/endpointsHooks.ts`:
- Around line 18-38: The useToastMutation helper is duplicated and should be
shared to avoid drift between endpointsHooks and partnerHooks. Extract the
common mutation/toast/query invalidation logic into a single reusable hook with
the same behavior (success toast, error toast, queryClient.invalidateQueries
handling), then update the existing useToastMutation references in
endpointsHooks and partnerHooks to import and use that shared implementation.
In `@frontend/web/src/features/partners/components/CreateSftpPartnerModal.tsx`:
- Line 24: The CreateSftpPartnerModal state is not fully reset when the modal is
closed and reopened because reset() only clears the form and leaves testResult
behind. Update the modal reset/close flow in CreateSftpPartnerModal and the
related form handlers so testResult is also set back to null whenever the form
is reset or the modal is dismissed, ensuring no stale connection test message
appears after reopening.
In `@frontend/web/src/features/partners/components/PartnershipDetails.tsx`:
- Around line 16-21: The PartnershipDetails component is bypassing its exported
PartnershipDetailsProps and weakening type safety by using an inline prop type
with availablePartners: any[]. Update PartnershipDetails to use
PartnershipDetailsProps directly, add the missing onCancel prop to that
interface, and keep the availablePartners item shape aligned with the fields
accessed in the component (.id, .name, .type, .is_local). This will restore
compile-time checking in PartnershipDetails and prevent the exported props type
from drifting out of sync.
In `@frontend/web/src/features/partners/components/SftpPartnerDetails.tsx`:
- Around line 85-95: The Cancel handler in SftpPartnerDetails is only calling
reset(), which leaves a previous testResult value behind so the test-connection
banner can persist after canceling. Update the Button onClick flow in
SftpPartnerDetails to clear testResult as part of the cancel/reset path,
alongside reset() and the optional onCancel callback, matching the behavior
expected for CreateSftpPartnerModal.
- Around line 20-30: The `SftpPartnerDetails` form is treating `port` as a
string in `useForm`, which causes dirty tracking and submit payload comparisons
to think it changed unnecessarily. Update the `port` field registration in
`SftpPartnerDetails` to use `valueAsNumber: true`, and ensure the submit path
uses the numeric `port` value directly so the form state and diff logic stay
aligned.
In `@frontend/web/src/features/routes/components/CreateRouteModal.tsx`:
- Around line 184-214: The destination option mapping in CreateRouteModal’s
SearchableSelect is duplicated in RouteDetails, so the two flows can drift
apart. Extract the shared endpoint/partner-to-option transformation into a
reusable helper or hook such as useRouteDestinationOptions(direction, endpoints,
partners), and use it from CreateRouteModal while keeping the same filtering and
label/searchString behavior in both places.
In `@frontend/web/src/features/routes/components/RouteDetails.tsx`:
- Around line 45-63: Outbound target switching in RouteDetails.onSubmit only
sets the new partner field and leaves the previous AS2/SFTP field intact, so a
route can end up with both populated. Update the RouteDetails target swap logic
to explicitly clear the opposite partner field when setting
payload.as2_partner_id or payload.sftp_partner_id, or adjust the backend update
path that consumes this payload so it supports explicit clears instead of
treating null as UNSET. Use the existing onSubmit, targetId, initialTargetId,
and route.direction branching to locate the fix.
In `@frontend/web/src/features/routes/components/RoutesTable.tsx`:
- Line 1: The React import in RoutesTable is empty and should be removed as dead
leftover code. Update the top-level import in RoutesTable.tsx so it no longer
imports nothing from react, and keep only the actual imports used by the
RoutesTable component and any related symbols referenced in this file.
In `@frontend/web/src/hooks/use-toast.tsx`:
- Around line 169-181: The copy handler in the button inside use-toast should
handle the async result of navigator.clipboard.writeText instead of firing the
success toast unconditionally. Update the onClick logic to await or chain the
writeText promise, add error handling for rejected clipboard writes, and only
call sonnerToast.success when the copy actually succeeds; on failure, surface an
error state or toast so the user gets accurate feedback.
In `@libs/database/src/database/encryption.py`:
- Around line 14-33: The `DatabaseEncryption.fernet` lazy initializer is caching
a missing `DB_ENCRYPTION_KEY` by setting `_initialized` even when no key is
available, so the singleton never recovers after the env var is populated later.
Update the `fernet` property in `DatabaseEncryption` so it only marks
initialization complete after a valid `Fernet` is created, and keeps retrying on
subsequent accesses when the key is absent or invalid; preserve the existing
`encrypt` guard behavior.
In
`@libs/database/src/database/migrations/global/versions/227fb01f972f_global_initial_schema.py`:
- Around line 218-263: The route-to-partner foreign keys in the global initial
schema use the default delete behavior, which is inconsistent with the tenant
schema’s cascade policy. Update the foreign key constraints in the
inbound_routes and outbound_routes table definitions to match the tenant-backed
behavior by enabling cascading deletes for the partner references, using the
relevant sa.ForeignKeyConstraint entries in the global_initial_schema migration.
In `@libs/patches/src/patches/paramiko.py`:
- Around line 42-56: The Paramiko compatibility patch in patches.paramiko is
accessing private Transport and RSAKey internals that no longer exist in
Paramiko 5.0+, causing import-time AttributeError. Update the patch logic to
detect Paramiko version or the presence of Transport._key_info,
Transport._preferred_keys, Transport._preferred_pubkeys, and RSAKey.HASHES
before mutating them, and skip the ssh-rsa backfill when those symbols are
unavailable; alternatively enforce a dependency pin below 5.0.0 in the patch’s
setup path.
In `@libs/pipeline/src/pipeline/adapters/sftp.py`:
- Around line 41-44: In the SFTP helper around set_missing_host_key_policy,
remove the silent AutoAddPolicy fallback for empty or malformed host_key_string
values and make the host key validation fail explicitly instead. Update the
logic in the client setup path so that only a valid parsed host key is accepted,
and have the reusable helper raise or reject when host_key_string cannot be
split into the expected parts rather than trusting any server key.
- Around line 28-44: The host key pinning logic in the SFTP adapter stores the
key under the bare host name, which breaks Paramiko lookups for non-22 ports.
Update the host key registration in the client setup code that calls
client.get_host_keys().add so it uses the port-aware host identifier (the same
format Paramiko expects for [host]:port) before applying RejectPolicy, while
keeping the existing key parsing and policy selection behavior intact.
- Around line 46-54: The SFTP connection setup in the helper is forcing RSA auth
fallback for every connection by always setting disabled_algorithms in
connect_kwargs. Update the connection logic in the SFTP adapter helper to make
this legacy ssh-rsa fallback opt-in only for known legacy partners, and keep the
default path using rsa-sha2 when the server supports it. Use the existing SFTP
connection builder symbols in this module to gate the patch based on
partner-specific configuration rather than applying it universally.
In `@libs/pipeline/src/pipeline/core/deliver.py`:
- Line 152: The `_deliver_sftp` flow is never passing a real host key into
`get_ssh_client`, so outbound SFTP still uses the fallback trust-on-first-use
policy. Update `_deliver_sftp` to read the partner’s configured host-key value
and thread it through the existing `host_key` parameter when calling
`get_ssh_client`, using the new `host_key_string` support in
`libs/pipeline/src/pipeline/adapters/sftp.py` so verified host-key pinning is
actually applied.
In `@services/api/src/api/adapters/http/dtos.py`:
- Around line 198-216: UpdateRouteRequest currently lets callers set
webhook_partner_id, as2_partner_id, and sftp_partner_id together, bypassing the
“exactly one destination” rule enforced on create requests. Add the same
cross-field validation to UpdateRouteRequest in dtos.py, using a validator
analogous to check_exactly_one_destination so only one of those partner IDs can
be present at a time. Keep the validation aligned with
CreateInboundRouteRequest/CreateOutboundRouteRequest and ensure the
UpdateRouteRequest model rejects multiple destination IDs in one payload.
- Around line 291-305: RouteItemResponse.status is using mixed-case defaults
while the API expects consistent uppercase values. Update the default status in
RouteItemResponse so it matches the existing ACTIVE/INACTIVE convention, and
verify any code that constructs or serializes RouteItemResponse keeps the same
casing for status across the DTO.
In `@services/api/src/api/adapters/repository.py`:
- Around line 372-402: Revalidate partner tenant ownership during route PATCH
updates in update_inbound_route and update_outbound_route, since these paths
currently assign partner UUIDs without the create-time tenant check. Before
flushing changes, reuse the existing partner lookup/validation logic for fields
like isa_sender_id, isa_receiver_id, webhook_partner_id, as2_partner_id, and
sftp_partner_id to ensure each referenced partner belongs to the same tenant. If
a referenced partner is invalid or cross-tenant, reject the update rather than
persisting the route.
In `@services/api/src/api/core/provisioning.py`:
- Around line 31-37: ProvisioningService currently takes an unused tenant_repo
argument, and the constructor is being called with None in at least one place,
so remove tenant_repo from ProvisioningService.__init__ and from any wiring or
call sites that pass it. Update the class initialization to keep only the
actually used ControlPlaneRepositoryPort dependency via global_repo, and adjust
any related references so the service no longer requires a non-Optional
data-plane repository unless a real data-plane write path is added back.
In `@services/api/src/api/main.py`:
- Around line 62-83: The global RequestValidationError handler in
validation_exception_handler is logging raw request payloads with print(), which
can expose secrets from sensitive DTOs. Remove the body dump entirely or switch
to structured logging via the app logger with strict redaction of sensitive
fields before emitting anything. Keep only minimal context such as
request.url.path and exc.errors(), and avoid parsing/printing request.body()
unless it is fully sanitized.
In `@services/api/src/api/ports/repository.py`:
- Line 41: Normalize the parameter order across the `*_by_ids` methods in
`Repository` so `get_as2_partners_by_ids`, `get_sftp_partners_by_ids`, and
`get_webhooks_by_ids` all take `tenant_id` before `ids`. Update the `Repository`
interface signature and the matching implementation/call site in
`provisioning.py` to use the same ordering, keeping the positional call aligned
with the revised method names.
- Around line 79-84: `ProvisioningService` is carrying an unused `tenant_repo`
dependency that does not match current call sites. Update the
`ProvisioningService` constructor and the `delete_platform_as2_partner` flow so
the signature reflects actual usage: either remove `tenant_repo` from
`ProvisioningService` entirely or make it `Optional[DataPlaneRepositoryPort]`
and handle `None` safely. Use the `ProvisioningService` class and
`DataPlaneRepositoryPort` protocol as the reference points while adjusting any
initialization or type hints that currently assume a required repository.
In `@services/api/src/api/routers/trading_partners/as2.py`:
- Around line 46-60: The AS2 export in the trading_partners router swallows
vault retrieval errors in the private key lookup paths, so the endpoint can
return a misleadingly successful response with missing key data. Update the
logic around vault.retrieve_private_key in the response-building code to surface
these failures to the caller instead of only logging them, using the existing
trading_partners/as2 response flow so an incomplete export cannot return 200
unnoticed.
- Around line 22-33: The AS2 certificate export path is only looking up partners
within the caller’s tenant, so global AS2 partners are missed and return 404.
Update export_as2_certificates to use the same tenant/platform lookup pattern as
list_trading_partners, checking tenant_id and tenant 0 via get_as2_partner or
equivalent control plane access, and then proceed with the certificate export
for a matched global partner.
In `@services/api/src/api/routers/trading_partners/platform/as2_partners.py`:
- Around line 34-86: The self-signed private key stored by
create_platform_as2_partner can be left behind if later partner creation fails.
Add cleanup around the vault.store_private_key path so that when
CreateAS2TradingPartnerCmd creation, uow.control_plane.create_as2_identity, or
uow.commit fails, the newly created Vault secret is deleted/rolled back before
re-raising the error. Keep the fix localized to create_platform_as2_partner and
use the existing private_key_vault_ref flow to track and clean up the generated
secret.
- Around line 159-169: The delete flow in the AS2 partner endpoint is using a
fragile string check on the caught exception type and falls back to exposing raw
exception text. Update the exception handling in the partner deletion path
around ProvisioningService.delete_as2_partner and uow.commit to catch
IntegrityError directly, matching the create endpoint’s pattern, and keep the
400 response for in-use partners; for all other errors, raise a generic 500
HTTPException without including str(e) in the detail.
- Around line 89-112: The list_platform_as2_partners endpoint is bypassing the
control-plane abstraction by querying AS2Partner directly through
uow.global_session.execute(select(...)); update it to call
uow.control_plane.list_as2_partners(0) and build AS2TradingPartnerResponse from
that result instead. Keep the response mapping in list_platform_as2_partners,
but remove the direct database access so this router stays aligned with the
control-plane pattern used elsewhere in trading_partners/__init__.py. If
AS2Partner and select are no longer referenced in this file, drop those unused
imports too.
- Around line 122-150: The update flow in as2_partners.py’s AS2 partner handler
catches every unexpected exception and turns it into HTTP 400 with the raw
exception text. Narrow the error handling around
update_as2_identity/get_as2_partner so only validation/client errors map to 400,
and let unexpected failures propagate as server errors or be converted to a
generic 500 without exposing str(e). Keep the explicit HTTPException re-raise
path, and preserve the existing
AS2TradingPartnerResponse/UpdateAS2TradingPartnerCmd logic.
In `@services/api/src/api/routers/trading_partners/sftp.py`:
- Around line 42-54: There is duplicated vault secret resolution and SSH key
decoding logic in both test_sftp_connection and test_existing_sftp_connection,
which should be centralized to prevent drift. Extract the shared
vault-fetch/client_key_string handling into a helper (for example, a private
resolver used by both endpoints) that returns the decoded key and any error
reason, then update both call sites to use it while preserving the existing
exception-to-response behavior.
- Around line 126-220: Handle IntegrityError and ValueError in
create_sftp_partner and update_sftp_partner so duplicate names or invalid
provisioning inputs return a user-facing 400 instead of bubbling up as 500s. Add
the same try/except pattern used in delete_sftp_partner around the
service.create_sftp_partner/service.update_sftp_partner flow and the uow.commit
call, then translate those exceptions into HTTPException with a clear detail
message. Use the create_sftp_partner and update_sftp_partner handler names to
locate the two endpoints consistently.
In `@services/api/src/api/routers/webhooks/webhook.py`:
- Around line 19-32: The webhook creation and delivery path still permits SSRF
by accepting internal destinations; update create_webhook and the
HttpxDeliveryAdapter delivery flow to reject RFC1918/ULA targets in addition to
loopback/link-local. Use the existing CreateWebhookRequest validation and the
code that posts partner["url"] to re-check the resolved IP(s) at send time, and
block any private or otherwise internal address before creating or dispatching
the webhook.
In `@services/api/tests/api_fakes.py`:
- Around line 116-211: The fake store methods are repeating lazy initialization
for sftp_partners and webhooks in multiple places, which should be centralized.
Add these collections to the fake class initializer in api_fakes.py, then remove
the per-method hasattr checks from create_sftp_partner, create_webhook,
get_sftp_partner, get_webhook, and any similar helpers that currently re-create
the lists.
- Around line 152-169: Mirror tenant scoping in
FakeControlPlaneRepository.get_all_routes by recording tenant_id on routes
created in create_inbound_route and create_outbound_route, then filtering the
returned inbound and outbound lists by that tenant_id instead of returning every
stored route. Update the FakeRoute usage and any related in-memory route storage
in api_fakes.py so the fake matches the SQLAlchemy repository’s tenant-isolated
behavior.
In `@services/worker/src/worker/provision/main.py`:
- Around line 279-295: The delete order in the provisioning flow needs to be
reversed so dependent tenant records are removed before their parent partner
records. Update the sequence in the sync_deletes calls within the provisioning
logic to delete TenantAS2Partnership, TenantInboundRoute, and
TenantOutboundRoute before TenantAS2Partner, TenantSFTPPartner, and
TenantWebhook. Keep the existing sync_deletes helper and model names, but
reorder the calls to avoid FK violations during tenant cleanup.
---
Outside diff comments:
In `@libs/pipeline/src/pipeline/adapters/repository.py`:
- Around line 179-190: Thread the SFTP host key through the partner/repository
delivery flow so outbound SFTP can authenticate successfully. Update the SFTP
partner mapping in the repository adapter to include a host-key field from the
stored record, then propagate that value through `DeliveryService._deliver_sftp`
instead of always passing None. Make sure `ParamikoSftpDeliveryAdapter` receives
the real host key and the `SFTPPartner` model exposes the corresponding property
used by the delivery path.
In `@libs/pipeline/tests/test_delivery_service.py`:
- Around line 88-123: The SFTP delivery test does not exercise the vault-backed
credential path, so the new `DeliveryService._deliver_sftp` logic for
`credentials_vault_ref` and `client_key` is untested. Update
`test_delivery_service_outbound_sftp` (or add a companion test) to pass a fake
vault into `make_service(...)`, configure `FakeSftpDeliveryAdapter` to assert
the vault-fetched secret is used, and verify the `client_key` branch is taken
instead of falling back to the plain password path.
In `@services/api/src/api/adapters/repository.py`:
- Around line 233-251: The update_sftp_partner flow in Repository currently
ignores cmd.password, so SFTP password changes are not persisted. Update the
existing partner-mutation block to handle cmd.password alongside the other
fields by encrypting/storing it into password_encrypted, using the same update
pattern as the other attributes before session.flush() is called.
In `@services/api/src/api/routers/trading_partners/platform/as2_partnerships.py`:
- Around line 156-168: The delete handler in delete_platform_as2_partnership is
over-catching errors and always returning a generic 500, unlike
create_platform_as2_partnership, update_platform_as2_partnership, and
delete_sftp_partner. Update its exception handling to map expected
domain/database failures like ValueError and IntegrityError to appropriate
client responses (for example 400/404 as used elsewhere), and reserve the broad
Exception catch for truly unexpected failures. Keep the existing flow around
uow, uow.control_plane.delete_as2_partnership, and uow.commit, but align the
response mapping with the sibling handlers.
- Around line 172-200: The list_platform_as2_partnerships handler is bypassing
the control-plane repository abstraction by querying AS2Partnership directly
through uow.global_session. Update this endpoint to use uow.control_plane in the
same style as the other handlers in this module, and move the lookup into the
repository layer so it can be exercised with FakeControlPlaneRepository. Keep
the response mapping in list_platform_as2_partnerships unchanged, but fetch the
partnerships through a repository method instead of a direct ORM select.
In `@services/api/tests/api_fakes.py`:
- Around line 247-299: Remove the unused SFTP/webhook helper methods from
FakeDataPlaneRepository: the create_sftp_partner, get_sftp_partner,
create_webhook, get_webhook, list_sftp_partners, get_sftp_partners_by_ids,
list_webhooks, and get_webhooks_by_ids methods are duplicating
FakeControlPlaneRepository and are not part of DataPlaneRepositoryPort. Update
the FakeDataPlaneRepository class so it only contains the data-plane behavior it
actually needs, and leave SFTP/webhook CRUD in the control-plane fake or
service.global_repo path.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 686ac5c6-9d84-4e5f-830a-b079308beca0
⛔ Files ignored due to path filters (2)
frontend/web/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamluv.lockis excluded by!**/*.lock
📒 Files selected for processing (96)
.env.exampleMakefilefrontend/web/package.jsonfrontend/web/src/components/ui/combobox.tsxfrontend/web/src/components/ui/data-table.tsxfrontend/web/src/components/ui/form-modal.tsxfrontend/web/src/components/ui/popover.tsxfrontend/web/src/components/ui/radio-group.tsxfrontend/web/src/components/ui/searchable-select.tsxfrontend/web/src/components/ui/skeleton.tsxfrontend/web/src/components/ui/toast.tsxfrontend/web/src/features/dashboard/components/ActiveRoutesTable.tsxfrontend/web/src/features/dashboard/components/RouteConfigCard.tsxfrontend/web/src/features/endpoints/api/endpointsApi.tsfrontend/web/src/features/endpoints/api/endpointsHooks.tsfrontend/web/src/features/endpoints/components/CreateWebhookEndpointModal.tsxfrontend/web/src/features/endpoints/components/EndpointsTable.tsxfrontend/web/src/features/endpoints/types.tsfrontend/web/src/features/partners/api/IPartnersRepository.tsfrontend/web/src/features/partners/api/partnerHooks.tsfrontend/web/src/features/partners/api/partnersApi.tsfrontend/web/src/features/partners/components/As2PartnerDetails.tsxfrontend/web/src/features/partners/components/As2PartnersTable.tsxfrontend/web/src/features/partners/components/CreatePartnerModal.tsxfrontend/web/src/features/partners/components/CreatePartnershipModal.tsxfrontend/web/src/features/partners/components/CreateSftpPartnerModal.tsxfrontend/web/src/features/partners/components/PartnersTable.tsxfrontend/web/src/features/partners/components/PartnershipDetails.tsxfrontend/web/src/features/partners/components/PartnershipsTable.tsxfrontend/web/src/features/partners/components/SftpPartnerDetails.tsxfrontend/web/src/features/partners/components/SftpPartnersTable.tsxfrontend/web/src/features/partners/components/SharedRowActions.tsxfrontend/web/src/features/partners/types.tsfrontend/web/src/features/platform/api/configHooks.tsfrontend/web/src/features/routes/api/routeHooks.tsfrontend/web/src/features/routes/api/routesApi.tsfrontend/web/src/features/routes/components/CreateRouteModal.tsxfrontend/web/src/features/routes/components/RouteDetails.tsxfrontend/web/src/features/routes/components/RoutesTable.tsxfrontend/web/src/features/routes/context/RoutesContext.tsxfrontend/web/src/features/routes/types.tsfrontend/web/src/hooks/use-toast.tsxfrontend/web/src/index.cssfrontend/web/src/routes/platform/partners.tsxfrontend/web/src/routes/tenant.tsxfrontend/web/src/routes/tenant/endpoints.tsxfrontend/web/src/routes/tenant/partners.tsxfrontend/web/src/routes/tenant/routes.tsxlibs/database/src/database/encryption.pylibs/database/src/database/migrations/global/versions/227fb01f972f_global_initial_schema.pylibs/database/src/database/migrations/tenant/versions/1cff61426f2b_tenant_initial_schema.pylibs/database/src/database/models/__init__.pylibs/database/src/database/models/control_plane.pylibs/database/src/database/models/data_plane.pylibs/patches/README.mdlibs/patches/pyproject.tomllibs/patches/src/patches/__init__.pylibs/patches/src/patches/paramiko.pylibs/pipeline/pyproject.tomllibs/pipeline/src/pipeline/adapters/repository.pylibs/pipeline/src/pipeline/adapters/sftp.pylibs/pipeline/src/pipeline/core/deliver.pylibs/pipeline/src/pipeline/ports/repository.pylibs/pipeline/src/pipeline/ports/sftp.pylibs/pipeline/tests/fakes.pylibs/pipeline/tests/test_delivery_service.pylibs/pipeline/tests/test_sftp_adapter.pypyproject.tomlservices/api/pyproject.tomlservices/api/src/api/adapters/http/dtos.pyservices/api/src/api/adapters/paramiko_sftp_tester.pyservices/api/src/api/adapters/repository.pyservices/api/src/api/core/provisioning.pyservices/api/src/api/dependencies.pyservices/api/src/api/domain/models.pyservices/api/src/api/main.pyservices/api/src/api/ports/repository.pyservices/api/src/api/ports/sftp_tester.pyservices/api/src/api/routers/partners.pyservices/api/src/api/routers/routes.pyservices/api/src/api/routers/trading_partners/__init__.pyservices/api/src/api/routers/trading_partners/as2.pyservices/api/src/api/routers/trading_partners/platform/__init__.pyservices/api/src/api/routers/trading_partners/platform/as2_partners.pyservices/api/src/api/routers/trading_partners/platform/as2_partnerships.pyservices/api/src/api/routers/trading_partners/platform/config.pyservices/api/src/api/routers/trading_partners/sftp.pyservices/api/src/api/routers/webhooks/__init__.pyservices/api/src/api/routers/webhooks/webhook.pyservices/api/tests/api_fakes.pyservices/api/tests/test_api_repository.pyservices/api/tests/test_provisioning_core.pyservices/api/tests/test_routers_partners.pyservices/api/tests/test_routers_routes.pyservices/worker/src/worker/provision/main.pyservices/worker/tests/test_provision_worker.py
💤 Files with no reviewable changes (4)
- frontend/web/src/features/dashboard/components/RouteConfigCard.tsx
- frontend/web/src/features/dashboard/components/ActiveRoutesTable.tsx
- services/api/src/api/routers/partners.py
- frontend/web/src/features/partners/components/PartnersTable.tsx
Comment on lines
+70
to
+79
| <tr | ||
| className={`hover:bg-slate-50/50 transition-colors group ${renderExpandedRow ? 'cursor-pointer' : ''} ${row.getIsExpanded() ? 'bg-slate-50/50' : ''}`} | ||
| onClick={renderExpandedRow ? () => row.toggleExpanded() : undefined} | ||
| > | ||
| {row.getVisibleCells().map((cell) => ( | ||
| <td key={cell.id} className="px-6 py-4 align-middle"> | ||
| {flexRender(cell.column.columnDef.cell, cell.getContext())} | ||
| </td> | ||
| ))} | ||
| </tr> |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Expandable rows are not keyboard-accessible.
The row toggle is only wired to onClick; there's no role, tabIndex, or onKeyDown handler, so keyboard users cannot expand/collapse rows. Since this is a shared primitive used across partners, routes, and endpoints tables, this a11y gap propagates broadly.
♿ Proposed fix
<tr
className={`hover:bg-slate-50/50 transition-colors group ${renderExpandedRow ? 'cursor-pointer' : ''} ${row.getIsExpanded() ? 'bg-slate-50/50' : ''}`}
onClick={renderExpandedRow ? () => row.toggleExpanded() : undefined}
+ role={renderExpandedRow ? 'button' : undefined}
+ tabIndex={renderExpandedRow ? 0 : undefined}
+ onKeyDown={renderExpandedRow ? (e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ row.toggleExpanded();
+ }
+ } : undefined}
>📝 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.
Suggested change
| <tr | |
| className={`hover:bg-slate-50/50 transition-colors group ${renderExpandedRow ? 'cursor-pointer' : ''} ${row.getIsExpanded() ? 'bg-slate-50/50' : ''}`} | |
| onClick={renderExpandedRow ? () => row.toggleExpanded() : undefined} | |
| > | |
| {row.getVisibleCells().map((cell) => ( | |
| <td key={cell.id} className="px-6 py-4 align-middle"> | |
| {flexRender(cell.column.columnDef.cell, cell.getContext())} | |
| </td> | |
| ))} | |
| </tr> | |
| <tr | |
| className={`hover:bg-slate-50/50 transition-colors group ${renderExpandedRow ? 'cursor-pointer' : ''} ${row.getIsExpanded() ? 'bg-slate-50/50' : ''}`} | |
| onClick={renderExpandedRow ? () => row.toggleExpanded() : undefined} | |
| role={renderExpandedRow ? 'button' : undefined} | |
| tabIndex={renderExpandedRow ? 0 : undefined} | |
| onKeyDown={renderExpandedRow ? (e) => { | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| e.preventDefault(); | |
| row.toggleExpanded(); | |
| } | |
| } : undefined} | |
| > | |
| {row.getVisibleCells().map((cell) => ( | |
| <td key={cell.id} className="px-6 py-4 align-middle"> | |
| {flexRender(cell.column.columnDef.cell, cell.getContext())} | |
| </td> | |
| ))} | |
| </tr> |
🤖 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 `@frontend/web/src/components/ui/data-table.tsx` around lines 70 - 79, The
expandable row behavior in data-table is mouse-only; update the table row in the
data-table component to be keyboard accessible when renderExpandedRow is
enabled. Add appropriate accessibility semantics on the <tr> used for row
toggling, including a focusable tabIndex, a suitable role, and an onKeyDown
handler that triggers row.toggleExpanded() for Enter/Space, while keeping the
existing onClick behavior and preserving the expanded state styling.
Comment on lines
+15
to
+40
| private async fetchApi(path: string, options: RequestInit = {}) { | ||
| const res = await fetch(`/api/v1${path}`, { | ||
| ...options, | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| Authorization: `Bearer ${this.token}`, | ||
| ...options.headers, | ||
| }, | ||
| }); | ||
|
|
||
| if (!res.ok) { | ||
| let errMessage = 'API Request Failed'; | ||
| try { | ||
| const errData = await res.json(); | ||
| errMessage = errData.detail || errMessage; | ||
| } catch { | ||
| errMessage = res.statusText; | ||
| } | ||
| throw new Error(errMessage); | ||
| } | ||
|
|
||
| if (res.status === 204) { | ||
| return null; | ||
| } | ||
| return res.json(); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Error handling drops validation detail and lacks a request timeout.
errData.detail can be a non-string (FastAPI 422 responses return detail as an array per main.py's validation_exception_handler), and new Error(errMessage) will stringify that into an unhelpful [object Object] toast. Also, unlike partnersApi.ts's request(), this fetch has no abort/timeout, so a hung request never resolves.
🛠️ Proposed fix aligning with partnersApi.ts's request()
private async fetchApi(path: string, options: RequestInit = {}) {
- const res = await fetch(`/api/v1${path}`, {
- ...options,
- headers: {
- 'Content-Type': 'application/json',
- Authorization: `Bearer ${this.token}`,
- ...options.headers,
- },
- });
+ const controller = new AbortController();
+ const timeoutId = setTimeout(() => controller.abort(), 15000);
+ let res: Response;
+ try {
+ res = await fetch(`/api/v1${path}`, {
+ ...options,
+ headers: {
+ 'Content-Type': 'application/json',
+ Authorization: `Bearer ${this.token}`,
+ ...options.headers,
+ },
+ signal: controller.signal,
+ });
+ } finally {
+ clearTimeout(timeoutId);
+ }
if (!res.ok) {
let errMessage = 'API Request Failed';
try {
const errData = await res.json();
- errMessage = errData.detail || errMessage;
+ errMessage =
+ typeof errData.detail === 'string'
+ ? errData.detail
+ : errData.detail
+ ? JSON.stringify(errData.detail)
+ : errMessage;
} catch {
errMessage = res.statusText;
}
throw new Error(errMessage);
}📝 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.
Suggested change
| private async fetchApi(path: string, options: RequestInit = {}) { | |
| const res = await fetch(`/api/v1${path}`, { | |
| ...options, | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| Authorization: `Bearer ${this.token}`, | |
| ...options.headers, | |
| }, | |
| }); | |
| if (!res.ok) { | |
| let errMessage = 'API Request Failed'; | |
| try { | |
| const errData = await res.json(); | |
| errMessage = errData.detail || errMessage; | |
| } catch { | |
| errMessage = res.statusText; | |
| } | |
| throw new Error(errMessage); | |
| } | |
| if (res.status === 204) { | |
| return null; | |
| } | |
| return res.json(); | |
| } | |
| private async fetchApi(path: string, options: RequestInit = {}) { | |
| const controller = new AbortController(); | |
| const timeoutId = setTimeout(() => controller.abort(), 15000); | |
| let res: Response; | |
| try { | |
| res = await fetch(`/api/v1${path}`, { | |
| ...options, | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| Authorization: `Bearer ${this.token}`, | |
| ...options.headers, | |
| }, | |
| signal: controller.signal, | |
| }); | |
| } finally { | |
| clearTimeout(timeoutId); | |
| } | |
| if (!res.ok) { | |
| let errMessage = 'API Request Failed'; | |
| try { | |
| const errData = await res.json(); | |
| errMessage = | |
| typeof errData.detail === 'string' | |
| ? errData.detail | |
| : errData.detail | |
| ? JSON.stringify(errData.detail) | |
| : errMessage; | |
| } catch { | |
| errMessage = res.statusText; | |
| } | |
| throw new Error(errMessage); | |
| } | |
| if (res.status === 204) { | |
| return null; | |
| } | |
| return res.json(); | |
| } |
🤖 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 `@frontend/web/src/features/endpoints/api/endpointsApi.ts` around lines 15 -
40, The fetchApi method is losing useful FastAPI validation details and has no
timeout handling. Update fetchApi to safely extract a readable message from
errData.detail even when it is an array or object, and format it into a string
before throwing the Error so 422 responses do not become [object Object]. Also
align its behavior with partnersApi.ts's request() by adding an
AbortController-based request timeout and ensuring the fetch call is canceled
and surfaced as a request timeout error.
Comment on lines
+152
to
+169
| async def create_inbound_route(self, tenant_id: int, cmd: CreateInboundRouteCmd) -> uuid.UUID: | ||
| r_id = uuid.uuid4() | ||
| if not hasattr(self, "inbound_routes"): | ||
| self.inbound_routes = [] | ||
| self.inbound_routes.append(FakeRoute(r_id, cmd)) | ||
| return r_id | ||
|
|
||
| async def create_outbound_route(self, tenant_id: int, cmd: CreateOutboundRouteCmd) -> uuid.UUID: | ||
| r_id = uuid.uuid4() | ||
| if not hasattr(self, "outbound_routes"): | ||
| self.outbound_routes = [] | ||
| self.outbound_routes.append(FakeRoute(r_id, cmd)) | ||
| return r_id | ||
|
|
||
| async def get_all_routes(self, tenant_id: int) -> dict[str, list[Any]]: | ||
| inbound = getattr(self, "inbound_routes", []) | ||
| outbound = getattr(self, "outbound_routes", []) | ||
| return {"inbound": inbound, "outbound": outbound} |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "def get_all_routes|class SqlAlchemyControlPlaneRepository|FakeRoute|create_inbound_route|create_outbound_route|tenant_id" services/api -g '!**/__pycache__/**'Repository: pramodnarayana/soopaedi
Length of output: 42274
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '140,250p' services/api/tests/api_fakes.py
printf '\n==== REAL REPOSITORY ====\n'
sed -n '490,505p' services/api/src/api/adapters/repository.pyRepository: pramodnarayana/soopaedi
Length of output: 5271
Mirror tenant scoping in FakeControlPlaneRepository.get_all_routes The fake still returns every route and never records tenant_id, so tests can miss tenant-isolation bugs even though the SQLAlchemy repository filters by tenant.
🤖 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 `@services/api/tests/api_fakes.py` around lines 152 - 169, Mirror tenant
scoping in FakeControlPlaneRepository.get_all_routes by recording tenant_id on
routes created in create_inbound_route and create_outbound_route, then filtering
the returned inbound and outbound lists by that tenant_id instead of returning
every stored route. Update the FakeRoute usage and any related in-memory route
storage in api_fakes.py so the fake matches the SQLAlchemy repository’s
tenant-isolated behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
services/api/src/api/routers/routes.py (1)
117-135: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve explicit
nullon route PATCH requests
routes.pytreats everyNoneasUNSET, so an explicit clear forwebhook_id,as2_partner_id, orsftp_partner_idis dropped and the old partner stays attached. Apply omitted-vs-null handling in bothupdate_inbound_routeandupdate_outbound_route.🤖 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 `@services/api/src/api/routers/routes.py` around lines 117 - 135, The PATCH mapping in update_inbound_route and update_outbound_route is treating every None as UNSET, so explicit clears for webhook_id, as2_partner_id, and sftp_partner_id are lost and existing partner links remain. Update the request-to-command conversion in routes.py to distinguish omitted fields from explicit nulls, preserving null as a real value while still using UNSET only for absent fields, and apply the same handling consistently in both route update handlers and their UpdateInboundRouteCmd/UpdateOutboundRouteCmd construction.libs/pipeline/src/pipeline/core/deliver.py (1)
151-168: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftWire
host_keythrough SFTP provisioning.deliver.pynow relies onhost_key, butCreateSFTPPartnerCmd/UpdateSFTPPartnerCmd, the SFTP router, and the repository still drop that field, so API-created or updated partners can’t populate it for delivery.🤖 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 `@libs/pipeline/src/pipeline/core/deliver.py` around lines 151 - 168, The SFTP delivery path now reads partner.get("host_key"), but the partner CRUD and routing flow still discard that field. Update CreateSFTPPartnerCmd and UpdateSFTPPartnerCmd to accept/persist host_key, pass it through the SFTP router into the repository, and ensure the stored partner payload includes it so deliver() can forward it to sftp_delivery.deliver.libs/pipeline/src/pipeline/adapters/sftp.py (1)
67-74: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSupport ECDSA client keys.
client_key_stringonly falls back from RSA to Ed25519, so valid ECDSA private keys will fail to load beforeconnect(). Addparamiko.ECDSAKey.from_private_keyto the fallback chain.🤖 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 `@libs/pipeline/src/pipeline/adapters/sftp.py` around lines 67 - 74, The client key loading logic in the SFTP adapter currently falls back from RSA directly to Ed25519, which means valid ECDSA private keys are rejected before connect(). Update the key parsing in the code path that builds connect_kwargs by extending the fallback chain in client_key_string handling to try paramiko.ECDSAKey.from_private_key before Ed25519Key, keeping the existing key_io reset behavior between attempts.services/worker/src/worker/provision/main.py (2)
61-65: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReplicate
tenant_id=0AS2 partners before copying route references.Control-plane routes can reference platform AS2 partners validated with
tenant_id=0, but this replication query only includes tenant-local orNULLglobal AS2 rows. A replicated route/partnership that references a platform AS2 partner can fail tenant-shard FK checks unless the AS2 replication andsync_deletesAS2 special case also treattenant_id=0as globally valid.Proposed fix
stmt = select(GlobalAS2Partner).where( - (GlobalAS2Partner.tenant_id == tenant_id) | (GlobalAS2Partner.tenant_id.is_(None)) + (GlobalAS2Partner.tenant_id == tenant_id) + | (GlobalAS2Partner.tenant_id == 0) + | (GlobalAS2Partner.tenant_id.is_(None)) )Also update the AS2-specific valid-ID selection in
sync_deletesto includeGlobalAS2Partner.tenant_id == 0; otherwise those tenant copies can be deleted again during the same sync.Also applies to: 224-224, 259-259, 293-294
🤖 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 `@services/worker/src/worker/provision/main.py` around lines 61 - 65, The AS2 replication flow in main() currently treats only tenant-local and NULL rows as valid, so platform AS2 partners with tenant_id=0 are skipped and can break route/partnership FK validation. Update the AS2 partner selection used for replication to also include GlobalAS2Partner rows where tenant_id equals 0, and make the same tenant_id=0 adjustment in the AS2-specific valid-ID handling inside sync_deletes so replicated platform partners are not removed again during the same sync.
153-178: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPropagate
host_keyto the tenant shard.
host_keyis part of both SFTP partner schemas and is read by the delivery path for SFTP host-key verification, but the worker omits it from insert/update replication. This silently drops host-key pinning in the data plane.Proposed fix
port=global_sftp.port, username=global_sftp.username, + host_key=global_sftp.host_key, inbound_remote_path=global_sftp.inbound_remote_path, outbound_remote_path=global_sftp.outbound_remote_path, password_encrypted=global_sftp.password_encrypted, @@ "port": global_sftp.port, "username": global_sftp.username, + "host_key": global_sftp.host_key, "inbound_remote_path": global_sftp.inbound_remote_path, "outbound_remote_path": global_sftp.outbound_remote_path, "password_encrypted": global_sftp.password_encrypted,🤖 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 `@services/worker/src/worker/provision/main.py` around lines 153 - 178, The SFTP tenant-shard replication in the worker is dropping `host_key`, so host-key pinning is lost during provision updates. Update the upsert logic in `main.py` where the `global_sftp` values are copied into the tenant record to include `host_key` in both the `.values(...)` payload and the `.on_conflict_do_update(..., set_=...)` map, keeping it aligned with the partner schema and the delivery path’s verification needs.frontend/web/src/features/partners/components/SftpPartnerDetails.tsx (1)
68-83: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winIgnore stale connection-test responses.
The effect clears
testResultwhen fields change, but an in-flight test can still resolve afterward and show a result for the previous host/port/username/password. Track the latest test run and ignore older callbacks.Proposed fix
-import { useState, useEffect } from 'react'; +import { useState, useEffect, useRef } from 'react'; @@ const [testResult, setTestResult] = useState<{success: boolean; message: string} | null>(null); + const testRunId = useRef(0); @@ useEffect(() => { + testRunId.current += 1; setTestResult(null); }, [watchAll.host, watchAll.port, watchAll.username, watchAll.password]); @@ onClick={() => { const vals = getValues(); + const runId = ++testRunId.current; testConnection.mutate( @@ - onSuccess: (data: any) => setTestResult({ success: data.success, message: data.reason || 'Connection successful!' }), - onError: (error: any) => setTestResult({ success: false, message: error.response?.data?.detail || error.message || 'Connection failed' }) + onSuccess: (data: any) => { + if (runId !== testRunId.current) return; + setTestResult({ success: data.success, message: data.reason || 'Connection successful!' }); + }, + onError: (error: any) => { + if (runId !== testRunId.current) return; + setTestResult({ success: false, message: error.response?.data?.detail || error.message || 'Connection failed' }); + }🤖 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 `@frontend/web/src/features/partners/components/SftpPartnerDetails.tsx` around lines 68 - 83, The connection test callbacks in SftpPartnerDetails.tsx can still update testResult after the user changes fields, causing stale results from an older host/port/username/password to appear. Update the onClick flow around testConnection.mutate so each test run is tracked with a unique token or incrementing request id, and only the latest run is allowed to call setTestResult in onSuccess/onError. Use the existing SftpPartnerDetails component, testConnection, and setTestResult to locate the change.frontend/web/src/features/partners/components/CreateSftpPartnerModal.tsx (1)
260-273: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCopy-to-clipboard doesn't handle rejection; success toast fires unconditionally.
navigator.clipboard.writeText(testResult.message)isn't awaited or caught, so on failure (insecure context, permission denied) the "Copied to clipboard" toast still fires misleadingly, and the rejection becomes unhandled. The same pattern was just fixed inuse-toast.tsx'sdispatchSonnerToast— apply the same treatment here.🩹 Suggested fix
onClick={() => { - navigator.clipboard.writeText(testResult.message); - toast({ title: 'Copied to clipboard' }); + navigator.clipboard.writeText(testResult.message) + .then(() => toast({ title: 'Copied to clipboard' })) + .catch(() => toast({ title: 'Failed to copy', variant: 'destructive' })); }}🤖 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 `@frontend/web/src/features/partners/components/CreateSftpPartnerModal.tsx` around lines 260 - 273, The copy action in CreateSftpPartnerModal’s clipboard button currently calls navigator.clipboard.writeText(testResult.message) without awaiting or handling failures, so the success toast can appear even when copying fails. Update the onClick handler to use the same async/error-handling pattern as dispatchSonnerToast in use-toast.tsx: await the clipboard write, show the success toast only on success, and catch/report failures so rejected clipboard writes do not become unhandled.services/api/src/api/main.py (1)
63-70: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
exc.errors()still leaks raw field values.RequestValidationError.errors()includes the submittedinputby default, so malformed secret-bearing fields can end up in logs. Stripinputbefore logging, and avoid returning the raw error payload if this 422 can reach untrusted clients.🤖 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 `@services/api/src/api/main.py` around lines 63 - 70, The validation_exception_handler currently logs and returns RequestValidationError.errors() directly, which can expose raw submitted values. Update validation_exception_handler to sanitize exc.errors() by removing the input field before calling logger.error, and ensure the JSONResponse for the 422 does not echo the raw error payload back to clients; keep the fix localized to validation_exception_handler and its use of exc.errors().
♻️ Duplicate comments (3)
services/api/src/api/adapters/http/dtos.py (2)
291-305: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
RouteItemResponse.statusdefault still mismatched, and appears to never be set from real state.
status: str = "Active"still uses different casing than the rest of the API's"ACTIVE"/"INACTIVE"convention (flagged previously, unresolved). More importantly, based onlist_routes()inprovisioning.py(cross-file evidence), the dicts unpacked intoRouteItemResponse(**r)only include"active": r.active— no"status"key — so every route response falls back to this default"Active"string regardless of whether the route is actually active or inactive.🤖 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 `@services/api/src/api/adapters/http/dtos.py` around lines 291 - 305, RouteItemResponse.status is still hardcoded to the wrong casing and is not populated from real route state. Update RouteItemResponse in dtos.py to align status with the API’s ACTIVE/INACTIVE convention, and then fix provisioning.py’s list_routes() so the data passed into RouteItemResponse(**r) includes a status derived from the route’s active flag instead of relying on the model default. Make sure the route-building logic and RouteItemResponse stay consistent so active/inactive routes never fall back to a misleading default.
198-215: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
UpdateRouteRequeststill allows multiple destination IDs simultaneously.No cross-field validator (analogous to
check_exactly_one_destinationon the create requests) exists here, so a PUT with bothas2_partner_idandsftp_partner_idset would bypass the invariant enforced at creation. This was flagged in a prior review round and remains unaddressed in this diff.🤖 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 `@services/api/src/api/adapters/http/dtos.py` around lines 198 - 215, UpdateRouteRequest currently allows multiple destination IDs to be set at once, so add the same cross-field exclusivity check used on the create DTOs (the check_exactly_one_destination validator) to this class. Make sure UpdateRouteRequest enforces that only one of webhook_id, as2_partner_id, or sftp_partner_id can be provided together, and keep the validation consistent with the route creation request models.services/api/src/api/routers/webhooks/webhook.py (1)
25-41: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftSSRF validation only checked at creation time — DNS rebinding still possible at delivery.
is_private/is_loopback/is_unspecifiednow cover RFC1918/ULA/loopback ranges, which addresses part of the prior concern. However, the check resolves the hostname once here at webhook-creation time; the actual delivery path (HttpxDeliveryAdapter, per the pipeline delivery code) re-resolves the same hostname when posting. An attacker can point a domain at a public IP during creation and repoint DNS to an internal/private address before (or during) delivery, bypassing this check entirely (classic TOCTOU/DNS-rebinding SSRF). The resolved IP should be pinned and reused for the actual outbound connection, or re-validated immediately before each delivery attempt.🤖 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 `@services/api/src/api/routers/webhooks/webhook.py` around lines 25 - 41, The webhook URL check in the creation flow only validates the hostname once, so `HttpxDeliveryAdapter` can still re-resolve a rebinding domain later and bypass SSRF protection. Update the webhook creation/delivery pipeline so the resolved public IP is pinned and reused for outbound delivery, or re-validate the resolved address immediately before each send. Make the fix around the existing `urlparse`/`socket.gethostbyname` validation and the `HttpxDeliveryAdapter` delivery path so both creation-time and delivery-time checks use the same trusted target.
🤖 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.
Inline comments:
In `@frontend/web/src/components/ui/data-table.tsx`:
- Around line 73-78: The row key handler in data-table should ignore keyboard
events that originated from interactive descendants so Enter/Space on buttons or
links inside a cell do not also toggle expansion. Update the onKeyDown logic
attached to the row in data-table and use the event target/currentTarget check
to bail out when the focused element is an interactive control, then only call
row.toggleExpanded() for genuine row-level keyboard actions.
In `@libs/config/src/config/settings.py`:
- Around line 107-114: The external_url validation in the settings check is too
permissive because urlparse can misread schemeless values and the current
hostname test only catches a few loopback literals. Update the validation around
self.env and self.server.external_url to reject schemeless inputs before
parsing, then normalize or inspect the host using ipaddress so all loopback
addresses in 127.0.0.0/8 plus localhost, 0.0.0.0, and ::1 are blocked
consistently.
In `@services/api/src/api/adapters/repository.py`:
- Around line 417-447: The PATCH flow in the repository update logic can leave
an outbound route with more than one destination because `record.as2_partner_id`
and `record.sftp_partner_id` are updated independently. In the route update
method that handles `cmd.webhook_id`, `cmd.as2_partner_id`, and
`cmd.sftp_partner_id`, add a post-update validation before flushing that
rechecks the effective destination count on `record` after assignments, using
the same rule as create. Apply the same fix in the outbound route update path
referenced by the matching destination update block, and raise the appropriate
error if `[record.as2_partner_id, record.sftp_partner_id]` results in multiple
active destinations.
In `@services/api/src/api/routers/trading_partners/as2.py`:
- Around line 55-57: The exception handling in the AS2 router is exposing
internal vault error details through HTTPException details. Update the handlers
around the private key retrieval logic in the as2 router to log the caught
exception with full context, but return a generic 500/502 message instead of
interpolating str(e) into the API response. Apply the same change to both
vault-related raise sites so consumers never see backend paths or
client-specific errors.
In `@services/api/src/api/routers/webhooks/webhook.py`:
- Around line 25-34: The webhook request handler is performing blocking DNS
lookup work on the async event loop via the hostname parsing/resolution path,
which can stall other requests under load. Update the webhook creation logic in
webhook.py to move the DNS resolution off the event loop, either by offloading
the socket.gethostbyname call to an executor/thread or replacing it with an
async DNS approach, and keep the surrounding URL validation/parsing flow in the
same webhook handler intact.
---
Outside diff comments:
In `@frontend/web/src/features/partners/components/CreateSftpPartnerModal.tsx`:
- Around line 260-273: The copy action in CreateSftpPartnerModal’s clipboard
button currently calls navigator.clipboard.writeText(testResult.message) without
awaiting or handling failures, so the success toast can appear even when copying
fails. Update the onClick handler to use the same async/error-handling pattern
as dispatchSonnerToast in use-toast.tsx: await the clipboard write, show the
success toast only on success, and catch/report failures so rejected clipboard
writes do not become unhandled.
In `@frontend/web/src/features/partners/components/SftpPartnerDetails.tsx`:
- Around line 68-83: The connection test callbacks in SftpPartnerDetails.tsx can
still update testResult after the user changes fields, causing stale results
from an older host/port/username/password to appear. Update the onClick flow
around testConnection.mutate so each test run is tracked with a unique token or
incrementing request id, and only the latest run is allowed to call
setTestResult in onSuccess/onError. Use the existing SftpPartnerDetails
component, testConnection, and setTestResult to locate the change.
In `@libs/pipeline/src/pipeline/adapters/sftp.py`:
- Around line 67-74: The client key loading logic in the SFTP adapter currently
falls back from RSA directly to Ed25519, which means valid ECDSA private keys
are rejected before connect(). Update the key parsing in the code path that
builds connect_kwargs by extending the fallback chain in client_key_string
handling to try paramiko.ECDSAKey.from_private_key before Ed25519Key, keeping
the existing key_io reset behavior between attempts.
In `@libs/pipeline/src/pipeline/core/deliver.py`:
- Around line 151-168: The SFTP delivery path now reads partner.get("host_key"),
but the partner CRUD and routing flow still discard that field. Update
CreateSFTPPartnerCmd and UpdateSFTPPartnerCmd to accept/persist host_key, pass
it through the SFTP router into the repository, and ensure the stored partner
payload includes it so deliver() can forward it to sftp_delivery.deliver.
In `@services/api/src/api/main.py`:
- Around line 63-70: The validation_exception_handler currently logs and returns
RequestValidationError.errors() directly, which can expose raw submitted values.
Update validation_exception_handler to sanitize exc.errors() by removing the
input field before calling logger.error, and ensure the JSONResponse for the 422
does not echo the raw error payload back to clients; keep the fix localized to
validation_exception_handler and its use of exc.errors().
In `@services/api/src/api/routers/routes.py`:
- Around line 117-135: The PATCH mapping in update_inbound_route and
update_outbound_route is treating every None as UNSET, so explicit clears for
webhook_id, as2_partner_id, and sftp_partner_id are lost and existing partner
links remain. Update the request-to-command conversion in routes.py to
distinguish omitted fields from explicit nulls, preserving null as a real value
while still using UNSET only for absent fields, and apply the same handling
consistently in both route update handlers and their
UpdateInboundRouteCmd/UpdateOutboundRouteCmd construction.
In `@services/worker/src/worker/provision/main.py`:
- Around line 61-65: The AS2 replication flow in main() currently treats only
tenant-local and NULL rows as valid, so platform AS2 partners with tenant_id=0
are skipped and can break route/partnership FK validation. Update the AS2
partner selection used for replication to also include GlobalAS2Partner rows
where tenant_id equals 0, and make the same tenant_id=0 adjustment in the
AS2-specific valid-ID handling inside sync_deletes so replicated platform
partners are not removed again during the same sync.
- Around line 153-178: The SFTP tenant-shard replication in the worker is
dropping `host_key`, so host-key pinning is lost during provision updates.
Update the upsert logic in `main.py` where the `global_sftp` values are copied
into the tenant record to include `host_key` in both the `.values(...)` payload
and the `.on_conflict_do_update(..., set_=...)` map, keeping it aligned with the
partner schema and the delivery path’s verification needs.
---
Duplicate comments:
In `@services/api/src/api/adapters/http/dtos.py`:
- Around line 291-305: RouteItemResponse.status is still hardcoded to the wrong
casing and is not populated from real route state. Update RouteItemResponse in
dtos.py to align status with the API’s ACTIVE/INACTIVE convention, and then fix
provisioning.py’s list_routes() so the data passed into RouteItemResponse(**r)
includes a status derived from the route’s active flag instead of relying on the
model default. Make sure the route-building logic and RouteItemResponse stay
consistent so active/inactive routes never fall back to a misleading default.
- Around line 198-215: UpdateRouteRequest currently allows multiple destination
IDs to be set at once, so add the same cross-field exclusivity check used on the
create DTOs (the check_exactly_one_destination validator) to this class. Make
sure UpdateRouteRequest enforces that only one of webhook_id, as2_partner_id, or
sftp_partner_id can be provided together, and keep the validation consistent
with the route creation request models.
In `@services/api/src/api/routers/webhooks/webhook.py`:
- Around line 25-41: The webhook URL check in the creation flow only validates
the hostname once, so `HttpxDeliveryAdapter` can still re-resolve a rebinding
domain later and bypass SSRF protection. Update the webhook creation/delivery
pipeline so the resolved public IP is pinned and reused for outbound delivery,
or re-validate the resolved address immediately before each send. Make the fix
around the existing `urlparse`/`socket.gethostbyname` validation and the
`HttpxDeliveryAdapter` delivery path so both creation-time and delivery-time
checks use the same trusted target.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 319d1814-899b-49ce-a9e1-367b82450919
📒 Files selected for processing (47)
.env.examplefrontend/web/src/components/ui/data-table.tsxfrontend/web/src/components/ui/toast.tsxfrontend/web/src/features/endpoints/api/endpointsApi.tsfrontend/web/src/features/endpoints/api/endpointsHooks.tsfrontend/web/src/features/partners/components/As2PartnersTable.tsxfrontend/web/src/features/partners/components/CreateSftpPartnerModal.tsxfrontend/web/src/features/partners/components/PartnershipDetails.tsxfrontend/web/src/features/partners/components/PartnershipsTable.tsxfrontend/web/src/features/partners/components/SftpPartnerDetails.tsxfrontend/web/src/features/partners/components/SftpPartnersTable.tsxfrontend/web/src/features/routes/components/CreateRouteModal.tsxfrontend/web/src/features/routes/components/RouteDetails.tsxfrontend/web/src/features/routes/components/RoutesTable.tsxfrontend/web/src/features/routes/hooks/useTenantDestinations.tsfrontend/web/src/features/routes/types.tsfrontend/web/src/hooks/use-toast-mutation.tsfrontend/web/src/hooks/use-toast.tsxlibs/config/src/config/settings.pylibs/database/src/database/encryption.pylibs/database/src/database/migrations/global/versions/227fb01f972f_global_initial_schema.pylibs/database/src/database/migrations/tenant/env.pylibs/database/src/database/migrations/tenant/versions/1cff61426f2b_tenant_initial_schema.pylibs/database/src/database/models/control_plane.pylibs/database/src/database/models/data_plane.pylibs/patches/src/patches/paramiko.pylibs/pipeline/src/pipeline/adapters/repository.pylibs/pipeline/src/pipeline/adapters/sftp.pylibs/pipeline/src/pipeline/core/deliver.pylibs/pipeline/tests/test_delivery_service.pyservices/api/src/api/adapters/http/dtos.pyservices/api/src/api/adapters/repository.pyservices/api/src/api/adapters/vault.pyservices/api/src/api/core/provisioning.pyservices/api/src/api/domain/models.pyservices/api/src/api/main.pyservices/api/src/api/ports/repository.pyservices/api/src/api/routers/routes.pyservices/api/src/api/routers/trading_partners/as2.pyservices/api/src/api/routers/trading_partners/platform/as2_partners.pyservices/api/src/api/routers/trading_partners/sftp.pyservices/api/src/api/routers/webhooks/webhook.pyservices/api/tests/api_fakes.pyservices/api/tests/test_api_repository.pyservices/api/tests/test_provisioning_core.pyservices/api/tests/test_routers_partners.pyservices/worker/src/worker/provision/main.py
This was referenced Jul 8, 2026
This was referenced Jul 15, 2026
Merged
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
1 participant
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
DB_ENCRYPTION_KEY.