RDKB-66802 : harden session lifecycle and cookie scope

[pavankumar464]

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

[Copilot]

🟡 Changes recommended

Five moderate review findings remain unresolved across session handling, cookie scope, legacy ID migration, and test error reporting.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Hardens session lifecycle and parsing for invalid sessions across the native backend, JavaScript runtime, and parser tests.

Changes:

• Adds scheme-bound validation and safer session-file handling.
• Updates session lifecycle, stale-session, cookie, and cleanup behavior.
• Expands regression tests and regenerates parser fixtures.

File summaries

File	Summary
tests/parser/jst_prefix.js	Updated parser runtime fixture; native create failures can detach the JavaScript session wrapper from an active native session (moderate, 1 vote).
tests/parser/jst_parser_template_block_string.jst.parsed	Regenerated template block string parser fixture.
tests/parser/jst_parser_template_block_content.jst.parsed	Regenerated template block content parser fixture.
tests/parser/jst_parser_skip_whitespace.jst.parsed	Regenerated whitespace-skipping parser fixture.
tests/parser/jst_parser_single_quotes.jst.parsed	Regenerated single-quote parser fixture.
tests/parser/jst_parser_line_feeds.jst.parsed	Regenerated line-feed parser fixture.
tests/parser/jst_parser_include_unknown.jst.parsed	Regenerated unknown-include parser fixture.
tests/parser/jst_parser_include_runtime.jst.parsed	Regenerated runtime-include parser fixture.
tests/parser/jst_parser_include_once.jst.parsed	Regenerated include-once parser fixture.
tests/parser/jst_parser_include_not_if_in_line_comment.jst.parsed	Regenerated line-comment include parser fixture.
tests/parser/jst_parser_include_not_if_in_content.jst.parsed	Regenerated content include parser fixture.
tests/parser/jst_parser_include_not_if_in_block_comment.jst.parsed	Regenerated block-comment include parser fixture.
tests/parser/jst_parser_include_nested.jst.parsed	Regenerated nested-include parser fixture.
tests/parser/jst_parser_include_malformed_1.jst.parsed	Regenerated malformed-include parser fixture.
tests/parser/jst_parser_include_code_before.jst.parsed	Regenerated pre-code include parser fixture.
tests/parser/jst_parser_include_code_after.jst.parsed	Regenerated post-code include parser fixture.
tests/parser/jst_parser_comment_tag.jst.parsed	Regenerated comment-tag parser fixture.
tests/parser/jst_parser_backslash.jst.parsed	Regenerated backslash parser fixture.
tests/parser_test.cpp	Adds lifecycle tests; evaluation errors can be converted to false and hide JavaScript failures (moderate, 1 vote).
tests/CMakeLists.txt	Configures parser tests with the updated prefix path.
source/jst_session.c	Adds session validation and secure file handling; the scheme check can reject legacy IDs without migration or reissue (moderate, 2 votes).
jsts/jst_prefix.js	Updates session lifecycle and cookie handling; create failures can detach an active wrapper, and cookies need a consistent explicit Path (moderate, 2 votes; moderate, 1 vote).

Review details

Suppressed comments (3)

jsts/jst_prefix.js:103

• The session cookie is created without an explicit Path, and this expiry cookie is also created without one. Browsers derive the default path from each request URL, so destroying a session from a different URL directory can target a different cookie and leave the original DUKSID in place. Set and expire the cookie with the same explicit Path (or otherwise preserve the original cookie scope).

function _jst_expire_session_cookie()
{
  var $cookie = "Set-Cookie: DUKSID=; Max-Age=0; httponly";
  if(ccsp_session.isSecure())
    $cookie += "; secure";

tests/parser/jst_prefix.js:180

• The native ccsp_session.create() is transactional and leaves an already-active native session in place when allocation or file creation fails, but this branch clears only the JavaScript wrapper. After such a failure session_status() can still be true while $_SESSION is a detached plain object; preserve the existing wrapper on failure (or explicitly destroy the native session).

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

tests/parser_test.cpp:214

• Evaluation errors are converted into false, so every new EXPECT_FALSE(evaluateJavaScriptBoolean(...)) can pass when the JavaScript throws (for example, a broken prefix or missing dependency). Record the evaluation failure with GoogleTest instead of returning an indistinguishable false result, otherwise these tests can silently validate the error path.

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

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

---

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

[pavankumar464]

This change is to prevent stale-session failures, session-file security issues, and CGI 500 errors caused by invalid session state.
Session safety

• Validates DUKSID cookie format and only accepts standalone DUKSID fields.

• Prevents HTTP-created sessions from being reused over HTTPS, and vice versa.

• Generates session IDs with an HTTP/HTTPS scheme marker:
  -- jst_sess0... for HTTP
  -- jst_sess1... for HTTPS

• Detects stale or deleted backing files and clears invalid native session state.

• Creates session files immediately when a session is created, preventing a fresh session from being mistaken for a stale one.

• Creates session files with owner-only permissions (0600).

• Replaces sessions transactionally: the old session is retained until the replacement session file is created successfully.

JavaScript session wrapper

• session_start() and session_create() now return success/failure and safely reset $_SESSION on failure.
• session_destroy() expires only the browser’s DUKSID cookie.
• session_unset() is implemented.
• Session proxies revalidate the native session before saving data.
• Stale proxies cannot recreate deleted session files.
• Proxies retained from an older session cannot write into a replacement session.

Secure-cookie behavior

• Adds native ccsp_session.isSecure().
• Uses HTTPS, REQUEST_SCHEME, and SSL_PROTOCOL to determine whether the cookie needs the Secure attribute.
• Ensures logout uses matching secure-cookie behavior.

Tests and fixtures

• Adds tests for cookie validation, stale sessions, secure-session detection, session replacement, empty sessions, file permissions, and stale proxy writes.
• Fixes quoting of JST_PREFIX_PATH in CMake.
• Regenerates all parser .jst.parsed golden fixtures to reflect the updated prefix.

[Copilot]

🟡 Changes recommended

An unresolved critical runtime-prefix issue and session compatibility and test-isolation findings remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (5)

source/jst_session.c:319

• This changes the first entropy character of every newly created ID into a scheme marker, while session_start() now requires that marker to match the request scheme. IDs and files created by the previous version used this position as random entropy, so almost all existing UI sessions will be rejected after rollout rather than migrated or explicitly rotated. Add a legacy-ID migration/rotation path (or document and handle the intentional mass invalidation).

  session_id[0] = request_session_scheme();

tests/parser/jst_prefix.js:27

• lstr is assigned without a declaration, so every call to header() creates or overwrites a global variable named lstr. A template that uses the same global can have its value unexpectedly changed; keep this temporary value local.

  lstr = str.toLowerCase();

tests/parser_test.cpp:502

• makeValidSessionId() now creates an HTTP-only (0) session ID, but this test does not clear HTTPS, REQUEST_SCHEME, or SSL_PROTOCOL. With any of those inherited from the test environment, the new scheme check rejects this otherwise valid cookie, making the acceptance test flaky; clear the scheme variables as the secure-session tests do.

  const std::string session_id = makeValidSessionId('A');
  const std::string cookie = "theme=dark; DUKSID=" + session_id + "; lang=en";

tests/parser_test.cpp:710

• This new test also expects the jst_sess0... ID to be accepted, but it only guards HTTP_COOKIE. If HTTPS, REQUEST_SCHEME, or SSL_PROTOCOL is inherited, request_session_scheme() becomes HTTPS and the test fails before checking the expired-file lifecycle; clear those variables to make the test deterministic.

  EnvVarGuard cookie_guard("HTTP_COOKIE");
  const std::string session_id = makeValidSessionId('C');

tests/parser_test.cpp:953

• The stale-proxy test expects session_start() to accept the HTTP-marked fixture, but it does not isolate the new scheme environment variables. A test process with HTTPS, REQUEST_SCHEME, or SSL_PROTOCOL set will reject this cookie and fail at line 967; clear those variables here as in the other prefix tests.

  EnvVarGuard cookie_guard("HTTP_COOKIE");
  const std::string session_id = makeValidSessionId('F');

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

[Copilot]

🔵 Needs a closer look

Six moderate review findings remain unresolved.

Review details

Suppressed comments (6)

jsts/jst_prefix.js:163

• When a stale, mismatched, or expired DUKSID is supplied, ccsp_session.start() fails and this branch clears only server-side state; it never expires the browser cookie. The client will resend the same invalid cookie on every request, so callers that only invoke session_start() remain stuck in this failure path. Expire DUKSID when a cookie was present while preserving the no-header behavior for requests without one.

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

jsts/jst_prefix.js:180

• When ccsp_session.create() fails, the C implementation leaves the current session_identifier untouched, but this branch clears the JavaScript session state. That leaves the native session active while old proxies can still pass _jst_session_is_current() and write to it, and a later session_start() can unexpectedly resume it. Preserve the existing JS session on create failure (or explicitly tear down the native session) instead of clearing only the JS state.

  {
    $_jst_session = null;
    $_SESSION = {};
    return false;

source/jst_session.c:249

• This scheme check invalidates sessions created before this change: old IDs used an unrestricted alphanumeric character at this offset, so almost every existing session will fail here unless that random character happens to be 0/1. Please preserve a legacy-ID migration/rotation path (or explicitly plan and handle the forced session invalidation) before enforcing the new marker.

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

source/jst_session.c:343

• Creating a zero-byte session file here makes every new session call getData() on an empty file. read_file() allocates buf but returns 0 for that file, and session_get_data() does not free the buffer on its zero-length/error path, so each session_create() leaks an allocation (and session_unset() now produces the same empty-file state). Please make empty session data a handled/valid case and free the buffer on that path.

  fd = open(filename, O_WRONLY | O_CREAT | O_EXCL, S_IRUSR | S_IWUSR);
  if(fd < 0)
  {
    CosaPhpExtLog("Failed to create session file %s: %s\n", filename, strerror(errno));
    free(new_session_identifier);

tests/parser_test.cpp:214

• This helper turns any Duktape evaluation exception into false, so negative assertions such as EXPECT_FALSE(session_start()) also pass when the prefix throws before exercising the lifecycle. Record the exception with the test framework before returning so these tests cannot silently pass on setup or runtime errors.

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

tests/parser_test.cpp:140

• This helper now hard-codes the HTTP scheme marker, but the valid-cookie callers (for example session_start_accepts_existing_valid_cookie_id) only isolate HTTP_COOKIE. The implementation also derives the expected scheme from HTTPS, REQUEST_SCHEME, and SSL_PROTOCOL, so an inherited HTTPS-related environment variable makes these fixtures invalid and causes unrelated tests to fail. Clear/guard those variables or make the fixture scheme match the request setup in each caller.

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

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