saml: Avoid NPE when LogoutRequest has no Issuer - #6908
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughSAML logout validation now rejects requests with a missing issuer. A test verifies that an HTTP-POST logout request without an issuer returns ChangesSAML logout validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
|
Tick the box to add this pull request to the merge queue (same as
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
saml/src/test/java/com/linecorp/armeria/server/saml/SamlServiceProviderTest.java (1)
584-597: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReindent the added method to match the project style.
Lines 584-597 use four spaces for the method body. Neighboring methods use eight spaces for the method body and deeper indentation for continued arguments. Reformat this block and run
./gradlew --parallel build.As per path instructions,
site/src/pages/community/developer-guide.mdxrequires code formatted with the LY OSS style.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@saml/src/test/java/com/linecorp/armeria/server/saml/SamlServiceProviderTest.java` around lines 584 - 597, Reindent the added test method in SamlServiceProviderTest so its body uses the project’s eight-space indentation and continued arguments align with neighboring methods. Preserve the existing logout request, response assertion, and method behavior, then run ./gradlew --parallel build to verify formatting and compilation.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@saml/src/test/java/com/linecorp/armeria/server/saml/SamlServiceProviderTest.java`:
- Around line 588-596: Update validateAndGetIdPConfig() to store
logoutRequest.getIssuer() in a local variable and validate it is non-null before
accessing getValue(). Ensure a missing Issuer follows the existing
invalid-request path and returns HTTP 400 instead of throwing
NullPointerException.
---
Nitpick comments:
In
`@saml/src/test/java/com/linecorp/armeria/server/saml/SamlServiceProviderTest.java`:
- Around line 584-597: Reindent the added test method in SamlServiceProviderTest
so its body uses the project’s eight-space indentation and continued arguments
align with neighboring methods. Preserve the existing logout request, response
assertion, and method behavior, then run ./gradlew --parallel build to verify
formatting and compilation.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e6fe748d-8eeb-43c4-8bf8-5adb1afd5a53
📒 Files selected for processing (1)
saml/src/test/java/com/linecorp/armeria/server/saml/SamlServiceProviderTest.java
|
Thanks for the suggestion! I've updated the implementation to check for a missing Issuer before dereferencing it, so malformed logout requests now follow the existing InvalidSamlRequestException path instead of throwing a NullPointerException. I also added a regression test covering this case. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6908 +/- ##
============================================
- Coverage 74.46% 0 -74.47%
============================================
Files 1963 0 -1963
Lines 82437 0 -82437
Branches 10764 0 -10764
============================================
- Hits 61385 0 -61385
+ Misses 15918 0 -15918
+ Partials 5134 0 -5134 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| final LogoutRequest logoutRequest = | ||
| getLogoutRequest("http://" + spHostname + ':' + server.httpPort() + "/saml/slo/post", | ||
| "http://idp.example.com/post"); | ||
|
|
||
| logoutRequest.setIssuer(null); | ||
|
|
||
| final AggregatedHttpResponse res = | ||
| sendViaHttpPostBindingProtocol("/saml/slo/post", | ||
| SAML_REQUEST, | ||
| logoutRequest, | ||
| idpCredential); | ||
|
|
||
| assertThat(res.status()).isEqualTo(HttpStatus.BAD_REQUEST); |
4 participants
Motivation
A
LogoutRequestwithout anIssuercurrently results in an internal server error (500)due to a
NullPointerExceptionduring request processing.According to the SAML protocol, a missing
Issueris an invalid request and should betreated as a client error rather than causing an unexpected server failure.
Modifications
IssuerinLogoutRequestvalidation.InvalidSamlRequestExceptioninstead of allowing aNullPointerExceptionto propagate.LogoutRequestwithout anIssuerreturns
400 Bad Request.Result
Malformed logout requests are now rejected gracefully with
400 Bad Requestinstead of returning
500 Internal Server Error.