Skip to content

SONARJAVA-6523 Don't raise S2092 on Cookie subtype self-instantiation#5781

Merged
nils-werner-sonarsource merged 1 commit into
masterfrom
sonarjava-6523-cookie-self-instantiation
Jul 13, 2026
Merged

SONARJAVA-6523 Don't raise S2092 on Cookie subtype self-instantiation#5781
nils-werner-sonarsource merged 1 commit into
masterfrom
sonarjava-6523-cookie-self-instantiation

Conversation

@nils-werner-sonarsource

@nils-werner-sonarsource nils-werner-sonarsource commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Problem

S2092 (SecureCookieCheck) flags cookie construction that lacks an explicit secure flag. It has a high false-positive rate on bridge/wrapper classes: a Cookie subtype that instantiates itself inside one of its own methods (typically clone()) purely to delegate cookie behavior — including setSecure — to an inner wrapped cookie. The rule fires on that self-instantiation as if it were a "naked" new Cookie(...), even though the secure flag is actually managed by the delegate cookie, not by this constructor call.

SONARJAVA-6523

Approach

Guiding principle: favor fewer false positives — the new suppression must fail safe, i.e. if either the instantiated type or the enclosing class's type can't be resolved, don't suppress (preserve prior reporting behavior).

Question Decision
How to detect self-instantiation? Track the lexically enclosing class via a Deque<Symbol.TypeSymbol>, pushed/popped on Tree.Kind.CLASS (mirrors the existing pattern in DebugFeatureEnabledCheck in the same package). In checkConstructor, skip adding to cookieConstructors when the NewClassTree's resolved type equals the enclosing class's type.
FQN string comparison or Type.equals(Type)? Type.equals(Type) — matches the existing idiom used elsewhere in this codebase (e.g. SingletonUsageCheck) for comparing two resolved types, and avoids unnecessary string materialization.
Does isSelfInstantiation also need to check that the enclosing class is itself a tracked Cookie subtype? No — redundant. checkConstructor already gates on isCookieClass(tree.symbolType()); if the instantiated type equals the enclosing class's type, the enclosing class is necessarily that same cookie subtype too.
Risk: Type.UNKNOWN is a shared singleton with default (reference) equals(), so two unrelated unresolved types would compare as "equal" Added an explicit !tree.symbolType().isUnknown() guard so unresolved types never trigger suppression. This directly encodes the fail-safe requirement rather than relying on the upstream isCookieClass gate as an implicit (and less obvious) invariant.
Scope: should sibling Cookie subtypes (not the enclosing class) also be suppressed? No — out of scope per the ticket. Type.equals() (exact type identity), not isSubtypeOf(), so a different Cookie subtype instantiated inside another Cookie subtype still raises.

Implementation

SecureCookieCheck.java: added Tree.Kind.CLASS to nodesToVisit(), a Deque<Symbol.TypeSymbol> enclosingClass field maintained via visitNode/leaveNode, and a new isSelfInstantiation guard in checkConstructor. No new abstractions or dependencies beyond what's needed.

Tests

Extended the existing SecureCookieCheckSampleB class (already extends Cookie with a matching constructor) in SecureCookieCheckSample.java:

  • A clone()-style method returning new SecureCookieCheckSampleB(...) — self-instantiation, compliant (no issue).
  • A sibling method returning new Cookie(...) from within the same class — different type than the enclosing class, still // Noncompliant (pins the out-of-scope carve-out).

Test plan

  • mvn -pl java-checks test -Dtest="SecureCookieCheckTest#test,SecureCookieCheckTest#test_jakarta,SecureCookieCheckTest#test_non_compiling" — all pass
  • Confirmed SecureCookieCheckTest#test_with_spring_3_2 fails identically on unmodified master (pre-existing, unrelated to this change)
  • Ran the full org.sonar.java.checks.security.*Test suite on both master and this branch — identical set of 14 pre-existing unrelated failures (Android/WebView checks) on both; no new regressions introduced

Bridge/wrapper Cookie subclasses that instantiate themselves (typically
in clone()) to delegate secure-flag handling to an inner cookie were
flagged as if constructing a plain insecure cookie. Skip reporting when
the instantiated type exactly matches the enclosing class, since the
type comparison already fails safe on unresolved types.
@hashicorp-vault-sonar-prod

hashicorp-vault-sonar-prod Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

SONARJAVA-6523

@gitar-bot

gitar-bot Bot commented Jul 13, 2026

Copy link
Copy Markdown
Code Review ✅ Approved

Prevents false-positive S2092 reports on Cookie subtype self-instantiation by tracking lexically enclosing types. This update preserves existing reporting behavior for unresolved types to ensure safe execution.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqube-next

Copy link
Copy Markdown

@Luqmansonar Luqmansonar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 💥

@nils-werner-sonarsource nils-werner-sonarsource merged commit 9dbde17 into master Jul 13, 2026
18 checks passed
@nils-werner-sonarsource nils-werner-sonarsource deleted the sonarjava-6523-cookie-self-instantiation branch July 13, 2026 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants