feat: support remote URLs for Rego policy in kosli evaluate#875
Conversation
Allow `--policy https://host/policy.rego` (or http://) on `evaluate trail`, `evaluate trails`, and `evaluate input`. Local file paths continue to work unchanged. Remote fetches honor `--http-proxy` and warn on plain HTTP.
|
Claude finished @mbevc1's task in 2m 11s —— View job Review of PR #875 — Remote HTTP(S) URLs for
|
| Feedback | Status |
|---|---|
| Status code checked before body read | ✅ Fixed in b6e7ba29 |
Unbounded io.ReadAll → io.LimitReader |
✅ Fixed with policyMaxBytes+1 trick |
| SSRF via redirect | ✅ sameHostRedirectPolicy added |
| 3s timeout too tight | ✅ Bumped to 10s |
Rename policyFile → policyRef |
✅ Done in 189d6600 |
| Comment about TLS/dial settings | ✅ Added in a526de21 |
Code Review
Overall: this is a well-structured PR. The design is clean — loadPolicy dispatches between local/remote, security mitigations are solid, and tests are thorough.
Remaining nit (non-blocking)
Line 24 comment is redundant: The text 5 * 2^20 (5*1MiB) at the end of the comment block duplicates the inline // 5 MiB. The prior review suggested simplifying to just the clean comment block without the math. Very minor.
What's good
policyMaxBytes+1pattern (evaluateHelpers.go:167-172): Reading one extra byte and checking length is a clean way to detect oversized responses without writing a custom reader.sameHostRedirectPolicy(evaluateHelpers.go:181-189): Good SSRF mitigation — blocks cross-host redirects while still allowing same-host redirects (e.g., path normalization).- Test coverage is strong (
evaluateInput_test.go:163-274): Tests cover local file, HTTPS fetch, non-2xx rejection, body-not-read-on-error, oversized body, cross-host redirect blocking, and proxy forwarding. This is exactly the right set of edge cases. - Consistent renaming:
policyFile→policyRefthroughout struct, flags, and function signatures. No leftover references. policyFetchTimeoutasvar(evaluateHelpers.go:20): Allows test overrides — good testability practice.- Clean separation:
loadPolicy,isRemotePolicyRef, andfetchRemotePolicyare small, focused, and independently testable. - Help text and examples updated in all three commands with remote URL examples.
Summary
Solid PR. Prior security and correctness feedback has been addressed well. The one remaining nit (redundant comment on the constant) is cosmetic and non-blocking. The code is clean, well-tested, and follows the codebase's established patterns.
Allow
--policy https://host/policy.rego(or http://) onevaluate trail,evaluate trails, andevaluate input. Local file paths continue to work unchanged. Remote fetches honor--http-proxyand warn on plain HTTP.