Skip to content

Refactor: Update error response status from 403 to 401 for internal s…#81

Open
yeomin4242 wants to merge 1 commit into
mainfrom
FIX-143-Refactoring-status-code-for-missing-invalid-X-internal-Secret
Open

Refactor: Update error response status from 403 to 401 for internal s…#81
yeomin4242 wants to merge 1 commit into
mainfrom
FIX-143-Refactoring-status-code-for-missing-invalid-X-internal-Secret

Conversation

@yeomin4242
Copy link
Copy Markdown
Contributor

@yeomin4242 yeomin4242 commented Mar 25, 2026

…ecret validation

목적

변경 요약

관련 이슈

Closes #

Related: #

리뷰 포인트

테스트 결과

  • 로컬 테스트 통과

  • CI 통과

  • 수동 검증 완료

Draft PR 전용 체크리스트

  • 현재 피드백 받고 싶은 범위를 명확히 작성했습니다.

  • 머지 전 남은 작업(TODO)을 정리했습니다.

  • 완료되지 않은 항목/리스크를 본문에 명시했습니다.

일반 PR 전용 체크리스트

  • self-review를 완료했습니다.

  • 머지 전 필수 작업이 모두 완료되었습니다.

  • 필요한 문서(README/API/주석)를 업데이트했습니다.

  • 브레이킹 체인지 여부를 확인했고 필요 시 명시했습니다.

Summary by CodeRabbit

  • Bug Fixes
    • Updated HTTP status code for internal secret authentication failures from 403 to 401 across all protected endpoints.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 25, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 08b9e7d0-2201-4de7-996b-d7a661771c2c

📥 Commits

Reviewing files that changed from the base of the PR and between c0e7f91 and 08062c1.

📒 Files selected for processing (9)
  • contracts/openapi/corebank-service.json
  • corebank-service/src/main/java/com/fix/corebank/config/CorebankOpenApiDocumentationConfig.java
  • corebank-service/src/main/java/com/fix/corebank/security/InternalSecretFilter.java
  • corebank-service/src/test/java/com/fix/corebank/contract/CorebankOpenApiCompatibilityTest.java
  • corebank-service/src/test/java/com/fix/corebank/controller/CorebankInternalApiSkeletonTest.java
  • corebank-service/src/test/java/com/fix/corebank/integration/LedgerIntegrityObservabilityIntegrationTest.java
  • corebank-service/src/test/java/com/fix/corebank/security/InternalSecretFilterTest.java
  • docs/testing/epic-10-acceptance-matrix.md
  • gradle/epic10-acceptance.gradle
📜 Recent review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: ci-fep-simulator
  • GitHub Check: ci-corebank
  • GitHub Check: ci-fep-gateway
  • GitHub Check: ci-channel
  • GitHub Check: ci-quality-gate
🔇 Additional comments (11)
corebank-service/src/main/java/com/fix/corebank/config/CorebankOpenApiDocumentationConfig.java (1)

64-64: OpenAPI auth failure status alignment looks correct.

401 documentation now matches the filter behavior for missing/invalid X-Internal-Secret.

contracts/openapi/corebank-service.json (1)

71-71: Consistent contract migration to 401 across internal routes.

The updated response codes remain aligned with existing error payload and tracing headers.

Also applies to: 163-163, 247-247, 350-350, 453-453, 556-556, 659-659, 762-762, 855-855, 956-956, 1035-1035, 1120-1120, 1218-1218, 1294-1294, 1387-1387, 1480-1480, 1573-1573, 1666-1666, 1750-1750

docs/testing/epic-10-acceptance-matrix.md (1)

14-14: Acceptance matrix scenario text is correctly updated.

E10-008 now matches the new unauthorized status expectation.

corebank-service/src/test/java/com/fix/corebank/controller/CorebankInternalApiSkeletonTest.java (1)

619-619: Test expectation update to 401 is correct.

This keeps the skeleton test consistent with current filter behavior.

gradle/epic10-acceptance.gradle (1)

82-82: Acceptance gate metadata is aligned with the status-code refactor.

Scenario description now correctly references 401.

corebank-service/src/test/java/com/fix/corebank/integration/LedgerIntegrityObservabilityIntegrationTest.java (1)

147-147: Integration assertion change to 401 is correct.

Good alignment with internal-secret validation behavior.

corebank-service/src/test/java/com/fix/corebank/contract/CorebankOpenApiCompatibilityTest.java (1)

291-296: Contract compatibility assertions are correctly migrated to 401.

Schema and tracing-header checks are now attached to the correct response code.

corebank-service/src/main/java/com/fix/corebank/security/InternalSecretFilter.java (1)

44-45: Incorrect: Auth boundary and error code are clearly separated in contracts.

The 403 returned in ChannelErrorContractTest is for error code AUTH-005 (account ownership mismatch), not for missing/invalid X-Internal-Secret. That is a business logic check that occurs after the InternalSecretFilter passes.

Per documented contracts (account-status-error-map.md):

  • Missing/invalid X-Internal-Secret → 401 / AUTH-003 (filter-level rejection)
  • Account ownership mismatch → 403 / AUTH-005 (business logic validation)

The InternalSecretFilter's 401 response is consistent with existing documentation and all downstream service implementations (fep-gateway, fep-simulator). No cross-service contract break exists because the 401 and 403 errors address different authorization boundaries.

			> Likely an incorrect or invalid review comment.
corebank-service/src/test/java/com/fix/corebank/security/InternalSecretFilterTest.java (3)

37-46: LGTM! HTTP 401 is semantically correct for authentication failures.

The change from 403 (Forbidden) to 401 (Unauthorized) is appropriate here. Missing or invalid X-Internal-Secret represents a failed authentication attempt (proving identity), not a failed authorization check (lacking permission after identity is established). The error code "AUTH-003" also aligns well with this being an authentication error.


48-54: LGTM! Method rename and assertion updates are consistent.

The method rename from shouldPreserveProvidedCorrelationIdForForbiddenInternalRoute to shouldPreserveProvidedCorrelationIdForUnauthorizedInternalRoute accurately reflects the new 401 behavior while preserving the important correlation ID propagation verification.


56-60: Test coverage is complete.

The unchanged success-path test complements the updated failure tests well, ensuring both authentication success (valid secret → 200) and failure (missing/invalid secret → 401) scenarios are covered.


📝 Walkthrough

Walkthrough

HTTP status code updated from 403 (Forbidden) to 401 (Unauthorized) for X-Internal-Secret validation failures across OpenAPI contracts, security filter implementation, configuration, tests, and documentation consistently throughout the codebase.

Changes

Cohort / File(s) Summary
API Contract & Configuration
contracts/openapi/corebank-service.json, corebank-service/src/main/java/com/fix/corebank/config/CorebankOpenApiDocumentationConfig.java
Updated documented authorization response codes from 403 to 401 in OpenAPI schema and configured error response status for internal secret authentication failures.
Security Implementation
corebank-service/src/main/java/com/fix/corebank/security/InternalSecretFilter.java
Changed HTTP response status from 403 Forbidden to 401 Unauthorized when X-Internal-Secret header is missing or invalid; renamed helper method from writeForbiddenResponse() to writeUnauthorizedResponse().
Unit & Integration Tests
corebank-service/src/test/java/com/fix/corebank/contract/CorebankOpenApiCompatibilityTest.java, corebank-service/src/test/java/com/fix/corebank/controller/CorebankInternalApiSkeletonTest.java, corebank-service/src/test/java/com/fix/corebank/integration/LedgerIntegrityObservabilityIntegrationTest.java, corebank-service/src/test/java/com/fix/corebank/security/InternalSecretFilterTest.java
Updated test assertions and expectations to verify 401 Unauthorized responses instead of 403 Forbidden for internal-secret validation failures; one test method renamed to reflect updated behavior.
Documentation & Configuration
docs/testing/epic-10-acceptance-matrix.md, gradle/epic10-acceptance.gradle
Updated acceptance scenario metadata and documentation describing authorization denial status from 403 to 401.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested labels

backend

Suggested reviewers

  • kyuoogle

Poem

🐰 From forbidden lands we hop,
Where secrets guard the door,
Now 401 makes status pop—
Unauthorized, no more 403 lore! 🔐✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and accurately summarizes the main change: updating error response status codes from 403 to 401 for internal secret validation across the codebase.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch FIX-143-Refactoring-status-code-for-missing-invalid-X-internal-Secret

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant