ftp server: call logout callback on client timeout for consistency#372
Conversation
Otherwise this is not symmetric with the other case where logout is also called when the client disconnects. Refactor performing the logout into a separate helper to avoid duplication.
vb-linetco
force-pushed
the
feat/ftp-server-logout-callback-on-inactivity-timeout
branch
from
1d93c06 to
dcf5a11
Compare
|
Thank you for this contribution @vb-linetco. I will review this soon. |
| /* 6.4.3 */ | ||
| /* AUTHOR */ | ||
| /* */ | ||
| /* TBD */ |
There was a problem hiding this comment.
@fdesbiens Since I've only moved this code, I was not sure which Autor to put here. What is the policy about this?
There was a problem hiding this comment.
@vb-linetco My apologies for the late reply. AUTHOR should be the original author. In this case, you could have both the original and yours since you initiated the refactoring.
fdesbiens
left a comment
fdesbiens
left a comment
There was a problem hiding this comment.
Thanks for this contribution, @vb-linetco.
I requested some small changes. Moreover, you did not add regression tests for the new timeout behaviour. Please add one that logs in, lets the FTP activity timeout fire, asserts the logout callback count increments, then reconnects and verifies commands are rejected until a fresh login occurs.
I look forward to merging this useful improvement.
| client_req_ptr -> nx_ftp_client_request_block_bytes = 0; | ||
|
|
||
| /* Logout the client. */ | ||
| _nx_ftp_server_logout_client(ftp_server_ptr, client_req_ptr); |
There was a problem hiding this comment.
You log out due to an inactivity timeout, but leave nx_ftp_client_request_authenticated set. The timed-out request is then disconnected and relistened, while _nx_ftp_server_connect_process does not reset authentication for the next accepted client. Since command processing only blocks unauthenticated clients at addons/ftp/nxd_ftp_server.c:1553, the next connection can inherit authenticated state and issue commands without USER/PASS. The timeout path should clear authentication as the disconnect path does. See _nx_ftp_server_control_disconnect_processing for more details.
| if (client_req_ptr -> nx_ftp_client_request_login) | ||
| { | ||
|
|
||
| /* Call the logout function. */ |
There was a problem hiding this comment.
There is some trailing whitespace here and in the comment block for your new helper. Please make sure to clean that up. Thanks!
- Fix security bug: stale authenticated flag after inactivity timeout.
Moving nx_ftp_client_request_authenticated = NX_FALSE into the
_nx_ftp_server_logout_client helper ensures every call site (QUIT,
disconnect, and now timeout) consistently resets auth state. Before
this fix, a new TCP connection accepted on a timed-out socket slot
could issue commands without USER/PASS.
- Remove the now-redundant per-callsite authenticated resets in the QUIT
handler and the control-disconnect processing path.
- Fix AUTHOR field in _nx_ftp_server_logout_client (was 'TBD').
- Remove trailing whitespace from the new function comment block and
from the blank line following the QUIT logout call.
- Add regression test netx_ftp_server_activity_timeout_test that:
* forces the inactivity timer to fire,
* asserts the logout callback is invoked,
* uses a raw TCP connection to verify commands are rejected with 530
before USER/PASS (demonstrating the auth-reset fix), and
* confirms a fresh authenticated session works normally.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
I edited the PR to implement my own feedback. Merging to dev now. This will ship with the Q2 2026 release. |
|
Thank you for addressing the feedback, I was planning to do it today 👍 |
2 participants
Otherwise this is not symmetric with the other case where logout is also called when the client disconnects.
Refactor performing the logout into a separate helper to avoid duplication.