Skip to content

Java: Diff-informed CleartextStorageCookie.ql - #19846

Merged
jbj merged 1 commit into
github:mainfrom
jbj:diff-informed-CleartextStorageCookie
Jun 27, 2025
Merged

Java: Diff-informed CleartextStorageCookie.ql#19846
jbj merged 1 commit into
github:mainfrom
jbj:diff-informed-CleartextStorageCookie

Conversation

@jbj

@jbjjbj commented Jun 23, 2025

Copy link
Copy Markdown
Contributor

I picked this commit out of #17846 because it doesn't rely on any of the controversial API changes that are holding back that PR. It appears there are no tests for CleartextStorageCookie.ql.

This query shares implementation with several other queries about cleartext storage, but it's the only one of them that's in the code-scanning suite. The sharing mechanism remains the same as before, but now each query has to override getASelectedLocation to become diff-informed.

Two other data-flow configurations are used in this query, but they can't easily be made diff-informed.

@jbjjbj added the no-change-note-required This PR does not need a change note label Jun 23, 2025
@jbj
jbj marked this pull request as ready for review June 23, 2025 11:29
CopilotAI review requested due to automatic review settings June 23, 2025 11:29
@jbj
jbj requested a review from a team as a code ownerJune 23, 2025 11:29

CopilotAI 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.

Pull Request Overview

Adds diff-informed support to the cleartext storage cookie query by extending the base sink API and implementing location selection for the cookie sink.

  • Introduce getASelectedLocation in CleartextStorageSink to enable diff-informed queries.
  • Update SensitiveSourceFlowConfig to observe diff-informed mode.
  • Implement CookieCleartextStorageSink with unification of the cookie variable and override of getASelectedLocation.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

FileDescription
java/ql/lib/semmle/code/java/security/CleartextStorageQuery.qllStubbed out getASelectedLocation on base sink and updated flow config
java/ql/lib/semmle/code/java/security/CleartextStorageCookieQuery.qllAdded CookieCleartextStorageSink with diff-informed location override
Comments suppressed due to low confidence (4)

java/ql/lib/semmle/code/java/security/CleartextStorageQuery.qll:14

  • [nitpick] Add a doc comment on getASelectedLocation to explain its role in diff-informed mode and that returning none() opts out of diff-informed execution.
 Location getASelectedLocation() { none() }

java/ql/lib/semmle/code/java/security/CleartextStorageCookieQuery.qll:10

  • [nitpick] Include a brief comment on the cookie field clarifying that it is bound via pattern unification in the constructor.
 Cookie cookie;

java/ql/lib/semmle/code/java/security/CleartextStorageCookieQuery.qll:14

  • [nitpick] Document the selection logic in getASelectedLocation to describe why each location branch is included and how they’re prioritized.
 override Location getASelectedLocation() {

java/ql/lib/semmle/code/java/security/CleartextStorageCookieQuery.qll:9

  • There are no tests covering CleartextStorageCookieQuery; consider adding tests to verify diff-informed sink selection and overall query correctness.
private class CookieCleartextStorageSink extends CleartextStorageSink {

@jbj
jbjforce-pushed the diff-informed-CleartextStorageCookie branch from 93ba865 to a4449b4CompareJune 23, 2025 17:15
This query shares implementation with several other queries about
cleartext storage, but it's the only one of them that's in the
code-scanning suite. The sharing mechanism remains the same as before,
but now each query has to override `getASelectedLocation` to become
diff-informed.
Two other data-flow configurations are used in this query, but they
can't easily be made diff-informed.
@jbj
jbjforce-pushed the diff-informed-CleartextStorageCookie branch from a4449b4 to fc2b18aCompareJune 26, 2025 07:34
@jbj

jbj commented Jun 26, 2025

Copy link
Copy Markdown
ContributorAuthor

I've rebased on top of main, which now includes a test for CleartextStorageCookie. The test passes with --check-diff-informed.

@aschackmullaschackmull 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.

Looks fine.

@jbj
jbj merged commit b446fe7 into github:mainJun 27, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Javano-change-note-requiredThis PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@jbj@aschackmull