Fix policy fail-open and redaction false positives - #41
Conversation
- validate profile policy references against configured policy names\n- make runtime unknown policy lookup deny by default\n- remove broad 32+ token redaction and keep benign identifiers\n- add regression tests for config, policy engine, MCP path, and redaction Closes #1 Closes #2 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR updates GitHub Docker presets and examples, validates profile policy references, denies unresolved policies at runtime, and replaces generic token redaction with provider-specific and key-based detection. ChangesGitHub preset credentials
Policy reference safety
Secret redaction
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MCPWrapper
participant PolicyEngine
participant ToolCall
MCPWrapper->>PolicyEngine: evaluate missing policy for write risk
PolicyEngine->>MCPWrapper: return deny decision
MCPWrapper->>ToolCall: return POLICY_BLOCKED error
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
- pin github-mcp-server image to v1.1.0 in generated preset and examples\n- pass -e GITHUB_PERSONAL_ACCESS_TOKEN to docker run\n- generate work/personal profile token references in github preset\n- add preset regression test for pinned image and token forwarding Closes #3 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/examples/github.md (1)
29-38: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDoc example is out of sync: missing
requireConfirmationonsafe-write.
presets.tsandexamples/github.miftah.jsonboth setrequireConfirmation: ["write"]on thesafe-writepolicy, but this doc snippet omits it. A user copying this example verbatim would lose the write-confirmation guardrail.📝 Suggested fix
"safe-write": { "allowRisk": ["read", "write"], - "denyRisk": ["destructive"] + "denyRisk": ["destructive"], + "requireConfirmation": ["write"] }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/examples/github.md` around lines 29 - 38, Update the safe-write policy example to include requireConfirmation: ["write"], matching the configuration in presets.ts and examples/github.miftah.json while preserving the existing allowRisk and denyRisk settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/config/presets.ts`:
- Around line 5-49: Refactor presetConfig to eliminate repeated preset ===
"github" branching by extracting preset-specific configuration into dedicated
builders or a preset map. Centralize construction of upstream, profiles,
description, defaultProfile, and policies, while preserving existing GitHub,
Sentry, and generic behavior. Use presetConfig and the related MiftahConfig
fields as the primary integration points.
In `@src/config/validate-config.ts`:
- Around line 11-15: Replace the substring-based classification in the config
validation error handler with structured issue metadata: have each validation
issue carry its explicit error code (for default-profile, policy, or schema
failures), then derive the MiftahErrorCode from that field rather than parsing
the joined message. Update the relevant schema/validation issue construction and
the handler around the code variable so user-provided names cannot affect
classification.
In `@src/secrets/redact.ts`:
- Around line 6-14: Update isSecretKey so secretKeyTerms recognizes common
plural and variant forms, including credentials, tokens, secrets, and passwords,
or normalize each key part to its singular equivalent before exact matching.
Preserve the existing API-key and private-key detection while ensuring names
such as GOOGLE_APPLICATION_CREDENTIALS are classified as secret keys.
In `@tests/presets.test.ts`:
- Around line 10-12: Update the docker argument assertion in the presets test to
use an exact array equality matcher instead of expect.arrayContaining,
preserving and validating the required order with the image as the final
argument.
- Around line 4-23: Extend the existing preset test for presetConfig("github",
"github") to assert that config.policies defines both "safe-write" and
"readonly", matching the policy references used by the work and personal
profiles. Keep the assertions focused on policy-key presence so missing or
renamed definitions fail the regression test.
---
Outside diff comments:
In `@docs/examples/github.md`:
- Around line 29-38: Update the safe-write policy example to include
requireConfirmation: ["write"], matching the configuration in presets.ts and
examples/github.miftah.json while preserving the existing allowRisk and denyRisk
settings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ffa83133-d046-4636-8472-8b45b01fb3b3
📒 Files selected for processing (15)
README.mddocs/examples/github.mdexamples/github.miftah.jsonexamples/multi-upstream.miftah.jsonsrc/config/presets.tssrc/config/schema.tssrc/config/validate-config.tssrc/policy/policy-engine.tssrc/secrets/redact.tssrc/utils/errors.tstests/config.test.tstests/mcp-wrapper.test.tstests/presets.test.tstests/routing-policy.test.tstests/secrets.test.ts
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/policy/policy-engine.ts (1)
25-29: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFail-closed check is bypassed by an explicit empty-string
policyName.
policyName ? this.policies[policyName] : undefinedand the subsequentif (policyName && !policy)both use truthy checks, soevaluate("", toolName)falls through to!policy→{ action: "allow", ... }instead of denying. This mirrors the same gap insrc/config/schema.ts's new policy-reference loop (line ~130); tightening the schema (policy: z.string().min(1).optional()) would prevent""from ever reaching this method, closing both gaps at the source.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/policy/policy-engine.ts` around lines 25 - 29, Empty-string policy names bypass fail-closed behavior in evaluate and schema validation. Update the policy reference schema to use a non-empty optional string, and in PolicyEngine.evaluate use an explicit undefined check rather than truthy checks when resolving policyName and denying unknown policies.src/config/schema.ts (1)
128-138: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEmpty-string
policy: ""bypasses this validation.
profile.policy && !policyNames.has(...)treats""as "no policy set", so an explicit empty-string policy reference silently skips validation here and, correspondingly, defaults to "allow" at runtime inPolicyEngine.evaluate(seesrc/policy/policy-engine.tslines 27-29) — undermining the fail-closed guarantee for what is arguably an "explicitly named" (if malformed) policy reference.🛡️ Proposed fix: reject empty policy names at the schema level
const profileSchema = z.object({ ... - policy: z.string().optional(), + policy: z.string().min(1).optional(), ... });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/config/schema.ts` around lines 128 - 138, Reject empty-string policy references during schema validation: update the profile-policy check in the profiles validation loop to distinguish an omitted policy from an explicitly provided empty string, and add a custom issue for the latter using the existing policy error conventions. Ensure PolicyEngine.evaluate receives only valid or absent policy names so empty references cannot default to allow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/config/presets.ts`:
- Line 4: Update the GITHUB_MCP_IMAGE constant in presets.ts from
ghcr.io/github/github-mcp-server:v1.1.0 to v1.5.0, unless a documented
compatibility constraint requires retaining the older version.
In `@src/secrets/redact.ts`:
- Line 25: Hoist the /[^a-z0-9]+/ regex into a module-level constant alongside
camelCaseBoundaryPattern, then update isSecretKey to use that constant when
splitting normalized keys instead of creating the literal per call.
---
Outside diff comments:
In `@src/config/schema.ts`:
- Around line 128-138: Reject empty-string policy references during schema
validation: update the profile-policy check in the profiles validation loop to
distinguish an omitted policy from an explicitly provided empty string, and add
a custom issue for the latter using the existing policy error conventions.
Ensure PolicyEngine.evaluate receives only valid or absent policy names so empty
references cannot default to allow.
In `@src/policy/policy-engine.ts`:
- Around line 25-29: Empty-string policy names bypass fail-closed behavior in
evaluate and schema validation. Update the policy reference schema to use a
non-empty optional string, and in PolicyEngine.evaluate use an explicit
undefined check rather than truthy checks when resolving policyName and denying
unknown policies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9a6adc9d-756c-4a0d-9824-2de6d9ad2653
📒 Files selected for processing (10)
docs/examples/github.mdsrc/config/presets.tssrc/config/schema.tssrc/config/validate-config.tssrc/policy/policy-engine.tssrc/secrets/redact.tssrc/utils/errors.tstests/config.test.tstests/presets.test.tstests/secrets.test.ts
- pin GitHub MCP Docker preset to verified v1.5.0 release\n- keep generated docs and examples aligned with the preset\n- avoid recompiling secret-key split regex on every redaction Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 99-100: Update the README wording in the GitHub preset upgrade
guidance from “that release notes” to “the release notes.”
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9c1e791a-446c-4185-a5a4-9b0cf10c9b83
📒 Files selected for processing (7)
README.mddocs/examples/github.mdexamples/github.miftah.jsonexamples/multi-upstream.miftah.jsonsrc/config/presets.tssrc/secrets/redact.tstests/presets.test.ts
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/config.test.ts`:
- Around line 58-72: Hoist the duplicated /POLICY_NOT_FOUND/ regex into a
module-level constant, then use that shared constant in both affected toThrow
assertions, including the tests around validateConfig.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7241ae16-31a3-4c62-853f-968b8b676c3e
📒 Files selected for processing (5)
README.mdsrc/config/schema.tssrc/policy/policy-engine.tstests/config.test.tstests/routing-policy.test.ts
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/config.test.ts (1)
45-75: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that errors retain profile and policy names.
These tests verify
POLICY_NOT_FOUNDand the typed error code, but they do not lock in the required message context. Add assertions for the affected profile name and supplied policy value so future changes cannot silently remove that diagnostic information.Also applies to: 77-96
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/config.test.ts` around lines 45 - 75, Extend both profile validation tests around validateConfig to assert the thrown error message includes the affected profile name "work" and the supplied policy values "missing-policy" and "" respectively, while retaining the existing policyNotFoundPattern and typed error-code assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/config.test.ts`:
- Around line 45-75: Extend both profile validation tests around validateConfig
to assert the thrown error message includes the affected profile name "work" and
the supplied policy values "missing-policy" and "" respectively, while retaining
the existing policyNotFoundPattern and typed error-code assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e3c9c1d3-6a30-45cd-9d6a-e5a470233b2f
📒 Files selected for processing (1)
tests/config.test.ts
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
Linked issues
Validation
Summary by CodeRabbit