feat(hermes-attestation-guardian): harden attestation verification and drift controls#192
Conversation
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| function bool(value, defaultValue = false) { | ||
| if (value === undefined || value === null) { | ||
| return defaultValue; | ||
| } | ||
| return !!value; |
There was a problem hiding this comment.
Since bool() coerces non-null config.json strings to true in buildAttestation() for gateways.*.enabled and security.*, should we normalize/validate config booleans like readEnvBool() before attesting so values like "false" don’t get treated as enabled?
Finding type: Logical Bugs | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
Before applying, verify this suggestion against the current code. In
skills/hermes-attestation-guardian/lib/attestation.mjs around lines 171-176 (`bool`) and
lines 209-229 (`buildAttestation()` where `gateways.*.enabled` and `security.*` are
computed), fix the logical bug where `bool()` treats any non-null string as truthy (so
config values like "false" become enabled). Refactor by either (a) updating `bool()` to
normalize boolean-like strings using the same rules as `readEnvBool()` (trim +
lowercase; accept 1/true/yes/on/enabled and 0/false/no/off/disabled) before coercing, or
(b) validating that the config fields are actual booleans and otherwise ignoring them in
favor of the env fallback/default. Ensure malformed/incorrect config types in
detectHermesConfig() do not flip posture; if the config type is invalid, default to the
provided env fallback.
| if (!isPlainObject(attestation.generator)) { | ||
| errors.push("generator object is required"); | ||
| } |
There was a problem hiding this comment.
generator is only validated as an object, but diffAttestations() reads generator.version and defaults to "unknown", so malformed attestations can slip through and skew diff output—should we require generator.version as a string in the schema?
Finding type: Type Inconsistency | Severity: 🟢 Low
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
Before applying, verify this suggestion against the current code. In
skills/hermes-attestation-guardian/lib/attestation.mjs around lines 342-344 inside
`validateAttestationSchema()`, the code only checks that `attestation.generator` is a
plain object and does not validate `attestation.generator.version`. Refactor the
validation to additionally require `generator.version` to be a string (and ideally
non-empty/trimmed) and push a clear schema error if it’s missing or not a string. This
will prevent malformed attestations from passing schema checks and producing misleading
diff results that hide version changes behind the `
| if (!isPlainObject(attestation.posture.runtime)) { | ||
| errors.push("posture.runtime object is required"); | ||
| } |
There was a problem hiding this comment.
posture.runtime only checks object-ness, so diffAttestations() can misread posture.runtime.gateways.* and posture.runtime.risky_toggles.* with !! on malformed shapes; can we validate both sub-objects and their leaf boolean values here?
Finding type: Type Inconsistency | Severity: 🟠 Medium
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
Before applying, verify this suggestion against the current code. In
skills/hermes-attestation-guardian/lib/attestation.mjs around lines 352-354 inside
`validateAttestationSchema`, extend validation for `attestation.posture.runtime`:
currently it only checks that `posture.runtime` is a plain object, but it never
validates `posture.runtime.gateways` and `posture.runtime.risky_toggles`. Refactor this
block to also assert both `gateways` and `risky_toggles` are plain objects and that each
expected leaf key exists (telegram/matrix/discord and
allow_unsigned_mode/bypass_verification) and each corresponding value is a boolean; if
values are missing or not booleans, add descriptive errors. Keep the existing `errors`
array behavior and ensure any malformed shapes that could later affect
`diffAttestations()` are rejected at schema validation time.
| if (!Array.isArray(integrity.watched_files)) { | ||
| errors.push("posture.integrity.watched_files must be an array"); | ||
| } | ||
| if (!Array.isArray(integrity.trust_anchors)) { | ||
| errors.push("posture.integrity.trust_anchors must be an array"); |
There was a problem hiding this comment.
posture.integrity.watched_files and trust_anchors only get array checks in validateAttestationSchema(), so objects missing a string path can slip through and be dropped later by mapByPath() in diffAttestations()—should we validate each integrity item shape here (e.g. path string, sha256 string|null, and required fields like exists) before accepting the attestation?
Finding type: Logical Bugs | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
Before applying, verify this suggestion against the current code. In
skills/hermes-attestation-guardian/lib/attestation.mjs around lines 323-375 inside
validateAttestationSchema(), tighten validation of posture.integrity.watched_files and
posture.integrity.trust_anchors: after confirming they are arrays, also iterate each
element and ensure it has the expected object shape (path must be a non-empty string;
sha256 must be either null or a 64-char hex string; and any required fields your
verifier/diff logic expects, e.g. exists boolean). If any element fails, push a specific
error message and return the accumulated errors. This prevents malformed integrity
entries from passing schema validation and then being silently dropped later in
diffAttestations().
| const schemaErrors = validateAttestationSchema(attestation); | ||
| for (const message of schemaErrors) { | ||
| verificationFindings.push({ severity: "critical", code: "SCHEMA_INVALID", message }); | ||
| failures.push(message); | ||
| } |
There was a problem hiding this comment.
The schema/digest validation flow (including SCHEMA_INVALID/CANONICAL_DIGEST_MISMATCH and BASELINE_SCHEMA_INVALID/BASELINE_CANONICAL_DIGEST_MISMATCH) is duplicated later, so changes could drift—should we extract something like validateAttestationDocument({ bytes, findings, failureList, codePrefix }) and reuse it for both flows before computing the diff?
Finding type: Code Dedup and Conventions | Severity: 🟢 Low
Want Baz to fix this for you? Activate Fixer
abutbul
force-pushed
the
skill/hermes-attestation-guardian-hardening-v0.0.1
branch
from
59eef65 to
aa30e7f
Compare
abutbul
force-pushed
the
skill/hermes-attestation-guardian-hardening-v0.0.1
branch
from
aa30e7f to
1f1dde4
Compare
| const outPath = resolveHermesScopedOutputPath(args.output); | ||
| fs.mkdirSync(path.dirname(outPath), { recursive: true }); | ||
| const body = stableStringify(attestation, args.compact ? 0 : 2); | ||
| fs.writeFileSync(outPath, `${body}\n`, "utf8"); | ||
|
|
There was a problem hiding this comment.
generate_attestation should write current.json atomically instead of directly to outPath, so a crash during fs.writeFileSync doesn’t leave partial invalid JSON — can we write to a temp file in the same dir and rename it into place?
Finding type: Architecture soundness design | Severity: 🟠 Medium
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
Before applying, verify this suggestion against the current code. In
skills/hermes-attestation-guardian/scripts/generate_attestation.mjs around lines 113-117
inside the run() logic that computes outPath and calls fs.writeFileSync, change the
output write to be atomic. Refactor to write the JSON to a temporary file in the same
directory (e.g., prefix/suffix the filename), flush it to disk (fsync) after writing,
and then replace the target outPath via a rename/move that is atomic on the same
filesystem. Also ensure the .sha256 file is written only after the rename succeeds so
watchers never observe a digest paired with a corrupted/partial current.json.
| if (typeof value === "string") { | ||
| const norm = value.trim().toLowerCase(); | ||
| if (["1", "true", "yes", "on", "enabled"].includes(norm)) return true; | ||
| if (["0", "false", "no", "off", "disabled"].includes(norm)) return false; | ||
| return defaultValue; | ||
| } | ||
| return defaultValue; |
There was a problem hiding this comment.
bool() silently falls back to readEnvBool(...) for malformed values like {} or unknown strings, and also duplicates readEnvBool's normalization logic — should we refactor bool() to explicitly accept only booleans, 0/1, and a fixed set of strings, returning false for anything else, and have readEnvBool reuse it as the single normalization source?
Finding types: Logical Bugs Code Dedup and Conventions | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
In skills/hermes-attestation-guardian/lib/attestation.mjs around lines 175-189, refactor
`bool()` to explicitly accept only boolean/number (0/1) and a fixed set of normalized
strings; for anything else, return false or throw an error — do not fall back to
reading `HERMES_*` env vars. Then have `readEnvBool` reuse `bool()` as the single
normalization source of truth. Adjust all call sites (especially the fail-closed
attestation path) so that a present-but-malformed config value is rejected/forced false
rather than triggering an env fallback.
1 participant
User description
This PR introduces the new
hermes-attestation-guardianskill and hardens Hermes runtime attestation verification and drift detection controls.Initial release note
v0.0.1.skills/hermes-attestation-guardian/CHANGELOG.mdis populated with a0.0.1entry dated 2026-04-15.Security fixes and hardening summary
Validation run (all passing)
python utils/validate_skill.py skills/hermes-attestation-guardianValidation PASSED - all checks passednode skills/hermes-attestation-guardian/test/attestation_schema.test.mjsattestation_schema.test.mjs: oknode skills/hermes-attestation-guardian/test/attestation_diff.test.mjsattestation_diff.test.mjs: oknode skills/hermes-attestation-guardian/test/attestation_cli.test.mjsattestation_cli.test.mjs: oknode skills/hermes-attestation-guardian/test/setup_attestation_cron.test.mjssetup_attestation_cron.test.mjs: okOverlap check findings
Searched upstream issues and PRs for:
hermes-attestation-guardianbaseline authenticityattestation cron markeroutput scope guarddigest bindingResult: no matching open/closed upstream issues or PRs found for these keywords at search time.
Generated description
Below is a concise technical summary of the changes proposed in this PR:
Deliver deterministic Hermes attestation generation, verification, and diffing by centralizing canonicalization, schema, and digest helpers plus CLI entry points so runtime posture snapshots and drift findings fail closed. Document the release metadata and add the Hermes-only cron helper with guardrails and tests to make safe scheduling and scoping straightforward.
Modified files (10)
Latest Contributors(0)
lib/attestation.mjs, wiring the generator/verifier CLIs, adding drift severity classification, and expanding schema/digest/baseline tests so any tamper or risky toggle regression fails closed.Modified files (7)
Latest Contributors(0)