[EnforceAuth] Add policy: EA Financial Global SSO#18
Conversation
📝 WalkthroughWalkthroughA new OPA (Open Policy Agent) Rego policy module is added for EA Financial Global SSO authorization. The policy imports rego.v1, defaults to denying access, and permits authorization when the input subject ID contains the substring "EAID". Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Regal (0.39.0)infra/opa/policies/ea_financial_global_sso.rego{ 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 |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds a global SSO policy for EA Financial. The implementation has a critical security vulnerability that must be fixed before merge.
Critical Issue:
The authentication check uses a substring match (contains "EAID") instead of proper prefix validation, allowing unauthorized access from any ID containing "EAID" anywhere in the string. This must be changed to startswith(input.subject.id, "EAID-") to properly validate EA Financial user IDs.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
|
|
||
| # Allow Rule | ||
| allow if { | ||
| input.subject.id contains "EAID" |
There was a problem hiding this comment.
🛑 Security Vulnerability: Replace substring check with prefix validation. The contains operator allows any ID with "EAID" anywhere in the string (e.g., "malicious-EAID-user", "FAKEEAID123"), enabling unauthorized access.1
| input.subject.id contains "EAID" | |
| startswith(input.subject.id, "EAID-") |
Footnotes
-
CWE-625: Permissive Regular Expression - https://cwe.mitre.org/data/definitions/625.html ↩
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@infra/opa/policies/ea_financial_global_sso.rego`:
- Line 12: The Rego condition using the invalid infix "contains" needs to be
changed: locate the occurrence `input.subject.id contains "EAID"` and replace it
with the correct function-call form `contains(input.subject.id, "EAID")` so the
substring check uses rego.v1's contains(input, substr) signature.
- Line 12: The policy currently uses the substring operator `contains` on
`input.subject.id`, which allows any ID with "EAID" anywhere; change this to a
strict check — either an exact equality or a prefix check — by replacing the
`input.subject.id contains "EAID"` condition with a precise match (e.g.,
`input.subject.id == "EAID"` or a prefix check such as
`startswith(input.subject.id, "EAID-")`) so only exact or properly prefixed
EAIDs are accepted; update the rule(s) that reference `input.subject.id`
accordingly and add unit tests for typical malicious embeddings like
"user@EAID.com" and "FAKE-EAID-123".
- Line 12: The rule currently checks input.subject.id which doesn't exist and
causes all requests to be denied; update the condition in
ea_financial_global_sso.rego from input.subject.id contains "EAID" to
input.user.id contains "EAID" so it matches the input structure produced by
opaService.ts (which uses input.user with id, username, role, permissions,
isActive); locate the expression containing input.subject.id and replace it with
input.user.id, then run the policy tests to confirm behavior.
- Around line 11-13: Add the ea_financial_global_sso policy into the main
decision flow by updating main.rego to import the package (e.g., import
data.ea_financial_global_sso as global_sso) and then delegate the allow decision
to it by adding an allow rule that calls global_sso.allow (following the
existing pattern of other delegated allow rules in main.rego) so the policy's
allow rule becomes reachable by data.main.allow.
🪄 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: CHILL
Plan: Pro
Run ID: 1d70c53a-b2d4-4cae-b3c9-c84e0bbc43b2
📒 Files selected for processing (1)
infra/opa/policies/ea_financial_global_sso.rego
| allow if { | ||
| input.subject.id contains "EAID" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify that main.rego doesn't import the new policy
echo "=== Checking imports in main.rego ==="
rg -n 'import.*ea_financial_global_sso' infra/opa/policies/main.rego
echo -e "\n=== Current import structure in main.rego ==="
rg -n '^import data\.' infra/opa/policies/main.rego
echo -e "\n=== Checking allow rules that delegate to sub-policies ==="
rg -n -A 3 '^allow' infra/opa/policies/main.regoRepository: EnforceAuth/ea-financial
Length of output: 8175
🏁 Script executed:
#!/bin/bash
# Check the package declaration in the policy file
echo "=== Package declaration in ea_financial_global_sso.rego ==="
head -5 infra/opa/policies/ea_financial_global_sso.rego
# Check OPA config entry point
echo -e "\n=== OPA Config entry point ==="
cat infra/opa/opa-config-ea.yaml | grep -A 2 -B 2 "entrypoint\|decision"
# Check if ea_financial_global_sso is referenced anywhere in the codebase
echo -e "\n=== References to ea_financial_global_sso ==="
rg 'ea_financial_global_sso' infra/opa/policies/Repository: EnforceAuth/ea-financial
Length of output: 544
🏁 Script executed:
#!/bin/bash
# Search for OPA config files
echo "=== OPA config files in the repo ==="
find infra/opa -name "*.yaml" -o -name "*.yml" -o -name "*config*" | head -20
# Also check if there are any Kubernetes manifests or other config references
echo -e "\n=== Files in infra/opa directory ==="
ls -la infra/opa/
# Check for any references to main.allow in the codebase
echo -e "\n=== References to main.allow ==="
rg 'main\.allow' infra/Repository: EnforceAuth/ea-financial
Length of output: 997
🏁 Script executed:
#!/bin/bash
# Read the actual OPA config file
echo "=== OPA Config Entry Point ==="
cat infra/opa/config/opa-config-ea.yaml | grep -A 5 -B 5 "query"
# Double-check main.rego to see if there's any way ea_financial_global_sso is indirectly referenced
echo -e "\n=== Check for any data flow to ea_financial_global_sso ==="
rg 'global.*sso|global_sso' infra/opa/policies/
# List all allow rules in main.rego to confirm pattern
echo -e "\n=== Count of allow rules in main.rego ==="
rg -c '^allow if {' infra/opa/policies/main.regoRepository: EnforceAuth/ea-financial
Length of output: 733
🏁 Script executed:
#!/bin/bash
# Count allow rules more carefully
echo "=== Allow rules in main.rego (checking pattern) ==="
rg 'allow if' infra/opa/policies/main.rego | wc -l
# Verify the import pattern suggested in review would be correct
echo -e "\n=== Verify import pattern consistency ==="
rg 'import data\.' infra/opa/policies/main.rego | grep 'as.*sso'Repository: EnforceAuth/ea-financial
Length of output: 220
Policy is unreachable—missing integration with main.rego.
This policy won't be evaluated by OPA because:
- The OPA config queries
data.main.allowas the decision entry point (ininfra/opa/config/opa-config-ea.yaml) - The
main.regofile doesn't import theea_financial_global_ssopackage - Without this import, the policy's rules are inaccessible to the decision engine
To integrate this policy, add the following import to main.rego:
import data.ea_financial_global_sso as global_ssoThen add an allow rule that delegates to it when appropriate (following the existing pattern):
allow if {
global_sso.allow
}🧰 Tools
🪛 GitHub Actions: Path-based Testing
[error] 12-12: opa fmt --diff policies/ failed: rego_parse_error: unexpected contains keyword: expected \n or ; or }. Parse error at: input.subject.id contains "EAID".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infra/opa/policies/ea_financial_global_sso.rego` around lines 11 - 13, Add
the ea_financial_global_sso policy into the main decision flow by updating
main.rego to import the package (e.g., import data.ea_financial_global_sso as
global_sso) and then delegate the allow decision to it by adding an allow rule
that calls global_sso.allow (following the existing pattern of other delegated
allow rules in main.rego) so the policy's allow rule becomes reachable by
data.main.allow.
|
|
||
| # Allow Rule | ||
| allow if { | ||
| input.subject.id contains "EAID" |
There was a problem hiding this comment.
Fix critical syntax error with contains operator.
The pipeline failure confirms that the contains keyword usage is invalid Rego syntax. In rego.v1, the correct syntax for substring checking is:
contains(input.subject.id, "EAID")🐛 Proposed fix for the syntax error
- input.subject.id contains "EAID"
+ contains(input.subject.id, "EAID")🧰 Tools
🪛 GitHub Actions: Path-based Testing
[error] 12-12: opa fmt --diff policies/ failed: rego_parse_error: unexpected contains keyword: expected \n or ; or }. Parse error at: input.subject.id contains "EAID".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infra/opa/policies/ea_financial_global_sso.rego` at line 12, The Rego
condition using the invalid infix "contains" needs to be changed: locate the
occurrence `input.subject.id contains "EAID"` and replace it with the correct
function-call form `contains(input.subject.id, "EAID")` so the substring check
uses rego.v1's contains(input, substr) signature.
Substring matching creates a critical security vulnerability.
Using contains to check if the subject ID contains "EAID" anywhere is extremely permissive and dangerous for a financial system. This would allow:
"FAKE-EAID-123"✓ allowed"not-an-EAID-user"✓ allowed"user@EAID.com"✓ allowed- Any malicious ID with "EAID" embedded ✓ allowed
For production financial authorization, use exact prefix matching or exact ID validation instead.
🛡️ Proposed fix using prefix matching
- input.subject.id contains "EAID"
+ startswith(input.subject.id, "EAID-")Or for exact validation:
- input.subject.id contains "EAID"
+ regex.match(`^EAID-[0-9]+$`, input.subject.id)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| input.subject.id contains "EAID" | |
| startswith(input.subject.id, "EAID-") |
| input.subject.id contains "EAID" | |
| regex.match(`^EAID-[0-9]+$`, input.subject.id) |
🧰 Tools
🪛 GitHub Actions: Path-based Testing
[error] 12-12: opa fmt --diff policies/ failed: rego_parse_error: unexpected contains keyword: expected \n or ; or }. Parse error at: input.subject.id contains "EAID".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infra/opa/policies/ea_financial_global_sso.rego` at line 12, The policy
currently uses the substring operator `contains` on `input.subject.id`, which
allows any ID with "EAID" anywhere; change this to a strict check — either an
exact equality or a prefix check — by replacing the `input.subject.id contains
"EAID"` condition with a precise match (e.g., `input.subject.id == "EAID"` or a
prefix check such as `startswith(input.subject.id, "EAID-")`) so only exact or
properly prefixed EAIDs are accepted; update the rule(s) that reference
`input.subject.id` accordingly and add unit tests for typical malicious
embeddings like "user@EAID.com" and "FAKE-EAID-123".
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Examine how OPA input is constructed and what field names are used
# Check opaService.ts for input construction patterns
echo "=== Input construction in opaService.ts ==="
rg -n -A 10 -B 2 'n\.user\s*=' projects/consumer-accounts-internal-api/src/services/opaService.ts
# Check all Rego policies for input field usage patterns
echo -e "\n=== Input field patterns used in existing policies ==="
rg -n 'input\.(user|subject)\.' infra/opa/policies/ --type rego -C 2Repository: EnforceAuth/ea-financial
Length of output: 213
🏁 Script executed:
# Find opaService.ts
find . -type f -name "opaService.ts" | head -20Repository: EnforceAuth/ea-financial
Length of output: 136
🏁 Script executed:
# Find the Rego policy files mentioned
find . -type f -name "*sso.rego" -o -name "ea_financial_global_sso.rego" | head -20Repository: EnforceAuth/ea-financial
Length of output: 178
🏁 Script executed:
# List all Rego files to understand structure
find . -type f -name "*.rego" | head -30Repository: EnforceAuth/ea-financial
Length of output: 1786
🏁 Script executed:
cat -n projects/consumer-accounts-internal-api/src/services/opaService.ts | head -100Repository: EnforceAuth/ea-financial
Length of output: 3492
🏁 Script executed:
cat -n infra/opa/policies/ea_financial_global_sso.regoRepository: EnforceAuth/ea-financial
Length of output: 448
🏁 Script executed:
cat -n infra/opa/policies/ea_financial/identity_provider/sso.regoRepository: EnforceAuth/ea-financial
Length of output: 2323
🏁 Script executed:
grep -n "\.sub" projects/consumer-accounts-internal-api/src/services/opaService.tsRepository: EnforceAuth/ea-financial
Length of output: 50
🏁 Script executed:
# Check if sub is added to the user object elsewhere
grep -rn "user.*sub" projects/consumer-accounts-internal-api/src/services/ --include="*.ts"Repository: EnforceAuth/ea-financial
Length of output: 50
🏁 Script executed:
# Check all Rego files for what fields they actually use from input.user
grep -n "input\.user\." infra/opa/policies/ -r --include="*.rego" | head -30Repository: EnforceAuth/ea-financial
Length of output: 1042
🏁 Script executed:
# Check if groups or sub fields are added to user object in opaService.ts
cat -n projects/consumer-accounts-internal-api/src/services/opaService.ts | tail -50Repository: EnforceAuth/ea-financial
Length of output: 1690
🏁 Script executed:
# Search for any transformation or mapping of user data in the service
grep -n "user\|sub\|groups" projects/consumer-accounts-internal-api/src/services/opaService.ts | head -40Repository: EnforceAuth/ea-financial
Length of output: 2029
🏁 Script executed:
# Check if there's a test or example showing actual input structure
find . -name "*opa*test*" -o -name "*opa*spec*" | head -20Repository: EnforceAuth/ea-financial
Length of output: 50
Incorrect input field path—policy will always deny.
The policy references input.subject.id, but the actual input structure constructed by opaService.ts uses input.user with fields: id, username, role, permissions, and isActive. The input.subject field does not exist, so this condition will always evaluate to undefined and deny all requests.
The correct field path is input.user.id:
🔧 Suggested fix
- input.subject.id contains "EAID"
+ input.user.id contains "EAID"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| input.subject.id contains "EAID" | |
| input.user.id contains "EAID" |
🧰 Tools
🪛 GitHub Actions: Path-based Testing
[error] 12-12: opa fmt --diff policies/ failed: rego_parse_error: unexpected contains keyword: expected \n or ; or }. Parse error at: input.subject.id contains "EAID".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infra/opa/policies/ea_financial_global_sso.rego` at line 12, The rule
currently checks input.subject.id which doesn't exist and causes all requests to
be denied; update the condition in ea_financial_global_sso.rego from
input.subject.id contains "EAID" to input.user.id contains "EAID" so it matches
the input structure produced by opaService.ts (which uses input.user with id,
username, role, permissions, isActive); locate the expression containing
input.subject.id and replace it with input.user.id, then run the policy tests to
confirm behavior.
1 participant
Add Policy: EA Financial Global SSO
Created via EnforceAuth
Summary by CodeRabbit