net: lib: downloader: limit number of redirects #20224

eivindj-nordic · 2025-02-06T12:57:54Z

Limit the number of redirects before downloader fails the download.
Cleanup formatting and some code indentation.

NordicBuilder · 2025-02-06T13:00:34Z

CI Information

^{To view the history of this post, clich the 'edited' button above}
Build number: 15

Inputs:

Sources:

sdk-nrf: PR head: 6c388d17536e9c148d7556e885272c94e100a9d1

more details

sdk-nrf:

PR head: 6c388d17536e9c148d7556e885272c94e100a9d1
merge base: ab80a432d15d247daea8f0af580669af9fc841f7
target head (main): ab80a432d15d247daea8f0af580669af9fc841f7
Diff

Github labels

Enabled	Name	Description
	ci-disabled	Disable the ci execution
	ci-all-test	Run all of ci, no test spec filtering will be done
	ci-force-downstream	Force execution of downstream even if twister fails
	ci-run-twister	Force run twister
	ci-run-zephyr-twister	Force run zephyr twister

List of changed files detected by CI (6)

include
│  ├── net
│  │  │ downloader.h
subsys
│  ├── net
│  │  ├── lib
│  │  │  ├── downloader
│  │  │  │  ├── Kconfig
│  │  │  │  ├── src
│  │  │  │  │  ├── downloader.c
│  │  │  │  │  ├── transports
│  │  │  │  │  │  │ http.c
tests
│  ├── subsys
│  │  ├── net
│  │  │  ├── lib
│  │  │  │  ├── downloader
│  │  │  │  │  ├── CMakeLists.txt
│  │  │  │  │  ├── src
│  │  │  │  │  │  │ main.c

Outputs:

Toolchain

Version: 4cff34261a
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:4cff34261a_bece0367df

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

◻️ Toolchain - Skipped: existing toolchain is used
✅ Build twister
- sdk-nrf test count: 361
✅ Integration tests
- ✅ test-fw-nrfconnect-boot
- ✅ test-fw-nrfconnect-nrf-iot_cloud
- ✅ test-fw-nrfconnect-nrf-iot_serial_lte_modem
- ✅ test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
- ✅ test-fw-nrfconnect-nrf-iot_samples
- ✅ test-fw-nrfconnect-nrf-iot_lwm2m
- ✅ test-fw-nrfconnect-nrf-iot_mosh
- ✅ test-sdk-mcuboot
- ⚠️ test-fw-nrfconnect-fw-update

Disabled integration tests

- desktop52_verification
- doc-internal
- test_ble_nrf_config
- test-fw-nrfconnect-apps
- test-fw-nrfconnect-ble_mesh
- test-fw-nrfconnect-ble_samples
- test-fw-nrfconnect-chip
- test-fw-nrfconnect-fem
- test-fw-nrfconnect-nfc
- test-fw-nrfconnect-nrf-iot_positioning
- test-fw-nrfconnect-nrf-iot_thingy91
- test-fw-nrfconnect-nrf_crypto
- test-fw-nrfconnect-proprietary_esb
- test-fw-nrfconnect-ps
- test-fw-nrfconnect-rpc
- test-fw-nrfconnect-rs
- test-fw-nrfconnect-tfm
- test-fw-nrfconnect-thread
- test-fw-nrfconnect-zigbee
- test-low-level
- test-sdk-audio
- test-sdk-dfu
- test-sdk-find-my
- test-sdk-pmic-samples
- test-sdk-sidewalk
- test-sdk-wifi
- test-secdom-samples-public

Note: This message is automatically posted and updated by the CI

NordicBuilder · 2025-02-06T13:15:35Z

You can find the documentation preview for this PR at this link.

Note: This comment is automatically posted by the Documentation Publish GitHub Action.

subsys/net/lib/downloader/src/transports/http.c

subsys/net/lib/downloader/Kconfig

lemrey · 2025-02-06T13:49:28Z

include/net/downloader.h

+	 * Maximum number of HTTP redirects.
+	 * Use 0 to set default value of CONFIG_DOWNLOADER_MAX_REDIRECTS.
+	 */
+	int redirects_max;


So, there is no way to disable this feature by setting the maximum number of redirects to zero?

I thought about that, but the case is that if we are disabling it and the server misbehaves (which it should not do), we will end in a endless loop. In reality there should not be many redirects to get to a file, so setting a high number will result in similar behavior. I would rather have a number of attempts by default than unlimited.

anhmolt

Looks good to me. Some nits.

anhmolt · 2025-02-07T09:04:56Z

subsys/net/lib/downloader/src/transports/http.c

-	if (dl->host_cfg.range_override) {
-		if (tls_force_range && dl->host_cfg.range_override > (TLS_RANGE_MAX)) {
+	if (tls_force_range) {
+		if (dl->host_cfg.range_override > (TLS_RANGE_MAX)) {


Suggested change

if (dl->host_cfg.range_override > (TLS_RANGE_MAX)) {

if (dl->host_cfg.range_override > TLS_RANGE_MAX) {

anhmolt · 2025-02-07T09:05:13Z

subsys/net/lib/downloader/src/transports/http.c

-	if (dl->host_cfg.range_override) {
-		if (tls_force_range && dl->host_cfg.range_override > (TLS_RANGE_MAX)) {
+	if (tls_force_range) {
+		if (dl->host_cfg.range_override > (TLS_RANGE_MAX)) {
 			LOG_WRN("Range override > TLS max range, setting to TLS max range");
 			dl->host_cfg.range_override = (TLS_RANGE_MAX);


Suggested change

dl->host_cfg.range_override = (TLS_RANGE_MAX);

dl->host_cfg.range_override = TLS_RANGE_MAX;

anhmolt · 2025-02-07T09:08:19Z

subsys/net/lib/downloader/src/transports/http.c

-				return len;
-			}
+	if (dl->progress + len == dl->file_size) {
+		/* A full file have been received */


My fault 😞

Suggested change

/* A full file have been received */

/* A full file has been received */

MirkoCovizzi · 2025-02-07T09:46:03Z

include/net/downloader.h

@@ -195,6 +196,11 @@ struct downloader_host_cfg {
 	 * This string is used in case you are requesting a proxied file from a CoAP server.
 	 */
 	const char *proxy_uri;
+	/**
+	 * Maximum number of HTTP redirects.
+	 * Use 0 to set default value of CONFIG_DOWNLOADER_MAX_REDIRECTS.


This seems incorrect. The implementation sets the value of CONFIG_DOWNLOADER_MAX_REDIRECTS, not its default (10).

Ok, I see your point. I thought of the value of CONFIG_DOWNLOADER_MAX_REDIRECTS as the default, but that is not the default value of CONFIG_DOWNLOADER_MAX_REDIRECTS. I'll rewrite.

MirkoCovizzi · 2025-02-07T09:46:59Z

subsys/net/lib/downloader/src/transports/http.c

-			dl->host_cfg.range_override = (TLS_RANGE_MAX);
+			dl->host_cfg.range_override = TLS_RANGE_MAX;
+		} else if (dl->host_cfg.range_override == 0) {
+			dl->host_cfg.range_override = TLS_RANGE_MAX;


Why no warning here?

Because here it is not explicitly set to something else by the application, as it is above.

MirkoCovizzi · 2025-02-07T09:49:13Z

tests/subsys/net/lib/downloader/src/main.c

@@ -960,6 +960,8 @@ static ssize_t z_impl_zsock_recvfrom_http_redirect_and_close(
 		return 0;
 	}

+	memcpy(buf, HTTP_HDR_REDIRECT, strlen(HTTP_HDR_REDIRECT));
+	return strlen(HTTP_HDR_REDIRECT);
 	return 0;


This is unreachable and should be removed.

nordicjm · 2025-02-11T07:35:54Z

subsys/net/lib/downloader/src/transports/http.c

@@ -95,6 +95,9 @@ struct transport_params_http {

 	/** Request new data */
 	bool new_data_req;
+	/** Redirect retries */
+	uint32_t redirects;
+


Suggested change

Fixed in the right commit now.

nordicjm · 2025-02-11T07:36:24Z

subsys/net/lib/downloader/Kconfig

@@ -26,6 +26,12 @@ config DOWNLOADER_MAX_FILENAME_SIZE
 	range 8 2048
 	default 255

+config DOWNLOADER_MAX_REDIRECTS
+	int "Default maximum number of redirects before download is aborted"
+	default 10


possible range value to limit range so someone doesn't set this to 0 or -40?

Added to the wrong commit, thanks!

MirkoCovizzi

Looks good!

lemrey

Looks good !

nordicjm

issues have been fixed in the wrong commit and need fixing in the first

Limit the number of redirects before downloader fails the download. Co-authored-by: Seppo Takalo <seppo.takalo@nordicsemi.no> Signed-off-by: Eivind Jølsgard <eivind.jolsgard@nordicsemi.no>

Cleanup formatting and some code restructuring. Co-authored-by: Andreas Moltumyr <andreas.moltumyr@nordicsemi.no> Signed-off-by: Eivind Jølsgard <eivind.jolsgard@nordicsemi.no>

github-actions · 2025-02-20T08:58:02Z

After documentation is built, you will find the preview for this PR here.

eivindj-nordic added this to the 3.0.0 milestone Feb 6, 2025

eivindj-nordic self-assigned this Feb 6, 2025

eivindj-nordic requested review from a team as code owners February 6, 2025 12:57

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Feb 6, 2025

lemrey reviewed Feb 6, 2025

View reviewed changes

eivindj-nordic force-pushed the downloader_http_redirect_max_retries branch 2 times, most recently from 882964d to d147faa Compare February 6, 2025 13:57

anhmolt approved these changes Feb 7, 2025

View reviewed changes

eivindj-nordic force-pushed the downloader_http_redirect_max_retries branch from d147faa to f6fd041 Compare February 7, 2025 09:18

MirkoCovizzi reviewed Feb 7, 2025

View reviewed changes

eivindj-nordic force-pushed the downloader_http_redirect_max_retries branch from f6fd041 to 36e2f23 Compare February 7, 2025 11:05

nordicjm reviewed Feb 11, 2025

View reviewed changes

eivindj-nordic force-pushed the downloader_http_redirect_max_retries branch 3 times, most recently from 2e20e9c to bc7785e Compare February 11, 2025 07:49

eivindj-nordic requested review from nordicjm, MirkoCovizzi and lemrey February 14, 2025 07:40

MirkoCovizzi approved these changes Feb 17, 2025

View reviewed changes

lemrey approved these changes Feb 17, 2025

View reviewed changes

nordicjm reviewed Feb 17, 2025

View reviewed changes

eivindj-nordic force-pushed the downloader_http_redirect_max_retries branch 3 times, most recently from 4b4faa9 to 6a24117 Compare February 18, 2025 11:55

eivindj-nordic requested a review from nordicjm February 20, 2025 08:49

eivindj-nordic and others added 2 commits February 20, 2025 09:50

net: lib: downloader: limit number of redirects

9785857

Limit the number of redirects before downloader fails the download. Co-authored-by: Seppo Takalo <seppo.takalo@nordicsemi.no> Signed-off-by: Eivind Jølsgard <eivind.jolsgard@nordicsemi.no>

net: lib: downloader: Formatting and code restructuring

6c388d1

Cleanup formatting and some code restructuring. Co-authored-by: Andreas Moltumyr <andreas.moltumyr@nordicsemi.no> Signed-off-by: Eivind Jølsgard <eivind.jolsgard@nordicsemi.no>

eivindj-nordic force-pushed the downloader_http_redirect_max_retries branch from 6a24117 to 6c388d1 Compare February 20, 2025 08:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: lib: downloader: limit number of redirects #20224

net: lib: downloader: limit number of redirects #20224

eivindj-nordic commented Feb 6, 2025

NordicBuilder commented Feb 6, 2025 •

edited

Loading

NordicBuilder commented Feb 6, 2025

lemrey Feb 6, 2025

eivindj-nordic Feb 6, 2025 •

edited

Loading

anhmolt left a comment

anhmolt Feb 7, 2025

eivindj-nordic Feb 7, 2025

anhmolt Feb 7, 2025

eivindj-nordic Feb 7, 2025

anhmolt Feb 7, 2025

eivindj-nordic Feb 7, 2025

MirkoCovizzi Feb 7, 2025 •

edited

Loading

eivindj-nordic Feb 7, 2025

MirkoCovizzi Feb 7, 2025

eivindj-nordic Feb 7, 2025

MirkoCovizzi Feb 7, 2025

eivindj-nordic Feb 7, 2025

nordicjm Feb 11, 2025

eivindj-nordic Feb 18, 2025

nordicjm Feb 11, 2025

eivindj-nordic Feb 11, 2025

MirkoCovizzi left a comment

lemrey left a comment

nordicjm left a comment

github-actions bot commented Feb 20, 2025

	if (dl->host_cfg.range_override > (TLS_RANGE_MAX)) {
	if (dl->host_cfg.range_override > TLS_RANGE_MAX) {

	dl->host_cfg.range_override = (TLS_RANGE_MAX);
	dl->host_cfg.range_override = TLS_RANGE_MAX;

	/* A full file have been received */
	/* A full file has been received */

net: lib: downloader: limit number of redirects #20224

Are you sure you want to change the base?

net: lib: downloader: limit number of redirects #20224

Conversation

eivindj-nordic commented Feb 6, 2025

NordicBuilder commented Feb 6, 2025 • edited Loading

CI Information

Inputs:

Sources:

Github labels

Outputs:

Toolchain

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

NordicBuilder commented Feb 6, 2025

Choose a reason for hiding this comment

eivindj-nordic Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

anhmolt left a 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

MirkoCovizzi Feb 7, 2025 • 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

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

MirkoCovizzi left a comment

Choose a reason for hiding this comment

lemrey left a comment

Choose a reason for hiding this comment

nordicjm left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 20, 2025

NordicBuilder commented Feb 6, 2025 •

edited

Loading

eivindj-nordic Feb 6, 2025 •

edited

Loading

MirkoCovizzi Feb 7, 2025 •

edited

Loading