Skip to content

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

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.

Sign up for GitHub

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ed9511a
Add functionality to write to a temp file through url arguments
vaishalikumanan Aug 5, 2021
cde0982
close file
vaishalikumanan Aug 6, 2021
25f91ea
Add argument for temp file path/prefix
vaishalikumanan Aug 6, 2021
53565c7
Concat temp file suffix without strcat
vaishalikumanan Aug 9, 2021
5a3ad2f
use snprintf instead of sprintf
vaishalikumanan Aug 9, 2021
4be5009
Reformat code
vaishalikumanan Aug 16, 2021
a487955
Merge branch 'main' into add-tmp-file
kahing Aug 27, 2021
092dc3d
fix build and error check
kahing Aug 27, 2021
b9b962b
fix a merge problem and addressed feedback
kahing Aug 30, 2021
c01676c
add a note to -f
kahing Sep 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
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

Copy link

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
View file
Open in desktop
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
View file
Open in desktop
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
28 changes: 25 additions & 3 deletions src/protocol.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,31 @@ 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) {
Copy link

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 :

ttyd/src/protocol.c

Line 65 in 5a3ad2f

} else {

int fd = -1;
char *filePath = strcat(server->arg_file, "XXXXXX");
Copy link

Choose a reason for hiding this comment

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

I think we should just choose a prefix here and not require users to choose a prefix

Copy link

Choose a reason for hiding this comment

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

also this is not a safe usage of strcat, since you may write past the allocated memory


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);
Copy link

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 +200,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
View file
Open in desktop
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.

Copy link

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!

Copy link

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;
Copy link

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
View file
Open in desktop
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