← Back to PRs

#22388: Feat/aligned safe openclaw

by ManqingLiu open 2026-02-21 03:34 View on GitHub →
agents size: XL
Summary - Problem: src/security/constitution.ts has unclosed if/try blocks at lines 42–44 that trap the rest of the function inside the constitutionPath branch — when no path is provided the function returns undefined. The file-loading code also parses customData but never uses it, and a malicious JSON file could inject "immutable": true principles that cannot be disabled. Separately, src/security/input-validator.ts has a regex.exec() loop with no guard against zero-length matches, risking an infinite loop. - Why it matters: The constitution loader is broken for the default (no-path) case, which is the primary usage path. The immutable injection vector undermines the safety model's trust boundary between built-in and user-supplied principles. The regex loop is a latent DoS/hang risk. - What changed: (1) Closed the if/try blocks, added a catch for invalid paths, merged file-loaded principles with immutable: false forced, removed a duplicate comment. (2) Added a match[0].length === 0 → regex.lastIndex++ guard in detectRepetition. - What did NOT change (scope boundary): output-filter.ts (already fixed in 4ff33ce17), no new dependencies, no config schema changes, no changes to the default constitution JSON, no test files included in this PR. Change Type (select all) - Bug fix - Feature - Refactor - Docs - Security hardening - Chore/infra Scope (select all touched areas) - Gateway / orchestration - Skills / tool execution - Auth / tokens - Memory / storage - Integrations - API / contracts - UI / DX - CI/CD / infra Linked Issue/PR - Related #19729 (addresses 3 of 4 greptile-apps review comments; the 4th was already fixed) User-visible / Behavior Changes - loadConstitution() called without a constitutionPath now correctly returns the default constitution instead of undefined. - Custom constitution files loaded via constitutionPath now actually merge their principles into the constitution (previously parsed and discarded). - File-loaded principles can no longer claim immutable: true status. Security Impact (required) - New permissions/capabilities? No - Secrets/tokens handling changed? No - New/changed network calls? No - Command/tool execution surface changed? No - Data access scope changed? No Repro + Verification Environment - OS: macOS (Darwin 25.2.0) - Runtime/container: Node.js with Vitest v4.0.18 - Model/provider: N/A - Integration/channel: N/A - Relevant config: Default security alignment config Steps 1. npx vitest run src/security/constitution.test.ts — verify constitution loading, invalid path fallback, custom principle merging 2. npx vitest run src/security/input-validator.test.ts — verify repetition detection completes without hanging 3. npx vitest run src/security/ — full suite (289 tests across 16 files) Expected - All tests pass, including "handles invalid constitution file path gracefully", immutable injection prevention, and zero-length match edge case Actual - 289/289 tests pass, 0 failures Evidence - Failing test/log before + passing after - Trace/log snippets - Screenshot/recording - Perf numbers constitution.test.ts "handles invalid constitution file path gracefully" threw an uncaught exception before the fix (no catch block). After: passes. Full suite output: ✓ src/security/constitution.test.ts (14 tests) 7ms ✓ src/security/input-validator.test.ts (15 tests) 4ms Test Files 16 passed (16) Tests 289 passed (289) Human Verification (required) - Verified scenarios: Default constitution loading (no config), invalid file path fallback, valid file path with principle merging, immutable injection attempt forced to false, regex loop termination on repetitive content - Edge cases checked: Malformed JSON in constitution file (caught by catch), customData.principles not an array (guarded by Array.isArray), zero-length regex match (guarded by lastIndex++) - What I did not verify: Behavior on Windows/Linux, interaction with the alignment plugin loader at runtime, performance under very large custom constitution files Compatibility / Migration - Backward compatible? Yes - Config/env changes? No - Migration needed? No Failure Recovery (if this breaks) - How to disable/revert this change quickly: git revert <commit-sha> — single commit, no migrations - Files/config to restore: src/security/constitution.ts, src/security/input-validator.ts - Known bad symptoms reviewers should watch for: loadConstitution() returning undefined or missing principles, detectRepetition hanging on unusual input Risks and Mitigations - Risk: File-loaded principles that previously relied on setting immutable: true will now be demoted to immutable: false and become disableable. - Mitigation: This is intentional — only built-in default principles should be immutable. External files are an untrusted input boundary. <!-- greptile_comment --> <h3>Greptile Summary</h3> Added security alignment and safety oversight framework with constitutional principles, prompt injection detection, and input validation. Previous review comments addressed: non-immutable defaults are now replaced (not appended) when loading custom constitution files, errors are logged instead of silently swallowed, and zero-length regex matches no longer inflate repetition counts in `detectRepetition`. <h3>Confidence Score: 4/5</h3> - This PR is generally safe to merge with minor concerns around test coverage verification - The core security logic in `constitution.ts` and `input-validator.ts` has been iteratively fixed to address identified issues. However, the PR description claims tests pass but no test files are included in the PR diff, making it impossible to verify test coverage. The implementation adds 21 new files (2677 lines) which is substantial, but the critical security boundaries (immutable principles, zero-length match guards, error logging) appear sound after the fixes in commits 004c0257 and 7df85c94. - No specific files require special attention - the previous review threads identified issues that have been addressed in subsequent commits <sub>Last reviewed commit: 7df85c9</sub> <!-- greptile_other_comments_section --> <!-- /greptile_comment -->

Most Similar PRs