Skip to content

Java: Diff-informed CleartextStorageCookie.ql #19846

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jbj
Copy link
Contributor

@jbj jbj commented Jun 23, 2025

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.

@jbj jbj 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
@Copilot Copilot AI review requested due to automatic review settings June 23, 2025 11:29
@jbj jbj requested a review from a team as a code owner June 23, 2025 11:29
@github-actions github-actions bot added the Java label Jun 23, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

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.

File Description
java/ql/lib/semmle/code/java/security/CleartextStorageQuery.qll Stubbed out getASelectedLocation on base sink and updated flow config
java/ql/lib/semmle/code/java/security/CleartextStorageCookieQuery.qll Added 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 {

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 jbj force-pushed the diff-informed-CleartextStorageCookie branch from 93ba865 to a4449b4 Compare June 23, 2025 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Java no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants