rebase

[nhanasi]

No description provided.

[github-actions]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[Copilot]

Pull request overview

This PR consolidates multiple changes from a rebase operation, including network script refactoring, log upload binary migration, and configuration updates.

Changes:

• Deleted updateGlobalIPInfo.sh and refactored its functionality into NM_Dispatcher.sh and NM_preDown.sh
• Added support for a binary log upload implementation with fallback to script-based approach in Start_MaintenanceTasks.sh
• Added com.comcast.viper_ipa to warehouse testing exceptions in wh_api_5.conf

Reviewed changes

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

Show a summary per file

File	Description
lib/rdk/wh_api_5.conf	Added viper_ipa app to warehouse testing exception list
lib/rdk/updateGlobalIPInfo.sh	Deleted file - functionality moved to NM dispatcher scripts
lib/rdk/Start_MaintenanceTasks.sh	Integrated binary-based log upload with fallback to shell script
lib/rdk/NM_preDown.sh	Refactored IP deletion logic from updateGlobalIPInfo.sh
lib/rdk/NM_Dispatcher.sh	Refactored IP addition logic from updateGlobalIPInfo.sh
CHANGELOG.md	Updated changelog for version 4.2.2 with merged PRs

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The check_valid_IPaddress function has inconsistent implementations between NM_preDown.sh and NM_Dispatcher.sh. In NM_preDown.sh, it uses POSIX-compliant single equals (=) and a case statement for IPv6 pattern matching. In NM_Dispatcher.sh, it uses bash-specific double equals (==) and double bracket conditionals. These functions should be identical to maintain consistency and predictable behavior across both scripts.

Suggested change

	if [ "x$mode" == "xipv6" ]; then
	if [[ $addr == fc* || $addr == fd* ]]; then
	return 1
	fi
	elif [ "x$mode" == "xipv4" ]; then
	autoIPTrunc=$(echo $addr | cut -d "." -f1-2)
	if [ "$autoIPTrunc" == "169.254" ]; then
	return 1
	fi
	if [ "x$mode" = "xipv6" ]; then
	case "$addr" in
	fc*|fd*)
	return 1
	;;
	esac
	elif [ "x$mode" = "xipv4" ]; then
	autoIPTrunc=$(echo "$addr" | cut -d "." -f1-2)
	if [ "$autoIPTrunc" = "169.254" ]; then
	return 1
	fi

[Copilot]

The echo "$addr" > /tmp/.$mode$ESTB_INTERFACE (and similar for MOCA_INTERFACE and WIFI_INTERFACE) writes to a predictable path in /tmp as root without any protection against symlink attacks. A local attacker who can create a symlink at /tmp/.$mode$ESTB_INTERFACE (or the other variants) can cause this script to truncate or overwrite an arbitrary file (e.g., /etc/shadow), leading to privilege escalation or data corruption. Use a safer pattern for temporary files (e.g., a dedicated directory with restricted permissions or APIs that open files with O_NOFOLLOW/proper checks) so that writes cannot be redirected via attacker-controlled symlinks.

[Copilot]

Pull request overview

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

Suppressed comments (6)

lib/rdk/startStunnel.sh:162

• Creating the FIFO path with mktemp -u is vulnerable to a TOCTOU race (another process can pre-create that path). Also, if mkfifo/exec fails the script continues, which can cause the passcode write to fail silently or block unexpectedly. Use mktemp without -u, create the FIFO with restrictive permissions, and exit on failure.

    # Create a named pipe
    PIPE=$(mktemp -u)
    if ! mkfifo "$PIPE" 2>/dev/null; then
        echo_t "STUNNEL: ERROR - Failed to create named pipe"
    fi

lib/rdk/startStunnel.sh:173

• echo "$(eval "$PASSCODE")" executes the content of PASSCODE as code. If PASSCODE can be influenced by untrusted input, this becomes a command-injection vector. If PASSCODE is meant to be a literal secret, prefer writing it directly (e.g., printf '%s\n' "$PASSCODE").

    echo "$(eval "$PASSCODE")" >&$FD_NUMBER &

lib/rdk/timesyncd-conf-update.sh:66

• get_bs_val() uses grep -E with an unescaped key containing . (e.g. Device.Time.NTPServer1). In ERE, . matches any character, so this can match the wrong line in bootstrap.ini. Prefer a non-regex match (shell pattern / fixed-string) for the key.

    get_bs_val() {
        key="$1"
        # Extract RHS after '=' and trim whitespace
        grep -m1 -E "^[[:space:]]*$key=" "$BOOTSTRAP" 2>/dev/null | \
            cut -d'=' -f2- | sed 's/^[[:space:]]*//; s/[[:space:]]*$//'
    }

lib/rdk/readBTAddress-generic.sh:31

• When Bluetooth is disabled, this script prints an empty string. getDeviceDetails.sh:getBluetoothMac() sets a default but then overwrites it with this script’s output, so an empty output here can erase the default value. Return a stable default (e.g. 00:00:00:00:00:00) when Bluetooth is disabled or unavailable.
  lib/rdk/NM_Bootstrap.sh:122
• If /opt/secure/NetworkManager/system-connections/ is empty, the glob .../* is passed literally to grep, which logs an error (and could cause unintended behavior if set -e is ever enabled). Guard the loop so it only processes real files.

         for f in /opt/secure/NetworkManager/system-connections/*; do
             if grep -q "type=wifi" "$f"; then
                 rm -f "$f"
             fi
         done

lib/rdk/alertSystem.sh:107

• PROCESS_NAME and MSG_DATA are interpolated into JSON without escaping. If either contains quotes, backslashes, or newlines, the JSON becomes invalid; additionally, the deepSleep branch uses MSG_DATA as a JSON key which can be abused to alter the payload shape. Escape these fields (at minimum \ and ") before constructing JSON.

if [ "x$PROCESS_NAME" == "xdeepSleepMgrMain" ]; then
    # Message data is actual metadata header in case of trigger from deepSleep manager process
    # This change is needed since there are data clouds in different deployment which are not flexible to accomodate any deviations in data format
    strjson="{\"searchResult\":[{\"Time\":\"$currentTime\"},{\"process_name\":\"$PROCESS_NAME\"},{\"mac\":\"$estb_mac\"},{\"Version\":\"$software_version\"},{\"PartnerId\":\"$partnerId\"},{\"$MSG_DATA\":\"1\"}]}"
else
    strjson="{\"searchResult\":[{\"process_name\":\"$PROCESS_NAME\"},{\"mac\":\"$estb_mac\"},{\"Version\":\"$software_version\"},{\"msgTime\":\"$currentTime\"},{\"PartnerId\":\"$partnerId\"},{\"logEntry\":\"$MSG_DATA\"}]}"
fi

[Copilot]

Pull request overview

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

Suppressed comments (5)

lib/rdk/timesyncd-conf-update.sh:66

• get_bs_val builds an ERE pattern from the raw TR-181 key (e.g., Device.Time.NTPServer1). Because . and other characters are regex metacharacters, this can match unintended lines in bootstrap.ini and return the wrong NTP servers. Escape the key (or avoid regex) before grepping.

    get_bs_val() {
        key="$1"
        # Extract RHS after '=' and trim whitespace
        grep -m1 -E "^[[:space:]]*$key=" "$BOOTSTRAP" 2>/dev/null | \
            cut -d'=' -f2- | sed 's/^[[:space:]]*//; s/[[:space:]]*$//'
    }

lib/rdk/readBTAddress-generic.sh:31

• When Bluetooth is disabled (or getDeviceBluetoothMac returns empty), this script echoes an empty string. Callers like getDeviceDetails.sh initialize a default MAC but then overwrite it with this output, resulting in a blank bluetooth_mac instead of a sentinel value.
  lib/rdk/startStunnel.sh:146
• get_next_fd does not actually detect a free file descriptor: true >&$fd will generally succeed regardless of whether the FD is already open, so the loop never finds a candidate and will hit the error path. This means the passcode pipe setup may fail even in normal operation.

get_next_fd() {
    local fd=3  # Start checking from FD 3 (since 0, 1, 2 are standard in/out/err)
    local max_fd=20  # Maximum FD to check

    # Try to redirect to /dev/null and check if the FD is free
    while [ $fd -le $max_fd ]; do
        if ! { true >&$fd; } 2>/dev/null; then
            echo "$fd" # Output the available FD (goes to standard output)
            return 0 # Set exit status to success
        fi
        fd=$((fd + 1))
    done

lib/rdk/alertSystem.sh:23

• Header comment says this script "is used to backup the Logs", but the implementation posts an alert payload to an upload endpoint. This mismatch makes the script’s purpose/usage unclear during ops/debug.

# Purpose: This script is used to backup the Logs
# Scope: RDK devices
# Usage: This script is triggered by systemd service 
##############################################################################

lib/rdk/alertSystem.sh:113

• CURL_INPUT uses -d '$strjson', which keeps $strjson as a literal string inside the command rather than sending the JSON payload. Unless exec_curl_mtls performs its own variable interpolation (not shown here), the server will receive $strjson instead of the intended JSON.

if [ -f $EnableOCSPStapling ] || [ -f $EnableOCSP ]; then
    CURL_INPUT=" -w '%{http_code}\n' -H \"Accept: application/json\" -H \"Content-type: application/json\" -X POST -d '$strjson' -o \"$HTTP_FILENAME\" \"$UPLOAD_END_POINT\" --cert-status --connect-timeout 30 -m 30 "
else
    CURL_INPUT=" -w '%{http_code}\n' -H \"Accept: application/json\" -H \"Content-type: application/json\" -X POST -d '$strjson' -o \"$HTTP_FILENAME\" \"$UPLOAD_END_POINT\" --connect-timeout 30 -m 30 "
fi

[Copilot]

Pull request overview

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

Suppressed comments (5)

lib/rdk/getDeviceDetails.sh:286

• getBluetoothMac() initializes a default MAC but then unconditionally overwrites it with the output of readBTAddress-*.sh. If that script returns an empty string (e.g., failure/unsupported), this function will return an empty value instead of the intended fallback MAC.

getBluetoothMac()
{
    bluetooth_mac="00:00:00:00:00:00"
    if [ -f /lib/rdk/readBTAddress-vendor.sh ]; then
        bluetooth_mac=`sh /lib/rdk/readBTAddress-vendor.sh`
    else
        bluetooth_mac=`sh /lib/rdk/readBTAddress-generic.sh`
    fi

lib/rdk/timesyncd-conf-update.sh:66

• get_bs_val() uses an extended-regex pattern that interpolates the key name directly. Keys like "Device.Time.NTPServer1" contain dots, which are treated as regex wildcards, so this can match unintended lines and return the wrong value from bootstrap.ini.

    # Helper to fetch key=value from bootstrap.ini (first match)
    get_bs_val() {
        key="$1"
        # Extract RHS after '=' and trim whitespace
        grep -m1 -E "^[[:space:]]*$key=" "$BOOTSTRAP" 2>/dev/null | \
            cut -d'=' -f2- | sed 's/^[[:space:]]*//; s/[[:space:]]*$//'
    }

lib/rdk/startStunnel.sh:162

• Creating the FIFO path with mktemp -u is vulnerable to a TOCTOU race (an attacker can pre-create the path in /tmp). This can break the pipe setup and may allow unintended access to data written to the FIFO. Prefer creating a private temp directory (mktemp -d) and placing the FIFO inside it.

    # Create a named pipe
    PIPE=$(mktemp -u)
    if ! mkfifo "$PIPE" 2>/dev/null; then
        echo_t "STUNNEL: ERROR - Failed to create named pipe"
    fi

lib/rdk/alertSystem.sh:107

• strjson is built by directly interpolating PROCESS_NAME and MSG_DATA into JSON without escaping. If MSG_DATA contains quotes, backslashes, or newlines, the payload becomes invalid JSON (or changes structure), causing upload failures or data integrity issues.

if [ "x$PROCESS_NAME" == "xdeepSleepMgrMain" ]; then
    # Message data is actual metadata header in case of trigger from deepSleep manager process
    # This change is needed since there are data clouds in different deployment which are not flexible to accomodate any deviations in data format
    strjson="{\"searchResult\":[{\"Time\":\"$currentTime\"},{\"process_name\":\"$PROCESS_NAME\"},{\"mac\":\"$estb_mac\"},{\"Version\":\"$software_version\"},{\"PartnerId\":\"$partnerId\"},{\"$MSG_DATA\":\"1\"}]}"
else
    strjson="{\"searchResult\":[{\"process_name\":\"$PROCESS_NAME\"},{\"mac\":\"$estb_mac\"},{\"Version\":\"$software_version\"},{\"msgTime\":\"$currentTime\"},{\"PartnerId\":\"$partnerId\"},{\"logEntry\":\"$MSG_DATA\"}]}"
fi

lib/rdk/NM_Bootstrap.sh:122

• The loop over /opt/secure/NetworkManager/system-connections/* will execute once with the literal glob pattern when the directory is empty, causing grep errors and noisy logs. Guard for the no-matches case before grepping/removing.

         for f in /opt/secure/NetworkManager/system-connections/*; do
             if grep -q "type=wifi" "$f"; then
                 rm -f "$f"
             fi
         done

[Copilot]

🟡 Changes recommended

Critical release-workflow and reboot-path findings, plus additional runtime issues, remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (17)

Previously missed (3) — in code that hasn't changed since the last review.

.github/workflows/component-release.yml:254

• Failure cleanup only deletes the remote release branch when the local branch still exists. git flow release finish removes the local release/<version> branch before the later pushes, so a post-finish push failure skips this cleanup and leaves the published remote release branch behind. The creation flag is sufficient to decide whether this run owns the remote branch; do not gate deletion on the local ref.
  lib/rdk/startStunnel.sh:199
• This new early exit bypasses the cleanup below, so a failed stunnel start leaves the generated configuration and temporary data file on disk. Move the existing cleanup into this failure path (at least the config and $D_FILE) before returning.
  .github/workflows/component-release-workflow-usage.md:92
• The documentation says to enforce the approvable-flow review rules on develop, but the workflow creates the approval PR against main (lines 160-162) and the finish workflow also requires base.ref == 'main'. This can leave the actual release gate unprotected; the prerequisite should name main.

.github/workflows/component-release-finish-on-approval.yml:76

• When the tag already exists, this step exits the entire job before the later Close the release PR step runs. A retry after a successful finish (or a pre-existing tag) therefore leaves the release PR open, contrary to the documented cleanup; handle the already-finished case while still closing the PR, rather than exiting the job here.

          # Skip if tag already exists
          if git ls-remote --exit-code --tags origin "refs/tags/${release_version}" >/dev/null 2>&1; then
            echo "Tag ${release_version} already exists on origin. Skipping release finish."
            exit 0

.github/workflows/component-release.yml:209

• This branch only checks for an existing local hotfix branch. If a prior run published hotfix/${RELEASE_VERSION} and then failed, a retry sees the remote branch but git checkout -b fails because that branch already exists on origin, contradicting the documented create/use behavior. Detect and check out the remote branch before creating a new one.

          if git show-ref --verify --quiet "refs/heads/${hotfix_branch}"; then
            git checkout "${hotfix_branch}"
          else
            git checkout -b "${hotfix_branch}" "${SOURCE_BRANCH}"

lib/rdk/alertSystem.sh:106

• strjson interpolates MSG_DATA directly into JSON. An alert containing a quote, backslash, or newline produces invalid JSON (and the value is also embedded in the shell-quoted curl command), causing otherwise valid alerts to fail or be sent with malformed metadata. JSON-escape all dynamic fields and pass the payload without relying on interpolated shell syntax.

if [ "x$PROCESS_NAME" == "xdeepSleepMgrMain" ]; then
    # Message data is actual metadata header in case of trigger from deepSleep manager process
    # This change is needed since there are data clouds in different deployment which are not flexible to accomodate any deviations in data format
    strjson="{\"searchResult\":[{\"Time\":\"$currentTime\"},{\"process_name\":\"$PROCESS_NAME\"},{\"mac\":\"$estb_mac\"},{\"Version\":\"$software_version\"},{\"PartnerId\":\"$partnerId\"},{\"$MSG_DATA\":\"1\"}]}"
else
    strjson="{\"searchResult\":[{\"process_name\":\"$PROCESS_NAME\"},{\"mac\":\"$estb_mac\"},{\"Version\":\"$software_version\"},{\"msgTime\":\"$currentTime\"},{\"PartnerId\":\"$partnerId\"},{\"logEntry\":\"$MSG_DATA\"}]}"

lib/rdk/alertSystem.sh:55

• This script never sources /etc/include.properties, while RDK_PATH is defined there (etc/include.properties:29) and is not defined by the checked-in device.properties. Consequently the check resolves to /exec_curl_mtls.sh, exits at line 58, and no alert is uploaded on the normal image; source the include properties before using RDK_PATH.

if [ -f $RDK_PATH/exec_curl_mtls.sh ]; then
    . $RDK_PATH/exec_curl_mtls.sh

lib/rdk/factory-reset.sh:50

• Without stopping wpeframework.service, the framework remains active while the cleanup below deletes persistent/configuration state under /opt (including /opt/persistent, /opt/QT, and connection profiles). It can race the reset and recreate or write data; keep this stop or otherwise quiesce the service before cleanup.

    /bin/systemctl stop syslog.socket

lib/rdk/getDeviceDetails.sh:285

• The Bluetooth-enabled guard was removed from this caller, but the new generic reader leaves bluetooth_mac unset when BLUETOOTH_ENABLED is false. On such devices this overwrites the existing 00:00:00:00:00:00 fallback with an empty value in the device-details cache. Keep the reader invocation conditional or preserve the fallback when Bluetooth is disabled.

    if [ -f /lib/rdk/readBTAddress-vendor.sh ]; then
        bluetooth_mac=`sh /lib/rdk/readBTAddress-vendor.sh`
    else
        bluetooth_mac=`sh /lib/rdk/readBTAddress-generic.sh`

lib/rdk/rebootNow.sh:1

• Deleting this implementation leaves in-tree callers such as lib/rdk/utils.sh:65 and warehouse-reset.sh:234, plus the Makefile's /rebootNow.sh link target, still pointing at /lib/rdk/rebootNow.sh. No replacement is added in this change, so warehouse, factory-reset, and image-upgrade reboot paths will invoke a missing script after packaging. Retain a compatibility wrapper or update every caller and the installed link to the replacement binary.
  lib/rdk/startLinkLocal.sh:55
• If avahi-autoipd cannot start, this script still logs Started and exits successfully because the command status is ignored. Callers then believe link-local setup succeeded and will not retry or report the failure. Check the command status and return a nonzero result on startup failure.

# Start avahi-autoipd
/usr/sbin/avahi-autoipd --daemonize --syslog "$INTERFACE"
Log "Started avahi-autoipd for $INTERFACE"

lib/rdk/startStunnel.sh:115

• The configuration always writes checkHost=$JUMP_FQDN and then writes a second checkHost in the non-empty device-type branches. Stunnel may reject duplicate options, and if it accepts them the later SAN silently overrides the jump hostname; emit exactly one selected checkHost value instead.

echo "checkHost   = $JUMP_FQDN"          >> $STUNNEL_CONF_FILE

lib/rdk/startStunnel.sh:160

• mktemp -u only prints an unused pathname; an attacker can create or replace that path before mkfifo/exec, causing the passcode written at line 173 to be redirected to an attacker-controlled FIFO or file. Use an atomically created private temporary directory/FIFO and clean it up on every error path.

    PIPE=$(mktemp -u)
    if ! mkfifo "$PIPE" 2>/dev/null; then

lib/rdk/startStunnel.sh:218

• After changing the generated certificate variable from CERT_FILE to CERT_PATH, this failure path no longer removes the client certificate. When the stunnel PID file is missing, the CA is deleted but the generated certificate is left behind; remove the renamed path as well.

        rm -f $CA_FILE

lib/rdk/start_ssh.sh:88

• This condition compares the literal string BUILD_TYPE, so it is always true. The dev branch is therefore unreachable, and even that branch currently clears USE_DEVKEYS despite logging that dev keys should be used; dev builds can select production keys instead of the intended default. Expand the variable and set the dev key file in the dev branch.

USE_DEVKEYS="-f authorized_keys_dev"
DEVICETYPE=$(tr181 -g Device.DeviceInfo.X_RDKCENTRAL-COM_RFC.Identity.DeviceType 2>&1)
if [ "$BUILD_TYPE" = "prod" ] && [ "$DEVICETYPE" = "PROD" ]; then
    USE_DEVKEYS=""
fi

/bin/systemctl set-environment USE_DEVKEYS="$USE_DEVKEYS"

#RFC check for MOCA SSH enable/not.
isMOCASSHEnable=$(/usr/bin/tr181Set -d  Device.DeviceInfo.X_RDKCENTRAL-COM_RFC.Feature.MOCASSH.Enable 2>&1 > /dev/null)

echo "RFC_ENABLE_MOCASSH:$isMOCASSHEnable"

lib/rdk/wh_api_5.conf:46

• This exact x1hello.object path is listed under [dirs], while this file defines [files] for file paths and [dirs] for directories. API_5 consumers that dispatch by section can attempt directory traversal on this file and skip the intended cleanliness check. Move the entry into [files].
  systemd_units/gstreamer-cleanup.service:1
• This removes the only systemd unit in this tree that performs the boot-time CDL/gstreamer registry cleanup. There is no replacement unit or script among the remaining files, so a post-CDL boot will no longer rebuild /opt/.gstreamer/registry.bin when the flashed image changes and can retain a stale registry. Restore the unit or wire its behavior to the replacement component.

• Files reviewed: 42/42 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings remain in release workflow security, reboot compatibility, and runtime behavior.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (16)

Previously missed (1) — in code that hasn't changed since the last review.

.github/workflows/component-release-workflow-usage.md:93

• The approvable workflow creates release/<version> pull requests targeting main (see component-release.yml:160-162), but these prerequisites tell operators to enforce review rules on develop. Following this documentation will leave the triggering PR unprotected and can prevent the approval-gated flow from matching repository policy.

.github/workflows/component-release-finish-on-approval.yml:76

• If an earlier run already pushed the tag but failed before closing the PR or deleting the branch, this early exit skips all cleanup and leaves the approved release PR open. Subsequent review events will hit the same condition, so handle the existing-tag state by verifying/completing the release and closing or cleaning up the PR.

          if git ls-remote --exit-code --tags origin "refs/tags/${release_version}" >/dev/null 2>&1; then
            echo "Tag ${release_version} already exists on origin. Skipping release finish."
            exit 0

lib/rdk/NM_Dispatcher.sh:187

• This branch logs Started avahi-autoipd before checking the command and ignores its exit status. A failed daemon launch is therefore reported as successful and the handler exits 0, preventing reliable recovery; check the return status and log the failure instead.

                    NMdispatcherLog "Started avahi-autoipd for $iface"
                    /usr/sbin/avahi-autoipd --daemonize --syslog "$iface"

lib/rdk/alertSystem.sh:106

• PROCESS_NAME and MSG_DATA are interpolated directly into the JSON string. A valid alert containing a quote, backslash, or newline will produce malformed JSON and cause the POST to fail; encode these fields with a JSON serializer before constructing the request payload.

if [ "x$PROCESS_NAME" == "xdeepSleepMgrMain" ]; then
    # Message data is actual metadata header in case of trigger from deepSleep manager process
    # This change is needed since there are data clouds in different deployment which are not flexible to accomodate any deviations in data format
    strjson="{\"searchResult\":[{\"Time\":\"$currentTime\"},{\"process_name\":\"$PROCESS_NAME\"},{\"mac\":\"$estb_mac\"},{\"Version\":\"$software_version\"},{\"PartnerId\":\"$partnerId\"},{\"$MSG_DATA\":\"1\"}]}"
else
    strjson="{\"searchResult\":[{\"process_name\":\"$PROCESS_NAME\"},{\"mac\":\"$estb_mac\"},{\"Version\":\"$software_version\"},{\"msgTime\":\"$currentTime\"},{\"PartnerId\":\"$partnerId\"},{\"logEntry\":\"$MSG_DATA\"}]}"

lib/rdk/getDeviceDetails.sh:285

• readBTAddress-generic.sh emits an empty string when BLUETOOTH_ENABLED is not true, but this function now overwrites the initialized 00:00:00:00:00:00 value with that output on the generic path. Callers on Bluetooth-disabled devices will therefore receive an empty MAC instead of the previous sentinel value.

    if [ -f /lib/rdk/readBTAddress-vendor.sh ]; then
        bluetooth_mac=`sh /lib/rdk/readBTAddress-vendor.sh`
    else
        bluetooth_mac=`sh /lib/rdk/readBTAddress-generic.sh`

lib/rdk/readBTAddress-generic.sh:31

• When BLUETOOTH_ENABLED is not true, bluetooth_mac is never initialized and this script prints an empty value. getDeviceDetails.sh now always replaces its 00:00:00:00:00:00 default with this output, so Bluetooth-disabled devices lose the required default MAC value.
  lib/rdk/rebootNow.sh:1
• Removing this script leaves the repository's Makefile:86 creating /rebootNow.sh as a dangling link, while current callers such as utils.sh, factory-reset.sh, warehouse-reset.sh, restore, and userInitiatedFWDnld.sh still execute that path. Those reboot paths will fail on images built from this tree; update the callers/install target to the replacement binary or retain a compatibility wrapper.
  lib/rdk/startLinkLocal.sh:55
• A failure from avahi-autoipd is only logged, then the script exits successfully with a misleading "Started" message. Callers cannot detect that IPv4LL startup failed and will not retry or report the failure; check the command status and return a nonzero result on failure.

/usr/sbin/avahi-autoipd --daemonize --syslog "$INTERFACE"
Log "Started avahi-autoipd for $INTERFACE"

lib/rdk/startStunnel.sh:115

• The generated stunnel configuration now writes checkHost = $JUMP_FQDN here and then appends another checkHost at lines 123 or 127. Duplicate directives can make stunnel reject the configuration or cause verification to use a value different from the intended device-type SAN; emit only the selected host-check directive.

echo "checkHost   = $JUMP_FQDN"          >> $STUNNEL_CONF_FILE

lib/rdk/startStunnel.sh:160

• mktemp -u only suggests a name; it does not reserve it. Because this path is in shared /tmp, another local process can win the race before mkfifo (and the code continues even when mkfifo fails), potentially redirecting the passcode written at line 173 to an attacker-controlled file. Create a private temporary directory and the FIFO inside it, and abort on creation/open failure.

    PIPE=$(mktemp -u)
    if ! mkfifo "$PIPE" 2>/dev/null; then

lib/rdk/startStunnel.sh:218

• The certificate path was changed from CERT_FILE to CERT_PATH above, but this failure cleanup now removes only the CA file. A failed stunnel startup therefore leaves the client certificate/P12 on disk, unlike the previous path, and the credential can persist after the retry marker is set. Remove the actual certificate path here as well.

        rm -f $STUNNEL_PID_FILE
        rm -f $CA_FILE

lib/rdk/system_info_collector.sh:98

• This hunk removes the only call to the still-defined log_disk_usage function. The scheduled system information collector will no longer emit disk-space usage, despite retaining the collector function and its documented system-information purpose; keep the call or remove/update that contract intentionally.

uptime
run_top_command
cpu_statistics

lib/rdk/temperature-telemetry.sh:25

• The repository invokes this utility as /QueryPowerState in lib/rdk/togglePower:23, but this change hard-codes /usr/bin/QueryPowerState. On images where the existing utility is installed at the root path, this command fails, powerState is empty, and the script falls through to reading the thermal zone during deep sleep—the behavior this change is meant to prevent.

powerState=$(/usr/bin/QueryPowerState)

lib/rdk/timesyncd-conf-update.sh:99

• The bootstrap fallback is only called when all five TR-181 values are empty. If TR-181 supplies one server but leaves other slots empty, the helper's stated behavior of filling missing values is skipped and those bootstrap servers are never used; merge bootstrap values for missing slots whenever the TR-181 result is partial.

    if [ "$hostName" ] || [ "$hostName2" ] || [ "$hostName3" ] || [ "$hostName4" ] || [ "$hostName5" ]; then
        break
    fi

     # If this is the last attempt, try bootstrap as fallback and then break
    if [ $attempts -eq $max_attempts ]; then
        ntpLog "TR-181 returned empty NTP server list; falling back to /opt/secure/RFC/bootstrap.ini..."
        get_ntp_hosts_from_bootstrap
        break

systemd_units/gstreamer-cleanup.service:1

• This removes the only gstreamer-cleanup.service from the component, and no replacement unit or executable is added. Consequently the existing boot-time registry cleanup and firmware-update ExecStop cleanup no longer run, leaving stale GStreamer state after CDL/update flows; retain the unit or add its replacement before deleting it.
  systemd_units/reboot-reason-logger.service:1
• This deletion removes the boot-time unit that was the in-tree caller of reboot-checker.sh bootup; the path unit and both reboot-info scripts are deleted as well, and no replacement unit/caller for update-prev-reboot-info is added. As a result, previous reboot information will no longer be processed or written on images built from this package. Add the replacement service/path or retain the existing trigger.

• Files reviewed: 42/42 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Critical unresolved release-automation and reboot-entry-point issues, plus additional runtime and security defects, must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (16)

.github/workflows/component-release-finish-on-approval.yml:77

• If a previous run pushed the tag but failed before deleting the release branch or closing the PR, this early exit leaves the approved release PR and branch permanently open. Handle the already-tagged case by completing cleanup/closure (or by verifying the remaining pushes) instead of returning before those steps.

          if git ls-remote --exit-code --tags origin "refs/tags/${release_version}" >/dev/null 2>&1; then
            echo "Tag ${release_version} already exists on origin. Skipping release finish."
            exit 0
          fi

.github/workflows/component-release-workflow-usage.md:92

• The approvable workflow creates a PR targeting main and the finish workflow requires base.ref == 'main', so telling operators to enforce review rules on develop does not protect the approval gate this workflow actually uses.

- For approvable flow: enforce PR review rules on `develop`.

.github/workflows/component-release.yml:210

• The hotfix lookup checks only local refs. If a prior run left hotfix/${RELEASE_VERSION} on origin but not in this fresh runner, this creates a new branch from SOURCE_BRANCH instead of using the existing hotfix state, then merges/pushes a different history. Check out the remote hotfix branch when it exists before creating a new one.

          if git show-ref --verify --quiet "refs/heads/${hotfix_branch}"; then
            git checkout "${hotfix_branch}"
          else
            git checkout -b "${hotfix_branch}" "${SOURCE_BRANCH}"
            echo "CREATED_HOTFIX_BRANCH=true" >> "$GITHUB_ENV"

lib/rdk/NM_Dispatcher.sh:187

• This path also logs Started avahi-autoipd without checking the daemon's exit status, then exits the global connectivity handler with success. A failed link-local start is therefore reported as a successful recovery and no retry is requested; check and log the command failure before returning.

                    NMdispatcherLog "Started avahi-autoipd for $iface"
                    /usr/sbin/avahi-autoipd --daemonize --syslog "$iface"

lib/rdk/alertSystem.sh:106

• The request body is assembled by interpolating MSG_DATA and other runtime values directly into JSON. An alert containing a quote, backslash, or newline produces invalid JSON (and can inject additional fields), so legitimate alerts can fail and untrusted message data can alter the payload. Serialize/JSON-escape every field before constructing the POST body.

    strjson="{\"searchResult\":[{\"Time\":\"$currentTime\"},{\"process_name\":\"$PROCESS_NAME\"},{\"mac\":\"$estb_mac\"},{\"Version\":\"$software_version\"},{\"PartnerId\":\"$partnerId\"},{\"$MSG_DATA\":\"1\"}]}"
else
    strjson="{\"searchResult\":[{\"process_name\":\"$PROCESS_NAME\"},{\"mac\":\"$estb_mac\"},{\"Version\":\"$software_version\"},{\"msgTime\":\"$currentTime\"},{\"PartnerId\":\"$partnerId\"},{\"logEntry\":\"$MSG_DATA\"}]}"

lib/rdk/alertSystem.sh:22

• The header describes a log-backup script, but this new script sends process alerts to a telemetry endpoint and requires two arguments. That misleading purpose/usage makes the installed interface difficult to operate and maintain; update the header to document the alert behavior and arguments.

# Purpose: This script is used to backup the Logs
# Scope: RDK devices
# Usage: This script is triggered by systemd service 

lib/rdk/alertSystem.sh:36

• The new comment contains a spelling error: use “Assignment” instead of “Assigment”.

# Argument Assigment

lib/rdk/alertSystem.sh:77

• The new comment contains a spelling error: use “Logging” instead of “Loggging”.

# Loggging should be handled by the caller 

lib/rdk/getDeviceDetails.sh:285

• The zero MAC initialized on line 281 is now overwritten by the generic helper even when BLUETOOTH_ENABLED is not true. readBTAddress-generic.sh emits an empty value in that case, so every device-details refresh on a Bluetooth-disabled device stores an empty Bluetooth MAC instead of the established all-zero value.

    if [ -f /lib/rdk/readBTAddress-vendor.sh ]; then
        bluetooth_mac=`sh /lib/rdk/readBTAddress-vendor.sh`
    else
        bluetooth_mac=`sh /lib/rdk/readBTAddress-generic.sh`

lib/rdk/readBTAddress-generic.sh:31

• When BLUETOOTH_ENABLED is not true, this script leaves bluetooth_mac unset and prints an empty value. getDeviceDetails.sh now always replaces its default 00:00:00:00:00:00 with this command's output, so disabled-Bluetooth devices lose the documented zero-MAC fallback; initialize the local value before the conditional.
  lib/rdk/rebootNow.sh:1
• Removing this script leaves the reboot entry point used throughout the remaining codebase unresolved: utils.sh, factory-reset.sh, warehouse-reset.sh, restore, and userInitiatedFWDnld.sh still invoke /rebootNow.sh, while the Makefile still creates a symlink to this deleted target. Reboot requests from those paths will fail unless all callers and the package entry point are migrated to the replacement binary.
  lib/rdk/startLinkLocal.sh:55
• The script logs Started and exits successfully regardless of whether avahi-autoipd starts. If the daemon is missing or rejects the interface, callers see a successful link-local recovery even though no process was launched; propagate the command failure and log an error.

/usr/sbin/avahi-autoipd --daemonize --syslog "$INTERFACE"
Log "Started avahi-autoipd for $INTERFACE"

lib/rdk/startStunnel.sh:103

• extract_stunnel_client_cert is followed by a check of CERT_PATH, but this change does not assign that variable anywhere in the repository and the previous code used CERT_FILE. On the existing certificate-util contract this expands to an empty path, so the script exits with SHORTS_STUNNEL_CERT_FAILURE on every start. Keep the established variable or explicitly populate CERT_PATH before this check.

if [ ! -f $CERT_PATH -o ! -f $CA_FILE ]; then

lib/rdk/startStunnel.sh:170

• mktemp -u only reserves a name and leaves a race before mkfifo; another local process can pre-create or replace $PIPE, causing the subsequent open and passcode write to target an attacker-controlled object. Create a secure temporary directory (or use an atomic temporary-file strategy) and place the FIFO inside it.

    PIPE=$(mktemp -u)
    if ! mkfifo "$PIPE" 2>/dev/null; then
        echo_t "STUNNEL: ERROR - Failed to create named pipe"
    fi

    # Open the pipe using the available FD, with error handling
    if ! eval "exec $FD_NUMBER<>$PIPE" 2>/dev/null; then
        echo_t "STUNNEL: ERROR - Failed to open pipe with file descriptor"
    fi

    # Removing the pipe after opening
    rm "$PIPE"

lib/rdk/startStunnel.sh:173

• eval "$PASSCODE" executes the passcode as shell code rather than writing the passcode value to the descriptor; shell metacharacters can also execute arbitrary commands in this privileged startup script. Write the variable directly to the descriptor instead of evaluating it.

    echo "$(eval "$PASSCODE")" >&$FD_NUMBER &

lib/rdk/startStunnel.sh:115

• This unconditional checkHost is emitted before the device-type branch, which still emits a second checkHost for every TEST/PROD device at lines 123 or 127. Stunnel treats this scalar option as a duplicate (or the later value silently overrides the jump-server hostname), so normal devices can fail configuration parsing or lose the intended host verification. Emit exactly one selected checkHost value, retaining the jump FQDN only in the unknown-device fallback.

echo "checkHost   = $JUMP_FQDN"          >> $STUNNEL_CONF_FILE

• Files reviewed: 42/42 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate issues affect reboot availability, credential cleanup, alert delivery, Bluetooth state, and release consistency.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (15)

Previously missed (2) — in code that hasn't changed since the last review.

lib/rdk/networkConnectionRecovery.sh:281

• GatewayLogTimeStamp is shared by both probes, but this line updates it at the end of every call. Because the caller invokes V4 first and V6 second, a V4 probe that logs will reset the timestamp and suppress the V6 gateway response/loss logs for that run; the old single-call implementation logged both stacks before updating the timestamp. Update the shared timestamp only after the V6 evaluation (or maintain one timestamp per stack).
  .github/workflows/component-release-workflow-usage.md:92
• The release PR created by component-release.yml targets main, and the finish workflow also requires base.ref == 'main'; the branch-protection guidance here instead tells operators to protect develop. Following this documentation leaves the approval gate on the actual release PR unprotected.

.github/workflows/component-release-finish-on-approval.yml:109

• These refs are pushed in separate operations, so a transient failure can publish main and/or the tag while leaving develop and the release PR/branch unfinished. That produces a partially completed release which cannot be safely retried from the current state. Push the release refs atomically (or add explicit rollback/recovery) before deleting the release branch.

          git push origin main
          git push origin --tags
          git push origin develop
          git push origin --delete "${RELEASE_BRANCH}" || true

.github/workflows/component-release-finish-on-approval.yml:91

• This workflow fetches remote refs but then reuses potentially stale local main and develop branches. If either branch advanced after the release branch was created, git flow release finish merges against stale tips and the subsequent push can fail or omit current changes. Reset both local branches to their origin/* refs before finishing.

          git checkout main 2>/dev/null || git checkout -b main origin/main
          git checkout develop 2>/dev/null || git checkout -b develop origin/develop

.github/workflows/component-release-finish-on-approval.yml:76

• When the tag already exists, this step exits before the later branch-deletion and PR-closing steps. A retry after a partial/previous release therefore leaves the approved release/* PR open and the release branch undeleted, despite the workflow documentation promising cleanup. Handle the already-finished case by performing the cleanup/close steps, or make the tag guard verify and finish the remaining actions instead of exiting the job.

          if git ls-remote --exit-code --tags origin "refs/tags/${release_version}" >/dev/null 2>&1; then
            echo "Tag ${release_version} already exists on origin. Skipping release finish."
            exit 0

.github/workflows/component-release.yml:129

• The auto-complete path has the same partial-release failure mode: separate pushes can leave main, the tag, and develop out of sync if any later push fails, while the local release branch has already been finished. Use one atomic ref push or provide recovery before deleting the remote release branch.

          git push origin main
          git push origin --tags
          git push origin develop
          git push origin --delete "release/${RELEASE_VERSION}" || true

.github/workflows/component-release.yml:34

• The concurrency key includes release_type, so a main release and a hotfix for the same version can run simultaneously. Both flows check and create the same global Git tag and may push overlapping refs, allowing a race that produces conflicting release results. Serialize by release version alone (or otherwise share a global tag-level lock).

  group: component-release-${{ github.event.inputs.release_type }}-${{ github.event.inputs.release_version }}

lib/rdk/alertSystem.sh:55

• This script sources device.properties, which does not define RDK_PATH in this tree, and never sources include.properties; on a normal invocation $RDK_PATH is therefore empty and this check looks for /exec_curl_mtls.sh, causing the new alert path to exit before sending anything. Use the fixed /lib/rdk path or load the file that defines RDK_PATH.

if [ -f $RDK_PATH/exec_curl_mtls.sh ]; then
    . $RDK_PATH/exec_curl_mtls.sh

lib/rdk/alertSystem.sh:106

• MSG_DATA (and the other arguments) is interpolated directly into JSON without escaping. A message containing a quote, backslash, or newline produces malformed JSON (and can alter the generated object), so the alert upload fails for valid alert text. Serialize/escape all dynamic fields before constructing the request body.

    strjson="{\"searchResult\":[{\"Time\":\"$currentTime\"},{\"process_name\":\"$PROCESS_NAME\"},{\"mac\":\"$estb_mac\"},{\"Version\":\"$software_version\"},{\"PartnerId\":\"$partnerId\"},{\"$MSG_DATA\":\"1\"}]}"
else
    strjson="{\"searchResult\":[{\"process_name\":\"$PROCESS_NAME\"},{\"mac\":\"$estb_mac\"},{\"Version\":\"$software_version\"},{\"msgTime\":\"$currentTime\"},{\"PartnerId\":\"$partnerId\"},{\"logEntry\":\"$MSG_DATA\"}]}"

lib/rdk/getDeviceDetails.sh:285

• The old BLUETOOTH_ENABLED guard was removed, so this now invokes the vendor/generic reader even when Bluetooth is disabled. The new generic reader emits an empty value in that case, replacing the existing 00:00:00:00:00:00 default and causing the cached BluetoothMac to be blank. Keep the readers inside an enabled guard.

    if [ -f /lib/rdk/readBTAddress-vendor.sh ]; then
        bluetooth_mac=`sh /lib/rdk/readBTAddress-vendor.sh`
    else
        bluetooth_mac=`sh /lib/rdk/readBTAddress-generic.sh`

lib/rdk/readBTAddress-generic.sh:31

• When BLUETOOTH_ENABLED is not true, bluetooth_mac is never assigned and this script prints an empty value. getDeviceDetails.sh now always replaces its 00:00:00:00:00:00 default with this output, so devices with Bluetooth disabled lose the previous sentinel MAC.
  lib/rdk/rebootNow.sh:1
• Removing this script leaves the package's Makefile:86 symlink pointing to /lib/rdk/rebootNow.sh, while multiple shipped callers still invoke /rebootNow.sh (for example lib/rdk/utils.sh:65 and lib/rdk/factory-reset.sh:199). Since no replacement or alternate install is added here, reboot requests will fail with command-not-found after this deletion. Restore/provide the implementation or migrate the symlink and every caller together.
  lib/rdk/startStunnel.sh:160
• mktemp -u only generates an unused pathname; between this call and mkfifo, another local process can create that path. The subsequent root exec ...<>$PIPE and passcode write could then target an attacker-controlled file or pipe. Create the FIFO in a private directory using a race-free setup and abort if creation/opening fails.

    PIPE=$(mktemp -u)
    if ! mkfifo "$PIPE" 2>/dev/null; then

lib/rdk/wh_api_5.conf:46

• This path names a single .object file but is added after the [dirs] header. API_5 uses the separate [files] and [dirs] sections, so the file will be checked with directory semantics (or ignored as a directory) instead of being validated as customer data. Move it into the [files] section, before [dirs].
  systemd_units/gstreamer-cleanup.service:1
• This removes the only unit in the repository that deletes/rebuilds /opt/.gstreamer after a CDL or firmware update (ExecStart/ExecStop in the deleted unit). No replacement cleanup path or service is added, so firmware upgrades can retain a stale GStreamer registry and skip the required rebuild. Retain this unit or move its equivalent cleanup into the replacement update flow before deleting it.

• Files reviewed: 42/42 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate findings affect release completion, callers, security, telemetry, and packaging.

Review effort: Lite
Findings: 26 · 23 · 6

Open (55)

• Gate PR closure on successful release completion · New
• Remove client certificate on stunnel timeout · New
• This new failure branch exits before the sensitive-file cleanup immediately below (`rm -f…
• Although this job is triggered by an approved PR, it does not merge that PR; it runs `git flow…
• The approval job trusts any PR whose head is named release/*; it does not verify that the branch…
• These independent pushes can publish main and then fail while pushing tags or develop, leaving…
• The finish job pushes main, tags, and develop in separate operations. A later push failure…
• CERT_PATH is used for the cert file but is never assigned anywhere in this repository (only…
• This script relies on $RDK_PATH to locate exec_curl_mtls.sh, but it never sources…
• get_next_fd() currently uses true >&$fd to detect whether an FD is free. That redirection usually…
• mktemp -u is unsafe (TOCTOU race): another process can create the path before mkfifo,…
• This script still invokes /rebootNow.sh (see later in the file), but lib/rdk/rebootNow.sh is…
• Using mktemp -u to choose a FIFO name is vulnerable to a TOCTOU race (an attacker can pre-create…
• This PR removes lib/rdk/rebootNow.sh, but the repo still references /rebootNow.sh (e.g.,…
• previous-log-backup.service now runs /usr/bin/backup_logs, but this repository no longer provides…
• This evaluates $PASSCODE as shell code (eval "$PASSCODE") and sends its output over the pipe. If…
• Using mktemp -u introduces a TOCTOU race (the path is never reserved), and the current code logs…
• The new FIFO/FD block uses mktemp -u (TOCTOU risk) and eval on $PASSCODE, which can execute…
• This creates the FIFO path using mktemp -u, which is vulnerable to a TOCTOU race (another process…
• ExecStart has a leading space after =, which systemd treats as part of the command path (i.e.,…

And 35 more that still need to be addressed.

Previously missed (1)

In code that hasn't changed since last review

Package the new systemd drop-in configuration

systemd_units/​NetworkManager_ecfs.conf:3

These new drop-in directives are not included in this component's package: the root Makefile installs systemd_units/*.service and *.timer, but never copies systemd_units/*.conf. Unless another packaging step outside this repository installs it, the added Wants/After dependency will never reach the device; add the conf installation (and its destination) to the packaging change.