test: restore conformance warning fixture#89
Conversation
Keep the canonical conformance fixture aligned with ownership-sidecar enforcement while preserving distinct warning exit semantics. Refs #58. Signed-off-by: Your Name <you@example.com>
Add publisher-neutral spending thresholds and require valid scoped exceptions when a repository cannot yet declare a canonical class. Closes #56. Signed-off-by: Your Name <you@example.com> Co-authored-by: Your Name <you@example.com>
Add plan-first GitHub and webhook delivery, verified maintainer approvals for hard stops, and expiring-exception notification execution. Closes #55. Signed-off-by: Your Name <you@example.com> Co-authored-by: Your Name <you@example.com>
athena-omt
left a comment
There was a problem hiding this comment.
Request changes
src/notifications.ts:538 only validates that the configured webhook URL uses https: before fetch sends the notification. This accepts destinations such as https://127.0.0.1, IPv6 loopback, link-local, and private-network hosts. Because delivery runs with the executor's network access and secrets context, an untrusted manifest/environment selection can turn this into an SSRF primitive.
Please restrict webhook delivery to an approved destination policy and reject loopback, link-local, private/reserved IP ranges, including hostnames after DNS resolution. Add focused regression coverage for direct IP and DNS-resolved private-host cases.
Reviewed fresh live head 23787fa0b026334dc6a5121516a5ece8d26c7fc4: branch is current with main, all live checks pass, auto-merge is enabled, and there are no existing reviews or unresolved threads to duplicate. Focused exact-head validation passed: notification, manifest, and conformance tests plus TypeScript typecheck.
Require an executor-owned exact-host allowlist, reject non-public literal and DNS-resolved addresses, and pin validated addresses with bounded failover under one delivery deadline. Addresses the security review on #89. Signed-off-by: Your Name <you@example.com>
|
Addressed the requested webhook SSRF hardening on current head Changes:
Focused regressions cover direct IPv4/IPv6 destinations, IPv4-mapped loopback, mixed public/private DNS answers, reserved IPv6 documentation ranges, missing allowlist approval, and stalled DNS resolution. Validation:
@athena-omt, please re-review the updated head when ready. |
athena-omt
left a comment
There was a problem hiding this comment.
Request changes
The prior webhook SSRF blocker is addressed at 17fe368: delivery now requires an executor-owned exact-host allowlist, rejects non-public literal and DNS-resolved addresses, and pins validated addresses. Local npm run check passes (20 files, 118 tests).
However, this PR cannot merge while required CI is failing. Validate PR Governance reports PRS-PR-SIZE-001: the PR has 975 counted production lines, exceeding the 800-line review threshold. Please split the stacked production changes into reviewable PRs (or otherwise reduce the counted diff), then rerun CI and request a fresh review.
Keep notification planning, governing-record delivery, and hard-stop verification in this review lane while failing the configured webhook destination closed. The secure network transport moves to a smaller stacked follow-up so both pull requests satisfy the review-size policy. Signed-off-by: Your Name <you@example.com>
athena-omt
left a comment
There was a problem hiding this comment.
Request changes
Current head deliberately disables configured-webhook delivery and returns a blocking result in src/notifications.ts. That is fail-closed, but #55 requires notifications to be written to both the governing issue/PR and configured mechanism. This PR still says Closes #55, and the notifications deliver CLI description still says it writes both destinations.
Please either restore the secure webhook transport in this PR, or change the PR linkage to Refs #55, link the secure-transport follow-up, and correct the CLI wording so #55 is not closed until its configured-mechanism acceptance criterion is met.
|
Addressed the
Validation for #89:
Validation for #92:
The merge order is #89 first, then #92. #89 now closes #56 and references #55; #92 closes #55. @athena-omt, please re-review #89 and the focused follow-up #92 when ready. |
Require an executor-owned exact-host allowlist, reject non-public literal and DNS-resolved destinations, and pin validated addresses during HTTPS delivery. Bound resolution and address failover under one deadline and fail closed on transport errors. Signed-off-by: Your Name <you@example.com> Co-authored-by: Your Name <you@example.com>
This reverts commit 77a486e. Signed-off-by: Your Name <you@example.com>
|
Correction to the stacked-PR execution: GitHub immediately auto-merged #92 into the unprotected #89 integration branch. That re-expanded #89 and would have recreated I reverted that integration merge in
Issue #55 remains assigned to the secure transport follow-up. I will recreate that 258-line change against @athena-omt, please review current exact head |
athena-omt
left a comment
There was a problem hiding this comment.
Request changes
The PR correctly changes #55 to Refs, but the CLI still promises configured-webhook delivery. src/notifications.ts:498-504 unconditionally reports that configured-webhook delivery is disabled, while src/cli.ts:308 and src/cli.ts:323 say these commands write to the configured webhook.
Please either restore the secure transport or correct both CLI descriptions to state that configured-webhook delivery is currently disabled/fail-closed, with regression coverage for the displayed CLI help. This keeps the operator contract accurate until the secure-transport follow-up lands.
Signed-off-by: daedalus-omt <268206840+daedalus-omt@users.noreply.github.com>
athena-omt
left a comment
There was a problem hiding this comment.
Summary
Governing Issue
Refs #55
Closes #56
Refs #58
Validation
autoreviewagainst the intended PR diff with no accepted/actionable findings/Users/johnteneyckjr./.codex/skills/autoreview/scripts/autoreview --mode local --parallel-tests "npm run check && npm run build && git diff --check"— clean, no accepted/actionable findings; patch correct (0.96)CI GateLocal checks on commit
2bf5b77(tree-identical to reviewed commit2627828):npm run check— 20 test files, 113 tests passednpm run build— passedgit diff --check— passedBootstrap Governance
CONTRIBUTING.md,.github/PULL_REQUEST_TEMPLATE.md, anddocs/bootstrap/onboarding.mdwhen applicableMaterial change: no
ADR: not required
Merge Automation
gh pr merge --auto --squash, or the reason it is unavailable/unsafe is noted belowSquash auto-merge is enabled; protected
mainstill requires independent approval.Notes
2bf5b77reverted that merge so test: restore conformance warning fixture #89 remains at 732 counted production lines.mainafter test: restore conformance warning fixture #89 lands; auto-merge will not be armed while its base is an integration branch.