Conversation
Detect mode re-decided with the pii summary cleared, but eval_condition binds an empty default, so an allow-clean rule (pii.count == 0) placed before an endpoint-bound deny matched on the second pass and silently dropped a Kubernetes/endpoint deny that block mode enforces. The engine now exposes decide_without_pii_rules(), which skips any rule whose CEL condition references pii (via the compiled program's reference set) instead of rebinding an empty summary — the enforced pass answers what the policy decides without consulting the scanner at all, so endpoint and Kubernetes rules are enforced in every mode as documented. Fixes #99 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses issue #99 by introducing 'decide_without_pii_rules' to skip rules reading the 'pii' variable entirely during detect mode, preventing 'allow-clean' rules from masking subsequent endpoint deny rules. The reviewer raised a medium-severity concern regarding redundant CEL program compilation (compiling twice per rule evaluation) and potential policy bypasses, suggesting refactoring the engine to compile the CEL program once and evaluate it unconditionally with empty PII facts instead of skipping rules.
| if skip_pii_rules && condition_reads_pii(&rule.condition) { | ||
| continue; | ||
| } | ||
| if endpoint_matches(&rule.endpoint, facts.endpoint.as_deref()) | ||
| && eval_condition(&rule.condition, facts) |
There was a problem hiding this comment.
[MEDIUM] Redundant CEL Program Compilation and Policy Bypass
Symptom: The CEL condition string is compiled twice per rule evaluation when skip_pii_rules is true: once in condition_reads_pii and once in eval_condition. Furthermore, skipping rules entirely when PII cannot be inspected bypasses metadata-based rules.
Consequence: Compiling CEL expressions twice per rule on every request significantly increases latency and CPU usage under high throughput. Additionally, bypassing the policy engine entirely when request bodies cannot be inspected prevents metadata-based rules (like path, method, or size limits) from being enforced.
Remedy: Refactor the evaluation logic to compile the CEL Program once per rule in decide_rules and pass it to eval_compiled_condition. Do not bypass the policy engine or skip rules when PII is unavailable; instead, evaluate the policy unconditionally with PII facts set to None or empty to ensure metadata-based rules are always enforced.
For example, refactor the evaluation as follows:
fn eval_compiled_condition(program: &Program, facts: &Facts) -> bool {
let mut ctx = Context::default();
if let Some(http) = &facts.http {
if let Ok(value) = cel_interpreter::to_value(http) {
ctx.add_variable_from_value("http", value);
}
}
if let Some(pii) = &facts.pii {
if let Ok(value) = cel_interpreter::to_value(pii) {
ctx.add_variable_from_value("pii", value);
}
}
matches!(program.execute(&ctx), Ok(Value::Bool(true)))
}
fn decide_rules(policy: &Policy, facts: &Facts) -> Outcome {
for rule in &policy.rules {
let Ok(program) = Program::compile(&rule.condition) else {
tracing::warn!(condition = %rule.condition, "policy rule condition failed to compile");
continue;
};
if endpoint_matches(&rule.endpoint, facts.endpoint.as_deref())
&& eval_compiled_condition(&program, facts)
{
return Outcome {
verdict: rule.verdict,
rule: Some(rule.name.clone()),
};
}
}
// ...
}References
- Separate policy evaluation from post-verdict rewriting or finalization. Policy evaluation must run unconditionally (even with empty or None content facts for uninspectable bodies) to ensure that metadata-based rules (like path, method, or size limits) are always enforced and cannot be bypassed.
- When request bodies cannot be inspected (e.g., due to size limits, decoding errors, or non-text content), do not bypass the policy engine entirely. Instead, evaluate the policy engine with content-specific facts (like PII) set to None/empty, ensuring that metadata-based rules (like path, method, or size limits) are still enforced.
Merging this PR will improve performance by 11.86%
Performance Changes
Tip Curious why performance improved? Comment Comparing 
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 868c68e9ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// is passed over as if it were not in the policy at all. | ||
| fn decide_rules(policy: &Policy, facts: &Facts, skip_pii_rules: bool) -> Outcome { | ||
| for rule in &policy.rules { | ||
| if skip_pii_rules && condition_reads_pii(&rule.condition) { |
There was a problem hiding this comment.
When a policy combines PII and HTTP/Kubernetes criteria in one CEL rule, such as pii.count > 0 || http.method == 'POST' with a deny verdict, this skips the entire rule in detect mode. A POST to an egress-allowed domain is then forwarded even though its non-PII predicate is true; the prior empty-summary evaluation would still have enforced that denial. Detect mode is meant to downgrade only PII-caused verdicts, so mixed conditions need their non-PII branch preserved rather than being discarded wholesale.
AGENTS.md reference: crates/AGENTS.md:L46-L47
Useful? React with 👍 / 👎.
 This PR is not safe to merge because detect mode can still forward requests that should be denied by non-PII policy logic.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ..facts.clone() | ||
| }, | ||
| ); | ||
| let without_pii = decide_without_pii_rules(&self.state.policy, &facts); |
There was a problem hiding this comment.
For a clean or uninspectable body, pii uses the empty default, so a leading pii.count == 0 allow rule can make the original outcome Allow. This branch then skips decide_without_pii_rules and forwards the request without evaluating a later endpoint or Kubernetes deny. Detect mode therefore still bypasses non-PII enforcement for these requests. The new regression test only covers a body containing PII, where the original outcome is already Deny.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/honmoon-proxy/src/mitm.rs
Line: 648
Comment:
**Allow Skips Enforced Pass**
For a clean or uninspectable body, `pii` uses the empty default, so a leading `pii.count == 0` allow rule can make the original outcome `Allow`. This branch then skips `decide_without_pii_rules` and forwards the request without evaluating a later endpoint or Kubernetes deny. Detect mode therefore still bypasses non-PII enforcement for these requests. The new regression test only covers a body containing PII, where the original outcome is already `Deny`.
**Knowledge Base Used:**
- [Policy decision engine](https://app.greptile.com/passionfactory/-/custom-context/knowledge-base/pleaseai/honmoon/-/docs/policy-engine.md)
- [Network enforcement proxy](https://app.greptile.com/passionfactory/-/custom-context/knowledge-base/pleaseai/honmoon/-/docs/network-proxy.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| if skip_pii_rules && condition_reads_pii(&rule.condition) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
A valid rule can combine PII and protocol checks, such as pii.count > 0 || k8s.verb == 'delete'. Because this code skips the whole rule whenever it references pii, detect mode also discards the independently sufficient Kubernetes branch. With an allowing egress fallback, the prohibited action is forwarded even though PII did not cause the deny.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/honmoon-core/src/engine.rs
Line: 60-62
Comment:
**Mixed Rules Lose Enforcement**
A valid rule can combine PII and protocol checks, such as `pii.count > 0 || k8s.verb == 'delete'`. Because this code skips the whole rule whenever it references `pii`, detect mode also discards the independently sufficient Kubernetes branch. With an allowing egress fallback, the prohibited action is forwarded even though PII did not cause the deny.
**Knowledge Base Used:**
- [Policy decision engine](https://app.greptile.com/passionfactory/-/custom-context/knowledge-base/pleaseai/honmoon/-/docs/policy-engine.md)
- [Policy and enforcement model](https://app.greptile.com/passionfactory/-/custom-context/knowledge-base/pleaseai/honmoon/-/docs/policy-and-enforcement.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/honmoon-core/src/engine.rs">
<violation number="1" location="crates/honmoon-core/src/engine.rs:60">
P2: Detect-mode enforcement now parses every rule before endpoint filtering, then parses each matching non-PII rule again during evaluation. Filter by endpoint first and reuse the compiled `Program` so policy size does not add avoidable per-request CEL parsing cost.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client as Client
participant Proxy as MITM Proxy (honmoon-proxy)
participant Engine as Policy Engine (honmoon-core)
participant Rules as Policy Rules (CEL Programs)
Note over Client,Proxy: Request interception flow
Client->>Proxy: HTTP request with body (K8s secret DELETE)
Proxy->>Proxy: Parse k8s request facts
Proxy->>Proxy: Run PII detection / secrets tokenizer
Proxy->>Engine: decide_explained(policy, facts with PII)
Engine->>Rules: Compile rule conditions
Rules-->>Engine: Rule programs + reference sets
Engine->>Engine: Evaluate rules in order
alt Rule matches (allow-clean: pii.count == 0)
Engine-->>Proxy: Allow (first pass)
else No rule matches
Engine-->>Proxy: Continue evaluation
end
Note over Proxy,Engine: Detect mode enforced pass (CHANGED)
alt Detect mode + non-Allow verdict
Proxy->>Engine: decide_without_pii_rules(policy, facts)
Engine->>Rules: Iterate rule programs
loop Each rule
alt Rule references "pii" variable
Engine->>Engine: SKIP rule entirely
else No pii reference
Engine->>Engine: Evaluate condition
end
end
Engine-->>Proxy: Outcome (endpoint deny survives)
end
alt Outcome verdict is Deny
Proxy-->>Client: 403 inline response + denied audit event
else Outcome verdict is Allow
Proxy-->>Client: Forward to upstream
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| /// is passed over as if it were not in the policy at all. | ||
| fn decide_rules(policy: &Policy, facts: &Facts, skip_pii_rules: bool) -> Outcome { | ||
| for rule in &policy.rules { | ||
| if skip_pii_rules && condition_reads_pii(&rule.condition) { |
There was a problem hiding this comment.
P2: Detect-mode enforcement now parses every rule before endpoint filtering, then parses each matching non-PII rule again during evaluation. Filter by endpoint first and reuse the compiled Program so policy size does not add avoidable per-request CEL parsing cost.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/honmoon-core/src/engine.rs, line 60:
<comment>Detect-mode enforcement now parses every rule before endpoint filtering, then parses each matching non-PII rule again during evaluation. Filter by endpoint first and reuse the compiled `Program` so policy size does not add avoidable per-request CEL parsing cost.</comment>
<file context>
@@ -36,7 +36,30 @@ pub fn decide(policy: &Policy, facts: &Facts) -> Verdict {
+/// is passed over as if it were not in the policy at all.
+fn decide_rules(policy: &Policy, facts: &Facts, skip_pii_rules: bool) -> Outcome {
for rule in &policy.rules {
+ if skip_pii_rules && condition_reads_pii(&rule.condition) {
+ continue;
+ }
</file context>
1 participant
Fixes #99.
Problem
Detect mode enforces only verdicts that PII did not cause, by re-deciding on the same facts with the
piisummary cleared. Buteval_conditionalways binds an empty default forpii, so a policy whose rule list starts with an allow-clean rule (pii.count == 0→ allow) ahead of an endpoint-bound Kubernetes deny behaved wrong: on the second pass the allow-clean rule matched first, the pass returnedAllow, and nothing was enforced. Detect mode silently dropped a deny that block mode enforces — contradicting the guarantee that endpoint/Kubernetes rules are enforced in every mode.Fix
The engine now exposes
decide_without_pii_rules(): it decides as if the content scanner never ran by skipping any rule whose CEL condition references thepiivariable (per-rule fact attribution via the compiled program's reference set —Program::references().has_variable("pii"), no text matching), instead of rebinding an empty summary. The detect-mode enforced pass inmitm.rsuses it, so an allow-clean rule can no longer mask a later endpoint deny.decide/decide_explainedsemantics and egress precedence are unchanged; a condition that fails to compile is treated as not readingpii(it can never match anyway — fail-closed behavior preserved).Tests
pii_rules_are_skipped_not_rebound_to_an_empty_summary(engine): pins the Detect mode drops endpoint/Kubernetes denies when an allow-clean PII rule precedes them #99 policy shape, including an assertion documenting the exact masking the old approach produced (decidewithpii: None→ Allow).detect_mode_enforces_an_endpoint_rule_behind_an_allow_clean_pii_ruleandblock_mode_enforces_an_endpoint_rule_behind_an_allow_clean_pii_rule(proxy MITM integration): a Kubernetes secret DELETE with a valid RRN in the body gets an inline 403 with ak8s-no-secret-deletedenied audit event in both modes — the acceptance criteria from the issue.🤖 Generated with Claude Code
Summary by cubic
Fixes #99 by changing how detect mode's enforced pass re-decides policies. Instead of clearing the
piisummary, it now skips rules whose CEL conditions referencepii, so an allow-clean rule (pii.count == 0) can no longer mask a later endpoint or Kubernetes deny.decide_without_pii_rules()inhonmoon-core, which skips rules via the compiled program's reference set, not text matching.mitm.rsto use this function; block mode behavior and egress precedence are unchanged.Written for commit 868c68e. Summary will update on new commits.