feat(monitor): config-hazard advisory — default user disabled + AOF silent data loss (valkey#3983) - #337
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
KIvanow
left a comment
KIvanow
left a comment
There was a problem hiding this comment.
Looks good, after the last bugbot review is merged, it should be good to go
…y#3983) - Pure evaluator + ConfigHazardService with per-connection TTL cache - Surface configHazards on /health/detailed and MCP instance health - Dashboard warning banner incl. distinct could-not-verify state - Full hazard-matrix, NOPERM, version-gate and caching tests
…not clean Only a positively verified safe config returns no finding — same contract as the NOPERM path (never a silent false negative)
8776b89 to
9e3029e
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7956703. Configure here.
| user.channels.split(/\s+/).includes('&*') || user.flags.includes('allchannels'); | ||
|
|
||
| return allCommands && allKeys && allChannels; | ||
| } |
There was a problem hiding this comment.
Workaround ignores later command denials
Medium Severity
hasUnrestrictedGrant treats +@all as full command access even when the effective rules later deny transaction/function commands (for example -@transaction, -exec, or -multi). That still leaves the AOF reload data-loss path open, but the evaluator returns clean. Nearby grantsMonitor in acl-checker.ts correctly lets explicit denials win over +@all.
Reviewed by Cursor Bugbot for commit 7956703. Configure here.
- probe() returns null on hard failure (missing connection, CONFIG GET error) instead of an empty finding list - getHazards() only caches real probe results, so transient startup or disconnect failures no longer suppress the advisory for a full TTL
2 participants
Summary
Adds a config-hazard advisory for valkey-io/valkey#3983 (maintainer-confirmed bug): when the
defaultACL user is disabled while AOF is enabled — anddefaultlacks the unrestricted+@all ~* &*grant — MULTI/EXEC and function-replicated writes are silently discarded on AOF reload. No error, the data is just gone.We can't fix the server behaviour, but the dangerous configuration is fully detectable client-side and the fix is one config line. This PR detects it and surfaces a standing WARNING everywhere an operator or agent would look. First of the five upstream-issue detector cards added to the board on 2026-07-20.
Changes
config-hazard.ts— pure evaluator: fires only in the{AOF on} × {default off, no workaround}cell; handles both RESP2 pair-array and RESP3 recordACL GETUSERshapes; version-gated below 6.0 (pre-ACL);ACL GETUSERdenied (NOPERM) → a distinct unverified finding rather than a silent false negative.ConfigHazardService— probesCONFIG GET appendonly→ (only if AOF is on)ACL GETUSER default, with a 60s per-connection TTL cache so health polling doesn't hammer the server. Degrades quietly when the config can't be read or the connection is gone.GET /health/detailednow returnsconfigHazards(shared type extended); optional injection so the service degrades to the previous payload when unwired. Deliberately not the capture-session preflight path — a data-loss hazard needs standing visibility, not visibility only when starting a MONITOR capture.get_healthincludesconfigHazardsin the instance health summary, so agents investigating an instance see the advisory (McpModulenow importsHealthModule).ConfigHazardBanner) — yellow warning with the exact remediation (user default off +@all ~* &*, or re-enable the user); muted "could not be verified" variant for the NOPERM case; hidden when clean; polls/health/detailedat 30s (server cache absorbs it).Verification
{AOF on/off} × {default on / off / off+workaround}+ partial-workaround (keys restricted) + flag-form grants (allkeys/allchannels/allcommands) + RESP2/RESP3 + NOPERM + version gate — 10 tests.tsc --noEmitclean on api and web; full api unit suite green except the known pre-existinglicense.service.spec.tsfailure on master.Notes for review
mcp.module.ts(any+require),Dashboard.tsx's legacy setState-in-effect, and the anomalyanygenerics inapps/web/src/api/metrics.ts.aof-use-rdb-preamble no) — the hazard applies to the AOF tail generally; happy to gate tighter if someone can demonstrate otherwise against a live build.Checklist
Note
Medium Risk
Adds read-only CONFIG/ACL probes on health paths with graceful degradation; advisory-only but touches health/MCP surfaces operators rely on.
Overview
Adds detection and surfacing of valkey#3983: AOF enabled with the
defaultACL user disabled (without the+@all ~* &*workaround) can silently lose MULTI/EXEC and function writes on AOF reload.Backend: New pure evaluator (
evaluateAclAofHazard) andConfigHazardServicethat probesCONFIG GET appendonlyandACL GETUSER default, with 60s per-connection caching and no cache on failed probes. OptionalconfigHazardsonGET /health/detailedand MCPinstance/:id/health;HealthModuleexports the service andMcpModuleimports it.Shared types:
ConfigHazardFindingand optionalconfigHazardsonDetailedHealthResponse.Web:
ConfigHazardBanneron the dashboard (confirmed vs unverified styling), fed bygetDetailedHealthpolled every 30s.Reviewed by Cursor Bugbot for commit 87ab2f5. Bugbot is set up for automated code reviews on this repo. Configure here.