fix(cron): validate custom session ids by eleqtrizit · Pull Request #59835 · openclaw/openclaw

eleqtrizit · 2026-04-02T17:50:19Z

Summary

Reject invalid session:<id> cron targets that contain path separator characters or NULs
Reuse the same validation when resolving the isolated cron run sessionKey

Changes

Added a shared cron custom-session validator in src/cron/normalize.ts
Applied that validator during cron job normalization so invalid custom session targets fall back to default isolated behavior
Applied the same validator in the gateway cron runner so previously stored invalid jobs fall back to cron:<job-id>
Added regression coverage for create-time normalization and run-time fallback behavior

Validation

Ran corepack pnpm test -- src/cron/normalize.test.ts src/gateway/server.cron.test.ts
Verified invalid session:../../... values no longer survive normalization
Verified stored invalid session: targets do not override the cron-scoped fallback session key
Attempted local agentic review with claude -p "/review"; it requested interactive GitHub approval and returned no code findings in this environment

Notes

Scope is limited to custom cron session IDs; valid session:<id> inputs still behave as before
This also hardens previously persisted invalid jobs by failing closed at execution time

greptile-apps · 2026-04-02T17:53:25Z

Greptile Summary

This PR hardens cron custom session ID handling by introducing normalizeCronCustomSessionId in src/cron/normalize.ts, which rejects path-separator characters and enforces SAFE_SESSION_ID_RE. The validator is applied at job-create time (invalid targets fall back to "isolated") and at run time (invalid stored targets fall back to cron:<job-id>). The run-time path also adds a fast-track for full agent-format session keys via parseAgentSessionKey before falling through to the custom-ID validator. Tests cover forward-slash, backslash, and NUL-byte rejection at both layers, and the integration test confirms the fallback session key used for malicious stored jobs.

Confidence Score: 5/5

Safe to merge — the fix is narrowly scoped, fails closed on invalid input, and all prior review concerns are addressed.

Both previous thread concerns (adding SAFE_SESSION_ID_RE enforcement and backslash/NUL test cases) are resolved in this diff. No new P0/P1 issues introduced. The two-path run-time logic (parseAgentSessionKey fast-track → normalizeCronCustomSessionId fallback → cron: default) is correct and covered by tests at both the unit and integration level.

No files require special attention.

_{Reviews (3): Last reviewed commit: "fix(cron): preserve colon-delimited sess..." | Re-trigger Greptile}

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 41557fe2a5

ℹ️ 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".

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 41557fe2a5

ℹ️ 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".

…on jobs

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c01bdd74fd

ℹ️ 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".

eleqtrizit · 2026-04-03T18:25:09Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e988192ec6

ℹ️ 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".

eleqtrizit · 2026-04-03T18:33:07Z

@codex review

chatgpt-codex-connector · 2026-04-03T18:38:07Z

Codex Review: Didn't find any major issues. Can't wait for the next one!

ℹ️ 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".

clawsweeper · 2026-06-14T14:07:43Z

Thanks for the context here. I swept through the related work, and this is now duplicate or superseded.

Keep open because the protected maintainer label prevents automatic cleanup, but the branch is substantively superseded by the merged opaque-session-target fix and is no longer a viable merge path.

Root-cause cluster
Relationship: superseded
Canonical: #86578
Summary: The merged opaque-session-target PR is the canonical resolution for the cron session-key validation problem, while this PR implements the older opposite stricter-validation shape.

Members:

canonical: fix(cron): accept opaque session target keys #86578 - Merged replacement that accepts opaque session target keys and preserves separate run-log path safety.
fixed_by_candidate: cron: encode sessionKey when deriving jobId to support channel-native conversation IDs #64030 - The canonical merged PR closed the reported DingTalk/native-separator cron session-target bug.

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Canonical path: Close this PR as superseded by #86578.

So I’m closing this here and keeping the remaining discussion on #86578.

Review details

Best possible solution:

Close this PR as superseded by #86578.

Do we have a high-confidence way to reproduce the issue?

Yes, source-level. The PR routes custom session:<id> values through separator and SAFE_SESSION_ID_RE validation, while current main tests accept slash/backslash-bearing opaque session keys and only reject NUL.

Is this the best way to solve the issue?

No. The best fix already landed in #86578: keep cron session targets as opaque runtime keys and enforce path safety at cron job-id/run-log boundaries.

Security review:

Security review cleared: No supply-chain changes were found; the functional risk is session-state compatibility, while current main keeps path safety at the cron run-log job-id boundary.

AGENTS.md: found and applied where relevant.

What I checked:

linked superseding PR: fix(cron): accept opaque session target keys #86578 (fix(cron): accept opaque session target keys) is merged at 2026-05-26T00:39:05Z.
cluster evidence: the durable review links that PR in the work cluster or recommended risk path.
no human follow-up: live comments and timeline hydrated by apply contain no non-automation activity after the ClawSweeper review.

Likely related people:

ferminquant: Authored the merged replacement that defines the current cron session-target contract as opaque non-empty/non-NUL routing keys. (role: recent area contributor; confidence: high; commits: c9364f03dcbb; files: src/cron/session-target.ts, src/cron/normalize.test.ts, src/cron/service.jobs.test.ts)
steipete: Introduced the earlier strict unsafe custom-session target guard that this PR follows and maintained adjacent cron/gateway behavior. (role: guard introducer and adjacent contributor; confidence: high; commits: 39bcf695dce4; files: src/cron/session-target.ts, src/cron/normalize.ts, src/cron/service/jobs.ts)
kkhomej33-netizen: Introduced custom cron session IDs and current-session auto-binding, which is the feature boundary affected by this PR. (role: feature introducer; confidence: high; commits: e7d9648fba27; files: src/cron/normalize.ts, src/cron/service/jobs.ts, docs/automation/cron-jobs.md)
mvanhorn: Recently hardened persisted cron job validation in the same sessionTarget load and support-spec path. (role: recent adjacent contributor; confidence: medium; commits: b33eb93aac8a; files: src/cron/service/jobs.ts, src/cron/service/store.ts)

Codex review notes: model internal, reasoning high; reviewed against 686a2876c7f4.

fix(cron): validate custom session ids

304cb42

openclaw-barnacle Bot added gateway Gateway runtime size: S labels Apr 2, 2026

greptile-apps Bot reviewed Apr 2, 2026

View reviewed changes

Comment thread src/cron/normalize.ts

Comment thread src/cron/normalize.test.ts

fix(cron): strengthen session id validation using SAFE_SESSION_ID_RE

41557fe

chatgpt-codex-connector Bot reviewed Apr 3, 2026

View reviewed changes

Comment thread src/gateway/server-cron.ts Outdated

chatgpt-codex-connector Bot reviewed Apr 3, 2026

View reviewed changes

Comment thread src/cron/normalize.ts

fix(cron): preserve colon-delimited session keys for current-bound cr…

c01bdd7

…on jobs

openclaw-barnacle Bot added the maintainer Maintainer-authored PR label Apr 3, 2026

chatgpt-codex-connector Bot reviewed Apr 3, 2026

View reviewed changes

Comment thread src/cron/normalize.ts Outdated

fix(cron): preserve explicit session targets

e988192

chatgpt-codex-connector Bot reviewed Apr 3, 2026

View reviewed changes

Comment thread src/cron/normalize.ts

clawsweeper Bot added the rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. label May 24, 2026

ferminquant mentioned this pull request May 25, 2026

fix(cron): accept opaque session target keys #86578

Merged

brokemac79 mentioned this pull request Jun 25, 2026

[codex] Route PR close reviews to autoclose openclaw/clawsweeper#371

Merged

eleqtrizit closed this Jun 30, 2026

Uh oh!

Uh oh!

Conversation

eleqtrizit commented Apr 2, 2026

Summary

Changes

Validation

Notes

Uh oh!

greptile-apps Bot commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

eleqtrizit commented Apr 3, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

eleqtrizit commented Apr 3, 2026

Uh oh!

chatgpt-codex-connector Bot commented Apr 3, 2026

Uh oh!

clawsweeper Bot commented Jun 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

greptile-apps Bot commented Apr 2, 2026 •

edited

Loading

clawsweeper Bot commented Jun 14, 2026 •

edited

Loading