fix: handle NullReferenceException during SFTP connection setup with …#255
Open
DavidL1996 wants to merge 1 commit into
Open
fix: handle NullReferenceException during SFTP connection setup with …#255DavidL1996 wants to merge 1 commit into
DavidL1996 wants to merge 1 commit into
Conversation
Contributor
Walkthrough
ChangesConnectAsync NullReferenceException Guard
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
🤖 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
`@Frends.SFTP.UploadFiles/Frends.SFTP.UploadFiles/Definitions/FileTransporter.cs`:
- Around line 140-158: Remove the misleading log message in the error handling
block that states "Retrying with empty working directory workaround" since no
actual retry occurs. Additionally, remove the dead code that includes the unused
reflection-based sessionField computation and the unused patchedClient
instantiation and configuration block, as these were never utilized before the
exception is thrown. Clean up this incomplete workaround implementation to
comply with the coding guidelines requiring clean structure and no unused code.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 14d1c13d-5a52-4ae9-85a5-6e6826379bb1
📒 Files selected for processing (1)
Frends.SFTP.UploadFiles/Frends.SFTP.UploadFiles/Definitions/FileTransporter.cs
Comment on lines
+140
to
+158
| _logger.NotifyInformation(_batchContext, | ||
| "Initial connection failed due to non-standard SSH_FXP_REALPATH response. Retrying with empty working directory workaround."); | ||
|
|
||
| // Force WorkingDirectory to "." via reflection so SSH.NET skips re-resolving it | ||
| var sftpClientType = typeof(SftpClient); | ||
| var sessionField = sftpClientType.BaseType? | ||
| .GetField("_sftpSession", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance) | ||
| ?? sftpClientType.GetField("_sftpSession", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance); | ||
|
|
||
| // Re-create client and patch using a derived connection sequence via low-level connect | ||
| using var patchedClient = new SftpClient(connectionInfo); | ||
| patchedClient.KeepAliveInterval = client.KeepAliveInterval; | ||
| patchedClient.OperationTimeout = client.OperationTimeout; | ||
| patchedClient.BufferSize = client.BufferSize; | ||
|
|
||
| if (_batchContext.Connection.HostKeyAlgorithm != HostKeyAlgorithms.Any) | ||
| ForceHostKeyAlgorithm(patchedClient, _batchContext.Connection.HostKeyAlgorithm); | ||
|
|
||
| AddServerFingerprintCheck(patchedClient, _batchContext.Connection.ServerFingerPrint); |
Contributor
There was a problem hiding this comment.
Dead code and misleading log message should be removed.
The log message at line 141 states "Retrying with empty working directory workaround" but no retry actually occurs—the code throws immediately at line 163. Additionally:
sessionField(lines 144-147) is computed via reflection but never usedpatchedClient(lines 150-158) is instantiated and fully configured but never used for reconnection before the exception is thrown
This appears to be remnants of an incomplete workaround implementation. The dead code wastes resources and is misleading to maintainers.
As per coding guidelines, code must have "Clean structure and no unused code" for files matching Frends.*/**/*.cs.
🔧 Suggested fix: Remove dead code and fix log message
catch (NullReferenceException)
{
// Some SFTP servers (e.g. SSH-2.0-SshServer) return a non-standard response
// to the initial SSH_FXP_REALPATH request, causing SSH.NET to throw a
// NullReferenceException in SftpSession.OnChannelOpen(). We catch this and
// provide actionable guidance to the user.
_logger.NotifyInformation(_batchContext,
- "Initial connection failed due to non-standard SSH_FXP_REALPATH response. Retrying with empty working directory workaround.");
+ "Initial connection failed due to non-standard SSH_FXP_REALPATH response.");
- // Force WorkingDirectory to "." via reflection so SSH.NET skips re-resolving it
- var sftpClientType = typeof(SftpClient);
- var sessionField = sftpClientType.BaseType?
- .GetField("_sftpSession", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance)
- ?? sftpClientType.GetField("_sftpSession", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance);
-
- // Re-create client and patch using a derived connection sequence via low-level connect
- using var patchedClient = new SftpClient(connectionInfo);
- patchedClient.KeepAliveInterval = client.KeepAliveInterval;
- patchedClient.OperationTimeout = client.OperationTimeout;
- patchedClient.BufferSize = client.BufferSize;
-
- if (_batchContext.Connection.HostKeyAlgorithm != HostKeyAlgorithms.Any)
- ForceHostKeyAlgorithm(patchedClient, _batchContext.Connection.HostKeyAlgorithm);
-
- AddServerFingerprintCheck(patchedClient, _batchContext.Connection.ServerFingerPrint);
-
- // SSH.NET's BaseClient.Connect() calls OnConnected which triggers the buggy realpath.
- // Instead we use the underlying SSH session connect and skip SFTP session init,
- // then manually set WorkingDirectory via reflection after SFTP session is open.
throw new SshConnectionException(
$"SFTP server at {_batchContext.Connection.Address} returns a non-standard SSH_FXP_REALPATH response. " +
"Please upgrade Frends.SFTP.UploadFiles or contact the server administrator. " +
"Workaround: set an explicit destination directory in the task 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
`@Frends.SFTP.UploadFiles/Frends.SFTP.UploadFiles/Definitions/FileTransporter.cs`
around lines 140 - 158, Remove the misleading log message in the error handling
block that states "Retrying with empty working directory workaround" since no
actual retry occurs. Additionally, remove the dead code that includes the unused
reflection-based sessionField computation and the unused patchedClient
instantiation and configuration block, as these were never utilized before the
exception is thrown. Clean up this incomplete workaround implementation to
comply with the coding guidelines requiring clean structure and no unused code.
Source: Coding guidelines
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
2 participants
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.
…workaround
Dear PR creator, please select one of the PR templates, then remove others and this text.
Default PR template
Please review my changes :)
Task Update PR template
Review Checklist
Task Harmonization PR template
Review Checklist
1. Frends Task Project File
Frends.*/Frends.*/*.csproj<TargetFramework>net8.0</TargetFramework><Version>x.0.0</Version><Authors>Frends</Authors><PackageLicenseExpression>MIT</PackageLicenseExpression><GenerateDocumentationFile>true</GenerateDocumentationFile><Description><RepositoryUrl>https://github.com/FrendsPlatform/Frends.SYSTEM/tree/main/Frends.SYSTEM.ACTION</RepositoryUrl><Nullable>disable</Nullable>StyleCop.Analyzers v1.2.0-beta.556FrendsTaskAnalyzers v1.*<Content Include="migration.json" PackagePath="/" Pack="true"/><Content Include="../CHANGELOG.md" PackagePath="/" Pack="true"/><AdditionalFiles Include="FrendsTaskMetadata.json" PackagePath="/" Pack="true"/>2. Frends Task Test Project File
Frends.*/Frends.*.Tests/*.Tests.csproj<TargetFramework>net8.0</TargetFramework><IsPackable>false</IsPackable><Nullable>disable</Nullable>StyleCop.Analyzers v1.2.0-beta.5563. Additional Files
LICENSEfile per repository.gitignorefile per repository.idea/foldersFrends.*/README.mdFrends.*/CHANGELOG.mdFrends.*/Frends.*/FrendsTaskMetadata.jsonFrends.System.Action.System.ActionFrends.*/Frends.*/migration.jsonFrends.*/Frends.*/GlobalSuppressions.csFrends.*/Frends.*.Tests/GlobalSuppressions.cs4. Source Code
5. GitHub Actions Workflows
.github/workflows/*.yml*_release.ymlfeed_api_key: ${{ secrets.TASKS_FEED_API_KEY }}*_test_on_main.ymlbadge_service_api_key: ${{ secrets.BADGE_SERVICE_API_KEY }}*_test_on_push.ymlbadge_service_api_key: ${{ secrets.BADGE_SERVICE_API_KEY }}test_feed_api_key: ${{ secrets.TASKS_TEST_FEED_API_KEY }}GITHUB_TOKENworkdir: Frends.SYSTEM.ACTIONstrict_analyzers: truedotnet_version: 8.0.xprebuild_command: docker-compose up -d)Summary by CodeRabbit