RDK-61551: Revssh Hardening - Entertainement devices

[Lasya-Prakarsha-D-V]

Reason for change: Enforcing SHORTS connection for PROD builds
Test procedure: NON-shorts ssh attempt for prod build should fail
Risks: Medium
Priority: P1

[Copilot]

🟡 Changes recommended

Required production-trigger test coverage is missing.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR hardens reverse SSH handling for production devices.

Changes:

• Validates SSH PID files.
• Rejects non-SHORTS triggers for BUILD_TYPE=prod.
• Improves SSH status and error handling.

File summaries

File	Finding
src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp	Critical (3 votes): Add tests for PROD non-SHORTS rejection and intended non-production/unset behavior, including expected NOK.

Review details

Suppressed comments (1)

src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:3099

• This gate is fail-open for a production process that does not inherit BUILD_TYPE=prod: the NULL case falls through to startTunnel.sh, so a non-SHORTS request can still establish a plain SSH tunnel. Since the requirement is to enforce SHORTS on PROD builds, derive the build identity from an immutable build-time/device source or reject non-SHORTS requests when the production marker is absent, rather than making this security control depend on the daemon environment.

                const char *buildType = getenv("BUILD_TYPE");
                if (buildType != NULL && strcmp(buildType, "prod") == 0) {

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

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[github-actions]

Code Coverage Summary

                                        Total:|79.6%  9682|93.9% 661|    -    0

[Copilot]

🟡 Changes recommended

Production enforcement can fail open, and environment cleanup and CI coverage issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

src/hostif/profiles/DeviceInfo/gtest/gtest_main.cpp:3230

• BUILD_TYPE is process-global and is set before the fatal ASSERT_NE; if getInstance(0) returns null, the assertion exits this test before unsetenv, leaving later tests running as prod. Acquire/assert the instance before mutating the environment, or use a scoped guard that restores the previous value on every exit path.

    setenv("BUILD_TYPE", "prod", 1);
    hostIf_DeviceInfo *pIface = hostIf_DeviceInfo::getInstance(0);
    ASSERT_NE(pIface, nullptr);

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

[Copilot]

🔵 Needs a closer look

Unresolved issues remain with production gating, CI coverage, and functional log validation.

Review details

Suppressed comments (4)

src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:3094

• This makes the production hardening depend on a mutable runtime environment variable rather than on the build itself. The repository only sets BUILD_TYPE in the L2 harness (which defaults to dev), so a production binary started without that variable—or with it overridden—will take the plaintext startTunnel.sh path. Use an immutable build-time/product configuration for the production decision, or make SHORTS-only behavior the default and explicitly opt into plaintext for development/test builds.

                if (buildType != NULL && strcmp(buildType, "prod") == 0) {
                    RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF,"[%s] plain reverse SSH trigger rejected on prod-built device \n",__FUNCTION__);
                    return NOK;
                }

src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:3094

• The public interface documentation currently describes this handler as gated only by privacyMode (docs/api/thunder-plugin-interfaces.md:324-326), but this adds another NOK path for a plain start on production builds. Update the handler's API/BDD documentation to state that non-SHORTS triggers are rejected on PROD devices.

                if (buildType != NULL && strcmp(buildType, "prod") == 0) {
                    RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF,"[%s] plain reverse SSH trigger rejected on prod-built device \n",__FUNCTION__);
                    return NOK;
                }

test/functional-tests/tests/tr69hostif_deviceip.py:189

• This new integration assertion is skipped unless the pytest process has BUILD_TYPE=prod, but the repository's L2 workflow invokes sh run_l2.sh without that variable and run_l2.sh defaults it to dev at line 41. Consequently the production rejection path is never exercised by the existing L2 CI job; add a production-mode L2 invocation/job that starts the daemon with the same setting.

@pytest.mark.skipif(os.environ.get("BUILD_TYPE") != "prod", reason="requires a production build environment")

test/functional-tests/tests/tr69hostif_deviceip.py:196

• The log check scans the entire persistent log without clearing it first, so a matching line from an earlier run can satisfy this assertion even if the current start request was rejected for a different reason (or the new guard is not reached). Clear the tr69hostif log immediately before issuing the request, or assert against a log position captured for this invocation.

    assert "plain reverse SSH trigger rejected on prod-built device" in grep_tr69hostiflogs(
        "plain reverse SSH trigger rejected on prod-built device"

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Unresolved moderate findings affect production enforcement and test coverage.

Review details

Suppressed comments (5)

run_l2.sh:41

• The L2 workflow invokes this script without BUILD_TYPE, so this defaults to dev and the new production-only test in tr69hostif_deviceip.py is skipped. Since no other workflow or script sets BUILD_TYPE=prod, the PR adds no automated execution of the production rejection path; add a dedicated prod L2 invocation/job (while retaining the dev run) so this hardening is actually exercised.

export BUILD_TYPE=${BUILD_TYPE:-dev}

src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:3093

• This gate relies solely on the inherited, mutable BUILD_TYPE environment variable. The only new assignment in this repository is in run_l2.sh; the installed tr69hostif systemd service instead loads /etc/device.properties, and this change does not ensure that production deployments set BUILD_TYPE=prod. If the variable is absent or overridden, the condition is false and the legacy plain startTunnel.sh path remains available on a production image, defeating the hardening. Use a build/deployment-controlled value and fail closed when production status cannot be established.

        }

        stMsgData->paramtype = hostIf_StringType;
        stMsgData->paramLen = strlen(stMsgData->paramValue);

src/hostif/profiles/DeviceInfo/gtest/gtest_main.cpp:3234

• This unconditionally removes BUILD_TYPE from the gtest process after the case. When the test binary is launched with an existing value (for example, prod), later tests no longer see the caller's environment, so their results depend on this test's position; save and restore the prior value instead of always unsetting it.

    addr.sin_port = htons(3000);

test/functional-tests/tests/tr69hostif_deviceip.py:192

• This log assertion scans the existing log without clearing it first, so a matching message from an earlier run can make the test pass even if this SET is accepted. Clear the log immediately before issuing the request so the assertion verifies this invocation; the repository's L2 test guidance calls for per-test log isolation.

    rstdout = rbus_set_data(DATA_ELEMENT_NAME, "string", "start")

test/functional-tests/tests/tr69hostif_deviceip.py:192

• This case is unmarked, so it runs after test_ReverseSSH_Set_Get_Handler (order 28), which has already issued start shorts and never stops it. On builds with __SINGLE_SESSION_ONLY__, isRsshactive() returns through the existing “session already active” path before reaching the new production check, so the expected rejection log is missing and this test does not exercise the hardening. Stop/clear the session or run this case while the tunnel is known to be inactive.

@pytest.mark.skipif(os.environ.get("BUILD_TYPE") != "prod", reason="requires a production build environment")
def test_ReverseSSH_Plain_Trigger_Rejected_On_Prod():
    DATA_ELEMENT_NAME = "Device.DeviceInfo.X_RDKCENTRAL-COM_xOpsDeviceMgmt.ReverseSSH.xOpsReverseSshTrigger"
    rstdout = rbus_set_data(DATA_ELEMENT_NAME, "string", "start")

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[github-actions]

Code Coverage Summary

                                        Total:|53.4%  9678|66.7% 661|    -    0

[Copilot]

🟡 Changes recommended

Production enforcement and associated test configuration need correction before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:3233

• This makes the production policy fail open: when BUILD_TYPE is unset (or has any value other than exactly prod), a plain start still reaches startTunnel.sh. The repository does not set this variable in the production build; the only in-repo assignment is the L2 harness defaulting it to dev, so a production daemon launched by its normal service manager can bypass the SHORTS requirement. Make the product mode a build/deployment-controlled invariant and reject plain triggers when that invariant indicates production (including handling an unset/invalid value), rather than relying on this optional environment variable.

                const char *buildType = getenv("BUILD_TYPE");
                if (buildType != NULL && strcmp(buildType, "prod") == 0) {

test/functional-tests/tests/tr69hostif_deviceip.py:189

• The L2 workflow invokes run_l2.sh without setting BUILD_TYPE, and that script defaults it to dev at line 41, so this skipif causes the new production rejection test to be skipped in CI. Add an explicit production L2 job/configuration (and retain a dev run separately) so the behavior described by the PR is actually exercised.

@pytest.mark.skipif(os.environ.get("BUILD_TYPE") != "prod", reason="requires a production build environment")

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

[github-actions]

Code Coverage Summary

                                        Total:|79.6%  9678|93.9% 661|    -    0

[Copilot]

🔵 Needs a closer look

Unresolved production-policy and test-coverage issues must be addressed before approval.

Review details

Suppressed comments (6)

run_l2.sh:41

• This defaults the daemon and pytest process to dev, so the new production-only functional test is skipped for the standard run_l2.sh invocation. The documented L2 path therefore does not exercise the only end-to-end assertion for this hardening; add an explicit production L2/CI invocation (while retaining a separate dev run) so the rejection path is actually validated.

export BUILD_TYPE=${BUILD_TYPE:-dev}

src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:3233

• This guard treats an unset BUILD_TYPE as prod. In the existing gtest sequence, set_xOpsReverseSshTrigger_blocksPlainStartOnProd unsets the variable before set_xOpsReverseSshTrigger_StartNoShorts, which expects OK; that test now returns NOK in the default environment. Since the requirement is production-only, treat only an explicit prod value as production (or explicitly set and restore dev in the non-production test).

                if (buildType == NULL || strcmp(buildType, "prod") == 0) {

src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:3233

• BUILD_TYPE is an externally supplied runtime environment variable rather than a property of the binary. A production daemon launched with BUILD_TYPE=dev (or through a launch path that sets it incorrectly) will take the non-SHORTS branch and bypass this hardening; in this repository the only setup is the test runner. Encode the production distinction as a build-time flag or trusted device configuration instead of allowing the process environment to select the security policy.

                const char *buildType = getenv("BUILD_TYPE");
                if (buildType == NULL || strcmp(buildType, "prod") == 0) {

src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:3235

• docs/api/thunder-plugin-interfaces.md:326 still says this handler's only NOK gate is privacy mode, but this adds a production plain-trigger gate. Update the public API notes to document the new behavior so consumers know why a SET can fail.

                if (buildType == NULL || strcmp(buildType, "prod") == 0) {
                    RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF,"[%s] plain reverse SSH trigger rejected on prod-built device \n",__FUNCTION__);
                    return NOK;

src/hostif/profiles/DeviceInfo/gtest/gtest_main.cpp:5100

• unsetenv leaves the process in the state that the new production guard treats as production. The next existing set_xOpsReverseSshTrigger_StartNoShorts test (line 5118) therefore returns NOK instead of its expected OK in the normal registration order. Restore the intended dev state or set BUILD_TYPE=dev in that non-production test.

    unsetenv("BUILD_TYPE");

test/functional-tests/tests/tr69hostif_deviceip.py:189

• The implementation treats an unset BUILD_TYPE as production (buildType == NULL rejects plain start), but this marker treats the same state as non-production and skips the test. A production daemon launched without the variable will therefore bypass this regression test even though the new code is enforcing the rejection; align the marker's default with the handler's fail-closed behavior.

@pytest.mark.skipif(os.environ.get("BUILD_TYPE") != "prod", reason="requires a production build environment")

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[github-actions]

Code Coverage Summary

                                        Total:|79.6%  9680|93.9% 661|    -    0

[Copilot]

🟡 Changes recommended

Production detection, header inclusion, and test environment cleanup issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:3236

• The security decision accepts every value other than the exact string prod, so a production daemon launched with BUILD_TYPE=dev (or a typo) will still execute the unrestricted startTunnel.sh path. Because this is a mutable process environment variable rather than a trusted build property, it does not reliably enforce SHORTS on production images; derive the decision from immutable build-time configuration or another trusted product-build indicator.

                const char *buildType = getenv("BUILD_TYPE");
                if (buildType == NULL || strcmp(buildType, "prod") == 0) {
                    RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF,"[%s] plain reverse SSH trigger rejected on prod-built device \n",__FUNCTION__);
                    return NOK;

src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:3233

• getenv is newly used here, but Device_DeviceInfo.cpp does not include <cstdlib>; the added <cstdlib> include is only in gtest_main.cpp. This relies on transitive headers and can fail on toolchains that do not expose the declaration, so add the standard header to this implementation.

                const char *buildType = getenv("BUILD_TYPE");

src/hostif/profiles/DeviceInfo/gtest/gtest_main.cpp:5096

• BUILD_TYPE is process-global, but it is set before the fatal ASSERT_NE. If getInstance(0) fails, the assertion returns from this test without reaching unsetenv, leaving later tests running with the production guard enabled. Acquire and assert the interface before changing the environment, or use a scoped environment restore.

    setenv("BUILD_TYPE", "prod", 1);
    hostIf_DeviceInfo *pIface = hostIf_DeviceInfo::getInstance(0);
    ASSERT_NE(pIface, nullptr);

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

[github-actions]

Code Coverage Summary

                                        Total:|79.6%  9680|93.9% 661|    -    0

[Copilot]

🟡 Changes recommended

Production gating and L2 coverage are not reliably enforced, and the test environment cleanup can leak across tests.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:3100

• This guard is not actually tied to the production build. In this repository BUILD_TYPE is only exported by run_l2.sh, where it defaults to dev; a production daemon launched by its normal service without that variable will take the fall-through path and still run start, bypassing this hardening. Make the production/dev selection a build-time or otherwise guaranteed deployment setting, and validate the production launch path rather than relying on an optional environment variable.

                const char *buildType = getenv("BUILD_TYPE");
                if (buildType != NULL && strcmp(buildType, "prod") == 0) {
                    RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF,"[%s] plain reverse SSH trigger rejected on prod-built device \n",__FUNCTION__);

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

[Copilot]

🔵 Needs a closer look

Production enforcement, test isolation, cleanup, and L2 build-type handling issues remain unresolved.

Review details

Suppressed comments (6)

run_l2.sh:41

• The L2 harness defaults BUILD_TYPE to dev, so running this procedure against a production image without an externally supplied variable starts the daemon in the non-production mode and causes the new pytest case to be skipped. That allows the stated PROD hardening test to pass without exercising it; the harness should obtain the build type from the image/build metadata or fail loudly when a production run is expected.

export BUILD_TYPE=${BUILD_TYPE:-dev}

src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:3101

• This enforcement depends on the daemon inheriting BUILD_TYPE=prod at runtime; it is not tied to how the binary was built. The only added propagation is in run_l2.sh, so a production image launched by its normal service without this variable (or with any other value) still reaches startTunnel.sh for a plain trigger and bypasses the hardening. Use a compile-time production definition or an immutable device/build configuration that is guaranteed to be available to the daemon, and cover the missing-variable case in the test.

                const char *buildType = getenv("BUILD_TYPE");
                if (buildType != NULL && strcmp(buildType, "prod") == 0) {
                    RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF,"[%s] plain reverse SSH trigger rejected on prod-built device \n",__FUNCTION__);
                    return NOK;

src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:1434

• A missing /var/tmp/rssh.pid is the normal representation of an inactive reverse-SSH session, and isRsshactive() is used by the status getter. Logging that expected state at RDK_LOG_ERROR will emit an error on every inactive status check and can create operational noise; the previous implementation logged this path at debug level. Keep the normal missing-file path at debug (or reserve error level for unexpected open failures).

    if (!pidstrm.is_open())
    {
        RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF,"[%s] SSH Session inactive; failed to open pid file %s (errno=%d:%s) \n",__FUNCTION__, pidfile.c_str(), errno, strerror(errno));
        RDK_LOG(RDK_LOG_TRACE1,LOG_TR69HOSTIF,"[%s] Exiting... \n",__FUNCTION__);
        return false;

src/hostif/profiles/DeviceInfo/gtest/gtest_main.cpp:3234

• This test overwrites any BUILD_TYPE supplied by the test runner and unconditionally unsets it afterward, so the process environment is not restored; an earlier assertion failure at ASSERT_NE also exits before cleanup. That makes the suite order-dependent and can change the mode seen by later tests. Save the prior value, acquire/assert the fixture before changing the environment, and restore the prior value on exit.

    setenv("BUILD_TYPE", "prod", 1);
    hostIf_DeviceInfo *pIface = hostIf_DeviceInfo::getInstance(0);
    ASSERT_NE(pIface, nullptr);

    EXPECT_EQ(pIface->set_xOpsReverseSshTrigger(&param), NOK);

    unsetenv("BUILD_TYPE");

test/functional-tests/tests/tr69hostif_deviceip.py:197

• This test is order-dependent: test_ReverseSSH_Set_Get_Handler immediately before it starts a start shorts session and never stops it. In builds with __SINGLE_SESSION_ONLY__, this request returns from isRsshactive() before reaching the new PROD rejection branch, so the expected rejection log is never emitted (and the test either fails or validates the wrong reason). Stop and wait for the prior tunnel to become inactive, or isolate this test's daemon/session state before issuing the plain trigger.

def test_ReverseSSH_Plain_Trigger_Rejected_On_Prod():
    DATA_ELEMENT_NAME = "Device.DeviceInfo.X_RDKCENTRAL-COM_xOpsDeviceMgmt.ReverseSSH.xOpsReverseSshTrigger"
    rstdout = rbus_set_data(DATA_ELEMENT_NAME, "string", "start")

    assert RBUS_SET_EXCEPTION_STRING in rstdout
    assert "plain reverse SSH trigger rejected on prod-built device" in grep_tr69hostiflogs(
        "plain reverse SSH trigger rejected on prod-built device"
    )

test/functional-tests/tests/tr69hostif_deviceip.py:196

• The log assertion scans the entire persistent log, but this test never clears it before issuing the request. A matching line left by an earlier test run can satisfy the assertion even if the current daemon accepts the plain trigger or fails to log the rejection, so this security regression test can produce a false pass. Clear the tr69hostif log immediately before the RBUS SET.

    rstdout = rbus_set_data(DATA_ELEMENT_NAME, "string", "start")

    assert RBUS_SET_EXCEPTION_STRING in rstdout
    assert "plain reverse SSH trigger rejected on prod-built device" in grep_tr69hostiflogs(
        "plain reverse SSH trigger rejected on prod-built device"

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

A compile-blocking issue and production enforcement gaps remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:3094

• This hardening is controlled only by the mutable runtime environment. If the production daemon is started without BUILD_TYPE=prod (the checked-in service files only import /etc/device.properties, and the L2 script defaults this variable to dev), a plain start still reaches startTunnel.sh and the production restriction is bypassed. Tie this to the production build configuration or otherwise make the deployed production service set and enforce an immutable production marker, failing closed when it is absent.

                const char *buildType = getenv("BUILD_TYPE");
                if (buildType != NULL && strcmp(buildType, "prod") == 0) {
                    RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF,"[%s] plain reverse SSH trigger rejected on prod-built device \n",__FUNCTION__);
                    return NOK;
                }

src/hostif/profiles/DeviceInfo/gtest/gtest_main.cpp:3234

• This test changes the process-wide BUILD_TYPE and always unsets it instead of restoring its original value. Running the suite with BUILD_TYPE already set to prod changes the environment for subsequent tests, and the ASSERT_NE early return can leave BUILD_TYPE=prod behind; use an RAII guard or otherwise restore the prior value on every exit path.

    setenv("BUILD_TYPE", "prod", 1);
    hostIf_DeviceInfo *pIface = hostIf_DeviceInfo::getInstance(0);
    ASSERT_NE(pIface, nullptr);

    EXPECT_EQ(pIface->set_xOpsReverseSshTrigger(&param), NOK);

    unsetenv("BUILD_TYPE");

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

[Copilot]

🔵 Needs a closer look

Production enforcement can be bypassed, and the test environment is not reliably restored.

Review details

Suppressed comments (2)

src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:3093

• This check is based only on a mutable process environment variable, not on the build itself. When BUILD_TYPE is unset (or omitted by a production launch path), getenv returns null and the code still executes startTunnel.sh for start, so the SHORTS-only requirement is bypassed. Use an immutable build-time/product configuration for the production decision and make the production path fail closed.

                const char *buildType = getenv("BUILD_TYPE");
                if (buildType != NULL && strcmp(buildType, "prod") == 0) {
                    RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF,"[%s] plain reverse SSH trigger rejected on prod-built device \n",__FUNCTION__);
                    return NOK;

src/hostif/profiles/DeviceInfo/gtest/gtest_main.cpp:3234

• This unconditionally removes BUILD_TYPE after the test, even when the process inherited a value, so subsequent tests observe a different environment. A fatal assertion before this line would also leave the test's prod override installed. Preserve whether the variable was originally set and restore its original value after the assertion.

    unsetenv("BUILD_TYPE");

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Critical and moderate issues remain in production-mode enforcement and test environment handling.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:3093

• BUILD_TYPE is an optional, mutable process environment variable, so this does not enforce a property of a production build. A production daemon started without BUILD_TYPE=prod—or launched by the updated run_l2.sh, which defaults it to dev—will still execute the plain-tunnel path. That leaves non-SHORTS reverse SSH available on PROD; use a build/device configuration with a production-safe default rather than an opt-in environment check.

                const char *buildType = getenv("BUILD_TYPE");
                if (buildType != NULL && strcmp(buildType, "prod") == 0) {
                    RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF,"[%s] plain reverse SSH trigger rejected on prod-built device \n",__FUNCTION__);
                    return NOK;

src/hostif/profiles/DeviceInfo/gtest/gtest_main.cpp:3230

• The test changes a process-global environment variable and then unconditionally unsets it at line 3234. If BUILD_TYPE was already set, or if the ASSERT_NE returns early, this leaves the test process in a different environment and can make later tests order-dependent. Save and restore the original value with an RAII guard, including assertion-failure paths.

    setenv("BUILD_TYPE", "prod", 1);
    hostIf_DeviceInfo *pIface = hostIf_DeviceInfo::getInstance(0);
    ASSERT_NE(pIface, nullptr);

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

[Copilot]

🔵 Needs a closer look

Production enforcement and test coverage remain dependent on mutable or missing BUILD_TYPE state, with environment cleanup issues in the unit test.

Review details

Suppressed comments (4)

run_l2.sh:41

• The L2 workflow invokes run_l2.sh without setting BUILD_TYPE, so this default makes the new production-only functional test skip (skipif checks for prod) in CI. As a result, the PR's only functional CI run does not verify the rejection it introduces; add a dedicated production-mode invocation/job while retaining the development run as appropriate.

export BUILD_TYPE=${BUILD_TYPE:-dev}

src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:3236

• This makes the hardening dependent on a mutable runtime environment variable rather than on the production build itself. In this repository BUILD_TYPE is only initialized by run_l2.sh; the installed daemon/service has no corresponding setting, so a production device started normally with this variable unset will take the non-shorts path and accept plain start. Encode the production/dev distinction in the build artifact or a trusted device configuration that is guaranteed to be present in production, and fail closed when it is unavailable.

                const char *buildType = getenv("BUILD_TYPE");
                if (buildType != NULL && strcmp(buildType, "prod") == 0) {
                    RDK_LOG(RDK_LOG_ERROR,LOG_TR69HOSTIF,"[%s] plain reverse SSH trigger rejected on prod-built device \n",__FUNCTION__);
                    return NOK;

src/hostif/profiles/DeviceInfo/gtest/gtest_main.cpp:5098

• This test unconditionally overwrites BUILD_TYPE and then unsets it, so a suite launched with an existing BUILD_TYPE loses that state for subsequent tests. Also, the ASSERT_NE can return before the cleanup, leaving the process set to prod. Preserve and restore the original environment value with an RAII guard (or restore it on every exit path).

    setenv("BUILD_TYPE", "prod", 1);
    hostIf_DeviceInfo *pIface = hostIf_DeviceInfo::getInstance(0);
    ASSERT_NE(pIface, nullptr);

    EXPECT_EQ(pIface->set_xOpsReverseSshTrigger(&param), NOK);

test/functional-tests/tests/tr69hostif_deviceip.py:190

• This skip condition reflects the pytest runner's environment, not the build identity of the already-running tr69hostif daemon. A production device launched by its service manager without BUILD_TYPE=prod will skip this regression entirely, while a dev device tested from a shell with that variable will run it. Derive the mode from a target build marker or require an explicit, verified production-test setup so the hardening cannot silently go untested.

@pytest.mark.skipif(os.environ.get("BUILD_TYPE") != "prod", reason="requires a production build environment")
def test_ReverseSSH_Plain_Trigger_Rejected_On_Prod():

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[github-actions]

Code Coverage Summary

                                        Total:|79.6%  9680|93.9% 661|    -    0

[Copilot]

🟡 Changes recommended

Production coverage and build-mode enforcement are incomplete, and test environment handling can cause failures or contamination.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (5)

run_l2.sh:41

• The L2 workflow invokes run_l2.sh without setting BUILD_TYPE, and this new default is dev, so the skipif on the new production functional test is always true in CI. As a result, the stated production non-SHORTS regression is not exercised; add a dedicated prod-mode daemon/test invocation (while retaining dev coverage if required).

export BUILD_TYPE=${BUILD_TYPE:-dev}

src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp:3234

• This gate is controlled only by the mutable runtime environment variable BUILD_TYPE. The repository does not wire that variable into a production build (the L2 script actually defaults it to dev), so a production daemon started without BUILD_TYPE=prod or with any other value still launches the non-SHORTS startTunnel.sh path. Make the production/dev distinction a build-time or product-supplied configuration and ensure the production configuration cannot fall through to the plain-SSH path.

                const char *buildType = getenv("BUILD_TYPE");
                if (buildType != NULL && strcmp(buildType, "prod") == 0) {

src/hostif/profiles/DeviceInfo/gtest/gtest_main.cpp:5104

• Environment restoration occurs only after EXPECT_EQ; if the assertion fails, BUILD_TYPE remains modified for every subsequent test in this process. Use an exception-/failure-safe scoped environment helper (or isolate the test process) so a failed test cannot contaminate the suite.

    EXPECT_EQ(pIface->set_xOpsReverseSshTrigger(&param), NOK);

    if (savedBuildType) {
        setenv("BUILD_TYPE", savedBuildTypeValue.c_str(), 1);
    } else {

test/functional-tests/tests/tr69hostif_deviceip.py:190

• This test is skipped unless the caller exports BUILD_TYPE=prod, while run_l2.sh defaults that variable to dev and never runs a production-mode variant. Consequently the normal L2 run does not exercise the new rejection path, so a regression can pass unnoticed. Add an explicit production-mode test job/invocation and ensure the daemon and pytest process receive the same mode.

@pytest.mark.skipif(os.environ.get("BUILD_TYPE") != "prod", reason="requires a production build environment")
def test_ReverseSSH_Plain_Trigger_Rejected_On_Prod():

test/functional-tests/tests/tr69hostif_deviceip.py:192

• grep_tr69hostiflogs scans the whole log file, but this test does not clear it before the SET. A stale matching line from an earlier invocation can therefore satisfy the assertion even when this request did not emit the rejection message.

    rstdout = rbus_set_data(DATA_ELEMENT_NAME, "string", "start")

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

[github-actions]

Code Coverage Summary

                                        Total:|79.6%  9680|93.9% 661|    -    0