SONARJAVA-6522 Don't raise S2092 for deletion cookies#5785
Conversation
new Cookie(name, null/"") + setMaxAge(0) is a common idiom for clearing an existing cookie, not storing a value, so whether it's marked secure is moot. Skip reporting when both the literal null/empty value and the literal setMaxAge(0)/maxAge(0) call are present for the same cookie; ambiguous (non-literal) cases still raise as before. Covers javax/jakarta servlet Cookie's constructor+setter pattern and Spring's ResponseCookie.from(...) fluent builder chain.
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
The new deletion-cookie suppression depends on resolved semantic types (constructor value literal, maxAge argument, ResponseCookie builder chain), which AutoScan's reduced-semantics mode can't resolve. The fail-safe guards correctly keep raising in that mode, widening the known Maven-vs-AutoScan divergence for S2092 by 2.
isDeletionCookieChain only walked backward from the secure(false) call via its receiver chain, so maxAge(0) placed after secure(false) in the same fluent chain (e.g. .secure(false).maxAge(0)) was invisible and still raised. Climb to the chain's outermost call first via parent() before walking, so the whole chain is inspected regardless of call order. Addresses Gitar review feedback on PR #5785.
|
Fixed — good catch. `isDeletionCookieChain` only walked backward from the `secure(false)` call via its receiver chain, so `maxAge(0)` placed after `secure(false)` in the same fluent chain was structurally an ancestor, not reachable by that walk. Added a `chainRoot` helper that climbs to the outermost call of the fluent chain via `Tree.parent()` before walking, so the whole chain is inspected regardless of call order. Added a regression test (`deleteCookieMaxAgeAfterSecure`) covering `.secure(false).maxAge(0)` ordering. |
Document why deletionCandidateCookies is a separate map from unsecuredCookies, why isDeletionValue assumes argument index 1, and why isDeletionCookieChain walks the whole chain instead of just the receiver sub-chain.
| Boolean secureArgument = valueResolution.value(); | ||
| boolean isFalse = secureArgument != null && !secureArgument; | ||
| if (isFalse) { | ||
| if (isFalse && !isDeletionCookieChain(mit)) { |
There was a problem hiding this comment.
This suppresses the wrong path for the servlet/JAX-RS Cookie case. isDeletionCookieChain (per its own javadoc) only recognizes the Spring ResponseCookie fluent-chain pattern — it walks receiver MethodInvocationTrees. For cookie.setSecure(false) where cookie is a plain identifier, the receiver is an IdentifierTree, so the loop in isDeletionCookieChain never executes and it always returns false. !isDeletionCookieChain(mit) is therefore always true here, and this line unconditionally reports whenever secure(false) is a literal — regardless of whether the cookie is a deletion cookie.
The deletionCandidateCookies/checkMaxAgeCall mechanism only removes the entry from cookieConstructors, which is consulted only in leaveFile() (for cookies that were never explicitly secured). It's never consulted here.
Repro — this is the ticket's own second compliant example:
Cookie cookie = new Cookie("refreshToken", null);
cookie.setHttpOnly(true);
cookie.setSecure(false);
cookie.setPath("/");
cookie.setMaxAge(0);
response.addCookie(cookie);Running SecureCookieCheck against this still raises S2092 on the cookie.setSecure(false) line. Verified with both call orders (setMaxAge(0) before and after setSecure(false)) — same result both times, so it's not an ordering issue.
None of the new sample cases added in this PR call .setSecure(false) explicitly (they only rely on the never-explicitly-secured constructor path), so this gap has no test coverage.
Suggested fix: this method needs to also check deletionCandidateCookies for the receiver's symbol (like checkMaxAgeCall does) before reporting, not just rely on the Spring-chain check.
There was a problem hiding this comment.
Good catch — confirmed and fixed. isDeletionCookieChain was indeed Spring-chain-only; the servlet/JAX-RS Cookie explicit setSecure(false) case never consulted deletionCandidateCookies at all.
Fixed by deferring the report decision to leaveFile() for deletion-candidate variables: setMaxAge(0) sightings are now recorded in a plain, never-removed fact set, so the check no longer cares whether setMaxAge(0) comes before or after setSecure(false) in the method. Added test coverage for both statement orders (including your exact repro) plus the still-raises case with no setMaxAge call at all.
isDeletionCookieChain only recognizes the Spring ResponseCookie fluent-chain pattern, so an explicit cookie.setSecure(false) on a servlet/JAX-RS Cookie deletion candidate was never suppressed and always reported immediately, regardless of a later/earlier setMaxAge(0) call on the same variable (the ticket's own second compliant example). Defer the report decision to leaveFile() for deletion candidates: record setMaxAge(0) sightings in a plain fact set (never removed, so order relative to setSecure(false) doesn't matter), and only report the deferred setSecure(false) call if that fact is still absent once the whole file has been scanned. Also fixes checkMaxAgeCall to stop removing from deletionCandidateCookies, since checkSecureCall may still need to consult it later in the same method. Adds test coverage for both statement orders, the still-raises case with no setMaxAge call, and the assignment-form (non-declaration) deletion-candidate tracking path. Addresses review feedback from Luqman on PR #5785.
…al maxAge - checkSecureCall's receiverSymbol branch, when the setSecure(false) receiver is a field access (this.c) rather than a bare identifier. - isDeletionCookieChain's maxAge branch, when the argument isn't a literal zero (out of scope per the ticket, must still raise).
checkSecureCall and checkMaxAgeCall cast an IdentifierTree's symbol directly to Symbol.VariableSymbol without checking isVariableSymbol() first, unlike the sibling addToUnsecuredCookies/addToDeletionCandidates methods and unlike the code this replaced (Map.remove(Object) never needed the cast in the first place). Extract a shared variableSymbolOf helper that guards the cast, consistent with the file's existing style and robust on non-compiling sources. Also bump the autoscan golden diff for S2092 (98->99 FN) to reflect the two new realistic test cases added for coverage in the previous commit. Addresses Gitar review feedback on PR #5785.
Code Review ✅ Approved 2 resolved / 2 findingsUpdates S2092 to suppress false positives for cookie deletion patterns in Servlet and Spring ResponseCookie APIs. Resolved issues include the unguarded identifier symbol cast and incorrect maxAge(0) detection within method chains. ✅ 2 resolved✅ Edge Case: Spring chain suppression misses maxAge(0) placed after secure(false)
✅ Bug: Unguarded cast of identifier symbol to VariableSymbol
OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
nils-werner-sonarsource
deleted the
sonarjava-6522-cookie-deletion-pattern
branch
Code Review ✅ Approved 2 resolved / 2 findingsUpdates S2092 to suppress false positives for cookie deletion patterns in Servlet and Spring ResponseCookie APIs. Resolved issues include the unguarded identifier symbol cast and incorrect maxAge(0) detection within method chains. ✅ 2 resolved✅ Edge Case: Spring chain suppression misses maxAge(0) placed after secure(false)
✅ Bug: Unguarded cast of identifier symbol to VariableSymbol
OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
2 participants
Problem
S2092(SecureCookieCheck) flags the common "delete this cookie" idiom —new Cookie(name, null)/new Cookie(name, "")combined withsetMaxAge(0), or the Spring equivalentResponseCookie.from(name, "").maxAge(0)...— as if it were a real, value-carrying cookie missing its secure flag. A deletion cookie carries no sensitive value, so whether it's marked secure is moot, but the rule doesn't know that.SONARJAVA-6522
Approach
Guiding principle (shared with the sibling ticket SONARJAVA-6523, #5781): favor fewer false positives, and fail safe on ambiguity — if either condition can't be resolved to a literal, don't suppress, keep raising as before.
The ticket spans two independent code paths in
SecureCookieCheck.java, since they behave differently today:javax.servlet.http.Cookie/jakarta.servlet.http.Cookie: how to detect the deletion pattern?new Cookie(name, value)(2-arg constructor, no secure param) is always flagged today unless a latersetSecure(true)cancels it — this is where the real FP lives. Added a second, independentMap<Symbol.VariableSymbol, NewClassTree>(deletionCandidateCookies), populated when the constructor's value argument is a literalnull/"", and consumed by a newcheckMaxAgeCallwhen a literalsetMaxAge(0)is later seen on the same variable — mirroring the existingunsecuredCookies/checkSecureCallcancellation idiom already in this file.unsecuredCookies?ResponseCookie.from(name, value).maxAge(0)...secure(false): how to detect the deletion pattern?.secure(false)in the chain triggers it, reported immediately. Since it's a single self-contained expression tree, no new tracking map is needed: added a backward structural walk (isDeletionCookieChain) from thesecure(false)call viamethodSelect().expression(), looking for a.maxAge(0)and a root.from(name, null/"")call anywhere in the chain. Order-independent by construction (it's a tree walk, not sequential-visitation-based).MethodMatchersfor.maxAge(...), or name+literal-arg matching?MethodMatchersformaxAge(long), consistent with this file's existing style for constructor-overload matching, and naturally excludes theDurationoverload (out of scope per the ticket's literal-only requirement).Implementation
SecureCookieCheck.java: newdeletionCandidateCookiesmap +addToDeletionCandidates/checkMaxAgeCallfor the servlet/JAX-RSCookiepath; newRESPONSE_COOKIE_FROM/RESPONSE_COOKIE_MAX_AGE_ZEROmatchers +isDeletionCookieChainbackward chain-walk for the SpringResponseCookiebuilder path; one sharedisNullOrEmptyLiteralhelper used by both. No new abstractions beyond what's needed.Tests
SecureCookieCheckSample.java(default sources) — servletCookiedeletion pattern:setMaxAge(0).setMaxAgecall (Condition B fails), non-zerosetMaxAge(Condition B fails), non-literalsetMaxAgeargument (ambiguous, fails safe).SecureCookieCheckSample.java(spring-3.2 module) —ResponseCookiebuilder chain:.maxAge(0)+.secure(false)in the same chain..maxAge(0), and real value with.maxAge(0)present (Condition A fails).Test plan
mvn -pl java-checks test -Dtest="SecureCookieCheckTest#test,SecureCookieCheckTest#test_jakarta,SecureCookieCheckTest#test_non_compiling"— all passSecureCookieCheckTest#test_with_spring_3_2fails identically (same "expect some issues, but there's none" pre-existing environment failure) on unmodifiedmasterand on this branch — pre-existing, unrelated to this change (same finding as SONARJAVA-6523 Don't raise S2092 on Cookie subtype self-instantiation #5781)org.sonar.java.checks.security.*Testsuite — identical set of 15 pre-existing unrelated failures (Android/WebView/ClearTextProtocol/CookieHttpOnly/DebugFeatureEnabled/EmptyDatabasePassword/ReceivingIntents + the knownSecureCookieCheckTestspring-3.2 failure) on this branch; no new regressions introduced