fix(security): prevent outbound webhook SSRF and DNS rebinding - #588
fix(security): prevent outbound webhook SSRF and DNS rebinding#588seonghobae wants to merge 71 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough웹훅 등록과 전송에 공개 HTTPS, DNS/IP 검증, 주소 고정, 리디렉션 차단, 제한된 재시도를 적용했다. 기존 부적합 대상은 시작 시 비활성화하고 감사 로그를 기록한다. Stripe 웹훅은 원문 서명 검증 후 처리한다. 요청 스트림 정리와 전송 타임아웃 테스트도 추가했다. Changes웹훅 보안 및 SaaS API
Estimated code review effort: 5 (Critical) | ~90 minutes Merge Risk: 🟠 High · up to The PR hardens outbound webhook delivery, but an existing payment-webhook path may still allow unauthorized plan upgrades when authentication and signature verification are absent; this is a high-impact security risk that should be fixed before merge. The legacy migration code also has a known lint error without exact-head clearance. Sequence Diagram(s)sequenceDiagram
participant Client
participant app_mjs
participant app_core_mjs
participant postWebhook
participant WebhookTransport
participant WebhookEndpoint
Client->>app_mjs: 웹훅 등록 요청
app_mjs->>app_core_mjs: 등록 요청 전달
app_core_mjs->>postWebhook: 서명된 이벤트 전달
postWebhook->>WebhookTransport: URL과 요청 데이터 전달
WebhookTransport->>WebhookEndpoint: 검증된 주소로 POST
WebhookEndpoint-->>WebhookTransport: HTTP 응답 반환
WebhookTransport-->>app_core_mjs: 전달 결과 반환
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@coderabbitai review Please review the freshly refetched exact contributor head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
expected='bc60476829ec98f37d83977a9432b79ba0b3c0d6'
actual="$(git rev-parse HEAD)"
printf 'HEAD=%s\nEXPECTED=%s\n' "$actual" "$expected"
if [ "$actual" != "$expected" ]; then
echo 'ERROR: checked-out revision does not match the requested exact head.' >&2
exit 2
fi
printf '\n== Exact-head commit ==\n'
git show -s --format='%H%n%P%n%s' HEAD
printf '\n== Changed files relative to first parent ==\n'
git diff-tree --no-commit-id --name-status -r HEAD
printf '\n== Module outlines ==\n'
ast-grep outline server/webhook_legacy_migration.mjs --items all
ast-grep outline server/db.mjs --items all
ast-grep outline server/webhook_transport.mjs --items all
printf '\n== Migration diff at exact head ==\n'
git diff HEAD^ HEAD -- server/webhook_legacy_migration.mjs tests/unit/webhook-legacy-migration.test.mjs tests/api/webhook-legacy-migration.test.mjs
printf '\n== Migration source ==\n'
cat -n server/webhook_legacy_migration.mjs
printf '\n== Relevant DB startup and policy call sites ==\n'
rg -n -C 5 -P '\bmigrateLegacyWebhookDestinations\s*\(|\bvalidateWebhookRegistrationUrl\s*\(' \
server/db.mjs server/webhook_legacy_migration.mjs server/webhook_transport.mjs \
tests/unit/webhook-legacy-migration.test.mjs tests/api/webhook-legacy-migration.test.mjs
printf '\n== Focused migration tests ==\n'
cat -n tests/unit/webhook-legacy-migration.test.mjs
cat -n tests/api/webhook-legacy-migration.test.mjsLength of output: 33185 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '== Pull-request range and changed files ==\n'
base="$(git merge-base HEAD origin/develop 2>/dev/null || git merge-base HEAD develop)"
printf 'BASE=%s\nHEAD=%s\n' "$base" "$(git rev-parse HEAD)"
git diff --stat "$base"..HEAD
git diff --name-status "$base"..HEAD
printf '\n== Registration policy implementation ==\n'
sed -n '72,225p' server/webhook_transport.mjs
printf '\n== Migration evolution across this PR ==\n'
git log --format='%H %s' --reverse "$base"..HEAD -- server/webhook_legacy_migration.mjs
git diff "$base"..HEAD -- server/webhook_legacy_migration.mjs
printf '\n== All migration callers and webhook table mutation statements ==\n'
rg -n -C 5 -P \
'\bmigrateLegacyWebhookDestinations\s*\(|\b(?:INSERT\s+INTO|UPDATE|DELETE\s+FROM)\s+webhooks\b|\.insert\([^)]*webhooks|\.update\([^)]*webhooks' \
server tests
printf '\n== Preflight-related tests and SQLite transaction use ==\n'
rg -n -C 4 -P \
'\b(activeWebhookDestinations|hasPolicyIncompatibleDestination|BEGIN IMMEDIATE|busy_timeout|contend|concurr|preflight)\b' \
server tests
printf '\n== Package test commands that include migration coverage ==\n'
node -e "const p=require('./package.json'); console.log(JSON.stringify({scripts:p.scripts}, null, 2))" 2>/dev/null || \
sed -n '/\"scripts\"[[:space:]]*:/,/^[[:space:]]*},/p' package.jsonLength of output: 36406 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo='ContextualWisdomLab/scopeweave'
pr='588'
sha='bc60476829ec98f37d83977a9432b79ba0b3c0d6'
printf '== Pull request head and mergeability ==\n'
gh pr view "$pr" --repo "$repo" \
--json number,headRefOid,baseRefOid,mergeStateStatus,isDraft,reviewDecision,statusCheckRollup \
--jq '{
number,
headRefOid,
baseRefOid,
mergeStateStatus,
isDraft,
reviewDecision,
statusCheckRollup: [.statusCheckRollup[] | {
name,
status,
conclusion,
detailsUrl,
headSha
}]
}'
printf '\n== Check runs attached to the requested exact commit ==\n'
gh api "repos/$repo/commits/$sha/check-runs?per_page=100" \
--jq '.check_runs[] | {
name,
status,
conclusion,
started_at,
completed_at,
details_url
}'
printf '\n== Registration route policy boundary ==\n'
sed -n '710,770p' server/app_core.mjs
printf '\n== Database startup context ==\n'
sed -n '145,186p' server/db.mjs
printf '\n== Latest exact-head patch ==\n'
git diff --check HEAD^ HEAD
git diff --word-diff=plain HEAD^ HEAD -- server/webhook_legacy_migration.mjsLength of output: 25547 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo='ContextualWisdomLab/scopeweave'
run='33115785070'
job='98670001775'
printf '== Failed job details ==\n'
gh api "repos/$repo/actions/jobs/$job" \
--jq '{id,name,status,conclusion,started_at,completed_at,html_url,steps:[.steps[]|{name,status,conclusion,number}]}'
printf '\n== Failed job log ==\n'
gh run view "$run" --repo "$repo" --job "$job" --log-failed
printf '\n== Required status checks on develop ==\n'
gh api "repos/$repo/branches/develop/protection/required_status_checks" \
--jq '{strict,contexts,checks}' || true
printf '\n== Workflow run event and commit association ==\n'
gh api "repos/$repo/actions/runs/$run" \
--jq '{event,head_sha,head_branch,conclusion,status,workflow_id,html_url}'Length of output: 4867 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo='ContextualWisdomLab/scopeweave'
run='33115785070'
job='98670001775'
archive="$(mktemp /tmp/strix-job-log.XXXXXX.zip)"
extract="$(mktemp -d /tmp/strix-job-log.XXXXXX)"
trap 'rm -f "$archive"; rm -rf "$extract"' EXIT
printf '== Download and inspect Strix job log archive ==\n'
gh api --follow "repos/$repo/actions/jobs/$job/logs" > "$archive"
file "$archive"
unzip -q "$archive" -d "$extract"
fd -t f . "$extract" -x sh -c '
printf "\n--- %s ---\n" "$1"
rg -n -i -C 8 "(error|fail|exception|traceback|finding|critical|high|medium|low)" "$1" || true
' sh {}
printf '\n== Required status checks on develop ==\n'
gh api "repos/$repo/branches/develop/protection/required_status_checks" \
--jq '{strict,contexts,checks}' || true
printf '\n== Workflow run event and commit association ==\n'
gh api "repos/$repo/actions/runs/$run" \
--jq '{event,head_sha,head_branch,conclusion,status,workflow_id,html_url}'Length of output: 2838 🏁 Script executed: #!/bin/bash
set -u -o pipefail
repo='ContextualWisdomLab/scopeweave'
run='33115785070'
job='98670001775'
archive="$(mktemp /tmp/strix-job-log.XXXXXX)"
extract="$(mktemp -d /tmp/strix-job-log.XXXXXX)"
trap 'rm -f "$archive"; rm -rf "$extract"' EXIT
printf '== Direct Strix job log request ==\n'
if gh api "repos/$repo/actions/jobs/$job/logs" > "$archive"; then
file "$archive"
if unzip -tqq "$archive"; then
unzip -q "$archive" -d "$extract"
fd -t f . "$extract" -x sh -c '
printf "\n--- %s ---\n" "$1"
rg -n -i -C 8 "(error|fail|exception|traceback|finding|critical|high|medium|low)" "$1" || true
' sh {}
else
printf '%s\n' 'The log response is not a ZIP archive.'
head -c 2000 "$archive" || true
fi
else
printf '%s\n' 'The job-log endpoint could not be retrieved.'
fi
printf '\n== Required status checks on develop ==\n'
gh api "repos/$repo/branches/develop/protection/required_status_checks" \
--jq '{strict,contexts,checks}' || true
printf '\n== Workflow run event and commit association ==\n'
gh api "repos/$repo/actions/runs/$run" \
--jq '{event,head_sha,head_branch,conclusion,status,workflow_id,html_url}' || trueLength of output: 2093
I found no reachable code issue in the read-only preflight.
The exact-head One validation item remains: exact-head You are interacting with an AI system. |
|
@opencode-agent review Review-only request for exact head |
seonghobae
added
bug
ChatGPT Codex Connector
1 participant
Security outcome
Refs #551.
This is the bounded protected-
developrepair lane for outbound ScopeWeave webhook SSRF and DNS rebinding only. It intentionally does not own Stripe inbound-webhook trust, OIDC/auth hardening, or rate-limit policy; those remain in their separate owning lanes.Exact current identity
bc60476829ec98f37d83977a9432b79ba0b3c0d6;d6d029a9d9ab31de4df3aa2a51466267d19d7681;fix/webhook-ssrf-551-develop;develop@2c328875e00e86537df3e965170be80532571cad;Any contributor-head, protected-base, review, or required-gate movement invalidates revision-sensitive evidence until freshly reconciled.
Current production boundary
server/webhook_transport.mjsowns destination parsing, public-address authorization, A+AAAA validation, IPv4/IPv6 special-use rejection, validated-address pinning through the connectionlookupseam, TLS-hostname preservation, redirect non-following, transport timeout/failure semantics, and per-attempt DNS re-resolution.server/app_core.mjsuses the protected webhook transport directly for outbound delivery and applies the same registration policy before persisting a destination. This avoids a process-globalfetchoverride and keeps SSRF protection independent of facade import order.server/app.mjsis the bounded public registration facade. It preserves the core global middleware envelope while canonicalizing the exact registration POST without consuming an unauthenticated caller's body before auth/RBAC/rate-limit decisions.SCOPEWEAVE_DEV=1permits only the explicit development-loopback exception; registration, delivery, and historical-row migration share that policy.webhook.security_blockaudit event. A read-only preflight now avoids taking SQLite's writer reservation when all active rows already satisfy policy; if mutation is required,BEGIN IMMEDIATEis still acquired and active rows are re-read inside the transaction before any mutation.Scope separation and current review truth
The branch does not claim authority for separate inbound billing-trust or unrelated auth controls. Historical review findings that exposed global-fetch mutation, auth-before-body-read ordering, route-replay/static-catch-all ordering, legacy destination migration, transport timeout, rollback error preservation, registration/storage validation, and related security/operability defects have been repaired in this lane or returned to their owning scope.
At the latest fresh refetch every posted review thread is resolved. The remaining historical informational observations describe tested trade-offs such as WHATWG numeric-host normalization, process-start development-mode semantics, deterministic startup migration, Hono route-replay coupling, duplicate idempotent storage-boundary validation, and the TLS-handshake replay boundary; none currently demonstrates a reachable additional outbound-webhook SSRF defect on exact head
bc604768....Regression and buyer traceability
The branch retains realistic regressions for:
docs/doctoring/webhook-destination-security.mdrecords active-PR versus protected-shipped truth, threat model, recovery/rollback semantics, executable acceptance, and primary standards references.CHANGELOG.mdrecords the active Unreleased security work without claiming protected shipment or certification.Fresh workflow truth
For exact contributor head
bc60476829ec98f37d83977a9432b79ba0b3c0d6, the currently associated repository workflow runs are terminal GitHub-success:33115786583;33115786568;33115786489;33115786575;33115786503;33115787008.These labels are not all promoted to immutable contributor-head merge authority while the known checkout-evidence defects remain unresolved. ScopeWeave #523 owns repository-native exact-head Server Tests/coverage integrity. The organization-required reusable exact-head SAST/Security repair remains owned by the central repository's existing control path. Synthetic/predecessor/status-only/model-only evidence is non-passing.
Merge boundary
Do not merge or enable auto-merge until the unchanged exact contributor head remains reconciled to live protected
develop, #523's repository exact-head Server Tests/coverage control and any central reusable exact-head SAST/Security control are protected-shipped and regenerated on this exact head, every applicable functional/browser/owned statement-branch-function-line coverage/docstring/CodeQL/SAST/security/dependency/supply-chain/package/provenance gate is substantively terminal-passing, valid unresolved current-head defects are zero, and a qualifying independent current-head/last-push approval exists under live branch protection/rulesets.Pending, queued, skipped-required, cancelled, absent, neutral-required, failed, stale, predecessor, synthetic-only, status-only, author-only, model-only, rate-limited, or infrastructure-only evidence is non-passing.