Repository navigation
fix: handle LinkageError in createCredentials and updateSubmit for invalid certificate credentials - #1060
Conversation
|
Pushed a follow-up commit that resolves the TODO I noticed in |
|
@timja Hey just following up on the status of this PR. Thank You!! |
There was a problem hiding this comment.
Pull request overview
This pull request improves user-facing error handling for invalid certificate credentials created/updated via CredentialsStoreAction endpoints by catching Stapler-wrapped LinkageError and returning JSON error responses (matching the established pattern used for the add-credentials popup flow).
Changes:
- Catch
LinkageErrorindoCreateCredentialsanddoUpdateSubmit, unwrap root causes, and return JSONnotificationType: ERRORforAccept: application/jsonrequests. - Add logging for these failures to avoid raw/unhandled stack traces.
- Add a new test covering invalid PEM certificate creation via the
/createCredentialsJSON form endpoint.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/main/java/com/cloudbees/plugins/credentials/CredentialsStoreAction.java |
Adds LinkageError handling + root-cause extraction to return JSON errors for invalid credential submissions. |
src/test/java/com/cloudbees/plugins/credentials/CredentialsStoreActionTest.java |
Adds a regression test asserting JSON error response when creating invalid PEM certificate credentials. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7bf1669 to
f621a95
Compare
Head branch was pushed to by a user without write access
f621a95 to
40f8e94
Compare
Invalid PEM data in CertificateCredentialsImpl makes Stapler's Descriptor.bindJSON wrap the constructor failure in a LinkageError. DomainWrapper.doCreateCredentials and CredentialsWrapper.doUpdateSubmit did not catch it, so JSON clients got a 500 stack trace instead of a user-facing error. Catch LinkageError in both handlers, unwrap root causes (IOException, IllegalArgumentException, GeneralSecurityException), and return notificationType ERROR for application/json requests. Same pattern as the popup add-credentials path in CredentialsStoreAction. When addCredentials returns false, check whether the domain still exists before reporting a duplicate credential ID. Signed-off-by: Lohit Kolluri <lohitkolluri@gmail.com>
Head branch was pushed to by a user without write access
40f8e94 to
7be6285
Compare
|
Hey! It looks like auto-merge was disabled after I updated the branch with the latest changes. |
Closes #971
Root Cause
When adding or updating CertificateCredentialsImpl with invalid PEM data, the constructor throws IllegalArgumentException. Stapler's
Descriptor.bindJSONwraps this in aLinkageError, which was unhandled inDomainWrapper.doCreateCredentials()andCredentialsWrapper.doUpdateSubmit(), resulting in a raw stack trace instead of a user-facing error message.Fix
Added
LinkageErrorcatch blocks to both methods, extracting the root cause viaExceptionUtils.getRootCauseand returning a JSON error response forapplication/jsonrequests. This matches the exact pattern established in PR #580 for the popup add-credentials path.Testing
createCredentialsWithInvalidPEMCertificateReturnsJsonError— POSTs invalid PEM CertificateCredentialsImpl JSON to/credentials/store/system/domain/_/createCredentialsand asserts 200 response withnotificationType: ERRORRisks
LinkageErrorwhere root cause isIOException,IllegalArgumentException, orGeneralSecurityException. All otherLinkageErrors are rethrown.DCO
Signed-off-by: Lohit Kolluri lohitkolluri@gmail.com