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

Add functionality to write to a temp file through url arguments #747

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ OPTIONS:
-g, --gid Group id to run with
-s, --signal Signal to send to the command when exit it (default: 1, SIGHUP)
-a, --url-arg Allow client to send command line arguments in URL (eg: http://localhost:7681?arg=foo&arg=bar)
-f, --arg-file File prefix for a unique generated temp file that URL arguments are written to (ex. /tmp/prefix); the generated file's full path is then passed in as a command line argument (ex. /tmp/prefix{unique string})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
-f, --arg-file File prefix for a unique generated temp file that URL arguments are written to (ex. /tmp/prefix); the generated file's full path is then passed in as a command line argument (ex. /tmp/prefix{unique string})
-f, --arg-file Similar to --url-arg but the command line argument is a file containing new line separated values

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this would make it clear that the argument file prefix needs to first be passed in and that the contents would be the URL arguments

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should also say that the child process can do whatever it wants with the file (including deleting it)

(how you say it is a matter of taste and given that im not a maintainer of this repo, i will not give my suggestion :p )

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @dotslash. and we need to tell user that the temp file will not be deleted by ttyd, or add some logic to delete it automatically on websocket closing?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a note to the end

-R, --readonly Do not allow clients to write to the TTY
-t, --client-option Send option to client (format: key=value), repeat to add more options
-T, --terminal-type Terminal type to report, default: xterm-256color
Expand Down
3 changes: 3 additions & 0 deletions man/ttyd.1
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ Cross platform: macOS, Linux, FreeBSD/OpenBSD, OpenWrt/LEDE, Windows
Allow client to send command line arguments in URL (eg:
\[la]http://localhost:7681?arg=foo&arg=bar\[ra])

.PP
\-f, \-\-arg\-file
File prefix for a unique generated temp file that URL arguments are written to (ex. /tmp/prefix); the generated file's full path is then passed in as a command line argument (ex. /tmp/prefix{unique string})
.PP
\-R, \-\-readonly
Do not allow clients to write to the TTY
Expand Down
3 changes: 3 additions & 0 deletions man/ttyd.man.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ ttyd 1 "September 2016" ttyd "User Manual"
-a, --url-arg
Allow client to send command line arguments in URL (eg: http://localhost:7681?arg=foo&arg=bar)

-f, --arg-file
File prefix for a unique generated temp file that URL arguments are written to (ex. /tmp/prefix); the generated file's full path is then passed in as a command line argument (ex. /tmp/prefix{unique string})

-R, --readonly
Do not allow clients to write to the TTY

Expand Down
31 changes: 28 additions & 3 deletions src/protocol.c
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,34 @@ static bool spawn_process(struct pss_tty *pss, uint16_t columns, uint16_t rows)
for (i = 0; i < server->argc; i++) {
argv[n++] = server->argv[i];
}
for (i = 0; i < pss->argc; i++) {
argv[n++] = pss->args[i];
if (server->url_arg) {
for (i = 0; i < pss->argc; i++) {
argv[n++] = pss->args[i];
}
}
else if (server->arg_file != NULL) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

typical code style

if (...) {
 ...
} else if (..) {
 ..
} else {
 ...
}

as opposed to

if (...) {
 ...
} 
else if (..) {
 ..
} 
else {
 ...
}

consistent with ttyd :

} else {

int fd = -1;
// mkstemp requires the file path to have suffix XXXXXX (len 7)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XXXXXX is len 6. no?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int file_path_len = strlen(server->arg_file) + 6 /*XXXXXX*/ + 1 /*null character*/;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(subjective opinion)
maybe strcpy,strcat might be "simpler" to follow

strcpy(filePath, server->arg_file);
strcat(filePath, "XXXXXX");

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah XXXXXX has len 6, but with the null terminator it's 7. I'll change this line to be more clear.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was using strcat before but it might not be safe: #747 (comment)

int file_path_len = strlen(server->arg_file) + 7;
char *filePath = xmalloc(file_path_len);
snprintf(filePath, file_path_len, "%sXXXXXX", server->arg_file);

if ((fd = mkstemp(filePath)) == -1) {
lwsl_err("Creation of temp file failed with error: %d (%s)\n", errno, strerror(errno));
return false;
}

for (i = 0; i < pss->argc; i++) {
if (dprintf(fd, "%s\n", pss->args[i]) < 0) {
lwsl_err("Write to temp file failed with error: %d (%s)\n", errno, strerror(errno));
return false;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will leak fd too.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filePath is not freed too.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

}
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to close the file

argv[n++] = filePath;
close(fd);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you might want to

int close_fd = close(fd)
if (close_fd != 0) {
 ... error handling ...
 return false
}
argv[n++] = filePath;

}

argv[n] = NULL;

pty_process *process = process_init((void *) pss, server->loop, argv);
Expand Down Expand Up @@ -178,7 +203,7 @@ int callback_tty(struct lws *wsi, enum lws_callback_reasons reason, void *user,
pss->wsi = wsi;
pss->lws_close_status = LWS_CLOSE_STATUS_NOSTATUS;

if (server->url_arg) {
if (server->url_arg || server->arg_file != NULL) {
while (lws_hdr_copy_fragment(wsi, buf, sizeof(buf), WSI_TOKEN_HTTP_URI_ARGS, n++) > 0) {
if (strncmp(buf, "arg=", 4) == 0) {
pss->args = xrealloc(pss->args, (pss->argc + 1) * sizeof(char *));
Expand Down
15 changes: 12 additions & 3 deletions src/server.c
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ static const struct option options[] = {
{"ssl-key", required_argument, NULL, 'K'},
{"ssl-ca", required_argument, NULL, 'A'},
{"url-arg", no_argument, NULL, 'a'},
{"arg-file", required_argument, NULL, 'f'},
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this required argument?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe an optional argument must be passed in without a space (-f{argument} instead of -f {argument}). I figured this might be confusing for users when the rest of the options can be listed as -{option} {argument}, especially since using -f {argument} would make ttyd try to use this argument as a command instead.

If this isn't an issue, I can change the argument to be optional.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TLDR: We can hold off discussion on this until ttyd maintainers respond as i feel this is a matter of taste mostly and we should please the reviewer 😛

skimmed through https://man7.org/linux/man-pages/man3/getopt.3.html. It seems the for optional arguments -f arg -f=arg both should work.

believe an optional argument must be passed in without a space
have you checked it?

In either case, optional might be the right thing to do. But if its an optional argument is there a way to distinguish between the the user running ttyd -f mycmd and ttyd mycmd?

IMO Ideally (you might have different thoughts, which is reasonable)

  1. ttyd mycmd : Our code should not trigger
  2. ttyd -f /tmp/myprefix mycmd : Our code should trigger and create a tmp file with /tmp/myprefix prefix
  3. ttyd -f mycmd : Our code should trigger and create a tmp file with /tmp/ prefix (or something like /tmp/ttyd-args- prefix)

overall this i feel this is quite opinionated!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also as @kahing was saying in another comment, not supporting the following is quite reasonable

ttyd -f /tmp/myprefix mycmd : Our code should trigger and create a tmp file with /tmp/myprefix prefix

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the cases you have listed there. I've tried making this argument optional but it seems like only cases 1 and 3 work as expected. For ttyd -f /tmp/myprefix mycmd, /tmp/myprefix isn't being passed in as an optional argument for -f, rather it is being used as the ttyd command.

Running ttyd -f=/tmp/myprefix mycmd sets the prefix to =/tmp/myprefix and running ttyd -f/tmp/myprefix mycmd sets the prefix to /tmp/myprefix.
There's a discussion about this behaviour here.

If this behaviour isn't too confusing, I can make the argument optional.

Alternatively, we could do something like -f -prefix /tmp/myprefix where -f has no argument (turns on the url argument to file feature) and -prefix has a required argument that specifies a path. Then if only -f is specified, we use some default prefix like /tmp/ttyd-args.

{"readonly", no_argument, NULL, 'R'},
{"terminal-type", required_argument, NULL, 'T'},
{"client-option", required_argument, NULL, 't'},
Expand All @@ -86,10 +87,10 @@ static const struct option options[] = {
{NULL, 0, 0, 0}};

#if LWS_LIBRARY_VERSION_NUMBER < 4000000
static const char *opt_string = "p:i:c:u:g:s:I:b:6aSC:K:A:Rt:T:Om:oBd:vh";
static const char *opt_string = "p:i:c:u:g:s:I:b:6af:SC:K:A:Rt:T:Om:oBd:vh";
#endif
#if LWS_LIBRARY_VERSION_NUMBER >= 4000000
static const char *opt_string = "p:i:c:u:g:s:I:b:P:6aSC:K:A:Rt:T:Om:oBd:vh";
static const char *opt_string = "p:i:c:u:g:s:I:b:P:6af:SC:K:A:Rt:T:Om:oBd:vh";
#endif

static void print_help() {
Expand All @@ -107,6 +108,7 @@ static void print_help() {
" -g, --gid Group id to run with\n"
" -s, --signal Signal to send to the command when exit it (default: 1, SIGHUP)\n"
" -a, --url-arg Allow client to send command line arguments in URL (eg: http://localhost:7681?arg=foo&arg=bar)\n"
" -f, --arg-file File prefix for a unique generated temp file that URL arguments are written to (ex. /tmp/prefix); the generated file's full path is then passed in as a command line argument (ex. /tmp/prefix{unique string})\n"
" -R, --readonly Do not allow clients to write to the TTY\n"
" -t, --client-option Send option to client (format: key=value), repeat to add more options\n"
" -T, --terminal-type Terminal type to report, default: xterm-256color\n"
Expand Down Expand Up @@ -182,6 +184,7 @@ static struct server *server_new(int argc, char **argv, int start) {

static void server_free(struct server *ts) {
if (ts == NULL) return;
if (ts->arg_file != NULL) free(ts->arg_file);
if (ts->credential != NULL) free(ts->credential);
if (ts->index != NULL) free(ts->index);
free(ts->command);
Expand Down Expand Up @@ -321,6 +324,11 @@ int main(int argc, char **argv) {
break;
case 'a':
server->url_arg = true;
server->arg_file = NULL;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we might want to add a comment in help text, documentation that if both are set, behaviour is non deterministic.

Alternatively we can make a choice that if both are set url_arg will win (or vice versa)

Based on that you want to structure your if .. else if .. in https://github.com/tsl0922/ttyd/pull/747/files#diff-0b6794e089b51873f4effd373d78e8d13d1ee1ca83c0dd1394b09bcad26254c7R105
(eitherways a comment will be helpful)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behaviour shouldn't be non deterministic here; the last specified argument of -f and -a will be set. I can add this to the help text for clarity.

break;
case 'f':
server->arg_file = strdup(optarg);
server->url_arg = false;
break;
case 'R':
server->readonly = true;
Expand Down Expand Up @@ -537,7 +545,8 @@ int main(int argc, char **argv) {
lwsl_notice(" websocket: %s\n", endpoints.ws);
}
if (server->check_origin) lwsl_notice(" check origin: true\n");
if (server->url_arg) lwsl_notice(" allow url arg: true\n");
if (server->url_arg) lwsl_notice(" allow url arg to cli arg: true\n");
if (server->arg_file != NULL) lwsl_notice(" temp file name prefix: %s\n", server->arg_file);
if (server->readonly) lwsl_notice(" readonly: true\n");
if (server->max_clients > 0)
lwsl_notice(" max clients: %d\n", server->max_clients);
Expand Down
1 change: 1 addition & 0 deletions src/server.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ struct server {
int sig_code; // close signal
char sig_name[20]; // human readable signal string
bool url_arg; // allow client to send cli arguments in URL
char *arg_file; // file prefix for a generated temp file that URL arguments are written to
bool readonly; // whether not allow clients to write to the TTY
bool check_origin; // whether allow websocket connection from different origin
int max_clients; // maximum clients to support
Expand Down