fix: default MSSQL SSL mode to disable and handle missing SSL config (#41627)#41818
Open
xiyaoeva wants to merge 1 commit into
Open
fix: default MSSQL SSL mode to disable and handle missing SSL config (#41627)#41818xiyaoeva wants to merge 1 commit into
xiyaoeva wants to merge 1 commit into
Conversation
Contributor
WalkthroughThe MSSQL plugin's SSL configuration handling is updated to default gracefully when configuration is absent. The Java code now appends ChangesMSSQL SSL Configuration Default
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@app/server/appsmith-plugins/mssqlPlugin/src/main/java/com/external/plugins/MssqlPlugin.java`:
- Around line 649-653: The code currently treats a missing ssl.authType the same
as a missing SSL block and silently appends "encrypt=false" to urlBuilder;
instead, only default to "encrypt=false" when the entire SSL block is absent
(datasourceConfiguration.getConnection() == null || getSsl() == null). If
getSsl() is present but getAuthType() is null, do not overwrite to
plaintext—either throw an IllegalArgumentException or log an explicit error and
abort building the URL so callers can correct the malformed/partial config;
update the logic around datasourceConfiguration.getConnection(),
datasourceConfiguration.getConnection().getSsl(),
datasourceConfiguration.getConnection().getSsl().getAuthType(), and the
urlBuilder usage accordingly so partially populated SSL configs are rejected
rather than silently downgraded.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9eaa8390-4d32-4e51-b44c-7f9e76f135b1
📒 Files selected for processing (2)
app/server/appsmith-plugins/mssqlPlugin/src/main/java/com/external/plugins/MssqlPlugin.javaapp/server/appsmith-plugins/mssqlPlugin/src/main/resources/form.json
Comment on lines
649
to
+653
| if (datasourceConfiguration.getConnection() == null | ||
| || datasourceConfiguration.getConnection().getSsl() == null | ||
| || datasourceConfiguration.getConnection().getSsl().getAuthType() == null) { | ||
| throw new AppsmithPluginException( | ||
| AppsmithPluginError.PLUGIN_ERROR, | ||
| "Appsmith server has failed to fetch SSL configuration from datasource configuration form. " | ||
| + "Please reach out to Appsmith customer support to resolve this."); | ||
| urlBuilder.append("encrypt=false;"); | ||
| return; |
Contributor
There was a problem hiding this comment.
Don't silently downgrade partially populated SSL configs.
Defaulting to encrypt=false when the entire SSL block is absent makes sense here, but doing the same for ssl.authType == null can turn a malformed or partially migrated secure config into plaintext without any signal.
Suggested adjustment
- if (datasourceConfiguration.getConnection() == null
- || datasourceConfiguration.getConnection().getSsl() == null
- || datasourceConfiguration.getConnection().getSsl().getAuthType() == null) {
+ if (datasourceConfiguration.getConnection() == null
+ || datasourceConfiguration.getConnection().getSsl() == null) {
urlBuilder.append("encrypt=false;");
return;
}
+ if (datasourceConfiguration.getConnection().getSsl().getAuthType() == null) {
+ throw new AppsmithPluginException(
+ AppsmithPluginError.PLUGIN_ERROR,
+ "Missing MSSQL SSL mode in datasource configuration.");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (datasourceConfiguration.getConnection() == null | |
| || datasourceConfiguration.getConnection().getSsl() == null | |
| || datasourceConfiguration.getConnection().getSsl().getAuthType() == null) { | |
| throw new AppsmithPluginException( | |
| AppsmithPluginError.PLUGIN_ERROR, | |
| "Appsmith server has failed to fetch SSL configuration from datasource configuration form. " | |
| + "Please reach out to Appsmith customer support to resolve this."); | |
| urlBuilder.append("encrypt=false;"); | |
| return; | |
| if (datasourceConfiguration.getConnection() == null | |
| || datasourceConfiguration.getConnection().getSsl() == null) { | |
| urlBuilder.append("encrypt=false;"); | |
| return; | |
| } | |
| if (datasourceConfiguration.getConnection().getSsl().getAuthType() == null) { | |
| throw new AppsmithPluginException( | |
| AppsmithPluginError.PLUGIN_ERROR, | |
| "Missing MSSQL SSL mode in datasource configuration."); | |
| } |
🤖 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
`@app/server/appsmith-plugins/mssqlPlugin/src/main/java/com/external/plugins/MssqlPlugin.java`
around lines 649 - 653, The code currently treats a missing ssl.authType the
same as a missing SSL block and silently appends "encrypt=false" to urlBuilder;
instead, only default to "encrypt=false" when the entire SSL block is absent
(datasourceConfiguration.getConnection() == null || getSsl() == null). If
getSsl() is present but getAuthType() is null, do not overwrite to
plaintext—either throw an IllegalArgumentException or log an explicit error and
abort building the URL so callers can correct the malformed/partial config;
update the logic around datasourceConfiguration.getConnection(),
datasourceConfiguration.getConnection().getSsl(),
datasourceConfiguration.getConnection().getSsl().getAuthType(), and the
urlBuilder usage accordingly so partially populated SSL configs are rejected
rather than silently downgraded.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
TL;DR
MSSQL datasource now defaults SSL mode to
DISABLEinstead ofNO_VERIFY, and missing SSL config no longer throws an exception; it falls back toencrypt=false;so non-encrypted servers can connect successfully.Motivation / Context
When connecting to MSSQL servers that do not require encryption, connection tests could fail due to SSL handshake errors.
Root causes:
NO_VERIFY(which setsencrypt=true).This caused avoidable failures for common MSSQL setups where encryption is not enforced.
Changes
app/server/appsmith-plugins/mssqlPlugin/src/main/resources/form.json"initialValue": "NO_VERIFY"->"initialValue": "DISABLE"app/server/appsmith-plugins/mssqlPlugin/src/main/java/com/external/plugins/MssqlPlugin.javaencrypt=false;return;Why this is safe
Dependencies
None.
Related docs/design links
None.
Fixes #41627
Automation
/ok-to-test tags=""
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit