[EnforceAuth] Add policy: EA Financial Global SSO#17
Conversation
📝 WalkthroughWalkthroughA new Rego authorization policy module was added at Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 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.
Code Review Summary
This PR adds a new OPA policy for EA Financial Global SSO. I've identified 1 critical security vulnerability that must be fixed before merge:
Critical Issue:
- Authentication Bypass Vulnerability: The policy uses
containsfor ID verification, which performs substring matching and allows unauthorized access. Any ID containing "EAID" anywhere in the string would be granted access (e.g., "HACKER_EAID", "fake_eaid_test").
Required Action:
Replace contains with startswith to ensure only IDs beginning with the proper prefix are authorized. A commit-able suggestion has been provided.
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: The contains operator performs substring matching, allowing any user with "EAID" anywhere in their ID to gain access. For example, IDs like "FAKE_EAID_HACKER", "my_eaid_test", or "compromised_EAID" would all incorrectly grant access. Use startswith to ensure only IDs beginning with "EAID" are authorized.1
| input.subject.id contains "EAID" | |
| startswith(input.subject.id, "EAID") |
Footnotes
-
CWE-185: Incorrect Regular Expression - https://cwe.mitre.org/data/definitions/185.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: 3
🤖 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`:
- Around line 11-13: The policy's rule uses the wrong input shape — update the
rule that defines allow to reference input.user.id instead of input.subject.id
so it matches the request producer shape; specifically edit the allow rule in
ea_financial_global_sso.rego to check input.user.id contains "EAID" (keeping the
rest of the condition intact) so authenticated users are correctly permitted.
- Around line 11-13: The policy's identity check uses a substring test in the
allow rule (input.subject.id contains "EAID"), which is too permissive; change
the check in the allow rule to a strict format and active-user gating instead —
e.g., validate input.subject.id with a prefix or anchored regex (use startswith
or match/regex like ^EAID...) and also require a truthy active flag (e.g.,
input.subject.active == true) before allowing; update the allow rule and any
helper functions referenced to perform these stricter checks.
- Line 12: The rule uses invalid Rego syntax and the wrong input path: replace
the infix use of contains and the incorrect input.subject.id with a proper
function call against input.user.id; specifically change the expression from
`input.subject.id contains "EAID"` to a valid Rego call such as
`contains(input.user.id, "EAID")`, or an equivalent string check, so the rule
references `input.user.id` and uses `contains(...)` as a function.
🪄 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: 4ebdefb9-8bbf-4ce4-9053-8ebe335d5002
📒 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.
Use the correct input shape (input.user.id), not input.subject.id.
The request producer builds input.user.id (projects/consumer-accounts-internal-api/src/services/opaService.ts:132-155), so this rule currently won’t match authenticated users and will deny by default.
💡 Proposed fix
allow if {
- contains(input.subject.id, "EAID")
+ is_string(input.user.id)
+ contains(input.user.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 } at line 12 (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, The
policy's rule uses the wrong input shape — update the rule that defines allow to
reference input.user.id instead of input.subject.id so it matches the request
producer shape; specifically edit the allow rule in ea_financial_global_sso.rego
to check input.user.id contains "EAID" (keeping the rest of the condition
intact) so authenticated users are correctly permitted.
Avoid substring-based identity checks for auth decisions.
contains(..., "EAID") is too permissive ("fooEAIDbar" passes). For identity enforcement, prefer strict format checks (e.g., prefix or regex) and, if required, active-user gating.
💡 Proposed hardening
allow if {
- contains(input.user.id, "EAID")
+ is_string(input.user.id)
+ startswith(input.user.id, "EAID")
+ input.user.isActive == true
}📝 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.
| allow if { | |
| input.subject.id contains "EAID" | |
| } | |
| allow if { | |
| is_string(input.subject.id) | |
| startswith(input.subject.id, "EAID") | |
| input.subject.isActive == true | |
| } |
🧰 Tools
🪛 GitHub Actions: Path-based Testing
[error] 12-12: opa fmt --diff policies failed. rego_parse_error: unexpected contains keyword: expected \n or ; or } at line 12 (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, The
policy's identity check uses a substring test in the allow rule
(input.subject.id contains "EAID"), which is too permissive; change the check in
the allow rule to a strict format and active-user gating instead — e.g.,
validate input.subject.id with a prefix or anchored regex (use startswith or
match/regex like ^EAID...) and also require a truthy active flag (e.g.,
input.subject.active == true) before allowing; update the allow rule and any
helper functions referenced to perform these stricter checks.
|
|
||
| # Allow Rule | ||
| allow if { | ||
| input.subject.id contains "EAID" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Rego contains() usage patterns and input id field references in policies (read-only).
rg -n --type-add 'rego:*.rego' --type rego -C2 '\bcontains\s*\(' infra/opa/policies
rg -n --type-add 'rego:*.rego' --type rego -C2 '\binput\.(subject|user)\.id\b' infra/opa/policiesRepository: EnforceAuth/ea-financial
Length of output: 10575
🏁 Script executed:
# Find references to ea_financial_global_sso policy
rg -n 'ea_financial_global_sso' --type-list | head -5
fd . -name '*sso*' -o -name '*ea_financial*'
find . -type f -name '*.ts' -o -name '*.js' | xargs grep -l 'ea_financial_global_sso' 2>/dev/null | head -5Repository: EnforceAuth/ea-financial
Length of output: 688
🏁 Script executed:
# Search for OPA input structure and where this policy is evaluated
rg -n 'subject.*id|user.*id' projects/consumer-accounts-internal-api/src/services/opaService.ts -A2 -B2Repository: EnforceAuth/ea-financial
Length of output: 431
🏁 Script executed:
# Look at the full content of ea_financial_global_sso.rego to understand context
cat infra/opa/policies/ea_financial_global_sso.regoRepository: EnforceAuth/ea-financial
Length of output: 350
Fix invalid Rego syntax and correct input field path.
Line 12 has two critical issues: (1) contains cannot be used as an infix operator (parse error blocking CI), and (2) the input field path is incorrect. The OPA service populates input.user.id, not input.subject.id.
Fix
allow if {
- input.subject.id contains "EAID"
+ contains(input.user.id, "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" | |
| contains(input.user.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 } at line 12 (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 uses
invalid Rego syntax and the wrong input path: replace the infix use of contains
and the incorrect input.subject.id with a proper function call against
input.user.id; specifically change the expression from `input.subject.id
contains "EAID"` to a valid Rego call such as `contains(input.user.id, "EAID")`,
or an equivalent string check, so the rule references `input.user.id` and uses
`contains(...)` as a function.
1 participant
Add Policy: EA Financial Global SSO
Created via EnforceAuth
Summary by CodeRabbit