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 @@ -71,6 +71,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}). The command is responsible for deleting the file.
.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 @@ -46,6 +46,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}). The command is responsible for deleting the file.

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

Expand Down
35 changes: 32 additions & 3 deletions src/protocol.c
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,37 @@ static char **build_args(struct pss_tty *pss) {
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) {
int fd = -1;
int file_path_len = strlen(server->arg_file) + 6 /*XXXXXX*/ + 1 /*null character*/;
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));
free(filePath);
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));
close(fd);
free(filePath);
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

if (close(fd) != 0) {
lwsl_err("Close temp file failed with error: %d (%s)\n", errno, strerror(errno));
free(filePath);
return false;
}
argv[n++] = filePath;
}

argv[n] = NULL;
Expand Down Expand Up @@ -219,7 +248,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
11 changes: 10 additions & 1 deletion src/server.c
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ static const struct option options[] = {{"port", required_argument, NULL, 'p'},
{"ssl-key", required_argument, NULL, 'K'},
{"ssl-ca", required_argument, NULL, 'A'},
{"url-arg", no_argument, NULL, 'a'},
{"arg-file", required_argument, NULL, 'f'},
{"readonly", no_argument, NULL, 'R'},
{"terminal-type", required_argument, NULL, 'T'},
{"client-option", required_argument, NULL, 't'},
Expand All @@ -81,7 +82,7 @@ static const struct option options[] = {{"port", required_argument, NULL, 'p'},
{"version", no_argument, NULL, 'v'},
{"help", no_argument, NULL, 'h'},
{NULL, 0, 0, 0}};
static const char *opt_string = "p:i:c:H:u:g:s:w:I:b:P:6aSC:K:A:Rt:T:Om:oBd:vh";
static const char *opt_string = "p:i:c:H:u:g:s:w:I:b:P:6af:SC:K:A:Rt:T:Om:oBd:vh";

static void print_help() {
// clang-format off
Expand All @@ -100,6 +101,7 @@ static void print_help() {
" -s, --signal Signal to send to the command when exit it (default: 1, SIGHUP)\n"
" -w, --cwd Working directory to be set for the child program\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}). The command is responsible for deleting the file.\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 @@ -146,6 +148,7 @@ static void print_config() {
if (server->auth_header != NULL) lwsl_notice(" auth header: %s\n", server->auth_header);
if (server->check_origin) lwsl_notice(" check origin: true\n");
if (server->url_arg) lwsl_notice(" allow url 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);
if (server->once) lwsl_notice(" once: true\n");
Expand Down Expand Up @@ -197,6 +200,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->auth_header != NULL) free(ts->auth_header);
if (ts->index != NULL) free(ts->index);
Expand Down Expand Up @@ -346,6 +350,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
1 change: 1 addition & 0 deletions src/server.h
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,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