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.

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

Jump to bottom

uclient-fetch: add support for --header cmdline argument #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

uclient-fetch: add support for --header cmdline argument #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions tests/cram/test-san_uclient-fetch.t
View file
Open in desktop
Expand Up @@ -12,6 +12,7 @@ check uclient-fetch usage:
\t-P <dir>\t\t\tSet directory for output files (esc)
\t--quiet | -q\t\t\tTurn off status messages (esc)
\t--continue | -c\t\t\tContinue a partially-downloaded file (esc)
\t--header=HEADER\t\t\tAdd HTTP header to request ("name: value")
\t--user=<user>\t\t\tHTTP authentication username (esc)
\t--password=<password>\t\tHTTP authentication password (esc)
\t--user-agent | -U <str>\t\tSet HTTP user agent (esc)
Expand Down
1 change: 1 addition & 0 deletions tests/cram/test_uclient-fetch.t
View file
Open in desktop
Expand Up @@ -12,6 +12,7 @@ check uclient-fetch usage:
\t-P <dir>\t\t\tSet directory for output files (esc)
\t--quiet | -q\t\t\tTurn off status messages (esc)
\t--continue | -c\t\t\tContinue a partially-downloaded file (esc)
\t--header=HEADER\t\t\tAdd HTTP header to request ("name: value")
\t--user=<user>\t\t\tHTTP authentication username (esc)
\t--password=<password>\t\tHTTP authentication password (esc)
\t--user-agent | -U <str>\t\tSet HTTP user agent (esc)
Expand Down
59 changes: 58 additions & 1 deletion uclient-fetch.c
View file
Open in desktop
Expand Up @@ -21,6 +21,7 @@
#include <sys/socket.h>
#include <unistd.h>
#include <stdio.h>
#include <ctype.h>
#include <getopt.h>
#include <fcntl.h>
#include <glob.h>
Expand All @@ -29,6 +30,7 @@
#include <signal.h>

#include <libubox/blobmsg.h>
#include <libubox/list.h>

#include "progress.h"
#include "uclient.h"
Expand All @@ -38,6 +40,12 @@
#define strdupa(x) strcpy(alloca(strlen(x)+1),x)
#endif

struct header {
struct list_head list;
char *name;
char *value;
};

static const char *user_agent = "uclient-fetch";
static const char *post_data;
static const char *post_file;
Expand All @@ -59,6 +67,7 @@ static char **urls;
static int n_urls;
static int timeout;
static bool resume, cur_resume;
static LIST_HEAD(headers);

static struct progress pmt;
static struct uloop_timeout pmt_timer;
Expand Down Expand Up @@ -317,6 +326,7 @@ static void check_resume_offset(struct uclient *cl)

static int init_request(struct uclient *cl)
{
struct header *h, *th;
int rc;

out_offset = 0;
Expand All @@ -339,6 +349,13 @@ static int init_request(struct uclient *cl)

uclient_http_reset_headers(cl);
uclient_http_set_header(cl, "User-Agent", user_agent);

list_for_each_entry_safe(h, th, &headers, list) {
uclient_http_set_header(cl, h->name, h->value);
Copy link

Choose a reason for hiding this comment

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

How does this handle POST data, if I want Content-Type: application/json? Looks like it would be overridden by the code down at line 363.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, actually both headers would end up being sent. First the one supplied via --header and then the Content-Type header which is set as a consequence of post_data or post_file being present.
I agree that this is kinda problematic as it would prevent you from overriding the Content-Type which could be desirable in some situations.

Copy link

Choose a reason for hiding this comment

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

So, maybe if there are any user-supplied --headers present, then disable the post_xxx sections from adding further ones? Seems like that would be pretty much backward compatible.

Copy link

Choose a reason for hiding this comment

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

The GNU wget allows overriding of a Content-Type when the --post-data is used.
Generally speaking this must be prohibited: the --post-data is to imitate a form post. If you wish to send a body with a custom CT e.g. apllication/json then you must use the --methodn=POST --body-data introduced in the #2

To implement the CT overriding we need more code: introduce a ct_header var, check each supplied header name for the name and set it there, then check if the ct_header is NULL and if wasn't set then use the application/x-www-form-urlencoded.

list_del(&h->list);
free(h);
}

if (cur_resume)
check_resume_offset(cl);

Expand Down Expand Up @@ -476,6 +493,7 @@ static int usage(const char *progname)
" -P <dir> Set directory for output files\n"
" --quiet | -q Turn off status messages\n"
" --continue | -c Continue a partially-downloaded file\n"
" --header=HEADER Add HTTP header to request (\"name: value\")\n"
Copy link

Choose a reason for hiding this comment

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

" --header='Header: value' Add HTTP header. Multiple allowed\n"

" --user=<user> HTTP authentication username\n"
" --password=<password> HTTP authentication password\n"
" --user-agent | -U <str> Set HTTP user agent\n"
Expand Down Expand Up @@ -517,6 +535,22 @@ static int no_ssl(const char *progname)
return 1;
}

static bool is_valid_header(char *str)
Copy link

Choose a reason for hiding this comment

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

This is a good thing to check a header name but not necessary.
To keep the code simpler and smaller we can drop the check. Unless a server is ok with a header name that's not our business.
Maybe the full GNU wget or curl do the check to avoid users mistake. But here we expect that a script was tested and a user is not dumb.

Copy link
Member Author

Choose a reason for hiding this comment

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

I mostly introduced this check to make sure uclient-fetch exists with an error and actively refuses to violate the protocol.

{
char *tmp = str;

if (isdigit(*tmp))
return false;

do {
if (!isalnum(*tmp) && *tmp != '-')
return false;

} while (*++tmp != '\0');

return true;
};

enum {
L_NO_CHECK_CERTIFICATE,
L_CA_CERTIFICATE,
Expand All @@ -532,6 +566,7 @@ enum {
L_PROXY,
L_NO_PROXY,
L_QUIET,
L_HEADER,
};

static const struct option longopts[] = {
Expand All @@ -549,11 +584,11 @@ static const struct option longopts[] = {
[L_PROXY] = { "proxy", required_argument, NULL, 0 },
[L_NO_PROXY] = { "no-proxy", no_argument, NULL, 0 },
[L_QUIET] = { "quiet", no_argument, NULL, 0 },
[L_HEADER] = { "header", required_argument, NULL, 0 },
Copy link

Choose a reason for hiding this comment

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

IMHO the better to place the L_HEADER after the L_USER_AGENT

Copy link
Member Author

Choose a reason for hiding this comment

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

Up to now this list has been appended chronologically rather than in semantic or alphabetic order. I basically just tried to keep the existing style without causing more work -- I agree though that semantic or alphabetic order would be much nicer, I was just too lazy to have another bunch of commits before the change I desire doing all the cleanup...

{}
};



int main(int argc, char **argv)
{
const char *progname = argv[0];
Expand All @@ -563,6 +598,8 @@ int main(int argc, char **argv)
struct uclient *cl;
int longopt_idx = 0;
bool has_cert = false;
struct header *h;
char *tmp;
int i, ch;
int rc;
int af = -1;
Expand Down Expand Up @@ -633,6 +670,26 @@ int main(int argc, char **argv)
case L_QUIET:
quiet = true;
break;
case L_HEADER:
tmp = strchr(optarg, ':');
if (!tmp)
return usage(progname);

*tmp++ = '\0';
while (isspace(*tmp))
++tmp;

if (*tmp == '\0' || !is_valid_header(optarg) || strchr(tmp, '\n'))
return usage(progname);

h = calloc(1, sizeof(*h));
Copy link

Choose a reason for hiding this comment

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

The calloc is for arrays, you can use just malloc(sizeof(*h))

Copy link
Member Author

Choose a reason for hiding this comment

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

I kinda developed the habit to always use calloc as it also zeros the allocated memory. In this case it is true that this is unneeded because we are writing to pointers there right after.

if (!h)
exit(ENOMEM);
Copy link

Choose a reason for hiding this comment

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

The exit is used incorrectly here. It should be just exit(1).

The ENOMEM is a value of a globa variable errno that is set by the calloc.
You can print the perror function:

perror("headers");
exit(1);

And on the OOM it will print: headers: Cannot allocate memory.

You can pragmatically set limit to test it's behavior:
https://stackoverflow.com/a/9154138/1049542


h->name = optarg;
h->value = tmp;
list_add_tail(&h->list, &headers);
break;
default:
return usage(progname);
}
Expand Down