Repository navigation
Improve FTP/FTPS/SFTP configuration and access controls - #232
Merged
Merged
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing: 88
✨ Finishing Touches
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 |
erikraft
changed the title
erikraft
marked this pull request as ready for review
October 16, 2026 20:39
Strix Security Review1 open security finding on this PR:
Review summaryThis PR improves FTP/FTPS/SFTP configuration (FTPS mode selection, read-only enforcement, network-state display, QR codes, path validation, localization) but introduces a security regression in the default server configuration. The diff changes the default FTP/FTPS/SFTP credentials from Fixed the findings? re-run the review, or tag Updated for Reviewed by Strix |
![strix-security[bot]](https://avatars.githubusercontent.com/in/2749804?s=60&v=4){kind=small}
Comment on lines
+102
to
+107
| String username = valueOrDefault(prefs.getString(getString(R.string.pref_ftp_username), ""), "android"); | ||
| String password = valueOrDefault(prefs.getString(getString(R.string.pref_ftp_password), ""), "android"); | ||
| boolean anonymous = prefs.getBoolean(getString(R.string.pref_ftp_anonymous), true); | ||
| boolean ftps = prefs.getBoolean(getString(R.string.pref_ftp_ftps), false); | ||
| boolean implicitFtps = prefs.getBoolean(getString(R.string.pref_ftp_ftps_implicit), false); | ||
| boolean readOnly = prefs.getBoolean(getString(R.string.pref_ftp_read_only), false); |
There was a problem hiding this comment.
🟠 Insecure FTP/FTPS/SFTP default configuration: guessable "android"/"android" credentials and TLS disabled by default expose device storage on the local network
Severity: HIGH · CWE-798
PR #232 changes the default FTP/FTPS/SFTP credentials from admin/erikraft to trivially guessable android/android, and flips the FTPS (TLS) default from true to false. Because the FTP and SFTP servers bind to all local interfaces, default to writable access (readOnly=false), and serve a directory of the device's storage, any device on the same local network can authenticate with the well-known android/android credential and read or write the exposed directory. For FTP the credential and all transferred data are now transmitted in cleartext. This is a security regression versus the prior defaults.
Suggested change
| String username = valueOrDefault(prefs.getString(getString(R.string.pref_ftp_username), ""), "android"); | |
| String password = valueOrDefault(prefs.getString(getString(R.string.pref_ftp_password), ""), "android"); | |
| boolean anonymous = prefs.getBoolean(getString(R.string.pref_ftp_anonymous), true); | |
| boolean ftps = prefs.getBoolean(getString(R.string.pref_ftp_ftps), false); | |
| boolean implicitFtps = prefs.getBoolean(getString(R.string.pref_ftp_ftps_implicit), false); | |
| boolean readOnly = prefs.getBoolean(getString(R.string.pref_ftp_read_only), false); | |
| String username = valueOrDefault(prefs.getString(getString(R.string.pref_ftp_username), ""), "android"); | |
| String password = valueOrDefault(prefs.getString(getString(R.string.pref_ftp_password), ""), ""); | |
| if (password.isEmpty()) { | |
| throw new IllegalStateException("A senha do servidor FTP deve ser definida nas configurações."); | |
| } | |
| boolean anonymous = prefs.getBoolean(getString(R.string.pref_ftp_anonymous), false); | |
| boolean ftps = prefs.getBoolean(getString(R.string.pref_ftp_ftps), true); | |
| boolean implicitFtps = prefs.getBoolean(getString(R.string.pref_ftp_ftps_implicit), false); | |
| boolean readOnly = prefs.getBoolean(getString(R.string.pref_ftp_read_only), false); |
Prompt to fix with AI
This is a security vulnerability found during a code review.
Vulnerability: Insecure FTP/FTPS/SFTP default configuration: guessable "android"/"android" credentials and TLS disabled by default expose device storage on the local network
Severity: HIGH
CWE: CWE-798
PR #232 changes the default FTP/FTPS/SFTP credentials from `admin`/`erikraft` to trivially guessable `android`/`android`, and flips the FTPS (TLS) default from `true` to `false`. Because the FTP and SFTP servers bind to all local interfaces, default to writable access (`readOnly=false`), and serve a directory of the device's storage, any device on the same local network can authenticate with the well-known `android`/`android` credential and read or write the exposed directory. For FTP the credential and all transferred data are now transmitted in cleartext. This is a security regression versus the prior defaults.
Location: app/src/main/java/com/erikraft/drop/FtpServerService.java:102-107
Context: FTP service insecure defaults (credential, TLS off, anonymous on)
```
// Before:
String username = valueOrDefault(prefs.getString(getString(R.string.pref_ftp_username), ""), "android");
String password = valueOrDefault(prefs.getString(getString(R.string.pref_ftp_password), ""), "android");
boolean anonymous = prefs.getBoolean(getString(R.string.pref_ftp_anonymous), true);
boolean ftps = prefs.getBoolean(getString(R.string.pref_ftp_ftps), false);
boolean implicitFtps = prefs.getBoolean(getString(R.string.pref_ftp_ftps_implicit), false);
boolean readOnly = prefs.getBoolean(getString(R.string.pref_ftp_read_only), false);
// After:
String username = valueOrDefault(prefs.getString(getString(R.string.pref_ftp_username), ""), "android");
String password = valueOrDefault(prefs.getString(getString(R.string.pref_ftp_password), ""), "");
if (password.isEmpty()) {
throw new IllegalStateException("A senha do servidor FTP deve ser definida nas configurações.");
}
boolean anonymous = prefs.getBoolean(getString(R.string.pref_ftp_anonymous), false);
boolean ftps = prefs.getBoolean(getString(R.string.pref_ftp_ftps), true);
boolean implicitFtps = prefs.getBoolean(getString(R.string.pref_ftp_ftps_implicit), false);
boolean readOnly = prefs.getBoolean(getString(R.string.pref_ftp_read_only), false);
```
Location: app/src/main/java/com/erikraft/drop/SftpServerService.java:105-110
Context: SFTP service insecure default credential
```
// Before:
String username = valueOrDefault(
prefs.getString(getString(R.string.pref_sftp_username), ""), "android");
String password = valueOrDefault(
prefs.getString(getString(R.string.pref_sftp_password), ""), "android");
String configuredHome = prefs.getString(getString(R.string.pref_sftp_save_location), "");
boolean readOnly = prefs.getBoolean(getString(R.string.pref_sftp_read_only), false);
// After:
String username = valueOrDefault(
prefs.getString(getString(R.string.pref_sftp_username), ""), "android");
String password = valueOrDefault(
prefs.getString(getString(R.string.pref_sftp_password), ""), "");
if (password.isEmpty()) {
throw new IllegalStateException("A senha do servidor SFTP deve ser definida nas configurações.");
}
String configuredHome = prefs.getString(getString(R.string.pref_sftp_save_location), "");
boolean readOnly = prefs.getBoolean(getString(R.string.pref_sftp_read_only), false);
```
How to fix:
Restore secure defaults by requiring the user to set a non-default password (or generate a strong random credential) before the server can start, keeping FTPS/TLS enabled by default (`true`), and keeping anonymous login disabled by default (`false`). Prefer not to ship any hard-coded fallback credential; if a fallback must exist, generate a per-installation random token rather than a shared static value. Display the current credential to the user through the existing QR code / settings UI so it remains discoverable without weakening security.
Please fix this vulnerability. If you propose a fix, make it concise and minimal.React 👍 / 👎 to tune Strix for this repo. A repo collaborator (or the PR author) can resolve this thread to dismiss the finding.
Comment on lines
105
to
+110
| String username = valueOrDefault( | ||
| prefs.getString(getString(R.string.pref_sftp_username), ""), "admin"); | ||
| prefs.getString(getString(R.string.pref_sftp_username), ""), "android"); | ||
| String password = valueOrDefault( | ||
| prefs.getString(getString(R.string.pref_sftp_password), ""), "admin"); | ||
| prefs.getString(getString(R.string.pref_sftp_password), ""), "android"); | ||
| String configuredHome = prefs.getString(getString(R.string.pref_sftp_save_location), ""); | ||
| boolean readOnly = prefs.getBoolean(getString(R.string.pref_sftp_read_only), false); |
There was a problem hiding this comment.
SFTP service insecure default credential
Suggested change
| String username = valueOrDefault( | |
| prefs.getString(getString(R.string.pref_sftp_username), ""), "admin"); | |
| prefs.getString(getString(R.string.pref_sftp_username), ""), "android"); | |
| String password = valueOrDefault( | |
| prefs.getString(getString(R.string.pref_sftp_password), ""), "admin"); | |
| prefs.getString(getString(R.string.pref_sftp_password), ""), "android"); | |
| String configuredHome = prefs.getString(getString(R.string.pref_sftp_save_location), ""); | |
| boolean readOnly = prefs.getBoolean(getString(R.string.pref_sftp_read_only), false); | |
| String username = valueOrDefault( | |
| prefs.getString(getString(R.string.pref_sftp_username), ""), "android"); | |
| String password = valueOrDefault( | |
| prefs.getString(getString(R.string.pref_sftp_password), ""), ""); | |
| if (password.isEmpty()) { | |
| throw new IllegalStateException("A senha do servidor SFTP deve ser definida nas configurações."); | |
| } | |
| String configuredHome = prefs.getString(getString(R.string.pref_sftp_save_location), ""); | |
| boolean readOnly = prefs.getBoolean(getString(R.string.pref_sftp_read_only), false); |
Owner
Author
|
The Strix HIGH finding is being addressed in a separate security PR: #234. It migrates insecure legacy defaults, enables FTPS by default, disables anonymous FTP by default, generates per-install random passwords when legacy credentials are weak, and rejects weak credentials at both FTP/FTPS and SFTP server boundaries. PR #234 is currently Draft while CI and manual Android validation are pending. |
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.
Summary
android, keep existing saved preferences authoritative, and show a QR code for the connection URI.About screen — ErikrafT Wi-Fi™
Internal storage / SD card — still blocking
The picker currently uses the Storage Access Framework (SAF), but the FTP and SFTP backends still require conventional filesystem paths. Converting a tree URI to a guessed
/storage/...path is not a valid access strategy under scoped storage.To complete this requirement, the server implementations need URI-backed filesystem adapters:
FileSystemFactory/FileSystemView/FtpFilecontract using Android document URIs andContentResolverstreams.This is intentionally not marked as complete yet. Internal storage and removable SD-card trees must both be verified with actual uploads, downloads, directory listing, rename/delete where allowed, and read-only rejection.
CI correction
The previous CI run (
38957085550) failed to compile because Apache FtpServer 1.2.1 does not provideorg.apache.ftpserver.usermanager.impl.ReadPermission. The latest commit removes that invalid import and leaves the authority list withoutWritePermissionwhen read-only mode is enabled. The new commit still needs a fresh CI result.Validation
Proposed Android version
v10.1.9(versionCode 32) tov10.1.10(versionCode 33).ReleaseContractTestto enforce the new version name/code pair.