Skip to content

RDKEMW-16405:[SECVULN] - HTTPS support for firmware download (fwupdater daemon) - #255

Open
mkadinti wants to merge 78 commits into
developfrom
topic/RDKEMW-16405
Open

RDKEMW-16405:[SECVULN] - HTTPS support for firmware download (fwupdater daemon)#255
mkadinti wants to merge 78 commits into
developfrom
topic/RDKEMW-16405

Conversation

@mkadinti

@mkadinti mkadinti commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

This pull request addresses two behavioral gaps in the Direct CDN download path: it ensures that HTTP 403 responses (token expiry) in Direct CDN mode immediately short-circuit the inner retry loop, and that mTLS certificate fetch is bypassed for token-authenticated CDN downloads (except during state-red recovery). The changes are fully backward compatible, gated by the direct_cdn flag, and do not alter legacy or non-Direct-CDN code paths. Additionally, the OpenSpec specifications, requirements, and unit tests are updated to reflect and validate these behaviors. The PR also improves handling and logging of per-artifact Direct CDN URLs in the D-Bus firmware update manager.

Direct CDN Download Path Improvements:

  • Token expiry short-circuit: The retryDownload() function now breaks immediately on HTTP 403 when direct_cdn is true, preventing wasted retries with expired tokens and allowing the outer orchestration loop to fetch fresh URLs. [1] [2] [3] [4] [5]
  • mTLS bypass: The downloadFile() function skips mTLS certificate acquisition when direct_cdn is true and not in state-red recovery, avoiding unnecessary I/O and potential spurious state-red entries. The recovery cert path remains unchanged when in state-red. [1] [2] [3] [4]

Specification and Testing Updates:

  • OpenSpec requirements updated: New requirements and scenarios are added to the OpenSpec documentation to specify the new behaviors for token expiry handling and mTLS bypass in Direct CDN mode. [1] [2] [3]
  • Unit tests: Test cases are added to verify the new behaviors for both HTTP 403 handling and mTLS bypass, ensuring no regressions in legacy paths.

D-Bus Firmware Update Manager Enhancements:

  • Per-artifact Direct CDN URL handling: The D-Bus handler now prefers and logs Direct CDN URLs (e.g., firmwareUrl, pdriUrl, remCtrlUrl) when available, ensuring accurate signaling and improved observability for Direct CDN downloads. [1] [2] [3]

These changes collectively restore behavioral parity with the RDKV-reference implementation for Direct CDN downloads, improve efficiency, and reduce failure risk in token-authenticated upgrade flows.

mkadinti and others added 30 commits May 12, 2026 09:07
…L path branching and Codebig bypass - unittests fix
…L path branching and Codebig bypass - update on task.md
Copilot AI review requested due to automatic review settings July 30, 2026 10:25
mkadinti and others added 2 commits July 30, 2026 15:57
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (3)

openspec/changes/direct-cdn-token-and-mtls-fix/proposal.md:33

  • The OpenSpec proposal's impact/scope summary is now inaccurate: this PR modifies additional files beyond src/rdkv_upgrade.c (e.g., src/directcdn.c, src/json_process.c, and D-Bus sources). This mismatch can mislead reviewers and future traceability; please update the bullets to reflect the actual touched files/scope.
- **Files modified**: `src/rdkv_upgrade.c` (two functions: `retryDownload()`, `downloadFile()`)
- **API changes**: None — no signature, struct, or Makefile changes
- **Test impact**: New unit tests for both paths; existing tests unaffected
- **Risk**: Low — both changes are additive guards with RFC kill-switch (`SWDLDirect.Enable=false` disables entire Direct CDN path)
- **Scope boundary**: Does NOT touch `directcdn.c`, `rdkv_main.c`, `json_process.c`, daemon handlers, or any other code paths

src/dbus/rdkv_dbus_server.c:2959

  • The URL-too-long error log always prints effective_download_url/firmware_name, which is incorrect when using Direct CDN URLs (where the filename is not appended). It can also leak signed query-string tokens into logs. Consider logging the legacy composed URL only in legacy mode and redacting Direct CDN URLs.
    if (url_len < 0 || url_len >= (int)sizeof(imageHTTPURL)) {
	    SWLOG_ERROR("[DOWNLOAD_WORKER] ERROR: URL too long or snprintf failed (len=%d, max=%zu)\n",
			    url_len, sizeof(imageHTTPURL));
	    SWLOG_ERROR("[DOWNLOAD_WORKER] URL would be: %s/%s\n", effective_download_url, ctx->firmware_name);
	    g_free(effective_download_url);

src/dbus/rdkFwupdateMgr_handlers.c:2134

  • This log prints remCtrlUrl verbatim. If remCtrlUrl is a Direct CDN signed URL, it can leak auth tokens into logs. Consider redacting it consistently (similar to firmwareUrl).
    if (g_cached_xconf_data.remCtrlUrl[0]) {
        SWLOG_INFO("[CACHE_MEM]   - DirectCDN remCtrlUrl: '%s'\n", g_cached_xconf_data.remCtrlUrl);
    }

Comment thread src/dbus/rdkFwupdateMgr_handlers.c
Copilot AI review requested due to automatic review settings July 30, 2026 10:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (4)

src/dbus/rdkFwupdateMgr_handlers.c:2131

  • Direct CDN URLs can include signed query tokens; logging pdriUrl verbatim can leak credentials into logs. Consider redacting (as is already done for firmwareUrl).
    if (g_cached_xconf_data.pdriUrl[0]) {
        SWLOG_INFO("[CACHE_MEM]   - DirectCDN pdriUrl: '%s'\n", g_cached_xconf_data.pdriUrl);
    }

openspec/changes/direct-cdn-token-and-mtls-fix/proposal.md:33

  • This proposal’s “Impact / Scope boundary” claims the change does NOT touch directcdn.c, json_process.c, or daemon handlers, but this PR does modify those files. Updating this avoids confusing future readers and keeps the OpenSpec change record accurate.
- **Files modified**: `src/rdkv_upgrade.c` (two functions: `retryDownload()`, `downloadFile()`)
- **API changes**: None — no signature, struct, or Makefile changes
- **Test impact**: New unit tests for both paths; existing tests unaffected
- **Risk**: Low — both changes are additive guards with RFC kill-switch (`SWDLDirect.Enable=false` disables entire Direct CDN path)
- **Scope boundary**: Does NOT touch `directcdn.c`, `rdkv_main.c`, `json_process.c`, daemon handlers, or any other code paths

unittest/basic_rdkv_main_gtest.cpp:3095

  • These comments contradict each other ("no Codebig fallback" vs "fallback may be attempted"), and the stray CR character on the first comment line suggests an accidental line-ending artifact. Consider removing the conflicting commentary and asserting the expected curl result for clarity.
    /* With legacy mode, retries exhaust; HTTP 403 does not trigger Codebig fallback */

    int result = rdkv_upgrade_request(&context, &test_curl, &http_code);
    /* With legacy mode, retries exhaust and codebig fallback may be attempted */
    EXPECT_EQ(http_code, 403);

src/dbus/rdkv_dbus_server.c:2958

  • The decision to treat URLs containing '?' as “Direct CDN full URL” is brittle: legacy URLs can include query params, and signed Direct CDN URLs might not. This can cause the worker to skip appending the firmware filename (or append when it shouldn’t). Also, the truncation error log always prints "%s/%s" even when the code used the URL as-is.
    int url_len;
    if (strchr(effective_download_url, '?') != NULL) {
        /* Direct CDN: URL already contains full path + signed query-string tokens — use as-is */
        url_len = snprintf(imageHTTPURL, sizeof(imageHTTPURL), "%s", effective_download_url);
        SWLOG_INFO("[DOWNLOAD_WORKER] Direct CDN: using full signed URL as-is\n");
    } else {
        /* Legacy: cloudFWLocation is a directory — append firmware filename */
        url_len = snprintf(imageHTTPURL, sizeof(imageHTTPURL), "%s/%s", effective_download_url, ctx->firmware_name);
        SWLOG_INFO("[DOWNLOAD_WORKER] Legacy: appending firmware name to location URL\n");
    }
    if (url_len < 0 || url_len >= (int)sizeof(imageHTTPURL)) {
	    SWLOG_ERROR("[DOWNLOAD_WORKER] ERROR: URL too long or snprintf failed (len=%d, max=%zu)\n",
			    url_len, sizeof(imageHTTPURL));
	    SWLOG_ERROR("[DOWNLOAD_WORKER] URL would be: %s/%s\n", effective_download_url, ctx->firmware_name);

Comment on lines +2132 to +2134
if (g_cached_xconf_data.remCtrlUrl[0]) {
SWLOG_INFO("[CACHE_MEM] - DirectCDN remCtrlUrl: '%s'\n", g_cached_xconf_data.remCtrlUrl);
}
Copilot AI review requested due to automatic review settings July 30, 2026 10:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (4)

src/dbus/rdkFwupdateMgr_handlers.c:2134

  • Cache logging prints remCtrlUrl verbatim. If this URL is signed (query-string tokens), this can leak credentials into logs. Redact similarly to firmwareUrl.
    if (g_cached_xconf_data.remCtrlUrl[0]) {
        SWLOG_INFO("[CACHE_MEM]   - DirectCDN remCtrlUrl: '%s'\n", g_cached_xconf_data.remCtrlUrl);
    }

unittest/basic_rdkv_main_gtest.cpp:3095

  • This test assigns to result but never asserts or otherwise uses it, which can trigger -Wunused-variable warnings/errors depending on build flags. The adjacent comments also contradict each other (fallback "does not trigger" vs "may be attempted") and one line includes a stray CR character. Consider asserting result and removing the contradictory comment.
    EXPECT_CALL(mockexternal, checkCodebigAccess()).WillRepeatedly(Return(false));
    /* With legacy mode, retries exhaust; HTTP 403 does not trigger Codebig fallback */

    int result = rdkv_upgrade_request(&context, &test_curl, &http_code);
    /* With legacy mode, retries exhaust and codebig fallback may be attempted */
    EXPECT_EQ(http_code, 403);

openspec/changes/direct-cdn-token-and-mtls-fix/tasks.md:33

  • This task list marks unit tests as verifying getMtlscert call/NULL-cert behavior, but the added GTest cases use a mocked downloadFile() and explicitly note that mTLS bypass internals are verified via integration tests. To avoid overstating coverage, please update these checklist items to describe what the unit tests actually validate (routing / error propagation).
## 5. Unit Tests — mTLS Bypass

- [x] 5.1 Test: `direct_cdn=true` + `state_red=0` → `getMtlscert` NOT called, NULL cert passed
- [x] 5.2 Test: `direct_cdn=true` + `state_red=1` → `getMtlscert` IS called (recovery cert)
- [x] 5.3 Test: `direct_cdn=false` + `state_red=0` → existing mTLS path unchanged (getMtlscert called)

openspec/changes/direct-cdn-token-and-mtls-fix/specs/retry-recovery/spec.md:11

  • This scenario claims no sleep(delay) is executed in the iteration where HTTP 403 is observed. In the current retryDownload() implementation, sleep(delay) happens at the start of each retry iteration before calling downloadFile(), so this statement is not accurate. Consider rewording to assert that no further retries/delays occur after observing the 403.
- **AND** `context->direct_cdn == true`
- **THEN** the retry loop SHALL break immediately
- **AND** no `sleep(delay)` SHALL be executed for this iteration
- **AND** the 403 HTTP code SHALL be preserved in `*httpCode` for the caller

if (url_len < 0 || url_len >= (int)sizeof(imageHTTPURL)) {
SWLOG_ERROR("[DOWNLOAD_WORKER] ERROR: URL too long or snprintf failed (len=%d, max=%zu)\n",
url_len, sizeof(imageHTTPURL));
SWLOG_ERROR("[DOWNLOAD_WORKER] URL would be: %s/%s\n", effective_download_url, ctx->firmware_name);
Comment thread src/dbus/rdkFwupdateMgr_handlers.c Outdated
Copilot AI review requested due to automatic review settings July 30, 2026 10:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (6)

src/dbus/rdkv_dbus_server.c:2958

  • The error-path log always prints "URL would be: %s/%s" even when the Direct CDN branch uses the signed URL as-is; this can both mislead debugging and leak signed query-string tokens into logs. Consider logging a redacted message for signed URLs and only using the %s/%s format for the legacy directory+filename case.
    if (url_len < 0 || url_len >= (int)sizeof(imageHTTPURL)) {
	    SWLOG_ERROR("[DOWNLOAD_WORKER] ERROR: URL too long or snprintf failed (len=%d, max=%zu)\n",
			    url_len, sizeof(imageHTTPURL));
	    SWLOG_ERROR("[DOWNLOAD_WORKER] URL would be: %s/%s\n", effective_download_url, ctx->firmware_name);

unittest/basic_rdkv_main_gtest.cpp:3095

  • These two comments are contradictory about whether Codebig fallback may be attempted, and line 3092 appears to contain a stray CR character (inconsistent line endings). Consolidating the comment and asserting result makes the test intention clearer and avoids flaky formatting diffs.
    /* With legacy mode, retries exhaust; HTTP 403 does not trigger Codebig fallback */

    int result = rdkv_upgrade_request(&context, &test_curl, &http_code);
    /* With legacy mode, retries exhaust and codebig fallback may be attempted */
    EXPECT_EQ(http_code, 403);

openspec/changes/direct-cdn-token-and-mtls-fix/proposal.md:33

  • The proposal claims the change "Does NOT touch directcdn.c, json_process.c, daemon handlers", but this PR does modify those files. This makes the change record misleading for reviewers and future archaeology.
- **Files modified**: `src/rdkv_upgrade.c` (two functions: `retryDownload()`, `downloadFile()`)
- **API changes**: None — no signature, struct, or Makefile changes
- **Test impact**: New unit tests for both paths; existing tests unaffected
- **Risk**: Low — both changes are additive guards with RFC kill-switch (`SWDLDirect.Enable=false` disables entire Direct CDN path)
- **Scope boundary**: Does NOT touch `directcdn.c`, `rdkv_main.c`, `json_process.c`, daemon handlers, or any other code paths

src/dbus/rdkFwupdateMgr_handlers.c:2134

  • remCtrlUrl is logged in cleartext while the other Direct CDN URLs are redacted. If remCtrlUrl can contain signed tokens (similar to firmwareUrl/pdriUrl), this leaks credentials into logs; it also makes redaction inconsistent.
    if (g_cached_xconf_data.remCtrlUrl[0]) {
        SWLOG_INFO("[CACHE_MEM]   - DirectCDN remCtrlUrl: '%s'\n", g_cached_xconf_data.remCtrlUrl);
    }

openspec/changes/direct-cdn-token-and-mtls-fix/tasks.md:33

  • Section 5 is marked as unit-tested but the current gtests mock downloadFile() and (per the test comments) cannot observe getMtlscert() calls because the real downloadFile() is excluded under #ifndef GTEST_BASIC. The checklist items should be reworded to match what the unit tests actually validate (routing / state-red propagation) or moved to integration-test verification.
## 5. Unit Tests — mTLS Bypass

- [x] 5.1 Test: `direct_cdn=true` + `state_red=0` → `getMtlscert` NOT called, NULL cert passed
- [x] 5.2 Test: `direct_cdn=true` + `state_red=1` → `getMtlscert` IS called (recovery cert)
- [x] 5.3 Test: `direct_cdn=false` + `state_red=0` → existing mTLS path unchanged (getMtlscert called)

src/rdkv_upgrade.c:1119

  • This disabled #if 0 block adds production-code test scaffolding (checking a magic file under /tmp to force HTTP 403). Even though it’s compiled out, it increases maintenance burden and can accidentally be re-enabled; it’s better kept in test-only code or behind an explicit build-time test macro.
#if 0 /* Approach A - file-based 403 simulation (Build A in Jenkins) */
        /* Test hook: simulate HTTP 403 for Direct CDN firmware downloads.
         * Only fires for actual artifact downloads (direct_cdn=true, SSR_DIRECT),
         * not XConf queries. Create /tmp/.force_403_direct_cdn to trigger. */
        if (context->direct_cdn && server_type == HTTP_SSR_DIRECT) {

Comment thread src/directcdn.c
Comment thread src/rdkv_upgrade.c
Comment thread src/rdkv_upgrade.c Outdated
Copilot AI review requested due to automatic review settings July 31, 2026 07:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (5)

src/rdkv_upgrade.c:1313

  • The Direct CDN 403 break condition in retryDownload() currently triggers purely based on *httpCode, even when downloadFile() failed at the transport layer (curl_ret_code != CURL_SUCCESS). This can incorrectly short-circuit retries on a stale/meaningless HTTP code and will break the DirectCDNRetryDownloadTest.Http403_CurlFailure_DoesNotBreak expectation.
            } else if (context->direct_cdn && server_type == HTTP_SSR_DIRECT && *httpCode == 403) {
                SWLOG_INFO("%s: HTTP 403 with Direct CDN - token expired, breaking retry loop\n", __FUNCTION__);
                break;

src/dbus/rdkFwupdateMgr_handlers.c:2134

  • save_cached_xconf_data() redacts firmwareUrl/pdriUrl in logs, but remCtrlUrl is still logged verbatim. If remCtrlUrl is also a signed Direct CDN URL, this can leak token/query parameters into logs.
    if (g_cached_xconf_data.remCtrlUrl[0]) {
        SWLOG_INFO("[CACHE_MEM]   - DirectCDN remCtrlUrl: '%s'\n", g_cached_xconf_data.remCtrlUrl);
    }

openspec/changes/direct-cdn-token-and-mtls-fix/proposal.md:33

  • The proposal's Impact/Scope boundary is now inaccurate: this PR does modify directcdn.c, json_process.c, and D-Bus handler/server code in addition to src/rdkv_upgrade.c. Keeping this up to date helps reviewers understand the actual blast radius.
- **Files modified**: `src/rdkv_upgrade.c` (two functions: `retryDownload()`, `downloadFile()`)
- **API changes**: None — no signature, struct, or Makefile changes
- **Test impact**: New unit tests for both paths; existing tests unaffected
- **Risk**: Low — both changes are additive guards with RFC kill-switch (`SWDLDirect.Enable=false` disables entire Direct CDN path)
- **Scope boundary**: Does NOT touch `directcdn.c`, `rdkv_main.c`, `json_process.c`, daemon handlers, or any other code paths

src/dbus/rdkv_dbus_server.c:2954

  • Direct CDN vs legacy URL detection is currently based on whether the URL contains '?'. This is brittle: signed Direct CDN URLs are not guaranteed to use query tokens, and legacy URLs could include query params. It can result in either incorrectly appending the filename (breaking Direct CDN) or failing to append it (breaking legacy). The error log also always prints "%s/%s" even when using the URL as-is.
    if (strchr(effective_download_url, '?') != NULL) {
        /* Direct CDN: URL already contains full path + signed query-string tokens — use as-is */
        url_len = snprintf(imageHTTPURL, sizeof(imageHTTPURL), "%s", effective_download_url);
        SWLOG_INFO("[DOWNLOAD_WORKER] Direct CDN: using full signed URL as-is\n");
    } else {
        /* Legacy: cloudFWLocation is a directory — append firmware filename */
        url_len = snprintf(imageHTTPURL, sizeof(imageHTTPURL), "%s/%s", effective_download_url, ctx->firmware_name);
        SWLOG_INFO("[DOWNLOAD_WORKER] Legacy: appending firmware name to location URL\n");
    }

unittest/basic_rdkv_main_gtest.cpp:3095

  • This test block includes a stray carriage-return at the end of the comment line and a follow-up comment that contradicts the expectations (codebigdownloadFile is explicitly expected Times(0)). This makes the test harder to read and can cause noisy diffs on different platforms.
    /* With legacy mode, retries exhaust; HTTP 403 does not trigger Codebig fallback */

    int result = rdkv_upgrade_request(&context, &test_curl, &http_code);
    /* With legacy mode, retries exhaust and codebig fallback may be attempted */
    EXPECT_EQ(http_code, 403);

Copilot AI review requested due to automatic review settings July 31, 2026 08:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

src/rdkv_upgrade.c:1313

  • In retryDownload(), the new Direct CDN HTTP 403 break triggers purely on *httpCode, even when downloadFile() returned a non-success curl error. That can prematurely stop retries on transport failures and contradicts the nearby 403 handling in rdkv_upgrade_request() (which guards on CURL_SUCCESS). It also makes the new unit test Http403_CurlFailure_DoesNotBreak fail (it expects the loop to continue when curl_ret_code != CURL_SUCCESS).
            } else if (context->direct_cdn && server_type == HTTP_SSR_DIRECT && *httpCode == 403) {
                SWLOG_INFO("%s: HTTP 403 with Direct CDN - token expired, breaking retry loop\n", __FUNCTION__);
                break;

src/dbus/rdkFwupdateMgr_handlers.c:2134

  • save_cached_xconf_data() redacts firmwareUrl and pdriUrl in logs, but remCtrlUrl is logged verbatim. If remCtrlUrl can be a signed Direct CDN URL, this can leak tokens into logs. Redact remCtrlUrl consistently as well.
    if (g_cached_xconf_data.remCtrlUrl[0]) {
        SWLOG_INFO("[CACHE_MEM]   - DirectCDN remCtrlUrl: '%s'\n", g_cached_xconf_data.remCtrlUrl);
    }

openspec/changes/direct-cdn-token-and-mtls-fix/proposal.md:33

  • proposal.md claims the change “Does NOT touch directcdn.c, json_process.c, daemon handlers…” but this PR does modify those paths (src/directcdn.c, src/json_process.c, src/dbus/, unittest/). This makes the OpenSpec proposal’s scope/impact section inaccurate for reviewers and future traceability.
- **Files modified**: `src/rdkv_upgrade.c` (two functions: `retryDownload()`, `downloadFile()`)
- **API changes**: None — no signature, struct, or Makefile changes
- **Test impact**: New unit tests for both paths; existing tests unaffected
- **Risk**: Low — both changes are additive guards with RFC kill-switch (`SWDLDirect.Enable=false` disables entire Direct CDN path)
- **Scope boundary**: Does NOT touch `directcdn.c`, `rdkv_main.c`, `json_process.c`, daemon handlers, or any other code paths

src/dbus/rdkv_dbus_server.c:2955

  • The Direct CDN URL detection here relies on the presence of a '?' to decide whether to append the firmware filename. However, this codebase already uses per-artifact Direct CDN URLs like "https://cdn.direct/fw/.bin" (no query string) in tests, and those would be incorrectly treated as legacy directories, producing "...bin/.bin". Use a check that the URL already includes the expected filename (or track whether the URL came from firmwareUrl/pdriUrl/remCtrlUrl) instead of checking for '?'.
    if (strchr(effective_download_url, '?') != NULL) {
        /* Direct CDN: URL already contains full path + signed query-string tokens — use as-is */
        url_len = snprintf(imageHTTPURL, sizeof(imageHTTPURL), "%s", effective_download_url);
        SWLOG_INFO("[DOWNLOAD_WORKER] Direct CDN: using full signed URL as-is\n");
    } else {
        /* Legacy: cloudFWLocation is a directory — append firmware filename */
        url_len = snprintf(imageHTTPURL, sizeof(imageHTTPURL), "%s/%s", effective_download_url, ctx->firmware_name);
        SWLOG_INFO("[DOWNLOAD_WORKER] Legacy: appending firmware name to location URL\n");
    }

Comment thread src/rdkv_upgrade.c Outdated
Copilot AI review requested due to automatic review settings July 31, 2026 08:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (5)

src/dbus/rdkv_dbus_server.c:2958

  • The error-path log always formats the URL as "%s/%s", but in Direct CDN mode the attempted URL is not constructed by appending the firmware name. This message can be misleading, and it can also leak signed query-string tokens into logs. Consider redacting the URL in this error log (or logging a sanitized form) and avoid implying a location/filename structure.
	    SWLOG_ERROR("[DOWNLOAD_WORKER] ERROR: URL too long or snprintf failed (len=%d, max=%zu)\n",
			    url_len, sizeof(imageHTTPURL));
	    SWLOG_ERROR("[DOWNLOAD_WORKER] URL would be: %s/%s\n", effective_download_url, ctx->firmware_name);

src/rdkv_upgrade.c:1313

  • In retryDownload(), the new Direct CDN HTTP 403 break condition triggers solely based on *httpCode. If downloadFile() returns a transport error but leaves *httpCode unchanged (e.g., from a previous iteration), the loop could incorrectly break and skip intended retries. Guard the break with curl_ret_code == CURL_SUCCESS so it only short-circuits on an actual HTTP 403 response.
            } else if (context->direct_cdn && server_type == HTTP_SSR_DIRECT && *httpCode == 403) {
                SWLOG_INFO("%s: HTTP 403 with Direct CDN - token expired, breaking retry loop\n", __FUNCTION__);
                break;

src/dbus/rdkFwupdateMgr_handlers.c:2134

  • The cache log prints remCtrlUrl verbatim, while firmwareUrl and pdriUrl are explicitly redacted. If remCtrlUrl is also a Direct CDN signed URL, this can leak token parameters in logs. Redact it consistently.
    if (g_cached_xconf_data.remCtrlUrl[0]) {
        SWLOG_INFO("[CACHE_MEM]   - DirectCDN remCtrlUrl: '%s'\n", g_cached_xconf_data.remCtrlUrl);
    }

openspec/changes/direct-cdn-token-and-mtls-fix/proposal.md:33

  • This proposal states the change "Does NOT touch directcdn.c, json_process.c, daemon handlers" but this PR does modify those files. Please update the Impact section to match the actual diff so the change artifact remains accurate.
- **Files modified**: `src/rdkv_upgrade.c` (two functions: `retryDownload()`, `downloadFile()`)
- **API changes**: None — no signature, struct, or Makefile changes
- **Test impact**: New unit tests for both paths; existing tests unaffected
- **Risk**: Low — both changes are additive guards with RFC kill-switch (`SWDLDirect.Enable=false` disables entire Direct CDN path)
- **Scope boundary**: Does NOT touch `directcdn.c`, `rdkv_main.c`, `json_process.c`, daemon handlers, or any other code paths

openspec/changes/direct-cdn-token-and-mtls-fix/tasks.md:33

  • The "Unit Tests — mTLS Bypass" items currently claim unit tests verify getMtlscert() is (not) called, but the added gtests mock downloadFile() and (per the comment in the test file) cannot observe cert-fetch internals. Update these checklist items to reflect that the unit tests validate routing/propagation, while cert-fetch bypass itself needs integration coverage.
## 5. Unit Tests — mTLS Bypass

- [x] 5.1 Test: `direct_cdn=true` + `state_red=0` → `getMtlscert` NOT called, NULL cert passed
- [x] 5.2 Test: `direct_cdn=true` + `state_red=1` → `getMtlscert` IS called (recovery cert)
- [x] 5.3 Test: `direct_cdn=false` + `state_red=0` → existing mTLS path unchanged (getMtlscert called)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants