fix(gateway): enforce allowRequestSessionKey gate on template-rendered mapping sessionKeys#69381
fix(gateway): enforce allowRequestSessionKey gate on template-rendered mapping sessionKeys#69381pgondhi987 merged 12 commits intoopenclaw:mainfrom
Conversation
openclaw-barnacle
Bot
added
gateway
Greptile SummaryThis PR closes a security gap where Confidence Score: 5/5This PR is safe to merge; it is a targeted security hardening with no P0/P1 issues and strong test coverage for all new decision points. All changes are within the gate chain and config-validation layer. The conservative default of treating any non-'static' sessionKeySource (including undefined) as 'mapping-templated' ensures unknown/missing sources are blocked, not silently allowed. Transform-injected session keys without a declared source also inherit the safe 'templated' default. Existing behaviour for static mapping keys and auto-generated keys is demonstrably unchanged. The five new test scenarios directly exercise each decision point. No P0/P1 findings. No files require special attention. Reviews (2): Last reviewed commit: "fix: address build failures" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81319435d5
ℹ️ 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".
|
@codex review |
|
@greptile review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d86a88fe2c
ℹ️ 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".
| mappings.some((mapping) => hasTemplatedHookSessionKey(mapping.sessionKey)) && | ||
| !allowedSessionKeyPrefixes |
There was a problem hiding this comment.
This guard treats any templated sessionKey in the resolved mapping list as a hard config error, but applyHookMappings evaluates explicit hooks.mappings before preset mappings (src/gateway/hooks-mapping.ts), so a static explicit mapping can intentionally shadow the templated Gmail preset. In that common override pattern (presets:["gmail"] plus a static gmail mapping), startup now fails with a prefixes error even though the effective mapping is static, breaking the documented “override preset with static sessionKey” path and introducing an unnecessary boot-time regression.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5f7fb2d8c
ℹ️ 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 (earlier.matchPath !== mapping.matchPath) { | ||
| return false; | ||
| } | ||
| return earlier.matchSource === undefined || earlier.matchSource === mapping.matchSource; |
There was a problem hiding this comment.
hasEffectiveTemplatedHookSessionKeyMapping is meant to ignore mappings that can never run, but isHookMappingShadowed only treats mappings as shadowed when matchPath strings are equal. An earlier mapping with no match.path (wildcard) already captures all paths, so a later templated mapping is unreachable; however, this code still marks it as effective and makes resolveHooksConfig throw the new allowedSessionKeyPrefixes error. That creates a startup regression for valid configs that intentionally place a static catch-all mapping before narrower templated mappings.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b1a549e34
ℹ️ 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".
| continue; | ||
| } | ||
| effectiveMappings.push(mapping); | ||
| if (hasTemplatedHookSessionKey(mapping.sessionKey)) { |
There was a problem hiding this comment.
hasEffectiveTemplatedHookSessionKeyMapping flags any effective mapping whose sessionKey contains {{...}}, but it does not check the mapping action. sessionKey is ignored for action: "wake" in buildActionFromMapping, so a wake-only mapping that happens to include a templated sessionKey now triggers a startup error requiring hooks.allowedSessionKeyPrefixes even though no session routing path can use that value. This is a regression for previously valid wake configs that included extra fields.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37a7e75607
ℹ️ 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 (earlier.matchPath !== undefined && earlier.matchPath !== mapping.matchPath) { | ||
| return false; | ||
| } | ||
| return earlier.matchSource === undefined || earlier.matchSource === mapping.matchSource; |
There was a problem hiding this comment.
isHookMappingShadowed uses !== undefined checks, but runtime matching treats empty strings as wildcards (mappingMatches only checks truthy matchPath/matchSource). Fresh evidence: normalizeMatchPath("/") returns "", so an earlier static catch-all mapping with match.path: "/" (or match.source: "") is considered non-shadowing here, and resolveHooksConfig can incorrectly throw the new allowedSessionKeyPrefixes error for later templated mappings that are actually unreachable.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
{{…}}template expressions in theirsessionKey(e.g. the built-ingmailpreset withsessionKey: \"hook:gmail:{{messages[0].id}}\") render attacker-controlled webhook payload values into the session key, then pass that value toresolveHookSessionKey()withsource: \"mapping\". TheallowRequestSessionKeygate only checkedsource === \"request\", so the mapping path was silently exempt.allowRequestSessionKey(the secure default) believed they were preventing external callers from steering session routing. Template-rendered mapping keys bypassed that control entirely — a hook-token holder could route the dispatched message into any session their payload value could produce.HookActiongainssessionKeySource: \"static\" | \"templated\";HookTransformResultgains the same field;resolveHookSessionKey'ssourcetype expands to\"mapping-static\" | \"mapping-templated\"; theallowRequestSessionKeygate now also fires onmapping-templated;resolveMergedSessionKeySourcehandles transform overrides with a safe\"templated\"default;resolveHooksConfigthrows if a mapping uses a templatedsessionKeywithoutallowedSessionKeyPrefixes; the sharedhasHookTemplateExpressionsutility eliminates the regex duplication.sessionKey: \"hook:gmail:fixed\") are unaffected and continue to bypass the gate as before.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
resolveHookSessionKey()was introduced with asource: \"request\" | \"mapping\"discriminator, but theallowRequestSessionKeygate was only wired tosource === \"request\". The\"mapping\"branch renders external webhook data throughrenderOptional(mapping.sessionKey, ctx)before reaching that function, making the distinction security-relevant, not just semantic.allowRequestSessionKey=false; no config-time validation requiredallowedSessionKeyPrefixeswhen template expressions were present.payload.*were added, that assumption no longer held.Regression Test Plan (if applicable)
src/gateway/hooks.test.ts,src/gateway/hooks-mapping.test.tsmapping-templatedsource is blocked byresolveHookSessionKeywhenallowRequestSessionKey=false; (2)mapping-staticis not blocked; (3) transform-providedsessionKeywithout explicitsessionKeySourcedefaults to\"templated\"; (4) transform-providedsessionKeywithsessionKeySource: \"static\"is respected; (5)resolveHooksConfigthrows when a templated mapping key is present butallowedSessionKeyPrefixesis absent.source: \"mapping\"which is now renamed.User-visible / Behavior Changes
resolveHooksConfigthrows\"hooks.allowedSessionKeyPrefixes is required when a hook mapping sessionKey uses templates\"if any mapping (or preset) has a template-drivensessionKeybutallowedSessionKeyPrefixesis unset. Operators who use thegmailpreset or write custom template-bearing session keys must now also setallowedSessionKeyPrefixes.allowRequestSessionKey: true, same as request-supplied keys. Operators who run those mappings with the defaultallowRequestSessionKey: falsewill get a 400 error until they either setallowRequestSessionKey: trueor supply a staticsessionKey.Diagram (if applicable)
Security Impact (required)
Repro + Verification
Environment
/hooks/<mapping>)hooks.enabled=true,hooks.token=<secret>,hooks.presets=[\"gmail\"],hooks.allowRequestSessionKey=falseSteps
hooks.presets: [\"gmail\"]andhooks.allowRequestSessionKey: false(the default)./hooks/gmailwith an attacker-chosenmessages[0].id.Expected
allowedSessionKeyPrefixesto be set at startup.Actual (before fix)
Evidence
hooks.test.tsandhooks-mapping.test.tscover all five scenarios; thesource: \"mapping\"→source: \"mapping-static\"rename in the existing passing test confirms the old neutral path still works.Human Verification (required)
resolveMergedSessionKeySource) verified to default to\"templated\"whensessionKeySourceis absent.sessionKeyexplicitly; transform sets dynamicsessionKeywithout declaring source; no-transform path (returns base directly);allowedSessionKeyPrefixesboth set and unset.Review Conversations
Compatibility / Migration
allowedSessionKeyPrefixesis now required when template session keys are used.hooks.presets: [\"gmail\"]or custom template session keys must addallowedSessionKeyPrefixesto their hooks config. Operators who also want to retain template-driven routing must additionally setallowRequestSessionKey: true.hooks.allowedSessionKeyPrefixes: [\"hook:gmail:\"](or the relevant prefix) to your hooks config.hooks.allowRequestSessionKey: true.Risks and Mitigations
gmailpreset will see a startup error until they addallowedSessionKeyPrefixes.allowRequestSessionKey: true.allowedSessionKeyPrefixes+allowRequestSessionKey: truepreserves the existing routing capability while constraining it to the declared prefix namespace.