[EnforceAuth] Update policy: POS Authorization#2
Conversation
|
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: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughReformatted many Rego tests and module metadata, added a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
Comment |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR updates the policy metadata title from "POS Transaction Authorization" to "POS Authorization" - a cosmetic simplification with no functional changes to the authorization logic.
The change is safe to merge as it only affects documentation metadata.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
dev/store-ops/pos/authorization.rego (2)
114-117:⚠️ Potential issue | 🟠 MajorAdd input validation for discount_percent field.
The helper doesn't validate that
discount_percentexists, is numeric, or is non-negative. A negative discount could bypass discount limits, and missing fields could cause policy evaluation errors.🛡️ Proposed fix to add validation
_within_discount_limit(user_claims) if { limit := _discount_limit_for_role[user_claims.role] + is_number(input.request.body.discount_percent) + input.request.body.discount_percent >= 0 input.request.body.discount_percent <= limit }As per coding guidelines, ensure input validation is sufficient in OPA/Rego policies to prevent malicious or malformed inputs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/store-ops/pos/authorization.rego` around lines 114 - 117, The helper _within_discount_limit should validate input.request.body.discount_percent before comparing to _discount_limit_for_role[user_claims.role]; update _within_discount_limit to first ensure discount_percent is present, is a number (use Rego's is_number/typeof checks), and is >= 0, and only then compare discount_percent <= limit from _discount_limit_for_role; treat any missing, non-numeric, or negative discount_percent as failing the helper (i.e., do not allow bypass) so policy evaluation remains safe.
104-107:⚠️ Potential issue | 🟠 MajorAdd input validation for amount field.
The helper doesn't validate that
amountexists, is numeric, or is non-negative. A negative amount could bypass return limits, and missing fields could cause policy evaluation errors.🛡️ Proposed fix to add validation
_within_return_limit(user_claims) if { limit := _return_limit_for_role[user_claims.role] + is_number(input.request.body.amount) + input.request.body.amount >= 0 input.request.body.amount <= limit }As per coding guidelines, ensure input validation is sufficient in OPA/Rego policies to prevent malicious or malformed inputs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/store-ops/pos/authorization.rego` around lines 104 - 107, The _within_return_limit helper currently compares input.request.body.amount to _return_limit_for_role[user_claims.role] without validating amount; update _within_return_limit to first ensure input.request and input.request.body.amount exist, the amount is a number (use is_number/typeof checks) and amount >= 0, and only then perform the comparison against _return_limit_for_role[user_claims.role]; if any validation fails the rule should not succeed (i.e., return false/leave the rule unsatisfied) so malformed or negative amounts cannot bypass limits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@dev/store-ops/pos/authorization.rego`:
- Around line 114-117: The helper _within_discount_limit should validate
input.request.body.discount_percent before comparing to
_discount_limit_for_role[user_claims.role]; update _within_discount_limit to
first ensure discount_percent is present, is a number (use Rego's
is_number/typeof checks), and is >= 0, and only then compare discount_percent <=
limit from _discount_limit_for_role; treat any missing, non-numeric, or negative
discount_percent as failing the helper (i.e., do not allow bypass) so policy
evaluation remains safe.
- Around line 104-107: The _within_return_limit helper currently compares
input.request.body.amount to _return_limit_for_role[user_claims.role] without
validating amount; update _within_return_limit to first ensure input.request and
input.request.body.amount exist, the amount is a number (use is_number/typeof
checks) and amount >= 0, and only then perform the comparison against
_return_limit_for_role[user_claims.role]; if any validation fails the rule
should not succeed (i.e., return false/leave the rule unsatisfied) so malformed
or negative amounts cannot bypass limits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a22ba45-cfb9-4928-9755-c1db567aca77
📒 Files selected for processing (1)
dev/store-ops/pos/authorization.rego
…resolution Strip trailing space from DIRS produced by tr '\n' ' ' and remove quotes around the dirs argument so multiple directories are word-split correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/actions/rego-lint/action.yml:
- Around line 32-40: The "Lint changed Rego files" step currently injects
unquoted ${{ steps.changed.outputs.dirs }} directly into the shell which allows
command injection and fails on paths with spaces; change the output `dirs` to be
newline-separated and update the step's run script to read that multiline output
into a shell array (e.g., using readarray/IFS-safe read) and invoke `regal lint`
with the array expansion quoted (so each path is passed as a separate,
safely-quoted argument) instead of interpolating the raw output into the
command.
🪄 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: 228f64a5-5cc4-491a-b5be-aabd5dcb78d9
📒 Files selected for processing (1)
.github/actions/rego-lint/action.yml
- Add .regal/config.yaml with project roots and ignore rules that don't apply to this multi-env repo structure - Auto-fix opa-fmt formatting across all test files - Fix pointless-reassignment in authentication.rego - Fix line-length violations in timeclock and api test/policy files
- Fix shell injection in rego-lint action by using multiline GHA output and mapfile array instead of unquoted interpolation - Add input validation (is_number, >= 0) to _within_return_limit and _within_discount_limit helpers across all environments
boorad
left a comment
boorad
left a comment
There was a problem hiding this comment.
Addressed both outside-diff-range findings:
_within_return_limit(lines 104-107): Addedis_number(input.request.body.amount)and>= 0guard so non-numeric or negative amounts fail the rule._within_discount_limit(lines 114-117): Addedis_number(input.request.body.discount_percent)and>= 0guard so negative discounts can't bypass limits.
Applied across all three environments (dev/prod/stage). All 153 OPA tests pass.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@prod/store-ops/pos/authorization.rego`:
- Around line 105-106: Add explicit deny-path unit tests in
prod/store-ops/pos/authorization_test.rego that exercise the new is_number
guards for input.request.body.amount and input.request.body.discount_percent:
create cases that pass string numbers ("49.99", "10"), null values, and negative
numbers for both amount and discount_percent and assert the policy denies those
requests; ensure each new test targets the same rule(s) exercised by the
allow-path tests (the rules that reference is_number(input.request.body.amount)
and is_number(input.request.body.discount_percent)) so both allow and deny
behaviors are covered per guidelines.
🪄 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: b01eb978-f71f-48f7-af22-564de67fbd82
📒 Files selected for processing (4)
.github/actions/rego-lint/action.ymldev/store-ops/pos/authorization.regoprod/store-ops/pos/authorization.regostage/store-ops/pos/authorization.rego
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/actions/rego-lint/action.yml
- dev/store-ops/pos/authorization.rego
Cover string, null, and negative inputs for both amount (returns) and discount_percent (price overrides) across all environments.
1 participant
Update Policy: POS Authorization
Created via EnforceAuth
Summary by CodeRabbit
Chores
Bug Fixes
Refactor
Tests
Documentation