Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes issue with Node FTP server #50

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fixes issue with Node FTP server #50

wants to merge 2 commits into from

Conversation

zelms
Copy link

@zelms zelms commented Aug 27, 2024

This pull requests fixes a known issue when attempting to connect to an FTP server hosted using NodeJS with ftp-srv 4.6.3 on Ubuntu 23.10. Resolves issue #49

@ldab
Copy link
Owner

ldab commented Aug 28, 2024

I understand that you had some problems with your server but I doubt the changes on the PR were the reason why it works. The server should wait for a new line to interpret the command, so I don't believe the changes proposed here, even tho harmless, offer any benefit.

Another argument to support what I wrote above is that the password is sent in the same fashion with client.print and it seems it does not impact your setup as it is not included on the PR.

Since reproducing and debugging would take significant effort from my side I suggest to not merge until we find the real reason for the problem. You have the server yourself, so could you check the server logs and see what it shows?

@zelms
Copy link
Author

zelms commented Aug 29, 2024

I definitely agree that it seems like my pull request shouldn't make a difference, but for whatever reason it worked for me. I ran some tests and below are my results. The code for the tests can be found in my repo in the "tests" branch and in the folder named "tests". First, I replicated my issue by using the "upload_image" example sketch found in your repo, and a very basic FTP server. I did modify the example sketch so it only uploaded a text file and didn't list the directory details, although I doubt that has much impact on the error occurring or not.

ESP output

Connecting Wifi...
.
IP address: 192.168.0.211
Connecting to: 192.168.0.197
Command connected
Send USER
FTP error: 501 Must provide username

Send PASSWORD
FTP error: 502 Command not allowed: USERNAME

Send SYST
FTP error: 503 Bad sequence of commands

Send TYPE
FTP error: 503 Bad sequence of commands

Send MKD
FTP error: 503 Bad sequence of commands

Send CWD
FTP error: 503 Bad sequence of commands

Send STOR
FTP error: 503 Bad sequence of commands

Write File
FTP error: 503 Bad sequence of commands

Close File
Connection closed

FTP server output

There's no output from the FTP server when these errors happen

You will notice that when the library attempts to send USER the FTP server returns an error stating that a username must be provided. I'm assuming the FTP server is simply receiving "USER ", and then processing that as the full request as opposed to waiting for the new line. The following error of "Command not allowed: USERNAME" supports this as "USERNAME" (the string "username"/"USERNAME" is the username credential needed to log in) is what should've came after "USER ", but instead the FTP server is assuming it's the next command.

If we apply the related change to send "USER username" as one string, as suggested in my PR, then the output from the ESP looks fairly normal, but the FTP server throws an error relating to an illegal operation on a directory.

ESP output

Connecting Wifi...
.
IP address: 192.168.0.211
Connecting to: 192.168.0.197
Command connected
Send USER
Send PASSWORD
Send SYST
Send TYPE
Type A
Send PASV
Data port: 1025
Data connection established
Send MKD
Send CWD
Send STOR
Write File
Close File
Connection closed

FTP server output

{"name":"ftp-srv","hostname":"zach","pid":69318,"id":"895cee70-4bfd-4129-bc7d-4567df558a02","ip":"192.168.0.211","directive":"STOR","level":50,"err":{"message":"EISDIR: illegal operation on a directory, open '/home/zach/ftp/my_new_dir'","name":"Error","stack":"Error: EISDIR: illegal operation on a directory, open '/home/zach/ftp/my_new_dir'","code":"EISDIR"},"msg":"EISDIR: illegal operation on a directory, open '/home/zach/ftp/my_new_dir'","time":"2024-08-28T23:15:08.930Z","v":0}

No error on the ESP side, but somethings not right with the FTP server. I've been able to replicate this error by only sending the STOR command, so I'm assuming the FTP server is receiving "STOR ", processing that, and then receiving what ever the file name is after that. Like before, this error goes away when you send STOR and the file name in the same string.

Curiously, while testing I did end up having issues with how the password is sent. I made the same change I did with the username and added it to the PR.

It is possible that this is an issue with ftp-srv, although I've had many different ftp clients access the ftp server without issue, including a GPRS one running off of the same ESP32 that I'm running the test code on. I also haven't seen any issues relating to this on ftp-srv's github page. I solved this for one of my projects anyways, so I figured I'd also share it here in case others are having a similar issue. If you decide not to accept this pull request, then I will update the original issue I started with the information above so that a solution to this issue is still easily findable by others. It's definitely a weird bug that seems like it shouldn't exist.

@ldab
Copy link
Owner

ldab commented Aug 29, 2024

EISDIR is a directory error, meaning that you should use STOR with the file name, from the logs you are using with the directory, which is incorrect.

@zelms
Copy link
Author

zelms commented Aug 30, 2024

I did use STOR with a file name. Like I mentioned, the code I'm using to test the library is your example code to upload an image, which I'd assume is using the STOR command correctly. The issue is with the library sending "STOR " and the file name in two different print commands. When modified to send "STOR filename.txt" (as I've done in this PR), then the error with the FTP server goes away. The only change made between when I get the EISDIR error and when I don't get it is if the STOR command is sent as two strings or one. I understand it shouldn't make a difference, but the tests above show that it does.

@ldab
Copy link
Owner

ldab commented Aug 30, 2024

I'm not convinced that this changes are needed, since several IP libraries, HTTP for example sends the command in the same fashion, and HTTP is over TCP the same as FTP, so it should be no difference.

If you want I can try to replicate, can you please share your server code? Preferable a minimal example

@zelms
Copy link
Author

zelms commented Aug 30, 2024

Here is my server code, it is from the example listed in the ftp-srv repo with a couple modifications.

// Quick start, create an active ftp server.
const FtpSrv = require('ftp-srv');

const port=21;
const ftpServer = new FtpSrv({
    url: "ftp://0.0.0.0:" + port,
    anonymous: true,
    pasv_url: "192.168.0.197",
});

ftpServer.on('login', ({ connection, username, password }, resolve, reject) => { 
    if(username === 'username' && password === 'password'){
        return resolve({ root:"/home/zelms/ftp" });    
    }
    return reject(new errors.GeneralError('Invalid username or password', 401));
});

ftpServer.listen().then(() => { 
    console.log('Ftp server is starting...')
});

Here is my package.json.

{
  "name": "tests",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "",
  "license": "ISC",
  "dependencies": {
    "ftp-srv": "^4.6.3"
  }
}

A quick npm install and then node server.js should get it up and going.

@ldab
Copy link
Owner

ldab commented Aug 30, 2024

I'm getting access denied on port 21 and if I try something else I can't connect, maybe my firewall is messing up, will try more later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants