fix(critical): close silent fail-open patterns in protect() init and rule eval#203
Open
anirudhp26 wants to merge 1 commit intomasterfrom
Open
fix(critical): close silent fail-open patterns in protect() init and rule eval#203anirudhp26 wants to merge 1 commit intomasterfrom
anirudhp26 wants to merge 1 commit intomasterfrom
Conversation
…rule eval
Audit Round 2, PR A — addresses the two CRITICAL findings: silent
fail-opens that masked broken policies as a working Veto instance.
1. **`protect()` no longer silently degrades to allow-all on init
failure.** Previously, any exception during Veto initialization
(malformed YAML, bad rule, transient network) was caught and
replaced with an allow-all wrapper. Every tool call passed; no
warning. The system became a no-op while looking healthy. Now:
- Default behaviour: re-raise. The user sees what broke.
- Opt-in `protect(safe_fallback=True)`: keeps the legacy
degradation, but prints a loud `WARNING: Veto initialization
failed — falling back to ALLOW-ALL` banner to stderr.
2. **`Veto.evaluateLocalExpression` (TS) now propagates compile and
evaluate errors** instead of catching them, logging at `warn`,
and returning `false`. Earlier behaviour: a `block` rule with a
parse error silently never matched and the call went through.
Now: error is logged at `error` level and re-raised; the
validator engine treats validator exceptions as a deny — fail-
closed by default for security-relevant rules.
3. **`matches` operator on a rejected regex now writes a one-time
ERROR line to stderr** in both SDKs (Python `condition_evaluator`,
TS `condition-evaluator`). The operator still returns `false`
(preserves matches-operator semantics — a non-matching pattern
isn't a hard error), but a fail-open block rule on a
misconfigured regex is no longer invisible at runtime. Pairs
with the load-time scan added in #201; this handles rules added
or mutated after init.
Tests
-----
* `packages/sdk-python/tests/test_failopen_critical.py` — 6 cases
(init-raises-by-default, opt-in fallback prints banner, success
path silent, matches one-time log, safe pattern silent, two
patterns log once each)
* `packages/sdk/tests/core/failopen-critical.test.ts` — 4 cases
(matches one-time log, safe pattern silent, two patterns log
once each, compile() malformed expression throws)
* Full suites: TS 1386/1386, Python 334/334
Behavioural change for downstream
---------------------------------
- `protect()` callers that depended on the allow-all degradation as a
soft fallback need to either fix their config or pass
`safe_fallback=True` (and accept every call is now permitted).
- Local rules with broken expressions now produce a deny instead of
an allow. For most users this is the safer default.
Contributor
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Audit Round 2 — PR A. Closes the two CRITICAL fail-open findings: silent paths that turned a misconfigured Veto into a working-looking allow-all.
Findings closed
1.
protect()silently degraded to allow-all on init exception (Python)Before —
packages/sdk-python/veto/core/protect.py:439-440:Any failure during init — typo in YAML, bad rule shape, transient network — silently replaced the Veto instance with an allow-all wrapper. Every tool call passed. No warning. No audit trail. The system looked healthy from the outside.
After:
raise. The caller sees what broke.protect(safe_fallback=True): keeps the legacy degrade-to-allow-all but prints a loud stderr banner:2.
Veto.evaluateLocalExpressionreturnedfalseon compile / eval errors (TS)packages/sdk/src/core/veto.ts:2030-2046caught both compile and evaluate errors, logged atwarn, and returnedfalse. Ablockrule with a parse error silently never matched → the call sailed through.Both catches now log at
errorlevel and re-raise. The validator engine's existing fail-closed treatment of validator exceptions turns this into a deny — security-relevant rules can no longer be silently bypassed by a broken expression.3.
matchesoperator on a rejected regex now logs a one-time error (both SDKs)The
matchesoperator returnsfalsewhen the safety heuristic rejects the pattern (preserves matches-operator semantics — a non-matching pattern isn't a hard error). Earlier this was completely silent; now a single[veto] ERROR: rule \matches` regex rejected by safety heuristic — the rule will never fire` line is written to stderr the first time each distinct pattern hits this path.Pairs with the load-time scan added in #201 — that catches rules at startup; this catches rules added or mutated after init.
Tests
packages/sdk-python/tests/test_failopen_critical.py— 6 casespackages/sdk/tests/core/failopen-critical.test.ts— 4 casesFull suites green: TS 1386/1386, Python 334/334.
Behavioural notes for downstream
protect()callers that relied on the allow-all degradation as a soft fallback need to either fix their config or setsafe_fallback=Trueand accept that every call is now permitted (with the loud banner).Test plan
protect()raises and the error message points at the bad rule.safe_fallback=Truein the same scenario; confirm the loud stderr banner prints and tools still wrap.matches: '(rm.*|wget.*)'(the heuristic rejects this); on first guard call, confirm a single ERROR line on stderr; on subsequent calls, confirm no further logs for that pattern.args.x ==; confirm the first guard call denies (rather than silently allowing).