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

dangowrt · 2024-04-02T21:40:56Z

Add --header=header-line option to allow adding custom HTTP headers to a request, compatible with wget.
The option can be used multiple times to add multiple headers.

stokito · 2024-04-03T08:50:16Z

Can you please also check my previous patch:
https://gitlab.com/openwrt/project/uclient/-/merge_requests/1

Maybe something should me borrowed from it.

Some discussion:
https://forum.openwrt.org/t/uclient-fetch-add-more-options-from-wget-and-create-uclient-curl-clone-of-curl/54060/8

dangowrt · 2024-04-03T12:52:59Z

@stokito Sorry I was not aware of your development, I wasn't even aware of that whole parallel universe on gitlab.com still going on and allowing merge requests for our projects there. It's tedious having to check 3 different places.
I had search patchwork before and seen your submission via the mailing lists, but sadly that didn't include the part adding support for --header
Obviously this is not your fault, but something we should address (warning signs? MIRROR ONLY. Don't allow merge requests? @ynezz)

Now moving forward and looking at your solution compared to mine, I would say using dynamically allocated list using libubox list.h is a bit nicer than statically allocating an array of pointers sized argc. And why do we need to change uclient-http.c at all? Isn't it enough to parse the parameter and use the existing uclient_http_set_header()? (Not meant to be rhetoric, I don't know much about HTTP, maybe there are good reasons for your approach).

Sorry to reply here, I don't have an account on gitlab.com and am not willing to have one there for historic reasons related to gitlab.com destroying gitorious.org without warning, which costs me weeks of my life to recover the damage and even though microsoft is bad, they haven't directly harmed me as severely as gitlab.com did and never even asked for an apology.

aparcar · 2024-04-07T22:18:30Z

Tested and works fine:

root@ap:~# uclient-fetch --header "OK: cool" http://192.168.1.2:8000/index.html
Downloading 'http://192.168.25.179:8000/index.html'
Connecting to 192.168.1.2:8000
Connection error: Connection timed out

❯ netcat -l -p  8000
stty: 'standard input': unable to perform all requested operations
GET /index.html HTTP/1.1
Host: 192.168.1.2:8000
User-Agent: uclient-fetch
OK: cool

stokito · 2024-04-15T03:47:54Z

uclient-fetch.c

+
+				h = calloc(1, sizeof(*h));
+				if (!h)
+					exit(ENOMEM);


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

stokito · 2024-04-15T03:48:51Z

uclient-fetch.c

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


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

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.

stokito · 2024-04-15T03:52:06Z

uclient-fetch.c

@@ -517,6 +535,22 @@ static int no_ssl(const char *progname)
 	return 1;
 }

+static bool is_valid_header(char *str)


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.

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

stokito · 2024-04-15T03:52:42Z

uclient-fetch.c

@@ -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"


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

stokito · 2024-04-15T05:29:10Z

uclient-fetch.c

@@ -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 },


IMHO the better to place the L_HEADER after the L_USER_AGENT

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...

stokito · 2024-04-15T06:06:41Z

It's good that you created the PR because I saw it :) Sending patches to devlist is almost same as sending to /dev/null. I hope that we can at least make a review and discussion here and then try to push to the upstream. For the same reason I created that PR on GitLab because the GitHub mirror didn't existed at the moment.

To compare, here is commit here on GitHub:
stokito@1c6cd22
I made a different design chooses:

Not to parse the header but to send it as is. This must make code smaller, no additional allocs, reduce attack vector but also just difficult to make :)
I extended the uclient API with the uclient_http_set_raw_headers that accept list of headers as a bulk. I think it may be useful additionally because it doesn't require to split header and it's value.
Instead of using a linked list with malloc for each item I made just a bulk allocation with calloc. We already know how many elements we need so we basically don't need a dynamically allocated linked list.

The uclient_http_set_header adds a header to blob_buf headers but I created a separate field char **raw_headers. That's because the headers field is KV store and I again need to parse the header.

So in comparison with my version your seems more advanced, with validation and more in line with existing code. My version is simpler and faster.

I would be happy if any version would be merged and we can finally use the --header.

@dhewg you were interested in my patches, maybe you can review this one?

I'll also create PR #2 that adds support of --method, --body-data and --body-file. Please review it if you'll have a minute.

efahl · 2024-04-22T21:55:18Z

uclient-fetch.c

@@ -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);


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.

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.

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.

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.

ahuse · 2024-05-18T13:31:12Z

Any way of merging one of the two versions? I am currently only depending on curl for the --header functionality. Would greatly appreciate this in uclient.

efahl · 2024-05-18T22:28:57Z

Would greatly appreciate this in uclient.

If you are running snapshots, then the uclient API has been exposed to ucode as ucode-mod-uclient. I switched over from using curl in my ucode-based auc-clone, https://github.com/efahl/owut/blob/main/files/owut#L245 Should be pretty easy just to write your own little my-wget-with-headers by copying pertinent chunks from there. (My only wish is that this ucode-mod would be backported to 23.05...)

aparcar · 2024-05-18T22:45:54Z

Would greatly appreciate this in uclient.

If you are running snapshots, then the uclient API has been exposed to ucode as ucode-mod-uclient. I switched over from using curl in my ucode-based auc-clone, https://github.com/efahl/owut/blob/main/files/owut#L245 Should be pretty easy just to write your own little my-wget-with-headers by copying pertinent chunks from there. (My only wish is that this ucode-mod would be backported to 23.05...)

Woah why do I only realize this tool exists by randomly following this PR? Will test this and I guess we should drop auc...

ahuse · 2024-05-19T06:33:46Z

If you are running snapshots, then the uclient API has been exposed to ucode as ucode-mod-uclient.

This is interesting. Thanks for the info. I am currently on 23.05, but I will have a look.

stokito · 2024-05-19T07:07:03Z

@dangowrt please fix the merge conflict and let's try to submit the patch to OpenWrt devel list.
Also please review and test my PR #2 and once you are ok with it we can submit it too.
Anyone here, I'll also really appreciate if you too can review and test.

stokito · 2024-06-26T19:53:37Z

uclient-fetch.c

@@ -705,7 +706,8 @@ int main(int argc, char **argv)
 				proxy = false;
 			break;
 		default:
-			return usage(progname);
+			usage(progname);


we may exit directly because nothing was initialized yet

That may not necessarily be true. It could be that one or more header arguments are followed by an invalid argument.

uclient-fetch.c

stokito · 2024-06-26T20:10:34Z

Hi @dangowrt thank you for the update. Please remove the perror("Auth string"); everything else looks good.

Make error handling to the main() function more uniform. Always free allocated memory. Signed-off-by: Daniel Golle <daniel@makrotopia.org>

Add --header='Header: value' option to allow adding custom HTTP headers to a request, compatible with wget. The option can be used multiple times to add multiple headers and also allows overriding the Content-Type header for POST requests. Signed-off-by: Daniel Golle <daniel@makrotopia.org>

stokito · 2024-06-27T10:41:57Z

@dangowrt great, thank you. Could you send a patch to the devel list https://lists.infradead.org/mailman/listinfo/openwrt-devel

To do this you need to create a patches with git format-patch -2 and then send the two files.
If you are not interested in this I may try to submit the patches for you

ahuse · 2024-10-10T14:24:59Z

Friendly ping (maybe @dangowrt ) to update the version in main branch, so that we can have the commit in 24.10.

dangowrt force-pushed the uclient-fetch-header branch from 31f6bb6 to 9aae442 Compare April 4, 2024 03:08

aparcar approved these changes Apr 7, 2024

View reviewed changes

This was referenced Apr 10, 2024

ffmuc-mesh-vpn-wireguard: add User-Agent if contacting broker freifunk-gluon/community-packages#105

Merged

ffmuc-mesh-vpn-wireguard: add headers freifunk-gluon/community-packages#111

Open

stokito suggested changes Apr 15, 2024

View reviewed changes

stokito reviewed Apr 15, 2024

View reviewed changes

efahl reviewed Apr 22, 2024

View reviewed changes

dangowrt force-pushed the uclient-fetch-header branch from 9aae442 to f25ebf9 Compare June 26, 2024 19:15

stokito reviewed Jun 26, 2024

View reviewed changes

stokito suggested changes Jun 26, 2024

View reviewed changes

uclient-fetch.c Outdated Show resolved Hide resolved

uclient-fetch.c Outdated Show resolved Hide resolved

dangowrt added 2 commits June 27, 2024 01:11

uclient-fetch: improve error handling

e035d57

Make error handling to the main() function more uniform. Always free allocated memory. Signed-off-by: Daniel Golle <daniel@makrotopia.org>

dangowrt force-pushed the uclient-fetch-header branch from f25ebf9 to a220818 Compare June 27, 2024 00:12

openwrt-bot merged commit a220818 into openwrt:master Jul 8, 2024

dangowrt deleted the uclient-fetch-header branch July 8, 2024 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dangowrt commented Apr 2, 2024

stokito commented Apr 3, 2024

dangowrt commented Apr 3, 2024

aparcar commented Apr 7, 2024

stokito Apr 15, 2024

stokito Apr 15, 2024

dangowrt Apr 22, 2024

stokito Apr 15, 2024

dangowrt Apr 22, 2024

stokito Apr 15, 2024

stokito Apr 15, 2024

dangowrt Apr 22, 2024

stokito commented Apr 15, 2024 •

edited

Loading

efahl Apr 22, 2024

dangowrt Apr 22, 2024

efahl Apr 22, 2024

stokito Apr 23, 2024

ahuse commented May 18, 2024

efahl commented May 18, 2024

aparcar commented May 18, 2024

ahuse commented May 19, 2024

stokito commented May 19, 2024

stokito Jun 26, 2024

dangowrt Jun 27, 2024

stokito commented Jun 26, 2024

stokito commented Jun 27, 2024

ahuse commented Oct 10, 2024

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

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

Conversation

dangowrt commented Apr 2, 2024

stokito commented Apr 3, 2024

dangowrt commented Apr 3, 2024

aparcar commented Apr 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stokito commented Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahuse commented May 18, 2024

efahl commented May 18, 2024

aparcar commented May 18, 2024

ahuse commented May 19, 2024

stokito commented May 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stokito commented Jun 26, 2024

stokito commented Jun 27, 2024

ahuse commented Oct 10, 2024

stokito commented Apr 15, 2024 •

edited

Loading