Feat/escrows release funds#428
Merged
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a ChangesWebhook routes and test infra
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant TrustlessWork
participant releaseFundsRoute
participant releaseFundsHandler
participant Hasura
TrustlessWork->>releaseFundsRoute: POST /api/escrows/release-funds
releaseFundsRoute->>releaseFundsHandler: forward request
releaseFundsHandler->>releaseFundsHandler: validate contractId & releaseSigner
releaseFundsHandler->>Hasura: camelCase mutation (update status=completed, balance=0)
alt Hasura returns errors
releaseFundsHandler->>Hasura: snake_case fallback mutation
end
Hasura-->>releaseFundsHandler: affected_rows
alt no rows updated
releaseFundsHandler-->>TrustlessWork: 404 escrow not found
else success
releaseFundsHandler-->>TrustlessWork: 200 {received: true}
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@KevinMB0220 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
![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: 5
🧹 Nitpick comments (1)
tests/karate/features/escrows/release-funds.feature (1)
9-38: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a mismatched-
releaseSignerscenario.The fixture seeds a releaser, but the suite never asserts that a different signer is rejected. That would have caught the current handler behavior where
releaseSigneris ignored during the update.🤖 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 `@tests/karate/features/escrows/release-funds.feature` around lines 9 - 38, Add a negative test in the release-funds Karate feature for a mismatched releaseSigner. Extend the existing scenarios in release-funds.feature by calling the /api/escrows/release-funds endpoint with the correct contractId but an unauthorized signer, then assert the request is rejected with the expected error/status. Use the existing releaseSigner, contractId, and db.query checks as reference points, and make sure the new scenario verifies the escrow row is not marked completed and the balance is not updated when the signer does not match.
🤖 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 `@docker-compose-test.yml`:
- Around line 152-157: The test setup step that downloads yq should not use the
moving latest release and should fail immediately on bad downloads. In the
docker-compose test command near the yq install and metadata setup flow, pin the
yq URL to a specific release version, switch the download to curl -fsSL so HTTP
and write errors stop the script, and add a yq --version verification before
setup-tenant.sh is invoked to ensure the binary is valid.
In `@webhook/src/routes/bid-requests/index.js`:
- Around line 7-8: The new POST route wiring in the bid-requests router exposes
a contract mismatch between `createRoute` and the Karate feature expectations.
Update `create.handler` (and any related response shaping) so it accepts the
same request field names the `/api/bid-requests` consumer sends, returns the
response shape the tests assert, and matches the expected 400/409 message text;
use the `createRoute` and `create.handler` symbols to locate the request parsing
and response body logic.
In `@webhook/src/routes/escrows/release-funds.handler.js`:
- Around line 2-39: The release-funds handler currently validates releaseSigner
but does not use it, so the escrow can be completed using only contractId.
Update the lookup logic in release-funds.handler.js to include releaseSigner in
both mutation paths used by the handler (the custom camelCase mutation and the
default snake_case mutation), or add an equivalent verification step before the
admin-secret mutation is sent. Make sure the existing request body handling
around contractId and releaseSigner is preserved and that the Hasura where
clauses or callback check uniquely match both fields.
In `@webhook/src/routes/escrows/release-funds.route.js`:
- Around line 4-5: The release-funds endpoint is mounted outside the secured API
middleware chain, so it bypasses the normal auth/audit/limiting protections.
Update the route wiring for releaseFundsHandler in release-funds.route.js and
its mount in routes/index.js so it is either placed under the existing /api
chain or wrapped with the webhook-specific middleware stack (verifyAdminSecret,
validateJWT, auditLog, createTenantLimiter) at the mount point. Make sure the
final router composition keeps this state-changing endpoint protected
consistently with the other API routes.
In `@webhook/src/routes/index.js`:
- Around line 13-14: The release-funds webhook is currently mounted before
authenticateFirebase, so releaseFundsRoute can be reached without request
verification. Update the route setup in the router that mounts releaseFundsRoute
to apply webhook authentication/signature or shared-secret verification before
the handler runs, and ensure the /api/escrows/release-funds path is protected at
the route level rather than relying only on the Hasura admin-secret mutation.
---
Nitpick comments:
In `@tests/karate/features/escrows/release-funds.feature`:
- Around line 9-38: Add a negative test in the release-funds Karate feature for
a mismatched releaseSigner. Extend the existing scenarios in
release-funds.feature by calling the /api/escrows/release-funds endpoint with
the correct contractId but an unauthorized signer, then assert the request is
rejected with the expected error/status. Use the existing releaseSigner,
contractId, and db.query checks as reference points, and make sure the new
scenario verifies the escrow row is not marked completed and the balance is not
updated when the signer does not match.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec7a4637-77c8-48ff-8cfd-c2ab5f75f197
📒 Files selected for processing (15)
docker-compose-test.ymlmetadata/deploy-tenant.shtests/karate/features/escrows/release-funds.featuretests/karate/fixtures/seed-test-escrows.sqlwebhook/src/index.jswebhook/src/middleware/auth.middleware.jswebhook/src/routes/auth/index.jswebhook/src/routes/auth/sync-user.handler.jswebhook/src/routes/auth/sync-user.route.jswebhook/src/routes/auth/sync-wallet.route.jswebhook/src/routes/bid-requests/index.jswebhook/src/routes/escrows/release-funds.handler.jswebhook/src/routes/escrows/release-funds.route.jswebhook/src/routes/index.jswebhook/src/routes/reconciliation/index.js
Comment on lines
+152
to
+157
| case \$(uname -m) in | ||
| aarch64|arm64) ARCH=arm64 ;; | ||
| *) ARCH=amd64 ;; | ||
| esac; | ||
| curl -L https://github.com/mikefarah/yq/releases/latest/download/yq_linux_\$ARCH -o /usr/local/bin/yq && chmod +x /usr/local/bin/yq; | ||
| cd metadata && ./setup-tenant.sh safetrust --endpoint http://graphql-engine-test:8080 --admin-secret myadminsecretkey |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## locate relevant files\n'
git ls-files | rg '^(docker-compose-test\.yml|metadata/(build-metadata\.sh|setup-tenant\.sh)|.*yq.*|README\.md)$'
printf '\n## line counts\n'
for f in docker-compose-test.yml metadata/build-metadata.sh metadata/setup-tenant.sh; do
if [ -f "$f" ]; then
wc -l "$f"
fi
done
printf '\n## relevant excerpts\n'
sed -n '130,190p' docker-compose-test.yml
printf '\n--- metadata/build-metadata.sh ---\n'
sed -n '1,120p' metadata/build-metadata.sh
printf '\n--- metadata/setup-tenant.sh ---\n'
sed -n '1,200p' metadata/setup-tenant.sh
printf '\n## search for yq usage/version pinning\n'
rg -n "yq|curl -L https://github.com/mikefarah/yq|releases/latest|--version" -S .Repository: safetrustcr/backend-SafeTrust
Length of output: 10719
Pin the yq download and fail fast.
releases/latest makes the test setup drift, and curl -L can still write a bad response to /usr/local/bin/yq; pin a specific release and use curl -fsSL plus a yq --version check before setup-tenant.sh runs.
🤖 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 `@docker-compose-test.yml` around lines 152 - 157, The test setup step that
downloads yq should not use the moving latest release and should fail
immediately on bad downloads. In the docker-compose test command near the yq
install and metadata setup flow, pin the yq URL to a specific release version,
switch the download to curl -fsSL so HTTP and write errors stop the script, and
add a yq --version verification before setup-tenant.sh is invoked to ensure the
binary is valid.
Comment on lines
7
to
+8
| router.use('/', updateRoute) | ||
| router.use('/', createRoute) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The new POST mount exposes a broken request/response contract.
webhook/src/routes/bid-requests/create.handler.js:19-66 reads camelCase fields (apartmentId, proposedPrice, desiredMoveIn) and returns { bidRequest }, but tests/karate/features/bid-requests/create-bid-request.feature:1-74 exercises /api/bid-requests with snake_case fields and asserts response.bid... plus different 400/409 messages. With Line 8 mounting this route, the advertised happy path will 400 on “missing required fields” instead of returning 201. Align the handler and consumer contract before merge.
🤖 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 `@webhook/src/routes/bid-requests/index.js` around lines 7 - 8, The new POST
route wiring in the bid-requests router exposes a contract mismatch between
`createRoute` and the Karate feature expectations. Update `create.handler` (and
any related response shaping) so it accepts the same request field names the
`/api/bid-requests` consumer sends, returns the response shape the tests assert,
and matches the expected 400/409 message text; use the `createRoute` and
`create.handler` symbols to locate the request parsing and response body logic.
Comment on lines
+2
to
+39
| const { contractId, releaseSigner } = req.body; | ||
|
|
||
| if (!contractId || !releaseSigner) { | ||
| return res.status(400).json({ | ||
| error: 'Missing required fields: contractId, releaseSigner' | ||
| }); | ||
| } | ||
|
|
||
| // 1. Custom camelCase mutation (applied when metadata is fully loaded) | ||
| const mutationCustom = ` | ||
| mutation ReleaseFunds($contractId: String!) { | ||
| update_trustlessWorkEscrows( | ||
| where: { contractId: { _eq: $contractId } } | ||
| _set: { | ||
| status: "completed", | ||
| balance: 0, | ||
| updatedAt: "now()" | ||
| } | ||
| ) { | ||
| returning { id contractId status balance } | ||
| } | ||
| } | ||
| `; | ||
|
|
||
| // 2. Default snake_case mutation (fallback for default tracked tables) | ||
| const mutationDefault = ` | ||
| mutation ReleaseFunds($contractId: String!) { | ||
| update_trustless_work_escrows( | ||
| where: { contract_id: { _eq: $contractId } } | ||
| _set: { | ||
| status: "completed", | ||
| balance: 0, | ||
| updated_at: "now()" | ||
| } | ||
| ) { | ||
| returning { id contract_id status balance } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Use releaseSigner in the escrow lookup.
The handler requires releaseSigner but never uses it, so a caller only needs a valid contractId to flip an escrow to completed. Please include the signer in both Hasura where clauses (or verify the callback signature before issuing the admin-secret mutation).
🤖 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 `@webhook/src/routes/escrows/release-funds.handler.js` around lines 2 - 39, The
release-funds handler currently validates releaseSigner but does not use it, so
the escrow can be completed using only contractId. Update the lookup logic in
release-funds.handler.js to include releaseSigner in both mutation paths used by
the handler (the custom camelCase mutation and the default snake_case mutation),
or add an equivalent verification step before the admin-secret mutation is sent.
Make sure the existing request body handling around contractId and releaseSigner
is preserved and that the Hasura where clauses or callback check uniquely match
both fields.
Comment on lines
+4
to
+5
| const router = express.Router(); | ||
| router.post('/api/escrows/release-funds', releaseFundsHandler); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
This route bypasses the /api auth middleware chain.
webhook/src/routes/index.js:10-19 mounts releaseFundsRoute before router.use('/api', authenticateFirebase), so this state-changing endpoint does not inherit the normal API protection. Please move it under the secured chain or attach the webhook-specific secret/JWT middleware at the mount point. Based on learnings, webhook routes should inherit the centralized verifyAdminSecret/validateJWT/auditLog/createTenantLimiter chain at router mounting.
🤖 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 `@webhook/src/routes/escrows/release-funds.route.js` around lines 4 - 5, The
release-funds endpoint is mounted outside the secured API middleware chain, so
it bypasses the normal auth/audit/limiting protections. Update the route wiring
for releaseFundsHandler in release-funds.route.js and its mount in
routes/index.js so it is either placed under the existing /api chain or wrapped
with the webhook-specific middleware stack (verifyAdminSecret, validateJWT,
auditLog, createTenantLimiter) at the mount point. Make sure the final router
composition keeps this state-changing endpoint protected consistently with the
other API routes.
Source: Learnings
Comment on lines
+13
to
14
| router.use(releaseFundsRoute) | ||
| router.use('/api', authenticateFirebase) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in webhook/src/routes/escrows/release-funds.route.js webhook/src/routes/escrows/release-funds.handler.js; do
echo "=== $f ==="
sed -n '1,220p' "$f"
echo
done
echo "=== auth / signature checks around the escrow webhook ==="
rg -n -C2 'authenticateFirebase|authMiddleware|Authorization|signature|secret|hmac|verify' webhook/src/routes/escrows webhook/src/middlewareRepository: safetrustcr/backend-SafeTrust
Length of output: 10174
Protect /api/escrows/release-funds with request authentication
releaseFundsRoute is mounted before authenticateFirebase, and this handler has no signature/shared-secret check of its own. As written, anyone who can reach the endpoint can mark an escrow as completed via the Hasura admin secret. Add route-level webhook verification before the mutation.
🤖 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 `@webhook/src/routes/index.js` around lines 13 - 14, The release-funds webhook
is currently mounted before authenticateFirebase, so releaseFundsRoute can be
reached without request verification. Update the route setup in the router that
mounts releaseFundsRoute to apply webhook authentication/signature or
shared-secret verification before the handler runs, and ensure the
/api/escrows/release-funds path is protected at the route level rather than
relying only on the Hasura admin-secret mutation.
Source: Learnings
sotoJ24
approved these changes
Jul 1, 2026
sotoJ24
requested changes
Jul 1, 2026
sotoJ24
left a comment
sotoJ24
left a comment
Contributor
There was a problem hiding this comment.
Dear @KevinMB0220, could you please update you're branch, everything else, well done, many thanks
sotoJ24
approved these changes
Jul 2, 2026
6 tasks
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
2 participants
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.
feat: implement escrow release-funds webhook handler and fix index routes
Description
This PR implements the webhook handler for
POST /api/escrows/release-fundsto handle TrustlessWork release callbacks and updates the escrow record status tocompletedand balance to0. It also resolves several critical structure issues and import failures present on themainbranch.Key Changes
1. Escrow Release Funds Webhook
release-funds.handler.js): Processes callbacks withcontractIdandreleaseSigner. Updates the escrow record inpublic.trustless_work_escrowstable via a Hasura GraphQL mutation.update_trustlessWorkEscrows), and falls back to snake_case (update_trustless_work_escrows) if metadata configuration is not fully loaded.release-funds.route.js): MountsPOST /api/escrows/release-fundsand exposes it.2. Main Branch Fixes & Restructuring
sync-user.handler.js), route, and index entrypoint which were absent or deleted onmain.auth.middleware.jsinstead of the non-existentauth.js.routes/reconciliation/index.jsto bundle reconciliation routes.deploy-tenant.shto extract database table names directly from the YAML files instead of the file names, preventingInconsistent objectexceptions in Hasura during migrations.yqCLI downloader todocker-compose-test.ymlduring test runs so metadata is compiled and applied correctly inside the test container.3. Verification & Tests
seed-test-escrows.sql): Seed script to populate a test escrow in the database withmilestone_approvedstatus and a balance of3200.0.release-funds.feature): Added feature tests covering:'completed', balance ->0.0).How Has This Been Tested?
The Karate integration test suite was run in Docker Compose:
docker compose -f docker-compose-test.yml run --rm --build karate
Closes #418
Summary by CodeRabbit
New Features
Bug Fixes
Tests