Add rots proxy probe command for live endpoint verification#26
Conversation
Introduces a curl-based probe command that verifies deployed Caddy behaviour: TLS cert chain, security headers, timing breakdown, and HTTP status. Supports DNS-independent staging tests via --resolve and --connect-to, CI assertion mode via --expect-status/--expect-header, and both human-readable and JSON output formats. Helpers: ProbeResult, build_curl_args, parse_curl_output, evaluate_assertions, run_probe. Tests: 27 new tests (20 helper, 7 command) — all 108 proxy tests pass.
…-remaining to proxy probe Five enhancements to `rots proxy probe` for CI and staging use cases: - --method: expose curl -X for HEAD/OPTIONS requests - --insecure: skip TLS verification for self-signed certs (-k) - --follow: follow redirects (-L), off by default - --retries/--retry-delay: retry loop wrapping both probe and assertions - --expect-cert-days-remaining: assert minimum days until cert expiry 22 new tests (130 total proxy tests passing).
|
Claude finished @delano's task —— View job Code Review for
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a powerful new Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable rots proxy probe command for live endpoint verification. However, a high-severity argument injection vulnerability was found in the probe command due to insufficient validation of the URL parameter and improper construction of the curl command line. Additionally, a significant issue was identified with how multi-value HTTP headers are handled, which could lead to incorrect assertion results, and a minor style issue regarding a local import. Addressing these concerns will ensure the tool is both robust and secure.
| if follow_redirects: | ||
| cmd.append("-L") | ||
|
|
||
| cmd.append(url) |
There was a problem hiding this comment.
The build_curl_args function is vulnerable to argument injection. The user-supplied url is appended directly to the cmd list. If the url starts with a hyphen (e.g., -K/etc/passwd), curl will interpret it as an option. This allows an attacker who can control the url parameter to inject arbitrary curl options, which could lead to sensitive file disclosure (using -K or --config) or arbitrary file write (using -o or --output).
To remediate this, use the -- separator to signal the end of options before appending the URL, or validate the URL to ensure it has a valid scheme and hostname.
| cmd.append(url) | |
| cmd.append("--") | |
| cmd.append(url) |
|
|
||
| @app.command | ||
| def probe( | ||
| url: Annotated[str, cyclopts.Parameter(help="URL to probe")], |
There was a problem hiding this comment.
The url parameter for the probe command is passed to run_probe without validation. This contributes to the argument injection vulnerability in the underlying curl command construction. Other similar commands in this file, such as trace, use parse_trace_url to validate the URL and ensure it contains a valid hostname, which effectively prevents strings starting with hyphens from being treated as URLs.
It is recommended to validate the url parameter using parse_trace_url before proceeding with the probe.
There was a problem hiding this comment.
Pull request overview
Adds a new rots proxy probe CLI command to perform live HTTP/TLS endpoint probing via curl, including timing diagnostics, assertions (status/headers/cert expiry), optional retries for CI readiness checks, and human/JSON output. The implementation is split between curl/probing mechanics in proxy/_helpers.py and CLI/retry/output logic in proxy/app.py.
Changes:
- Introduce
probecommand with retry loop, assertion handling, and human/JSON output modes. - Add probe helper utilities (
ProbeResult, curl argv construction, output parsing, assertion evaluation, probe execution). - Add unit/CLI tests covering argument construction, parsing, assertions, retries, and error handling.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/commands/proxy/test_helpers.py | Adds unit tests for probe helper functions (curl args, parsing, cert expiry, assertions, run_probe). |
| tests/commands/proxy/test_app.py | Adds CLI-level tests for probe output modes, assertions, and retry behavior. |
| src/rots/commands/proxy/app.py | Adds probe command implementation and output formatters. |
| src/rots/commands/proxy/_helpers.py | Adds probe dataclass and curl-based probe building/parsing/assertion logic. |
| src/rots/commands/proxy/init.py | Exposes probe via module exports. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cmd = [ | ||
| "curl", | ||
| "-s", | ||
| "-o", | ||
| "/dev/null", | ||
| "-D", | ||
| "-", | ||
| "-w", | ||
| f"\n{_CURL_SENTINEL}\n%{{json}}", | ||
| "--max-time", | ||
| str(timeout), |
There was a problem hiding this comment.
build_curl_args uses -s (silent), which suppresses curl's error messages. Since run_probe relies on proc.stderr/result.stderr to explain failures, many real-world failures will surface as curl failed ...: with an empty message. Consider switching to -sS (or -s + -S) so errors remain available while keeping progress output quiet.
| try: | ||
| from datetime import datetime | ||
|
|
||
| expiry = datetime.strptime(cert_expiry, "%b %d %H:%M:%S %Y %Z") |
There was a problem hiding this comment.
_parse_cert_expiry_days uses datetime.strptime(..., "%b %d %H:%M:%S %Y %Z") to parse the timezone. %Z parsing is platform/locale-dependent and may not reliably accept GMT, causing valid expiries to be treated as unparseable and cert assertions to fail. Since you replace tzinfo with UTC anyway, consider parsing with a literal " GMT" suffix (or using a more robust parser) and then attaching UTC.
| expiry = datetime.strptime(cert_expiry, "%b %d %H:%M:%S %Y %Z") | |
| # Normalize and strip the fixed " GMT" suffix instead of relying on | |
| # platform-dependent %Z parsing. We then explicitly attach UTC. | |
| normalized = cert_expiry.strip() | |
| if normalized.endswith(" GMT"): | |
| normalized = normalized[: -len(" GMT")] | |
| expiry = datetime.strptime(normalized, "%b %d %H:%M:%S %Y") |
…rsing, multi-value headers - Use -sS instead of -s so curl error messages appear in stderr for debugging - Parse only the final HTTP response header block when following redirects, preventing stale headers from earlier hops leaking into assertions - Support multi-value HTTP headers (dict[str, list[str]]) throughout the probe pipeline: parsing, assertions, human output, and JSON output - Add -- separator before URL to prevent argument injection via URLs starting with hyphens - Move datetime import to module level (ruff/style consistency)
Summary
Adds a
rots proxy probecommand that wraps curl to perform live HTTP/TLS endpoint probes with timing diagnostics. The command supports assertions (HTTP status, response headers, certificate expiry), retry logic for CI readiness checks, and both human-readable and JSON output modes.The implementation splits cleanly between low-level curl mechanics in
_helpers.py(argument construction, output parsing, assertion evaluation) and the CLI interface inapp.py(retry loop, output formatting, exit codes).Key capabilities:
--expect-cert-days-remaining)--resolveand--connect-tofor staging verification--jsonoutput for automation pipelinesTest plan
TestBuildCurlArgs)TestParseCurlOutput)TestParseCertExpiryDays)TestEvaluateAssertions)TestRunProbe)test_app.py)