Improve policy definition validation and description quality #40
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughUpdates to ~40 policy-definition YAMLs: concise description rewrites, tighter validation (minItems/minLength/required/additionalProperties), schema restructures (properties blocks, systemParameters), and added conditional rules (anyOf/allOf) to enforce context-dependent field presence. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (4)
policies/semantic-prompt-guard/policy-definition.yaml (1)
33-50:⚠️ Potential issue | 🟡 MinorConsider adding
minItems: 1toallowedPhrasesanddeniedPhrases.The
anyOfconstraint correctly enforces that at least one of the two phrase lists is provided. However,requiredonly checks that the key is present — a user could supplyallowedPhrases: [](an empty array) and pass validation, leaving the policy with nothing to match against at runtime.Based on learnings, policy configurations are validated against these schemas at API creation/policy attachment time, so tightening this here prevents a misconfiguration from reaching runtime.
Proposed fix
allowedPhrases: type: array description: | List of phrases that are considered safe. The prompt must match one of these within allowSimilarityThreshold. Embeddings will be automatically generated for these phrases during policy initialization. + minItems: 1 items: type: string description: Phrase text to allow deniedPhrases: type: array description: | List of phrases that should block the prompt when similar within the denySimilarityThreshold. Embeddings will be automatically generated for these phrases during policy initialization. + minItems: 1 items: type: string description: Phrase text to denyAlso applies to: 55-59
policies/semantic-cache/policy-definition.yaml (1)
56-61:⚠️ Potential issue | 🟡 MinorAdd
minimumconstraints to integer system parameters.
embeddingDimension,dbPort, andttlaccept any integer, including zero or negative values, which are never valid for these fields. Since these schemas are the validation gate at policy-attach time, adding bounds here prevents invalid configs from reaching runtime. Based on learnings, policy configurations are validated against their policy-definition.yaml schemas when APIs are created or policies are attached, before runtime execution.Proposed fix
embeddingDimension: type: integer description: | Dimension of embedding vectors (required for some vector databases) Common values: 1536 (OpenAI ada-002), 1024 (Mistral), etc. + minimum: 1 "wso2/defaultValue": "${config.embedding_provider_dimension}"dbPort: type: integer description: Vector database port (required for Redis/Milvus) + minimum: 1 + maximum: 65535 "wso2/defaultValue": "${config.vector_db_provider_port}"ttl: type: integer description: Vector database TTL (optional) + minimum: 0 "wso2/defaultValue": "${config.vector_db_provider_ttl}"Also applies to: 82-85, 98-101
policies/url-guardrail/policy-definition.yaml (1)
28-34:⚠️ Potential issue | 🟡 MinorA
timeoutof0would cause DNS/HTTP HEAD checks to fail instantly.
minimum: 0allows a zero-millisecond timeout, which is almost certainly not useful for network operations. Considerminimum: 1(or a more practical floor like100) to prevent misconfiguration.policies/azure-content-safety-content-moderation/policy-definition.yaml (1)
92-94:⚠️ Potential issue | 🟡 MinorResponse-phase
passthroughOnErrordescription incorrectly references "requests".This appears to be copy-pasted from the request block. In the response phase, it should reference responses rather than requests.
📝 Proposed fix
description: | - If true, allows requests to proceed if Azure Content Safety API call fails. - If false (default), blocks requests on API errors. + If true, allows responses to proceed if Azure Content Safety API call fails. + If false (default), blocks responses on API errors.
🤖 Fix all issues with AI agents
In `@policies/mcp-authz/policy-definition.yaml`:
- Around line 66-70: The anyOf block is currently nested under the properties
map and thus treated as a property instead of a validation keyword; move the
anyOf key out of the properties object so it becomes a sibling of
properties/additionalProperties/required within the item schema (i.e., place
anyOf at the same level as properties, required, additionalProperties in the
items object) and ensure the anyOf entries reference the requiredClaims and
requiredScopes property requirements so the array item validates correctly given
additionalProperties: false.
In `@policies/pii-masking-regex/policy-definition.yaml`:
- Around line 24-25: The pattern for piiEntity names in policy-definition.yaml
currently allows underscore-only values; update the "pattern" value so the regex
requires at least one letter (e.g., start with an uppercase letter and then
allow uppercase letters or underscores) instead of permitting strings composed
solely of underscores, ensuring valid entity names like "EMAIL" or "PII_ENTITY"
but not "_" or "___".
🧹 Nitpick comments (7)
policies/semantic-cache/policy-definition.yaml (1)
102-109: Add conditional validation forembeddingModelwhenembeddingProvideris OPENAI or MISTRAL.
embeddingModelis documented as required for OPENAI and MISTRAL providers but is not listed in therequiredfields. Many other policy files in this codebase (word-count-guardrail, url-guardrail, set-headers, request-rewrite, and 10+ others) useanyOf/allOfconditional validation to enforce provider-specific requirements. Applying the same pattern here would catch misconfiguration at policy attach time rather than runtime.policies/token-based-ratelimit/policy-definition.yaml (1)
26-26: Pre-existing: duration pattern allows negative values.The regex
^[-+]?...permits negative durations (e.g.,"-5m"), which is semantically invalid for a rate-limit time window. This is pre-existing and outside the scope of this PR, but worth a follow-up to tighten the pattern (e.g., remove the[-+]?prefix or replace with[+]?).policies/set-headers/policy-definition.yaml (1)
10-14: MissingminItems: 1onrequestHeadersandresponseHeaders— inconsistent with sibling policies.The
add-headerspolicy now enforcesminItems: 1on both header arrays, butset-headersdoes not. An empty array would pass validation here but serve no purpose. Consider adding the same constraint for consistency.Suggested diff
requestHeaders: type: array description: Array of headers that need to set to the request during request phase. At least one of requestHeaders or responseHeaders must be specified. + minItems: 1 items:responseHeaders: type: array description: Array of headers that need to set to the response during response phase. At least one of requestHeaders or responseHeaders must be specified. + minItems: 1 items:Also applies to: 32-36
policies/mcp-acl-list/policy-definition.yaml (1)
8-77: Consider adding ananyOfconstraint to require at least one oftools,resources, orprompts.Other policies in this PR consistently add
anyOfblocks to ensure at least one meaningful configuration section is provided. Without it, a user could attach this policy with an empty object{}, which would be a no-op. If that's intentional (no ACL = pass-through), please disregard.Suggested addition after line 77
+ anyOf: + - required: + - tools + - required: + - resources + - required: + - promptspolicies/aws-bedrock-guardrail/policy-definition.yaml (1)
99-106:awsRoleARNandawsRoleRegionare missingminLength: 1unlike other string fields.Other optional string fields in this block (
awsAccessKeyID,awsSecretAccessKey,awsSessionToken,awsRoleExternalID) all haveminLength: 1. These two fields omit it, which would allow empty strings to pass validation if provided. Consider addingminLength: 1for consistency if empty strings are not meaningful.Proposed fix
awsRoleARN: type: string description: AWS IAM role ARN to assume (for role-based authentication). If specified, runtime assumes this role instead of using static credentials. + minLength: 1 "wso2/defaultValue": "${config.awsbedrock_role_arn}" awsRoleRegion: type: string description: AWS region for role assumption (required if awsRoleARN is specified). + minLength: 1 "wso2/defaultValue": "${config.awsbedrock_role_region}"policies/advanced-ratelimit/policy-definition.yaml (2)
62-91: Duplicated keyExtraction item schema between per-quota and global levels.The item schema for
keyExtractionat the per-quota level (lines 69-91) and the global level (lines 177-199) are identical. If JSON Schema$refor YAML anchors/aliases are supported by the schema consumer, you could DRY this up. Not blocking, but worth considering if the schema grows further.Also applies to: 170-199
67-67:minItems: 0is redundant.Arrays allow zero items by default in JSON Schema, so
minItems: 0has no effect. Same applies to line 175. Removing them would reduce noise, but this is purely cosmetic.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@policies/mcp-authz/policy-definition.yaml`:
- Around line 49-65: The schema allows empty requiredClaims ({}) or
requiredScopes ([]) which satisfy presence checks but enforce nothing; update
the policy-definition.yaml schema by adding minProperties: 1 to the
requiredClaims object definition and minItems: 1 to the requiredScopes array
definition so empty values are rejected at validation time (look for the
requiredClaims and requiredScopes definitions and add the respective
minProperties/minItems constraints).
2 participants
Fixes: wso2/api-platform#1074
Summary by CodeRabbit
New Features
Bug Fixes
Documentation