feat(auth): enforce purpose-bound route authorization - #34
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
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:
📝 WalkthroughWalkthroughHTTP 회계 경로가 요청마다 검증된 주체를 확인하도록 변경되었습니다. Changes목적 기반 인가
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The change adds purpose-bound authorization and durable authorization decisions, but it is not currently merge-ready because required checks are not passing, dependency integration and branch reconciliation remain outstanding, and a localized resolver type mismatch still needs correction. Sequence Diagram(s)sequenceDiagram
participant Client
participant JournalProposalServer
participant request_principal_resolver
participant authorize
participant record_authorization_decision
Client->>JournalProposalServer: 회계 HTTP 요청
JournalProposalServer->>request_principal_resolver: 현재 요청의 검증된 주체 조회
request_principal_resolver-->>JournalProposalServer: AuthenticatedPrincipal 반환
JournalProposalServer->>authorize: 경로 operation과 테넌트로 인가
authorize-->>JournalProposalServer: AuthorizationDecision 반환
JournalProposalServer->>record_authorization_decision: 결정과 correlation_reference 기록
JournalProposalServer-->>Client: 200, 403 또는 503 응답
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 91.53% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 59 functions across 10 files. (9 skipped: 8 unsupported, 1 too large.) ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@OpenCode review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Current-head validation: |
|
Devin informational note confirmed on exact head |
|
Current-head authorization repair on |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Current-head validation for
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Current-head validation for
|
|
@OpenCode review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Scheduled review-feedback autofix for this PR head.
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Authorization policy-owner follow-up on the current branch: RED |
seonghobae
added
enhancement
ChatGPT Codex Connector
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| principal_reference: str | ||
| tenant_reference: str | ||
| requested_tenant_reference: str | ||
| authentication_context_reference: str | ||
| credential_evidence_reference: str | ||
| operation_code: str | ||
| permission_code: str | ||
| purpose_code: str | ||
| policy_version: str | ||
| decision_code: str | ||
| allowed: bool |
There was a problem hiding this comment.
🟡 Audit records omit caller classification
Every authorization decision drops principal_kind before persistence. Audit evidence cannot distinguish human, service, and agent callers or verify agent restrictions.
Prompt for agents
Add principal_kind to AuthorizationDecision and its fingerprinted values, populate it for authenticated and unauthenticated decisions, persist it in database/migrations/0015_authorization_decision_evidence.sql with the same closed vocabulary, and extend unit and PostgreSQL tests to verify it survives into immutable audit evidence.
Was this helpful? React with 👍 or 👎 to provide feedback.
| for value, label in ( | ||
| (self.principal_reference, "principal reference"), | ||
| (self.tenant_reference, "tenant reference"), | ||
| (self.authentication_context_reference, "authentication context reference"), | ||
| (self.credential_evidence_reference, "credential evidence reference"), | ||
| ): | ||
| _require_reference(value, label) | ||
| _require_code(self.purpose_code, "purpose code") |
There was a problem hiding this comment.
🟡 Long identity references disable authorized routes
A valid CWL reference above 255 bytes passes AuthenticatedPrincipal but fails evidence storage. Every affected request returns 503 instead of its route response.
Prompt for agents
Enforce the migration's 255-octet CWL-reference profile in AuthenticatedPrincipal before authorization. Apply the same bound to principal_reference, tenant_reference, authentication_context_reference, and credential_evidence_reference, and validate the configured/requested tenant reference before evidence persistence. Add boundary tests for ASCII and multibyte references so application acceptance exactly matches migration 0015.
Was this helpful? React with 👍 or 👎 to provide feedback.
| authorization evaluator; caller-constructed or post-issuance-mutated `allowed` decisions | ||
| cannot be promoted into durable audit evidence, while copied evaluator decisions retain | ||
| their provenance. | ||
| - Malformed or non-object period-close request bodies no longer create an allowed hard-close authorization record before the request is rejected; authorization evidence is reserved for a structurally valid close command. |
There was a problem hiding this comment.
🔍 Malformed-close documentation contradicts runtime
The changelog says malformed close bodies never create allowed evidence. _post_authorization_operation classifies them as hard closes, records an allow, then returns 400.
Was this helpful? React with 👍 or 👎 to provide feedback.
| USING (tenant_account_id = accounting_core.current_tenant_account_id()) | ||
| WITH CHECK (tenant_account_id = accounting_core.current_tenant_account_id()); | ||
|
|
||
| REVOKE ALL ON accounting_integration.authorization_decision_record FROM PUBLIC; |
There was a problem hiding this comment.
🔍 Runtime insert grant needs deployment verification
The migration revokes public access without granting INSERT. Production authorization returns 503 for every routed request unless deployment provisions the runtime role explicitly.
Was this helpful? React with 👍 or 👎 to provide feedback.
| tenant_matches = principal.tenant_reference == requested_tenant_reference | ||
| agent_restricted = principal.principal_kind == "agent" and operation_code in _HIGH_IMPACT_OPERATIONS | ||
| allowed = ( | ||
| bool(permission_code) | ||
| and tenant_matches | ||
| and not agent_restricted | ||
| and permission_code in principal.granted_permission_codes | ||
| ) |
There was a problem hiding this comment.
🟥 Purpose claims do not restrict access
authorize records purpose_code without evaluating it. A permission issued for one purpose authorizes the same sensitive operation under any other purpose.
Was this helpful? React with 👍 or 👎 to provide feedback.
![cwl-noema-review[bot]](https://avatars.githubusercontent.com/u/295022177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Noema LLM review
The PR implements purpose-bound route authorization, replacing server-wide principal inheritance with a request-scoped resolver and establishing an immutable, tenant-isolated audit trail. It successfully resolves multiple critical security findings from prior threads: (1) closing the authorization bypass for malformed period-close requests, (2) enforcing explicit principal kinds to prevent privilege escalation, (3) preventing 503 errors for multibyte correlation keys by using character-length bounds in PostgreSQL, and (4) ensuring audit provenance by rejecting mutated authorization decisions. The database schema is correctly bounded to prevent storage inflation and uses RLS for tenant isolation. Documentation across ADR 0064, SECURITY.md, and OPERABILITY.md is comprehensively updated to reflect these new security boundaries.
Reviewed changed lines
database/migrations/0015_authorization_decision_evidence.sql:13 (RIGHT): Correctly implements octet-length bounds (255) and URN regex for principal references to prevent unbounded storage and ensure normalized identity format.database/migrations/0015_authorization_decision_evidence.sql:56 (RIGHT): Correctly uses char_length(correlation_reference) <= 512, aligning with the HTTP boundary contract and preventing 503 errors for multibyte UTF-8 keys.database/migrations/0015_authorization_decision_evidence.sql:75 (RIGHT): Implements an append-only trigger to ensure authorization evidence is immutable, satisfying audit requirements.docs/adr/0064-purpose-bound-authorization.md:35 (RIGHT): Explicitly defines the fail-closed behavior for malformed /period-closes bodies, ensuring they are classified as hard_close_period for authorization purposes.docs/SECURITY.md:31 (RIGHT): Documents the requirement for a request-scoped resolver and the rejection of omitted principal_kind, closing the gap where a server-wide principal could be inherited.
Adversarial validation
database/migrations/0015_authorization_decision_evidence.sql:56 (RIGHT)falsified: A multibyte UTF-8 correlation key that is < 512 characters but > 512 bytes will cause a 503 error during evidence persistence. — The migration useschar_length(correlation_reference) <= 512rather thanoctet_length, which counts characters regardless of byte size.docs/adr/0064-purpose-bound-authorization.md:35 (RIGHT)falsified: A malformed JSON body in a POST /period-closes request can bypass the authorization check and reach the handler. — ADR 0064 and the associated doctoring records confirm that malformed bodies are conservatively classified ashard_close_periodfor the authorization step, ensuring a 403 is returned if the user lacks that permission.- Residual risk: Low. The system relies on the 'trusted host identity adapter' to provide the AuthenticatedPrincipal; if the adapter itself is compromised, the application-level authorization is bypassed. This is an external boundary defined in the architecture.
Findings
- No blocking findings.
- Result: APPROVE
- Head SHA:
9cdafb76c0a727943692a1217872618d9251314b - Reviewer credential:
noema-review-github-app-refresh - Actor:
cwl-noema-review[bot]
|
Scheduled review-feedback autofix for this PR head.
|
|
Scheduled review-feedback autofix for this PR head.
|
1 participant
Current exact-head state — 2026-09-03
develop@239008c4edc7d305c97704c5102b593c6622b36f;develop@81a2920b0ae9b054fcdcc95902341e3a5bc809c5and therefore requires a later non-force reconciliation/restack after the dependency root integrates;9cdafb76c0a727943692a1217872618d9251314b;Purpose-bound authorization boundary
This branch adds a host-neutral
AuthenticatedPrincipal, request-scoped trusted-principal resolution, versioned route-to-operation authorization, and tenant-scoped append-only authorization-decision evidence. The trusted host identity adapter remains deployment-owned and must validate signature, issuer, audience, expiry and token binding before AIS receives opaque principal claims. Tenant identity remains distinct from operation authority.Authorization policy
accounting-authorization-v3reserves separate high-impact operationscomplete_reconciliation/accounting.complete_reconciliationandresolve_reconciliation_exception/accounting.resolve_reconciliation_exception. Completion, exception resolution, posting, read, soft/hard-close, tax, outbox, bank-ingest and bank-account-management permissions are non-equivalent; agent/model contexts remain denied high-impact authority by default. This branch reserves policy only and does not consume mutable #47 implementation bytes.Caller-derived identity vocabulary remains bounded before durable audit storage. Correlation evidence intentionally follows the database's 512-character contract rather than an unrelated UTF-8 byte heuristic; the retained Korean multibyte regression proves an identity over 512 bytes but within 512 characters remains valid.
ADR numbering was reconciled so the purpose-bound authorization decision is ADR 0064. RED
4ef1f4226ea3d432048215fa2fe11c976cda4b05requires theCWL purpose-bound authorization contracttraceability row to cite ADR 0064 rather than retired ADR 0055; successor9cdafb76c0a727943692a1217872618d9251314brepairs that documentation contract without changing runtime authorization semantics.Merge boundary
Keep Draft while #29 remains an unmet prerequisite. First integrate #47 -> #43 -> #29 through ordinary protection, then non-destructively restack/reconcile authorization against the exact protected base and canonical docs, and require one unchanged exact head to pass repository/organization CI, real PostgreSQL authorization behavior, exact 100% owned production statement/branch/docstring/edge-case gates, SAST/security/dependency checks, current-head reviews, stack correctness and every applicable protection rule.
Do not merge, auto-merge, bypass protection, force-push, destructively rebase, version, tag or release from this branch while any prerequisite or gate is non-passing.