cron: reject webhook URLs with embedded credentials#51822
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1329a47f5a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Do not allow embedded credentials in webhook URLs. | ||
| if (parsed.username || parsed.password) { | ||
| return null; |
There was a problem hiding this comment.
Prevent raw logging of rejected credential URLs
When a stored cron job already has delivery.mode = "webhook" with a user:pass@ URL, this new null path makes buildGatewayCronService fall into the warning at src/gateway/server-cron.ts:381-388, which logs deliveryTo: job.delivery.to without redaction. In that scenario the gateway will now write the full embedded credentials to logs on every run, so the hardening change introduces the secret leak it is trying to prevent.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR adds a four-line security hardening check to Key observations from the review:
Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/cron/webhook-url.ts
Line: 19-21
Comment:
**Degenerate `@`-separator URL passes through**
A URL like `http://:@example.com/path` has both `parsed.username` and `parsed.password` as `""` (empty strings), so `"" || ""` evaluates as falsy and the URL is accepted. Technically these are empty credentials and not a real secret-leakage risk, but the `@` in the URL could still confuse downstream parsers or loggers into treating it as a credential-bearing URL.
If the intent is to reject any URL with the `user:pass@host` authority format (even empty), the check could be expanded:
```suggestion
if (parsed.username || parsed.password || parsed.href.includes("@")) {
return null;
}
```
Alternatively, leave as-is with a comment noting the empty-credential edge case is intentionally allowed.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/cron/webhook-url.ts
Line: 19-21
Comment:
**Missing test coverage for security validation**
There is no test file for `normalizeHttpWebhookUrl` — no `webhook-url.test.ts` exists anywhere in the codebase. Since this change rejects inputs that were previously accepted, automated tests would lock in the expected behavior and guard against regressions. The rest of the `src/cron/` directory uses dedicated per-concern test files, so adding one here would be consistent.
Suggested cases to cover at minimum:
- URL with embedded username and password → `null`
- URL with only a username in the authority → `null`
- URL with only a password in the authority → `null`
- Plain `http://` URL without any credentials → accepted
- Plain `https://` URL without any credentials → accepted
- Non-http/https protocol → `null` (existing protocol check)
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "cron: reject webhook..." |
| if (parsed.username || parsed.password) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Degenerate
@-separator URL passes through
A URL like http://:@example.com/path has both parsed.username and parsed.password as "" (empty strings), so "" || "" evaluates as falsy and the URL is accepted. Technically these are empty credentials and not a real secret-leakage risk, but the @ in the URL could still confuse downstream parsers or loggers into treating it as a credential-bearing URL.
If the intent is to reject any URL with the user:pass@host authority format (even empty), the check could be expanded:
| if (parsed.username || parsed.password) { | |
| return null; | |
| } | |
| if (parsed.username || parsed.password || parsed.href.includes("@")) { | |
| return null; | |
| } |
Alternatively, leave as-is with a comment noting the empty-credential edge case is intentionally allowed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cron/webhook-url.ts
Line: 19-21
Comment:
**Degenerate `@`-separator URL passes through**
A URL like `http://:@example.com/path` has both `parsed.username` and `parsed.password` as `""` (empty strings), so `"" || ""` evaluates as falsy and the URL is accepted. Technically these are empty credentials and not a real secret-leakage risk, but the `@` in the URL could still confuse downstream parsers or loggers into treating it as a credential-bearing URL.
If the intent is to reject any URL with the `user:pass@host` authority format (even empty), the check could be expanded:
```suggestion
if (parsed.username || parsed.password || parsed.href.includes("@")) {
return null;
}
```
Alternatively, leave as-is with a comment noting the empty-credential edge case is intentionally allowed.
How can I resolve this? If you propose a fix, please make it concise.| if (parsed.username || parsed.password) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Missing test coverage for security validation
There is no test file for normalizeHttpWebhookUrl — no webhook-url.test.ts exists anywhere in the codebase. Since this change rejects inputs that were previously accepted, automated tests would lock in the expected behavior and guard against regressions. The rest of the src/cron/ directory uses dedicated per-concern test files, so adding one here would be consistent.
Suggested cases to cover at minimum:
- URL with embedded username and password →
null - URL with only a username in the authority →
null - URL with only a password in the authority →
null - Plain
http://URL without any credentials → accepted - Plain
https://URL without any credentials → accepted - Non-http/https protocol →
null(existing protocol check)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cron/webhook-url.ts
Line: 19-21
Comment:
**Missing test coverage for security validation**
There is no test file for `normalizeHttpWebhookUrl` — no `webhook-url.test.ts` exists anywhere in the codebase. Since this change rejects inputs that were previously accepted, automated tests would lock in the expected behavior and guard against regressions. The rest of the `src/cron/` directory uses dedicated per-concern test files, so adding one here would be consistent.
Suggested cases to cover at minimum:
- URL with embedded username and password → `null`
- URL with only a username in the authority → `null`
- URL with only a password in the authority → `null`
- Plain `http://` URL without any credentials → accepted
- Plain `https://` URL without any credentials → accepted
- Non-http/https protocol → `null` (existing protocol check)
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98e38956ec
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (parsed.username || parsed.password) { | ||
| return null; |
There was a problem hiding this comment.
Surface invalid legacy cron.webhook targets
For installations that still rely on legacy notify: true plus a global cron.webhook, rejecting credentialed URLs here turns previously working webhook deliveries into silent no-ops. resolveCronWebhookTarget now feeds cron.webhook through normalizeHttpWebhookUrl (src/gateway/server-cron.ts:74-77), but the only invalid-target warning is behind job.delivery.mode === "webhook" (src/gateway/server-cron.ts:381-388), so legacy jobs never emit any warning when their credentialed URL starts returning null. After upgrade those jobs just stop posting finished-run callbacks with no migration hint.
Useful? React with 👍 / 👎.
| if (parsed.username || parsed.password) { | ||
| return null; |
There was a problem hiding this comment.
Prevent doctor from rewriting notify jobs to invalid webhooks
For users following the documented repair flow, this change makes openclaw doctor --fix destructive when cron.webhook contains embedded credentials. The repair code still migrates notify: true jobs by copying cron.webhook verbatim into delivery.to and deleting notify (src/commands/doctor-cron.ts:95-102), but after this helper starts rejecting credentialed URLs the migrated job becomes an invalid per-job webhook that the scheduler skips. In that scenario the recommended migration path converts a working legacy job into one that never delivers.
Useful? React with 👍 / 👎.
|
Codex review: needs real behavior proof before merge. Reviewed July 9, 2026, 2:17 AM ET / 06:17 UTC. Summary PR surface: Source +4. Total +4 across 1 file. Reproducibility: yes. from source inspection: current main's cron helper delegates to isHttpUrl, and the net-policy tests classify user:pass HTTP(S) URLs as valid. I did not run a live cron add/update or dispatch flow in this read-only review. Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review findings
Review detailsBest possible solution: Implement credentialed cron webhook hardening on current main with an explicit maintainer-approved fail-closed policy or doctor/audit migration, redacted warnings, focused regression coverage, and real cron-path proof. Do we have a high-confidence way to reproduce the issue? Yes, from source inspection: current main's cron helper delegates to isHttpUrl, and the net-policy tests classify user:pass HTTP(S) URLs as valid. I did not run a live cron add/update or dispatch flow in this read-only review. Is this the best way to solve the issue? No; rejecting userinfo in the shared helper is a plausible hardening point, but it is not the best merge shape until stored cron URLs have an upgrade-safe policy, migration/audit behavior, and real cron-path proof. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against a780541ea6c5. Label changesLabel justifications:
Evidence reviewedPR surface: Source +4. Total +4 across 1 file. View PR surface stats
Security concerns:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (4 earlier review cycles)
|
|
ClawSweeper PR egg 🔥 Warming up: real-behavior proof passed; findings, security review, or rank-up moves are still in progress. Hatch commandComment Hatchability rules:
What is this egg doing here?
|
|
Heads up: this PR needs to be updated against current |
|
This pull request has been automatically marked as stale due to inactivity. |
|
This pull request has been automatically marked as stale due to inactivity. |
Summary
Describe the problem and fix in 2–5 bullets:
Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
Cron webhook URLs with embedded credentials (e.g. https://user:secret@host/path) are now rejected as invalid. URLs without credentials continue to work.
Security Impact (required)
No)No)No)No)No)Yes, explain risk + mitigation:Repro + Verification
Environment
Steps
Expected
Actual
Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
Review Conversations
If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.
Compatibility / Migration
No)No)Yes)Update any cron job webhook URLs that include user:pass@ in the URL. Remove embedded credentials and use headers or query params for auth instead.
Failure Recovery (if this breaks)
Risks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None.