RDKB-66802 : Fix invalid session handling

[pavankumar464]

Reason for change: Fix invalid session handling
Test Procedure: Test for UI Sessions
Risks: Low
Priority: P1

[pavankumar464]

• Prevent expired or invalid sessions from causing HTTP 500 error.
• Hardened session handling by rejecting invalid, missing, expired, and cross-protocol session IDs.
• Bound session IDs to HTTP or HTTPS and set the cookie Secure attribute based on detected request scheme.
• Prevented stale sessions from leaving an active $_SESSION proxy or recreating deleted session data.
• Added proper boolean return handling for session_start() / session_create() and implemented session_unset().
• Added native and wrapper-level automated tests for session lifecycle, secure cookies, and replay protection.

[Copilot]

🟡 Changes recommended

session_start() can become non-retryable after a failed start (due to $_jst_session = {}) and HTTPS detection can misclassify some environments (e.g., HTTPS=0) leading to incorrect scheme decisions.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR tightens session handling by rejecting invalid/stale session cookies and preventing cross-scheme (HTTP↔HTTPS) session replay, while extending unit coverage around these edge cases.

Changes:

• Add scheme-tagging to session IDs and reject cookies from the wrong request scheme in the session module.
• Make session start/data operations safer under missing/invalid session state (including returning an empty object from getData when no session exists).
• Expand parser/session tests for missing cookie, wrong scheme cookie, expired session, and related behaviors.

File summaries

File	Description
tests/parser/jst_prefix.js	Updates test JS session helpers to handle invalid session starts and implements session_unset().
tests/parser_test.cpp	Adjusts session-id helper and adds new test cases for invalid/missing/wrong-scheme/expired sessions.
source/jst_session.c	Adds request-scheme detection, scheme-tagging for session IDs, new isSecure(), and adjusts start/getData behaviors.
jsts/jst_prefix.js	Updates runtime JS prefix to reject invalid starts, set secure cookies correctly, and guard writes when session is inactive.

Review details

Suppressed comments (1)

jsts/jst_prefix.js:102

• session_start() returns a boolean in other paths (true on success / false on failure), but the early-exit path returns undefined. This makes the API inconsistent and can break callers that check the return value.

  if($_jst_session)
    return;

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

---

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

[Copilot]

🔵 Needs a closer look

jsts/jst_prefix.js still has inconsistent session_start() return values and can emit DUKSID=false if ccsp_session.create() fails.

Review details

Suppressed comments (3)

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

jsts/jst_prefix.js:102

• session_start() returns undefined when a session is already initialized (if($_jst_session) return;), but returns true/false on other paths. This makes the return value unreliable for callers checking success/failure.

This issue also appears on line 103 of the same file.

  if($_jst_session)
    return;

jsts/jst_prefix.js:146

• session_create() does not check whether ccsp_session.create() succeeded before emitting the Set-Cookie header. If create() fails, ccsp_session.getId() returns false (see session_get_id), which would set a DUKSID=false cookie.

jsts/jst_prefix.js:107

• session_start() also returns undefined in the $_val_input == 1 early-exit path, which breaks the new boolean return contract (other paths return true/false). Return false here so callers can consistently treat non-true as failure.

  if($_val_input == 1) 
  {
    $_val_input = 0;
    return;
  }

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

[Copilot]

🟡 Changes recommended

There are test fragility concerns around scheme-tagged session IDs and a correctness issue where session_create()’s $_SESSION proxy still persists data unconditionally without checking session activity.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

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

source/jst_session.c:555

• ccsp_session.isSecure() is a new exported API used to decide whether to set the cookie Secure attribute, but there is no unit test coverage for its behavior under common environment variable combinations (e.g., HTTPS=on, REQUEST_SCHEME=https, SSL_PROTOCOL set/unset). Adding tests would help prevent regressions in secure-cookie handling.

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

[Copilot]

🟡 Changes recommended

The updated tests still risk being environment-dependent due to a hardcoded session scheme tag, and the new isSecure() API is currently untested.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

tests/parser_test.cpp:140

• makeValidSessionId() hardcodes the scheme tag to HTTP ('0'). Since ccsp_session.start() rejects cookies whose scheme tag doesn’t match the current request scheme (derived from env like HTTPS/REQUEST_SCHEME/SSL_PROTOCOL), these tests can become environment-dependent/flaky when the runner has HTTPS-related env vars set. Consider deriving the scheme tag from the current environment in this helper so existing tests remain stable across environments.

static std::string makeValidSessionId(char fill)
{
  return std::string("jst_sess0") + std::string(31, fill);
}

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

[Copilot]

🟡 Changes recommended

The new scheme-tag enforcement will reject most pre-existing (legacy) session IDs created before tagging, potentially causing widespread session invalidation after upgrade unless legacy IDs are handled intentionally.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

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

[Copilot]

🟡 Changes recommended

Unresolved session validation, compatibility, build, and test issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

jsts/jst_prefix.js:169

• Checking only getStatus() does not prevent a proxy from an older session from writing into a newly created session. For example, retaining var old = $_SESSION, calling session_destroy() and then session_create(), and assigning old.value sees the new session as active and persists the old proxy's data to the new ID. Capture the session ID when each proxy is created and require it to still match before calling setData() in both proxy handlers.

      if(ccsp_session.getStatus())
        ccsp_session.setData(obj);

source/jst_session.c:245

• This scheme check is not backward-compatible with the IDs already issued by the previous implementation: those IDs used a random character at offset 8 rather than 0/1. After deployment, nearly all existing cookies will be rejected despite valid session files, logging users out; preserve or migrate legacy IDs, or explicitly handle this rollout compatibility before enforcing the new marker.

          if(parsed_sesid[SESSION_SCHEME_OFFSET] != request_session_scheme())
          {

tests/parser_test.cpp:776

• This assertion cannot pass: the evaluated prefix initializes _jst_header_buffer to "Content-type: text/html", and a failed session_start() only avoids adding a Set-Cookie header; it does not make the buffer empty. Assert that no DUKSID cookie header was emitted instead.

  EXPECT_TRUE(evaluateJavaScriptBoolean(ctx, "_jst_header_buffer === ''"));

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

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings affect session security, lifecycle correctness, and test validity.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (6)

jsts/jst_prefix.js:163

• A failed ccsp_session.create() does not always mean that the C session is inactive: the implementation can fail during random-ID allocation before it replaces an existing session_identifier. This branch nevertheless discards $_jst_session and replaces $_SESSION, leaving session_status() true while the existing session is no longer reachable through the session proxy. Only reset the JavaScript state when getStatus() is false (or make creation transactional).

  if(!ccsp_session.create())
  {
    $_jst_session = null;
    $_SESSION = {};
    return false;

jsts/jst_prefix.js:111

• session_create() only assigns a new session_identifier; it does not create the /tmp backing file until setData() is called. Therefore a caller that creates an empty session and then calls session_start() enters this branch, ccsp_session.start() fails its utime(), and the newly created session is discarded. Materialize an empty session file when creating it, or otherwise distinguish an unsaved newly created session from a stale active session before revalidating here.

  if($_jst_session)
  {
    if(ccsp_session.start())
      return true;

tests/parser/jst_prefix.js:86

• This fixture still returns success whenever $_jst_session is non-null and never revalidates the backing session. If the file is removed after the first start, a later session_start() reports success and leaves the stale proxy active, unlike jsts/jst_prefix.js; mirror the revalidation/reset branch here.

  if($_jst_session)
    return true;

tests/parser/jst_prefix.js:92

• Changing this prefix without regenerating the parser fixtures leaves the *.jst.parsed files with the old session block (for example, tests/parser/jst_parser_include_runtime.jst.parsed still has the old session_start/session_unset code). parser_test compares generated templates with those files, so the parser test will fail; regenerate the expected files or keep this fixture unchanged.

  if(!ccsp_session.start())
  {
    /* A stale cookie must not create a proxy backed by an inactive session. */
    $_jst_session = null;
    $_SESSION = {};
    return false;

tests/parser_test.cpp:862

• This test only clears HTTPS, but request_is_https() also treats REQUEST_SCHEME=https and a non-empty SSL_PROTOCOL as secure. In a CGI environment where either variable is inherited, the supposedly HTTP request will be classified as HTTPS and this test can accept the jst_sess1 cookie instead of exercising the rejection path. Guard and clear all three scheme-related variables before setting the cookie.

  EnvVarGuard cookie_guard("HTTP_COOKIE");
  EnvVarGuard https_guard("HTTPS");
  const std::string session_id = std::string("jst_sess1") + std::string(31, 'E');
  const std::string session_file = "/tmp/" + session_id;

tests/parser_test.cpp:876

• As above, _jst_header_buffer starts as "Content-type: text/html", so the === '' conjunct makes this assertion fail despite correct rejection. Assert that no Set-Cookie: header was appended instead.

  EXPECT_TRUE(evaluateJavaScriptBoolean(ctx,
      "_jst_header_buffer === '' && !session_status() && $_jst_session === null"));

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

[Copilot]

🟡 Changes recommended

Unresolved session liveness, proxy identity, legacy-ID, fixture, and environment-isolation issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (6)

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

jsts/jst_prefix.js:216

• getStatus() only indicates that the C module still holds an identifier; it does not verify that the backing file is still present. After that file is deleted, session_unset() still calls setData(), whose fopen(..., "w") recreates the invalid session. Perform the same liveness check as session_start() (and reset the JS session on failure) before persisting, or expose an equivalent C-side check.

source/jst_session.c:245

• The scheme check does not distinguish IDs issued before scheme tagging was introduced. Those legacy IDs have an arbitrary alphanumeric character at this offset, so any old ID whose random character happens to be 0 or 1 passes on the corresponding request scheme and can still be replayed across schemes. Use a versioned/session-prefix marker or explicitly invalidate legacy IDs before relying on this character as a scheme tag.

          if(parsed_sesid[SESSION_SCHEME_OFFSET] != request_session_scheme())
          {

tests/parser/jst_prefix.js:86

• This already-initialized path still returns without calling ccsp_session.start(), so it skips the new backing-file liveness check. If the session file is removed after the first start, a later session_start() reports success and the proxy remains active; a subsequent write can recreate the deleted file through setData(). Mirror the runtime prefix here by retrying ccsp_session.start() and clearing $_jst_session/$_SESSION when it fails.

  if($_jst_session)
    return true;

tests/parser/jst_prefix.js:142

• getStatus() is not a backing-file liveness check here either. Once an existing session file is removed, this implementation still persists the cleared object with setData(), which opens the file for writing and recreates the invalid session instead of rejecting it. Keep this test/runtime prefix aligned with the invalid-session behavior by validating the session before setData().

  if(!$_jst_session || !ccsp_session.getStatus())
    return false;

  for(var $key in $_jst_session)
    delete $_jst_session[$key];

  return ccsp_session.setData($_jst_session);

tests/parser/jst_prefix.js:87

• This prefix is prepended by parser_test and its output is compared byte-for-byte with the checked-in .jst.parsed fixtures (tests/parser_test.cpp:287-295). Those fixtures still contain the old session_start body, so changing this prefix without regenerating the expected files makes the parser test fail for every template. Please update the generated fixtures together with this prefix (or otherwise exclude the prefix from that comparison).

function session_start()
{
  if($_jst_session)
    return true;
  if(!ccsp_session.start())

tests/parser_test.cpp:860

• This test intends to model an HTTP request, but request_is_https() also treats REQUEST_SCHEME=https and any non-empty SSL_PROTOCOL as secure. If either variable is inherited from the test runner, the jst_sess1... cookie will be accepted and this assertion fails. Clear and guard all three scheme-related environment variables before the test.

  EnvVarGuard cookie_guard("HTTP_COOKIE");
  EnvVarGuard https_guard("HTTPS");

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

[Copilot]

🟡 Changes recommended

Unresolved issues include stale-session writes, outdated parser snapshots, and test assertions that may mask errors.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

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

tests/parser_test.cpp:889

• request_is_https() also treats REQUEST_SCHEME=https and any non-empty SSL_PROTOCOL as secure. This test only clears HTTPS, so an inherited value in either of those variables makes the supposedly HTTP request accept the jst_sess1 cookie and causes the EXPECT_FALSE to fail. Guard and unset both variables here, as the secure-cookie test does above.

jsts/jst_prefix.js:116

• This guard only checks whether the in-memory identifier is set. getStatus() remains true after the backing file is removed, and session_set_data() opens the path with fopen(..., "w"), so mutating a retained proxy can recreate a session that has already become invalid. Make the write path reject a missing backing file (or have status revalidate it) before persisting.

      if(ccsp_session.getStatus() && ccsp_session.getId() === $session_id)
        ccsp_session.setData(obj);

tests/parser_test.cpp:214

• Evaluation errors are converted into false, so every negative assertion using this helper can pass when the JavaScript throws (or the expression evaluates to a non-boolean). That can hide regressions in the new session-prefix tests; record a test failure or propagate the exception instead of treating evaluation failure as a valid false result.

  if (duk_peval_string(ctx, source) != DUK_EXEC_SUCCESS)
  {
    duk_pop(ctx);
    return false;
  }

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

[Copilot]

🟡 Changes recommended

Unresolved session lifecycle, cookie expiry, legacy-ID compatibility, and test-environment issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

jsts/jst_prefix.js:187

• The new expiry header is lost when an application follows the common session_destroy(); header('Location: ...') sequence: header() replaces _jst_header_buffer for Location responses, so it discards the Set-Cookie: DUKSID=; Max-Age=0 queued here. Preserve previously accumulated headers when handling a redirect (or otherwise ensure the expiry is emitted after the redirect) so destroying a session before redirecting actually clears the browser cookie.

  header(_jst_expire_session_cookie());

source/jst_session.c:246

• The new scheme check is applied to every existing 40-character session ID, but IDs created before this change also use the character at offset 8 as random entropy rather than an HTTP/HTTPS marker. After an upgrade, almost all persisted pre-change sessions will therefore be rejected (and their stale cookies are not refreshed), causing existing users to lose their sessions. Please either define a compatibility/migration path for legacy IDs or explicitly account for this logout behavior.

          if(parsed_sesid[SESSION_SCHEME_OFFSET] != request_session_scheme())
          {
            CosaPhpExtLog("%s: SessionID scheme mismatch, rejecting\n", __PRETTY_FUNCTION__);

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

[Copilot]

🔵 Needs a closer look

Five unresolved moderate findings affect cookie deletion, persistence, session consistency, legacy IDs, and test isolation.

Review details

Suppressed comments (5)

jsts/jst_prefix.js:96

• This cookie is emitted without a Path, so its default path depends on the URL that created it. If session_create() and session_destroy() run under different UI routes, the browser may not match this deletion cookie and will keep sending the invalid DUKSID; use one explicit path for both cookie builders.

  var $cookie = "Set-Cookie: DUKSID=" + ccsp_session.getId() + "; httponly";
  if(ccsp_session.isSecure())
    $cookie += "; secure";

jsts/jst_prefix.js:202

• Each delete here goes through the Proxy's deleteProperty trap, which calls ccsp_session.setData() for every key, and the function then writes the empty object once more. A session with N properties therefore rewrites its backing file N+1 times; batch the deletions (or suppress proxy persistence during the loop) and persist once.

  for(var $key in $_jst_session)
    delete $_jst_session[$key];

  return ccsp_session.setData($_jst_session);

jsts/jst_prefix.js:168

• If ccsp_session.create() fails before replacing the native identifier (for example during allocation or getrandom() in source/jst_session.c), the C module can still have the previous session active. Clearing the JavaScript session here then leaves the two layers inconsistent, and a later session_start() can silently resume the old session after the failed create. Preserve the existing JS session when the native status is still active, or make native create() roll back/clear atomically.

  if(!ccsp_session.create())
  {
    $_jst_session = null;
    $_SESSION = {};
    return false;

source/jst_session.c:247

• The new scheme check rejects almost every session ID issued before this change: older session_create() generated all 32 characters randomly, so the character at SESSION_SCHEME_OFFSET was not a reliable 0/1 tag. Existing cookies will therefore fail session_start() after upgrade (and leave the old session file behind). Please add a migration/versioning path—such as accepting legacy IDs and reissuing a scheme-tagged ID—or otherwise explicitly handle preexisting sessions before enforcing this check.

          if(parsed_sesid[SESSION_SCHEME_OFFSET] != request_session_scheme())
          {
            CosaPhpExtLog("%s: SessionID scheme mismatch, rejecting\n", __PRETTY_FUNCTION__);
          }

tests/parser_test.cpp:884

• request_is_https() also treats REQUEST_SCHEME=https and any non-empty SSL_PROTOCOL as secure, but this test only clears HTTPS. On a runner with either of those variables set, the supposed HTTP request is detected as HTTPS and the test can accept the jst_sess1... cookie instead of exercising the rejection path.

  EnvVarGuard cookie_guard("HTTP_COOKIE");
  EnvVarGuard https_guard("HTTPS");

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

[Copilot]

🟡 Changes recommended

Unresolved review findings include three critical and one moderate issue.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

source/jst_session.c:245

• The scheme check now interprets the first suffix character as a mandatory 0/1 tag, but sessions created before this change had all 32 suffix characters generated randomly (the assignment at line 315 is new). Consequently, virtually every active pre-upgrade cookie is rejected after deployment. Preserve/recognize the legacy ID format or provide an explicit migration/expiry path before enforcing the new tag.

          if(parsed_sesid[SESSION_SCHEME_OFFSET] != request_session_scheme())

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

[Copilot]

🟡 Changes recommended

Moderate unresolved findings remain around preserving active session state and buffered deletion cookies.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

jsts/jst_prefix.js:199

• The deletion cookie is only buffered here. If the usual logout flow calls header("Location: ...") afterward, the Location: branch replaces _jst_header_buffer and drops this Set-Cookie, so the browser keeps the session cookie. Preserve already-buffered headers when handling redirects (or otherwise ensure the expiration header is emitted after the redirect).

  header(_jst_expire_session_cookie());

tests/parser/jst_prefix.js:180

• The C implementation leaves the existing session_identifier intact when ccsp_session.create() fails (all failure paths occur before the replacement at source/jst_session.c:354-360). Clearing $_jst_session and $_SESSION here therefore makes session_status() remain true while the JavaScript session object is detached and empty, for example on a file-creation failure during an active session. Return false without clearing the current JavaScript state so the operation remains atomic.

  if(!ccsp_session.create())
  {
    $_jst_session = null;
    $_SESSION = {};
    return false;

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