Skip to content

require-nan-check-after-split-index-parse: Logical/Conditional-wrapped split-index access escapes detection entirely #53054

Description

@github-actions

Summary

require-nan-check-after-split-index-parse only recognizes a numeric-parse call's first argument as a tracked "split-index access" when it is directly a <expr>.split(...)[<index>] MemberExpression. If that access is wrapped in a LogicalExpression (|| / ??) or ConditionalExpression (ternary), the call is never added to the rule's unvalidated set at all — it is not merely unvalidated-but-tracked, it is invisible to the rule.

This is the same bug class that was already found and fixed in the sibling rule require-nan-check-after-env-numeric-parse (closed issues #50495 and #50496, "ternary-wrapped env parse escapes detection" / Number.isFinite() FP). That rule's containsEnvAccess helper recursively unwraps LogicalExpression and ConditionalExpression nodes when tracing whether a parse argument originates from process.env. require-nan-check-after-split-index-parse's equivalent isSplitIndexAccess never received the same treatment.

Where

eslint-factory/src/rules/require-nan-check-after-split-index-parse.ts, isNumericParseCallFromSplitIndex (lines 85-111) calls isSplitIndexAccess(firstArg) (lines 75-79) directly, with no unwrapping of LogicalExpression/ConditionalExpression around firstArg.

Compare to the already-fixed sibling eslint-factory/src/rules/require-nan-check-after-env-numeric-parse.ts, containsEnvAccess (lines 29-57), which recursively handles MemberExpression, ChainExpression, CallExpression, LogicalExpression, and ConditionalExpression.

Reproduction (false negative — rule never fires)

function parsePort(endpoint, useFallback, fallbackEndpoint) {
  const port = parseInt(useFallback ? fallbackEndpoint.split(":")[1] : endpoint.split(":")[1], 10);
  return port; // never validated anywhere; NaN silently propagates if either split produces undefined
}
function parsePort(endpoint, altEndpoint) {
  const port = parseInt(endpoint.split(":")[1] || altEndpoint.split(":")[1], 10);
  return port; // same issue with || instead of a ternary
}

In both cases, firstArg is a ConditionalExpression/LogicalExpression, not a MemberExpression, so isSplitIndexAccess returns false immediately and isNumericParseCallFromSplitIndex returns false — the VariableDeclarator is never added to unvalidated, so Program:exit never reports it, regardless of whether any NaN validation exists downstream.

Grounding notes on the live corpus

The only two live split-index-parse call sites in actions/setup/js (add_workflow_run_comment.cjs:419 and :428, both parseInt(endpoint.split(":")[1], 10)) use the direct (non-wrapped) form, so this exact FN pattern doesn't currently appear in the codebase. The bug is nonetheless a genuine latent gap: it's the identical structural weakness that was worth fixing in the env-numeric-parse sibling once discovered there, and it will silently swallow future code that adds a fallback/alternate split source — a very natural pattern when parsing delimited config strings with multiple possible formats.

Suggested fix

Add a recursive unwrap step before calling isSplitIndexAccess, mirroring containsEnvAccess's LogicalExpression/ConditionalExpression handling:

function isSplitIndexAccessDeep(node: TSESTree.Node): boolean {
  if (node.type === "LogicalExpression") {
    return isSplitIndexAccessDeep(node.left) || isSplitIndexAccessDeep(node.right);
  }
  if (node.type === "ConditionalExpression") {
    return isSplitIndexAccessDeep(node.consequent) || isSplitIndexAccessDeep(node.alternate);
  }
  return isSplitIndexAccess(node);
}

Acceptance criteria

  • isNumericParseCallFromSplitIndex recognizes split-index access wrapped in one or more levels of LogicalExpression (||, ??) and/or ConditionalExpression, matching the recursive approach already used in require-nan-check-after-env-numeric-parse's containsEnvAccess.
  • New rule tests cover: ternary-wrapped split-index access on both branches, ||-wrapped split-index access on both operands, and a mixed case (one branch a split-index access, the other a plain literal/identifier) to confirm the rule still fires when validation is missing.
  • Existing passing tests for the direct (non-wrapped) case continue to pass unchanged.
  • add_workflow_run_comment.cjs:419/:428 (direct, non-wrapped form) remain correctly flagged as unvalidated (regression check — these are real live unguarded call sites already within the rule's existing detection surface).

Generated by 🤖 ESLint Refiner · agent · 219.3 AIC · ⌖ 4.9 AIC · ⊞ 5.2K ·

  • expires on Aug 22, 2026, 9:32 PM UTC-08:00

Activity

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

Metadata

Metadata

Labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions