Fix/41627 mssql ssl handshake failure#41882
Open
Po11uxx wants to merge 2 commits into
Open
Conversation
MSSQL connections failed with SSL handshake errors when connecting to servers that do not require encryption. The root cause was that the SSL mode defaulted to NO_VERIFY (encrypt=true), forcing a TLS handshake. Changes: - Set default SSL mode to DISABLE (encrypt=false) in form.json - Handle missing SSL config gracefully by defaulting to encrypt=false instead of throwing an exception Users who need SSL can still explicitly select "Enabled with no verify". Fixes appsmithorg#41627 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughThe MSSQL plugin now supports configurable connection timeout with validation during datasource setup and Hikari pool creation. SSL mode defaults gracefully to ChangesMSSQL Connection Timeout and SSL Mode Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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
🧹 Nitpick comments (1)
app/server/appsmith-plugins/mssqlPlugin/src/main/resources/form.json (1)
51-65: ⚡ Quick winConnection timeout configuration looks correct.
The form properly configures a hidden key field with
"connectionTimeout"and a visible NUMBER value field with default60. This matches the backend constant.However, note that the backend's
getConnectionTimeoutSecondsmethod relies on this being at index 0, creating a fragile dependency. See comment in MssqlPlugin.java for details.🤖 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/resources/form.json` around lines 51 - 65, The backend relies on datasourceConfiguration.properties[0] for the connection timeout which is fragile; update the getConnectionTimeoutSeconds method to search datasourceConfiguration.properties for an entry whose key equals "connectionTimeout" (or use a helper that maps properties by key) and parse its value as a number with a fallback to the current default (60), instead of directly reading index 0; ensure the code handles missing/null values and non-numeric values gracefully.
🤖 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 658-672: The getConnectionTimeoutSeconds method currently reads
properties.get(0), which is fragile; change it to search the
datasourceConfiguration.getProperties() list for a Property whose key equals
"connectionTimeout" (use Property.getKey()), then if found and its getValue() is
non-null try to parse its trimmed string value to an int and return it,
otherwise fall back to DEFAULT_CONNECTION_TIMEOUT_SECONDS; keep the existing
NumberFormatException handling and default behavior.
---
Nitpick comments:
In `@app/server/appsmith-plugins/mssqlPlugin/src/main/resources/form.json`:
- Around line 51-65: The backend relies on datasourceConfiguration.properties[0]
for the connection timeout which is fragile; update the
getConnectionTimeoutSeconds method to search datasourceConfiguration.properties
for an entry whose key equals "connectionTimeout" (or use a helper that maps
properties by key) and parse its value as a number with a fallback to the
current default (60), instead of directly reading index 0; ensure the code
handles missing/null values and non-numeric values gracefully.
🪄 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: 30cbb488-2f7a-40f0-be7a-68fd35a36193
📒 Files selected for processing (4)
app/server/appsmith-plugins/mssqlPlugin/src/main/java/com/external/plugins/MssqlPlugin.javaapp/server/appsmith-plugins/mssqlPlugin/src/main/java/com/external/plugins/exceptions/MssqlErrorMessages.javaapp/server/appsmith-plugins/mssqlPlugin/src/main/resources/form.jsonapp/server/appsmith-plugins/mssqlPlugin/src/test/java/com/external/plugins/MssqlPluginTest.java
Comment on lines
+658
to
+672
| static int getConnectionTimeoutSeconds(DatasourceConfiguration datasourceConfiguration) { | ||
| List<Property> properties = datasourceConfiguration.getProperties(); | ||
| if (properties != null && !properties.isEmpty()) { | ||
| Property timeoutProperty = properties.get(0); | ||
| if (timeoutProperty != null && timeoutProperty.getValue() != null) { | ||
| try { | ||
| return Integer.parseInt( | ||
| String.valueOf(timeoutProperty.getValue()).trim()); | ||
| } catch (NumberFormatException e) { | ||
| // fall through to default | ||
| } | ||
| } | ||
| } | ||
| return DEFAULT_CONNECTION_TIMEOUT_SECONDS; | ||
| } |
Contributor
There was a problem hiding this comment.
Hard-coded property index is fragile and error-prone.
The method assumes the connection timeout is always at properties.get(0), but this creates a brittle coupling to the form configuration. If properties are reordered or new properties are added, this will silently fail or read the wrong value.
Instead, search for the property by its key "connectionTimeout" to make the code resilient to configuration changes.
🔍 Proposed fix to search by key
static int getConnectionTimeoutSeconds(DatasourceConfiguration datasourceConfiguration) {
List<Property> properties = datasourceConfiguration.getProperties();
- if (properties != null && !properties.isEmpty()) {
- Property timeoutProperty = properties.get(0);
- if (timeoutProperty != null && timeoutProperty.getValue() != null) {
+ if (properties != null) {
+ Property timeoutProperty = properties.stream()
+ .filter(p -> "connectionTimeout".equals(p.getKey()))
+ .findFirst()
+ .orElse(null);
+ if (timeoutProperty != null && timeoutProperty.getValue() != null) {
try {
return Integer.parseInt(
String.valueOf(timeoutProperty.getValue()).trim());
} catch (NumberFormatException e) {
// fall through to default
}
}
}
return DEFAULT_CONNECTION_TIMEOUT_SECONDS;
}🤖 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 658 - 672, The getConnectionTimeoutSeconds method currently reads
properties.get(0), which is fragile; change it to search the
datasourceConfiguration.getProperties() list for a Property whose key equals
"connectionTimeout" (use Property.getKey()), then if found and its getValue() is
non-null try to parse its trimmed string value to an int and return it,
otherwise fall back to DEFAULT_CONNECTION_TIMEOUT_SECONDS; keep the existing
NumberFormatException handling and default behavior.
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
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
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
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
New Features
Bug Fixes