AS2 inbound and outbound connection - #15
Merged
pramodnarayana merged 3 commits intoJul 8, 2026
Merged
Conversation
Contributor
|
Warning Review limit reached
Next review available in: 35 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 (8)
Note
|
| Layer / File(s) | Summary |
|---|---|
EdiEditorPane component frontend/web/src/components/ui/edi-editor-pane.tsx, frontend/web/src/utils/monaco-edi.ts |
New Monaco-based editor component with drag/drop and file upload, empty-state UI, and registerEdiLanguageAndTheme for EDI language/theme registration. |
Editor integration frontend/web/src/features/partners/components/CertificateInput.tsx, frontend/web/src/routes/tenant/edi_tool.tsx |
CertificateInput and the EDI tool page replace prior upload/paste/drag-drop implementations with EdiEditorPane. |
FormModal focus handling frontend/web/src/components/ui/form-modal.tsx |
Dialog set to modal={false} with an added onFocusOutside prevention handler. |
AS2 Partnership Testing & Inbound Receive
| Layer / File(s) | Summary |
|---|---|
edi_version removal & algorithm config contracts frontend/web/src/features/partners/types.ts, frontend/web/src/features/platform/api/configHooks.ts, services/api/src/api/adapters/http/dtos.py, services/api/src/api/domain/models.py, libs/database/src/database/models/*, migrations, services/worker/src/worker/provision/main.py, tests |
Removes edi_version from types/DTOs/models/migrations/replication; adds SupportedAlgorithm and advanced_flags. |
Frontend test connection UI CreatePartnershipModal.tsx, PartnershipDetails.tsx, PartnershipsTable.tsx, partnerHooks.ts, partnersApi.ts, IPartnersRepository.ts |
Dynamic algorithm selects, a new Test Connection section, and a mutation hook/API method for testAs2PartnershipConnection. |
Backend test connection endpoint httpx_as2_tester.py, as2_tester.py, dependencies.py, vault.py, dtos.py, as2_partnerships.py, config.py, tests |
New /test endpoint building a synthetic AS2 ping, retrieving keys from Vault, and returning success/reason based on MDN disposition. |
Inbound AS2 receive pipeline builder.py, mdn.py, as2_receive_service.py, as2_receive.py, repository.py, ports/repository.py, data_plane.py, migrations, tests |
Full decrypt/verify/MIC pipeline, build_mdn, persistence via create_edi_message/create_outbox_event, and a new /as2/receive router. |
Native S/MIME Encryption/Verification
| Layer / File(s) | Summary |
|---|---|
Native decrypt/encrypt/verify implementation libs/security/src/security/smime.py, libs/patches/src/patches/cryptography.py, pyproject.toml, libs/security/pyproject.toml |
Replaces subprocess-based verification with endesive-based native flow, adds ASN.1 fallback decryption, cipher registries, and a 3DES legacy patch. |
S/MIME test coverage libs/security/tests/*, scratch/test_email.py, libs/as2_core/test_parse_smime.py |
New tests for encrypt/decrypt/sign/verify plus exploratory scripts. |
Docs, Tooling and DB Encryption Tests
| Layer / File(s) | Summary |
|---|---|
Technical debt doc & script annotations TECHNICAL_DEBT.md, scripts/build_frontend_dictionary.py, scripts/build_registry.py |
Documents AS2 async MDN and frontend test gaps; adds -> None return annotations. |
DBEncryptionAdapter tests libs/database/tests/test_encryption.py |
New tests for key initialization, encrypt/decrypt, and error handling. |
Estimated code review effort: 5 (Critical) | ~120 minutes
Sequence Diagram(s)
sequenceDiagram
participant PartnershipDetails
participant useTestAs2PartnershipConnectionMutation
participant HttpPartnersRepository
participant TestEndpoint as as2_partnerships router
PartnershipDetails->>useTestAs2PartnershipConnectionMutation: mutate({id, custom_payload})
useTestAs2PartnershipConnectionMutation->>HttpPartnersRepository: testAs2PartnershipConnection(id, custom_payload)
HttpPartnersRepository->>TestEndpoint: POST /partnerships/{id}/test
TestEndpoint-->>HttpPartnersRepository: TestAS2ConnectionResponse
HttpPartnersRepository-->>PartnershipDetails: success, mdn_disposition, raw_mdn
sequenceDiagram
participant Sender
participant as2_receive_router
participant As2ReceiveService
participant Vault
participant DataPlaneRepo
participant MDN as build_mdn
Sender->>as2_receive_router: POST /as2/receive
as2_receive_router->>As2ReceiveService: process_inbound_message(headers, body)
As2ReceiveService->>As2ReceiveService: lookup partnership
As2ReceiveService->>Vault: retrieve_secret(vault_ref)
As2ReceiveService->>As2ReceiveService: decrypt/verify/compute MIC
As2ReceiveService->>DataPlaneRepo: create_edi_message + create_outbox_event
As2ReceiveService->>MDN: build_mdn(disposition, mic)
MDN-->>as2_receive_router: MDN body + headers
as2_receive_router-->>Sender: 200 Response (MDN)
Possibly related PRs
- pramodnarayana/soopaedi#7: Overlaps in core AS2 building/MDN/S-MIME exports and inbound receive/disposition handling.
- pramodnarayana/soopaedi#8: Related to inbound AS2 receive persistence via
create_edi_message/edi_dataand associated DB schema changes. - pramodnarayana/soopaedi#11: Shares control/data plane repository and port decoupling underlying the inbound receive and outbox wiring.
Poem
A rabbit hops through code so bright,
Editing EDI panes with delight 🐇
Signatures verified, no more subprocess fright,
MDNs bloom back with "processed" in sight,
Hop, hop, hooray — the AS2 pipes take flight! ✨
🚥 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 is related to the main AS2 inbound and outbound connection work, even if it is a bit broad. |
| 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
feature/as2-partnership-test-connection
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: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/api/src/api/adapters/vault.py (1)
56-79: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
retrieve_secretshould read the stored secret generically
It’s called forpublic_cert_vault_ref, but always readsinner_data["private_key_pem"]. Any Vault secret that stores the PEM under a different key will fail those certificate retrieval paths. Read the single stored value generically, or accept the field name as an argument.🤖 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/vault.py` around lines 56 - 79, The retrieve_secret method in VaultAdapter is hardcoded to fetch inner_data["private_key_pem"], which breaks callers like the certificate retrieval path that store the PEM under a different key. Update retrieve_secret to read the stored value generically from the Vault payload, or add a field-name parameter and use it in retrieve_private_key and the public_cert_vault_ref path so the method works for any PEM-backed secret.
🤖 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/edi-editor-pane.tsx`:
- Around line 47-78: The Monaco language and theme setup in
handleEditorWillMount is duplicated and will drift from the matching
implementation in edi_tool.tsx; extract the shared edi registration logic into a
reusable helper such as registerEdiLanguageAndTheme(monaco), move the edi
Monarch tokenizer and soopa-theme definition there, and call that helper from
both edi-editor-pane.tsx and the read-only editor path in edi_tool.tsx so both
editors stay consistent.
- Line 105: The hardcoded accept list in the file picker is too narrow for the
certificate flow, since `CertificateInput` reuses `EdiEditorPane` but cannot
choose PEM/certificate key files. Update `EdiEditorPane` to take the accepted
file extensions as a prop and pass that through to the hidden input, then keep
the current `.edi/.json/.txt/.x12` default for the EDI use case while letting
the certificate usage supply `.pem/.crt/.cer/.key` via the same component.
- Around line 17-40: The drag-and-drop handlers in edi-editor-pane use
React.DragEvent and React.ChangeEvent without React in scope, which breaks
type-checking under the current JSX setup. Fix this by adding a type-only import
for the React namespace or by replacing the handler annotations with specific
event types imported from React, and update the handler definitions in the
component accordingly.
In `@libs/as2_core/src/as2_core/builder.py`:
- Around line 87-89: Clean up the MIC computation comment in the builder flow by
removing the stream-of-consciousness “Wait, RFC 4130 says…” text and replacing
it with a direct invariant statement in the MIC step; this should make the
intent clear without debug-style narration. Also fix the step labels in the same
sequence so the sign block and encrypt block under the builder’s step comments
are numbered distinctly instead of both being marked “Step 3”, keeping the
ordering consistent throughout the AS2 builder logic.
In `@libs/as2_core/src/as2_core/mdn.py`:
- Around line 72-113: The build_mdn function currently always creates an
unsigned MDN, which breaks partners that require signed receipts. Update
build_mdn to inspect the inbound Disposition-Notification-Options and, when
signed-receipt-protocol is requested, generate a signed MDN rather than only
building the multipart/report body. Use the existing build_mdn logic and related
MDNResponse/header assembly as the entry point, and ensure the returned receipt
includes the MIC in the signed response when required.
In `@libs/database/src/database/models/data_plane.py`:
- Around line 34-41: The current `process_bind_param` logic in `data_plane.py`
silently corrupts raw EDI bytes by decoding with UTF-8 replacement before
storing. Update `process_bind_param` so `edi_data` stays lossless: either
preserve `bytes` without forcing a decode, or decode using the payload’s
declared charset instead of `errors="replace"`, while keeping the existing
null-byte cleanup behavior for text values.
In `@libs/database/src/database/repository.py`:
- Around line 42-50: The partnership lookup in repository query building is
missing active-state checks, so inactive records can still be returned and
processed. Update the query in the repository method that constructs the
`select(AS2Partnership, LocalPartner, RemotePartner)` statement to require
`AS2Partnership.active` and the related
`LocalPartner.active`/`RemotePartner.active` flags, and ensure
`_lookup_partnership` also rejects any inactive result instead of only checking
for None.
In `@libs/security/src/security/smime.py`:
- Around line 96-98: The fallback decryptor in the PKCS7 unpadding path is
trusting padded_plaintext[-1] as the padding length without validating it.
Update the unpadding logic in the decryptor to verify pad_len is within the
valid block range and that the trailing bytes all match the expected padding
pattern before slicing. If validation fails, raise a decryption/unpadding error
instead of returning truncated plaintext from the smime.py decrypt flow.
- Around line 168-183: `asn1crypto` is only being used indirectly through
runtime imports in the S/MIME flow, so make it a direct dependency instead of
relying on a transitive package. Update the dependency declaration used by the
security S/MIME path, and ensure the runtime helpers
`_manual_asn1crypto_decrypt` and `_inject_certificate_into_cms` can import
`asn1crypto` without depending on `endesive` pulling it in.
In `@libs/security/test_newline.py`:
- Around line 1-24: The issue is that `test_newline.py` is a scratch script
sitting in the production library tree and may be picked up by pytest because of
its `test_` prefix. Remove this exploratory script from `libs/security/` or move
it to a non-test location like `scratch/`, and ensure there are no executable
side effects such as RSA key generation or printing at import time. Keep real
tests in the proper `tests` directory and use proper test functions/classes if
this behavior needs coverage.
In `@libs/security/tests/test_smime.py`:
- Around line 47-75: The current S/MIME test only checks that sign_payload
embeds the payload, but it never exercises the real round-trip through
verify_signature. Update test_sign_verify_smime to use the output of
sign_payload as input to verify_signature and assert the signature verifies
successfully and returns the original payload, so the production path is covered
end-to-end.
In `@pyproject.toml`:
- Around line 10-11: Add asn1crypto as a direct dependency in the project
configuration so smime.py does not rely on endesive to bring it in transitively.
Update the dependency list alongside cryptography and endesive, and keep the
change scoped to the package metadata where direct runtime requirements are
declared.
In `@services/api/src/api/adapters/httpx_as2_tester.py`:
- Around line 46-75: The AS2 connection test currently hardcodes signing
behavior and ignores the configured partnership signature algorithm, so the test
ping may not match real partner expectations. Thread the signature algorithm
through AS2TesterPort and HttpxAS2TesterAdapter.test_connection, then pass it
into sign_payload instead of relying on the default SHA256. Use the existing
sign_fn setup in test_connection and the sign_payload helper in smime.py as the
key places to update.
In `@services/api/src/api/ports/repository.py`:
- Around line 83-115: `DataPlaneRepositoryPort` is missing the
`create_outbox_event` contract, even though `SqlAlchemyDataPlaneRepository` is
expected to handle it. Add `create_outbox_event` to `DataPlaneRepositoryPort` so
the data plane interface matches the methods used by the inbound receive service
and any alternate implementations are forced to provide it. Keep the change
aligned with the existing `OutboxRepositoryPort` method signature and the
`create_edi_message` pattern in `DataPlaneRepositoryPort`.
In `@services/api/src/api/routers/trading_partners/as2_receive.py`:
- Around line 39-44: The AS2 receive path in as2_receive should not return bare
HTTP 400/500 once the sender and partnership are identifiable; instead, generate
and return a negative MDN with an appropriate failure disposition for
post-resolution business-rule or processing failures. Update the exception
handling around the ValueError and generic Exception branches so the trading
partner resolution flow produces an MDN response rather than raising
HTTPException, using the existing AS2 receive/MDN generation logic in this
router to preserve RFC 4130 behavior.
In `@services/api/src/api/services/as2_receive_service.py`:
- Around line 142-168: In as2_receive_service.py, the signed/encrypted handling
in the receive flow currently falls through when required credentials are
missing, which should be treated as a hard failure. Update the main processing
path around the verification logic in the receive method so that if is_signed is
true but remote_cert is missing, or if is_encrypted is true but local_priv_key
is missing, execution stops by raising an error instead of continuing to
calculate MIC or extract content. Keep the fix localized to the same receive
flow and its helpers such as _verify_and_calculate_mic and _decrypt_entity so
the message cannot be marked processed without successful
verification/decryption.
In `@services/api/tests/test_as2_receive_service.py`:
- Line 197: The test setup in test_as2_receive_service uses
MagicMock(name="shard1"), but that only sets the mock’s repr name and leaves
shard.name as a child mock. Update the mock_shard fixture in the AS2 receive
service test to explicitly set the name attribute with configure_mock (or
equivalent) so the test reflects the real shard object shape and avoids
misleading future readers.
In `@services/as2_server/tests/test_receive_as2.py`:
- Around line 86-106: The signed AS2 receive test is missing the assertion that
the MDN disposition is actually processed, so update the test in
test_receive_as2_signed_message to also check for b"processed" in the response
content alongside the existing message-id check. Use the same expectation
already covered by test_plain_as2_message_returns_processed_mdn so the test
verifies successful signature handling, not just a 200 response or echoed
message ID.
---
Outside diff comments:
In `@services/api/src/api/adapters/vault.py`:
- Around line 56-79: The retrieve_secret method in VaultAdapter is hardcoded to
fetch inner_data["private_key_pem"], which breaks callers like the certificate
retrieval path that store the PEM under a different key. Update retrieve_secret
to read the stored value generically from the Vault payload, or add a field-name
parameter and use it in retrieve_private_key and the public_cert_vault_ref path
so the method works for any PEM-backed secret.
🪄 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: 51e93ff8-1067-4121-a101-25095826ef52
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (52)
TECHNICAL_DEBT.mdfrontend/web/src/components/ui/edi-editor-pane.tsxfrontend/web/src/components/ui/form-modal.tsxfrontend/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/CertificateInput.tsxfrontend/web/src/features/partners/components/CreatePartnershipModal.tsxfrontend/web/src/features/partners/components/PartnershipDetails.tsxfrontend/web/src/features/partners/components/PartnershipsTable.tsxfrontend/web/src/features/partners/types.tsfrontend/web/src/features/platform/api/configHooks.tsfrontend/web/src/routes/tenant/edi_tool.tsxlibs/as2_core/src/as2_core/builder.pylibs/as2_core/src/as2_core/mdn.pylibs/as2_core/test_parse_smime.pylibs/database/src/database/migrations/global/versions/142d93fd6c4c_global_initial_schema.pylibs/database/src/database/migrations/tenant/versions/34e9d4ab146a_tenant_initial_schema.pylibs/database/src/database/models/control_plane.pylibs/database/src/database/models/data_plane.pylibs/database/src/database/repository.pylibs/database/tests/test_encryption.pylibs/patches/src/patches/cryptography.pylibs/security/src/security/smime.pylibs/security/test_newline.pylibs/security/tests/test_smime.pylibs/security/tests/test_smime_verify.pypyproject.tomlscratch/test_email.pyscripts/build_frontend_dictionary.pyscripts/build_registry.pyservices/api/src/api/adapters/http/dtos.pyservices/api/src/api/adapters/httpx_as2_tester.pyservices/api/src/api/adapters/repository.pyservices/api/src/api/adapters/vault.pyservices/api/src/api/dependencies.pyservices/api/src/api/domain/models.pyservices/api/src/api/main.pyservices/api/src/api/ports/as2_tester.pyservices/api/src/api/ports/repository.pyservices/api/src/api/ports/vault.pyservices/api/src/api/routers/trading_partners/as2_receive.pyservices/api/src/api/routers/trading_partners/platform/as2_partnerships.pyservices/api/src/api/routers/trading_partners/platform/config.pyservices/api/src/api/services/as2_receive_service.pyservices/api/tests/api_fakes.pyservices/api/tests/test_as2_partnerships.pyservices/api/tests/test_as2_receive_service.pyservices/api/tests/test_httpx_as2_tester.pyservices/as2_server/tests/test_receive_as2.pyservices/worker/src/worker/provision/main.pyservices/worker/tests/test_provision_worker.py
💤 Files with no reviewable changes (3)
- libs/database/src/database/models/control_plane.py
- services/worker/src/worker/provision/main.py
- libs/database/src/database/migrations/global/versions/142d93fd6c4c_global_initial_schema.py
Comment on lines
+47
to
+78
| const handleEditorWillMount = (monaco: any) => { | ||
| if (!monaco.languages.getLanguages().some((l: any) => l.id === 'edi')) { | ||
| monaco.languages.register({ id: 'edi' }); | ||
| monaco.languages.setMonarchTokensProvider('edi', { | ||
| tokenizer: { | ||
| root: [ | ||
| [/^[A-Z0-9]{2,3}(?=\*)/, 'keyword'], | ||
| [/(~\s*)([A-Z0-9]{2,3})(?=\*)/, ['delimiter', 'keyword']], | ||
| [/\*/, 'delimiter'], | ||
| [/~/, 'delimiter'], | ||
| [/[^*~\n\r]+/, 'string'], | ||
| ], | ||
| }, | ||
| }); | ||
| } | ||
| monaco.editor.defineTheme('soopa-theme', { | ||
| base: 'vs', | ||
| inherit: true, | ||
| rules: [ | ||
| { token: 'keyword', foreground: '0451a5', fontStyle: 'bold' }, | ||
| { token: 'string', foreground: '065f46' }, | ||
| { token: 'delimiter', foreground: '000000' }, | ||
| { token: 'string.key.json', foreground: '0451a5', fontStyle: 'bold' }, | ||
| { token: 'string.value.json', foreground: '065f46' }, | ||
| { token: 'number.json', foreground: '065f46' }, | ||
| { token: 'keyword.json', foreground: '0451a5', fontStyle: 'bold' }, | ||
| ], | ||
| colors: { | ||
| 'editor.background': '#ffffff', | ||
| } | ||
| }); | ||
| }; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicated Monaco language/theme registration.
handleEditorWillMount here is a near-identical copy of the one in edi_tool.tsx (Lines 45-82), including the edi Monarch tokenizer and soopa-theme. These will drift over time. Extract into a shared helper (e.g. registerEdiLanguageAndTheme(monaco)) and call it from both this pane and the read-only output editor in edi_tool.tsx.
🤖 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/edi-editor-pane.tsx` around lines 47 - 78, The
Monaco language and theme setup in handleEditorWillMount is duplicated and will
drift from the matching implementation in edi_tool.tsx; extract the shared edi
registration logic into a reusable helper such as
registerEdiLanguageAndTheme(monaco), move the edi Monarch tokenizer and
soopa-theme definition there, and call that helper from both edi-editor-pane.tsx
and the read-only editor path in edi_tool.tsx so both editors stay consistent.
Comment on lines
+72
to
+113
| def build_mdn( | ||
| as2_to: str, as2_from: str, message_id: str, disposition: str, mic: str | None = None | ||
| ) -> MDNResponse: | ||
| boundary = f"----=_Part_{uuid.uuid4().hex}" | ||
|
|
||
| lines = [] | ||
| lines.append(f"--{boundary}") | ||
| lines.append("Content-Type: text/plain; charset=us-ascii") | ||
| lines.append("Content-Transfer-Encoding: 7bit") | ||
| lines.append("") | ||
| lines.append("The AS2 message has been received successfully.") | ||
| lines.append("") | ||
| lines.append(f"--{boundary}") | ||
| lines.append("Content-Type: message/disposition-notification") | ||
| lines.append("Content-Transfer-Encoding: 7bit") | ||
| lines.append("") | ||
| lines.append("Reporting-UA: SoopaEDI") | ||
| lines.append(f"Original-Recipient: rfc822; {as2_to}") | ||
| lines.append(f"Final-Recipient: rfc822; {as2_to}") | ||
|
|
||
| # if message_id isn't wrapped in <>, wrap it, otherwise use it directly | ||
| message_id_str = f"<{message_id}>" if not message_id.startswith("<") else message_id | ||
|
|
||
| lines.append(f"Original-Message-ID: {message_id_str}") | ||
| lines.append(f"Disposition: {disposition}") | ||
| if mic: | ||
| lines.append(f"Received-Content-MIC: {mic}") | ||
| lines.append("") | ||
| lines.append(f"--{boundary}--") | ||
| lines.append("") | ||
|
|
||
| body = "\r\n".join(lines).encode("ascii") | ||
|
|
||
| headers = { | ||
| "AS2-From": as2_to, | ||
| "AS2-To": as2_from, | ||
| "Message-ID": f"<mdn-{uuid.uuid4()}@soopa>", | ||
| "Content-Type": f'multipart/report; report-type=disposition-notification; boundary="{boundary}"', | ||
| "Connection": "close", | ||
| } | ||
|
|
||
| return MDNResponse(body=body, headers=headers) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy lift
build_mdn always returns an unsigned MDN.
Many AS2 partners set Disposition-Notification-Options requiring a signed receipt (the outbound builder itself requests signed-receipt-protocol=required). An unsigned MDN will be rejected by those partners, and the sender cannot verify the returned MIC. Consider honoring the inbound Disposition-Notification-Options and signing the MDN when requested.
🤖 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/as2_core/src/as2_core/mdn.py` around lines 72 - 113, The build_mdn
function currently always creates an unsigned MDN, which breaks partners that
require signed receipts. Update build_mdn to inspect the inbound
Disposition-Notification-Options and, when signed-receipt-protocol is requested,
generate a signed MDN rather than only building the multipart/report body. Use
the existing build_mdn logic and related MDNResponse/header assembly as the
entry point, and ensure the returned receipt includes the MIC in the signed
response when required.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
frontend/web/src/components/ui/edi-editor-pane.tsx (1)
29-47: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd error handling and file-size guard to file ingestion.
applyFileawaitsfile.text()without a try/catch. If the read fails (e.g., oversized file, special file type), the rejection is unhandled and the user gets no feedback. Additionally, drag-and-drop does not respect theacceptattribute, so a user can drop an arbitrarily large file that could freeze the tab.🛡️ Proposed fix
const applyFile = async (file: File) => { + const MAX_FILE_SIZE = 10 * 1024 * 1024; // 10 MB + if (file.size > MAX_FILE_SIZE) { + return; + } + try { const text = await file.text(); onChange(text); + } catch { + // Silently ignore file read errors + } };🤖 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/edi-editor-pane.tsx` around lines 29 - 47, Add error handling and a file-size check around the file ingestion flow in `applyFile`, `handleDrop`, and `handleFileUpload` so failed reads do not reject silently and oversized dropped files are rejected before reading. Wrap `file.text()` in `applyFile` with try/catch and surface a user-facing error via the existing component state/handler, and validate file size in both drag-and-drop and input upload paths before calling `applyFile` so `accept`-only restrictions are enforced consistently.services/api/tests/test_httpx_as2_tester.py (1)
14-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests don't verify
signature_algorithmis threaded tosign_payload.All tests pass
Nonefor key material, sosign_fnis alwaysNoneandbuild_outbound_messageis mocked — thesignature_algorithmparameter is never exercised. Add at least one test that provides key material and assertssign_payloadreceives the correctalgorithmargument (or capture thesign_fnpartial passed tobuild_outbound_messageand verify itsalgorithmkeyword).🤖 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/test_httpx_as2_tester.py` around lines 14 - 226, The test coverage in test_connection does not verify that signature_algorithm is propagated into sign_payload. Add a test around api.adapters.httpx_as2_tester.test_connection that supplies key material so sign_fn is created, then assert the algorithm argument passed to sign_payload is the expected signature_algorithm value, or inspect the partial passed into build_outbound_message to confirm it carries that keyword. Keep the existing adapter.test_connection and build_outbound_message mocking pattern, but make sure the new test exercises the signing path instead of leaving sign_fn as None.libs/database/src/database/models/data_plane.py (1)
24-41: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReturn
strhere, not raw bytes.edi_datais written frompure_edi_bytes, but thisText/Mapped[str]column will handbytesto asyncpg’s text path. Decode before returning, or switch the column toBYTEA/LargeBinaryif binary storage is intended. The docstring is also stale.🤖 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/database/src/database/models/data_plane.py` around lines 24 - 41, The SanitizedText.process_bind_param method is returning raw bytes for byte inputs, which conflicts with the Text/Mapped[str] behavior used by edi_data. Update SanitizedText so it always returns a string by decoding bytes (after stripping NULs) before bind time, or change the column type to binary storage if that is the intended format. Also refresh the SanitizedText docstring to match the actual behavior.services/api/src/api/services/as2_receive_service.py (2)
176-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog first decryption attempt failures before fallback.
The
except Exception: passon line 191-192 silently discards all error context from the initial decryption attempt. If the fallback also fails, only the fallback error is logged — the root cause from the first attempt is lost. Adding a debug log preserves observability for troubleshooting without changing behavior.📝 Proposed fix: add debug log
if decrypted: return decrypted - except Exception: - pass + except Exception as e: + logger.debug(f"Initial decryption attempt failed, retrying with reconstructed headers: {e}")🤖 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/services/as2_receive_service.py` around lines 176 - 192, The first decryption attempt in _decrypt_entity silently swallows exceptions, so add a debug log inside the except block before falling back to preserve the original failure context. Keep the existing behavior unchanged, but log the exception and relevant context around decrypt_payload so troubleshooting can trace why the initial attempt failed even if the fallback later succeeds or fails.
74-81: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass a signer into
build_mdn()when signed receipts are required.As2ReceiveService.process_inbound_message()always emits an unsigned MDN, even though the outbound flow advertisesDisposition-Notification-Options: signed-receipt-protocol=required. Partners expecting a signed receipt will reject this response and may retransmit the message.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/api/src/api/services/as2_receive_service.py` around lines 74 - 81, Signed MDNs are not being generated in As2ReceiveService.process_inbound_message(), so receipts stay unsigned even when signed receipts are required. Update the MDN creation path around build_mdn() to pass a signer when the inbound message or partner settings require a signed receipt, and keep the unsigned path only as a fallback when signing is not required. Use the existing as2_msg data and process_inbound_message()/build_mdn() flow to decide whether to attach signing credentials before sending the response.services/api/tests/test_as2_receive_service.py (1)
193-224: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
create_outbox_eventandcommitin save success test.The test only asserts
create_edi_messagewas awaited, but_save_to_data_planealso callscreate_outbox_eventandtenant_session.commit(). If either is accidentally removed, the test would still pass — a coverage gap on a critical persistence path.💚 Proposed fix: add missing assertions
res = await service._save_to_data_plane(mock_partnership, mock_as2_msg, b"EDI") assert res == "msg-1" mock_repo.create_edi_message.assert_awaited_once() + mock_repo.create_outbox_event.assert_awaited_once()🤖 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/test_as2_receive_service.py` around lines 193 - 224, The `_save_to_data_plane` success test is missing coverage for the full persistence flow, so it can pass even if the outbox write or transaction commit is removed. Update `test_save_to_data_plane_success` to also assert that `SqlAlchemyDataPlaneRepository.create_outbox_event` is awaited and that the tenant session’s `commit()` is called after `create_edi_message`. Use the existing `mock_repo` and the mocked tenant session returned by `db_router.get_tenant_session` to verify these calls alongside the current `create_edi_message` assertion.
🤖 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 `@libs/security/src/security/smime.py`:
- Around line 151-157: The signature algorithm selection in smime.py is silently
falling back to SHA-256 when an unsupported algorithm is requested, which hides
bad configuration. Update the algorithm lookup near the alg_map/hash_alg logic
to fail closed by rejecting unknown values instead of using a default, mirroring
the behavior used by encrypt_payload. Make the validation explicit in the
signing path so invalid AS2 partnership config is surfaced before transmission.
In `@services/api/src/api/adapters/vault.py`:
- Around line 75-84: The fallback logic in retrieve_secret is too specific
because it always prefers private_key_pem even when no field is requested, which
can return the wrong secret for certificate lookups. Update
VaultAdapter.retrieve_secret so that only an explicit field lookup uses
field-based selection, and when field is absent it simply returns the first
available value from inner_data without any private_key_pem preference; keep any
private-key-specific behavior isolated to retrieve_private_key.
In `@services/api/src/api/routers/trading_partners/as2_receive.py`:
- Around line 49-54: The MDN disposition in as2_receive.py is exposing the raw
exception string from the exception handler. Update the MDN construction in the
AS2 receive flow to stop interpolating {e} directly in build_mdn and instead map
known ValueError cases to RFC 4130-style disposition tokens such as
decryption-failed, authentication-failed, or integrity-check-failed. Keep the
logic anchored around build_mdn and the exception handling path in the AS2
receive router, and sanitize any fallback text so no internal details are sent
to the trading partner.
---
Outside diff comments:
In `@frontend/web/src/components/ui/edi-editor-pane.tsx`:
- Around line 29-47: Add error handling and a file-size check around the file
ingestion flow in `applyFile`, `handleDrop`, and `handleFileUpload` so failed
reads do not reject silently and oversized dropped files are rejected before
reading. Wrap `file.text()` in `applyFile` with try/catch and surface a
user-facing error via the existing component state/handler, and validate file
size in both drag-and-drop and input upload paths before calling `applyFile` so
`accept`-only restrictions are enforced consistently.
In `@libs/database/src/database/models/data_plane.py`:
- Around line 24-41: The SanitizedText.process_bind_param method is returning
raw bytes for byte inputs, which conflicts with the Text/Mapped[str] behavior
used by edi_data. Update SanitizedText so it always returns a string by decoding
bytes (after stripping NULs) before bind time, or change the column type to
binary storage if that is the intended format. Also refresh the SanitizedText
docstring to match the actual behavior.
In `@services/api/src/api/services/as2_receive_service.py`:
- Around line 176-192: The first decryption attempt in _decrypt_entity silently
swallows exceptions, so add a debug log inside the except block before falling
back to preserve the original failure context. Keep the existing behavior
unchanged, but log the exception and relevant context around decrypt_payload so
troubleshooting can trace why the initial attempt failed even if the fallback
later succeeds or fails.
- Around line 74-81: Signed MDNs are not being generated in
As2ReceiveService.process_inbound_message(), so receipts stay unsigned even when
signed receipts are required. Update the MDN creation path around build_mdn() to
pass a signer when the inbound message or partner settings require a signed
receipt, and keep the unsigned path only as a fallback when signing is not
required. Use the existing as2_msg data and
process_inbound_message()/build_mdn() flow to decide whether to attach signing
credentials before sending the response.
In `@services/api/tests/test_as2_receive_service.py`:
- Around line 193-224: The `_save_to_data_plane` success test is missing
coverage for the full persistence flow, so it can pass even if the outbox write
or transaction commit is removed. Update `test_save_to_data_plane_success` to
also assert that `SqlAlchemyDataPlaneRepository.create_outbox_event` is awaited
and that the tenant session’s `commit()` is called after `create_edi_message`.
Use the existing `mock_repo` and the mocked tenant session returned by
`db_router.get_tenant_session` to verify these calls alongside the current
`create_edi_message` assertion.
In `@services/api/tests/test_httpx_as2_tester.py`:
- Around line 14-226: The test coverage in test_connection does not verify that
signature_algorithm is propagated into sign_payload. Add a test around
api.adapters.httpx_as2_tester.test_connection that supplies key material so
sign_fn is created, then assert the algorithm argument passed to sign_payload is
the expected signature_algorithm value, or inspect the partial passed into
build_outbound_message to confirm it carries that keyword. Keep the existing
adapter.test_connection and build_outbound_message mocking pattern, but make
sure the new test exercises the signing path instead of leaving sign_fn as None.
🪄 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: e52d4223-717d-41b6-a1b1-0a23ab51f820
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
frontend/web/src/components/ui/edi-editor-pane.tsxfrontend/web/src/routes/tenant/edi_tool.tsxfrontend/web/src/utils/monaco-edi.tslibs/as2_core/src/as2_core/__init__.pylibs/as2_core/src/as2_core/builder.pylibs/as2_core/src/as2_core/mdn.pylibs/database/src/database/models/data_plane.pylibs/database/src/database/repository.pylibs/security/pyproject.tomllibs/security/src/security/smime.pylibs/security/tests/test_smime.pyservices/api/src/api/adapters/httpx_as2_tester.pyservices/api/src/api/adapters/vault.pyservices/api/src/api/ports/as2_tester.pyservices/api/src/api/ports/repository.pyservices/api/src/api/routers/trading_partners/as2_receive.pyservices/api/src/api/routers/trading_partners/platform/as2_partnerships.pyservices/api/src/api/services/as2_receive_service.pyservices/api/tests/test_as2_receive_service.pyservices/api/tests/test_httpx_as2_tester.pyservices/api/tests/test_routers_as2_receive_errors.pyservices/as2_server/tests/test_receive_as2.py
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
New Features
Bug Fixes