fix(hooks): bound HOOK.md reads during hook install validation by cxbAsDev · Pull Request #101469 · openclaw/openclaw

cxbAsDev · 2026-07-07T07:56:08Z

What Problem This Solves

resolveHookNameFromDir read hook-package HOOK.md metadata without a size bound. A malicious or malformed package could therefore make the install path buffer a very large file before frontmatter parsing.

Why This Change Was Made

Route the read through the repository's canonical regular-file helper with a 1 MiB byte cap. That helper checks size before opening, enforces the cap while reading, verifies file identity, and rejects symlinked or otherwise non-regular metadata leaves; this keeps the package boundary on one race-safe path instead of adding a hook-specific reader.

User Impact

Normal hook packages continue to install. Oversized, symlinked, or non-regular HOOK.md metadata now fails fast with a clear validation error instead of risking unbounded allocation or reading outside the package leaf.

Evidence

Blacksmith Testbox tbx_01kxbm12g35zctaf2wg478ebv2, Actions 29201000714: pnpm test src/hooks/install.test.ts passed 30/30.
Same Testbox: targeted oxfmt and oxlint passed for both touched files.
Regression coverage exercises the 1 MiB + 1 byte limit and symlink rejection; the ordinary install-metadata path remains covered.
Direct dependency inspection: fs-safe/src/regular-file.ts prechecks size, performs capped incremental reads, pins file identity, and rejects symlink/non-file leaves.
Fresh branch autoreview: no accepted/actionable findings, correctness 0.98.

clawsweeper · 2026-07-07T08:16:14Z

Codex review: needs maintainer review before merge. Reviewed July 12, 2026, 9:53 AM ET / 13:53 UTC.

Summary
The PR replaces the install-time unbounded HOOK.md read with a 1 MiB safe regular-file read and adds an oversized-file regression test.

PR surface: Source +5, Tests +13. Total +18 across 2 files.

Reproducibility: yes. at high confidence from source and real-path evidence. Current main reads the entire file, while the contributor’s installer transcript shows a normal file accepted and a 1 MiB-plus-one-byte file rejected on the patched path.

Review metrics: 1 noteworthy metric.

Hook metadata constraints: 2 enforced. The patch introduces both a 1 MiB ceiling and a regular-file-only accepted-input contract.

Stored data model
Persistent data-model change detected: serialized state: src/hooks/install.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Risk before merge

[P1] Merging intentionally rejects HOOK.md files larger than 1 MiB and symlinked or non-regular metadata that the previous generic read could consume.
[P1] The branch is behind current main, so exact-head checks should be refreshed before merge even though the observed failures are unrelated to this patch.

Maintainer options:

Confirm and land strict metadata reads (recommended)
Approve the regular-file-only 1 MiB boundary, refresh the branch, and let exact-head checks gate merge.
Preserve broader file forms
Replace the helper with a byte-bounded implementation only if symlinked or non-regular metadata must remain supported.

Next step before merge

[P2] No code repair or contributor proof work is identified; the assigned owner should confirm the stricter metadata contract and refresh the behind branch before merge.

Maintainer decision needed

Question: Should hook installation intentionally require HOOK.md to be a regular, non-symlink file no larger than 1 MiB?
Rationale: The patch fixes unbounded allocation but also narrows accepted file forms through the selected dependency, so the security-versus-compatibility boundary requires explicit owner intent.
Likely owner: steipete — Peter introduced the affected install flow and is explicitly assigned to the live PR.
Options:
- Approve strict hook metadata (recommended): Require regular, non-symlink HOOK.md files within the 1 MiB limit and proceed after refreshing the branch.
- Limit allocation only: Use a different bounded reader if symlinked or non-regular metadata is an intended supported install contract.

Security
Cleared: The patch reduces a memory-exhaustion risk at an untrusted package metadata boundary without expanding dependencies, permissions, secrets, or executable sources.

Review details

Best possible solution:

Adopt one shared regular-file-only, 1 MiB metadata boundary for hook installation, refresh the branch, and merge after the assigned owner confirms that compatibility policy and exact-head checks pass.

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

Yes, at high confidence from source and real-path evidence. Current main reads the entire file, while the contributor’s installer transcript shows a normal file accepted and a 1 MiB-plus-one-byte file rejected on the patched path.

Is this the best way to solve the issue?

Yes, provided the regular-file-only contract is intentional. Reusing the pinned safe-file helper is narrower and more maintainable than introducing a duplicate hook-specific bounded reader.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

P2: This is a localized but concrete memory-exhaustion hardening fix for optional hook installation.
merge-risk: 🚨 compatibility: Previously readable oversized, symlinked, or non-regular HOOK.md inputs can be rejected after merge.
rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes direct terminal output from installHooksFromPath showing the after-fix normal and oversized cases on the real code path.
proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes direct terminal output from installHooksFromPath showing the after-fix normal and oversized cases on the real code path.

Evidence reviewed

PR surface:

Source +5, Tests +13. Total +18 across 2 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	1	8	3	+5
Tests	1	13	0	+13
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	2	21	3	+18

What I checked:

Current-main behavior: Current main reads the complete HOOK.md into memory before parsing frontmatter, so the reported memory-exhaustion path remains present. (src/hooks/install.ts:361, ef196b68b48d)
Proposed implementation: The PR head calls the shared readRegularFile helper with a 1 MiB limit before frontmatter parsing. (src/hooks/install.ts:363, 6750c7d1a8cd)
Regression coverage: The added test passes a 1 MiB-plus-one-byte HOOK.md through installHooksFromPath and expects bounded-read rejection. (src/hooks/install.test.ts:469, 6750c7d1a8cd)
Caller boundary: The unified plugins installer invokes installHooksFromPath for both package probing and installation, making the changed resolver part of the real public install flow. (src/cli/plugins-install-command.ts:282, ef196b68b48d)
Dependency contract: The pinned upstream regular-file helper explicitly enforces a byte cap and refuses directories, FIFOs, sockets, and symlinks; OpenClaw’s local dependency patch does not alter read semantics. (src/infra/regular-file.ts:4, 6f358b3ebf47)
Feature provenance: The unified hook installation path appears to originate in Peter Steinberger’s commit 3a6ee5ee00176c88aee32bb8bfd543780014c079. (src/hooks/install.ts:355, 3a6ee5ee0017)

Likely related people:

steipete: Peter introduced the unified hook install path and is the live assignee for this PR’s security-versus-compatibility decision. (role: introduced behavior and assigned decision owner; confidence: high; commits: 3a6ee5ee0017; files: src/hooks/install.ts)
Vincent Koc: Vincent recently carried the surrounding hook install module on current main and is relevant for adjacent implementation context. (role: recent area contributor; confidence: medium; commits: f151306e7b2e; files: src/hooks/install.ts)

What the crustacean ranks mean

🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
🌊 off-meta tidepool: rating does not apply to this item.

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

ClawSweeper keeps one durable marker-backed review comment per issue or PR.
Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
Maintainers can also comment @clawsweeper review to request a fresh review only.
Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Review history (20 earlier review cycles; latest 8 shown)

reviewed 2026-07-12T02:06:59.136Z sha 3423e5b :: needs maintainer review before merge. :: none
reviewed 2026-07-12T02:32:00.605Z sha 6cc0457 :: needs maintainer review before merge. :: none
reviewed 2026-07-12T04:48:17.225Z sha 770b605 :: needs maintainer review before merge. :: none
reviewed 2026-07-12T08:11:32.634Z sha 5b1deaa :: needs maintainer review before merge. :: none
reviewed 2026-07-12T08:44:31.999Z sha 3362490 :: needs maintainer review before merge. :: none
reviewed 2026-07-12T09:25:33.902Z sha 5c69a29 :: needs maintainer review before merge. :: none
reviewed 2026-07-12T10:44:30.859Z sha c9f9918 :: needs maintainer review before merge. :: none
reviewed 2026-07-12T10:52:59.033Z sha c9f9918 :: needs maintainer review before merge. :: none

cxbAsDev · 2026-07-07T09:45:47Z

@clawsweeper re-review\n\nI added real behavior proof to the PR body, including the local test command and output that exercises the oversized-file rejection path.

cxbAsDev · 2026-07-07T10:26:57Z

@clawsweeper re-review

Added real behavior proof that exercises src/hooks/install.ts resolveHookNameFromDir / installHooksFromPath with normal and oversized HOOK.md inputs.

clawsweeper · 2026-07-07T10:27:00Z

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

steipete · 2026-07-12T17:11:24Z

Land-ready maintainer proof:

Replaced the unbounded HOOK.md read with the canonical regular-file helper and a 1 MiB byte cap.
Added explicit regression coverage for oversized metadata and symlinked metadata leaves; ordinary hook-package metadata remains covered.
Direct fs-safe source inspection confirmed pre-open size checks, capped incremental reads, file-identity pinning, and symlink/non-file rejection.
Blacksmith Testbox tbx_01kxbm12g35zctaf2wg478ebv2, Actions 29201000714: focused hook install tests passed 30/30; targeted oxfmt and oxlint passed.
Fresh branch autoreview: no accepted/actionable findings, correctness 0.98.
Exact-head CI 29201315988 passed on 3de3df12ffb92bcdde88f497dc4f38ee38e11610.

Known gap: no external service is involved; focused install-path proof is the relevant live boundary.

steipete · 2026-07-12T17:11:42Z

Merged via squash.

Prepared head SHA: 3de3df12ffb92bcdde88f497dc4f38ee38e11610
Landed commit: 5d34ea774a79cd6718298d8fdc9bbaea97c3605b

…law#101469) * fix(hooks): bound HOOK.md reads during hook install validation * fix(hooks): remove unused fs import after switching to bounded reader * test(hooks): cover symlinked install metadata --------- Co-authored-by: Peter Steinberger <[email protected]>

openclaw-barnacle Bot added the size: XS label Jul 7, 2026

cxbAsDev force-pushed the fix/bound-hook-install-md-read branch 2 times, most recently from 8361b80 to 39c3cd1 Compare July 7, 2026 09:27

cxbAsDev force-pushed the fix/bound-hook-install-md-read branch from 39c3cd1 to c0af17f Compare July 7, 2026 09:58

clawsweeper Bot mentioned this pull request Jul 7, 2026

fix(hooks): bound hook workspace manifest and HOOK.md reads #101472

Merged

cxbAsDev force-pushed the fix/bound-hook-install-md-read branch 4 times, most recently from e2dfc3c to 31234cb Compare July 11, 2026 14:32

clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jul 11, 2026

cxbAsDev force-pushed the fix/bound-hook-install-md-read branch 6 times, most recently from 3ff04c4 to 00cfcbc Compare July 12, 2026 01:26

cxbAsDev force-pushed the fix/bound-hook-install-md-read branch from 5b1deaa to 3362490 Compare July 12, 2026 08:18

clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jul 12, 2026

cxbAsDev force-pushed the fix/bound-hook-install-md-read branch 4 times, most recently from dd5a9ee to 9d76ba3 Compare July 12, 2026 10:20

cxbAsDev force-pushed the fix/bound-hook-install-md-read branch 4 times, most recently from 603cad4 to c9f9918 Compare July 12, 2026 10:38

cxbAsDev force-pushed the fix/bound-hook-install-md-read branch from c9f9918 to 6750c7d Compare July 12, 2026 13:38

cxbAsDev and others added 3 commits July 12, 2026 18:03

fix(hooks): bound HOOK.md reads during hook install validation

9d60b4e

fix(hooks): remove unused fs import after switching to bounded reader

933cbba

test(hooks): cover symlinked install metadata

3de3df1

steipete force-pushed the fix/bound-hook-install-md-read branch from 6750c7d to 3de3df1 Compare July 12, 2026 17:04

steipete merged commit 5d34ea7 into openclaw:main Jul 12, 2026
103 checks passed

github-actions Bot mentioned this pull request Jul 12, 2026

📡 Upstream Digest — 2026-07-12 18:53 UTC curtismercier/openclaw-mods#1275

Open

cxbAsDev deleted the fix/bound-hook-install-md-read branch July 15, 2026 03:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(hooks): bound HOOK.md reads during hook install validation#101469

fix(hooks): bound HOOK.md reads during hook install validation#101469
steipete merged 3 commits into
openclaw:mainfrom
cxbAsDev:fix/bound-hook-install-md-read

cxbAsDev commented Jul 7, 2026 •

edited by steipete

Loading

Uh oh!

clawsweeper Bot commented Jul 7, 2026 •

edited

Loading

Uh oh!

cxbAsDev commented Jul 7, 2026

Uh oh!

cxbAsDev commented Jul 7, 2026

Uh oh!

clawsweeper Bot commented Jul 7, 2026 •

edited

Loading

Uh oh!

steipete commented Jul 12, 2026

Uh oh!

Uh oh!

steipete commented Jul 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

cxbAsDev commented Jul 7, 2026 • edited by steipete Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What Problem This Solves

Why This Change Was Made

User Impact

Evidence

Uh oh!

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

Uh oh!

Uh oh!

cxbAsDev commented Jul 7, 2026

Uh oh!

cxbAsDev commented Jul 7, 2026

Uh oh!

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

Uh oh!

Uh oh!

steipete commented Jul 12, 2026

Uh oh!

Uh oh!

steipete commented Jul 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cxbAsDev commented Jul 7, 2026 •

edited by steipete

Loading

clawsweeper Bot commented Jul 7, 2026 •

edited

Loading

clawsweeper Bot commented Jul 7, 2026 •

edited

Loading