Added confidential checker - #6981
Conversation
…and SqlSanitizedUser
…bined SqlQuoteless and SqlEvenQuotes; revised documentations
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
prek.toml (1)
29-48: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWhitespace hooks were broadened over byte-exact test fixtures. Both exclude lists lost an entry protecting golden files that tests compare literally, so the hooks will now rewrite fixture content and break expected-output comparisons.
prek.toml#L29-L48: re-add"**/*.goal"to theend-of-file-fixerexclude globs.prek.toml#L55-L68: re-add"**/expected*"to thetrailing-whitespaceexclude globs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prek.toml` around lines 29 - 48, Restore the missing fixture exclusions in prek.toml: add "**/*.goal" to the end-of-file-fixer exclude globs at prek.toml lines 29-48, and add "**/expected*" to the trailing-whitespace exclude globs at prek.toml lines 55-68.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.circleci/config.yml:
- Around line 14-17: The workflows.build configuration currently runs only prek;
restore the removed build/test job matrix and its dependencies alongside prek.
Ensure the workflow again compiles the framework and executes the
checker/tests/confidential tests, preserving the existing job dependency
relationships.
- Around line 8-13: Update the CircleCI steps around “Install prek” and “Run
prek hooks” to persist uv’s tool binary directory, ~/.local/bin, in $BASH_ENV
after installing prek so later isolated run shells can resolve the prek command.
Keep the existing uv installation and hook invocation unchanged.
In @.pmd-ruleset.xml:
- Around line 26-28: Update the PMD ruleset properties near logLevels and
guardsMethods by removing only the duplicate info entry from logLevels. Keep
both properties enabled and preserve the existing guard method configuration.
---
Outside diff comments:
In `@prek.toml`:
- Around line 29-48: Restore the missing fixture exclusions in prek.toml: add
"**/*.goal" to the end-of-file-fixer exclude globs at prek.toml lines 29-48, and
add "**/expected*" to the trailing-whitespace exclude globs at prek.toml lines
55-68.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 17c7b2d1-4f30-4f8d-88ab-1fdb9b0815cd
📒 Files selected for processing (3)
.circleci/config.yml.pmd-ruleset.xmlprek.toml
| <property name="logLevels" value="trace,debug,info,warn,error,log,finest,finer,fine,info,warning,severe" /> | ||
| <!-- Adds "isEnabled" to the default list. --> | ||
| <property name="guardsMethods" value="isTraceEnabled,isDebugEnabled,isInfoEnabled,isWarnEnabled,isErrorEnabled,isLoggable,isEnabled" /> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the pinned PMD version to validate the property names.
rg -n 'pmd' -i build.gradle | head -50
fd -t f 'gradle.properties|versions.gradle' -H | xargs -r rg -n -i pmdRepository: typetools/checker-framework
Length of output: 270
🏁 Script executed:
#!/bin/bash
set -u
echo "== PMD occurrences =="
git ls-files | xargs rg -n -i 'pmd' || true
echo
echo "== build.gradle relevant section =="
sed -n '130,260p' build.gradle
echo
echo "== PMD ruleset =="
cat -n .pmd-ruleset.xml
echo
echo "== gradle files candidates =="
git ls-files | rg '(^|/)(settings\.gradle|build\.gradle|gradle\.properties|versions\.gradle|libs\.versions\.toml)$' || trueRepository: typetools/checker-framework
Length of output: 34623
🌐 Web query:
PMD GuardLogStatement property names logLevels guardsMethods version
💡 Result:
The GuardLogStatement rule in PMD is a best-practice rule designed to ensure that log statements (such as trace, debug, or error) are guarded by an appropriate condition check (e.g., isDebugEnabled) to avoid unnecessary object creation and method calls [1][2]. Rule Properties The rule uses two primary multi-valued properties to define which log methods require guarding [1][2]: 1. logLevels: A list of log method names to be guarded. By default, these include: trace, debug, info, warn, error, log, finest, finer, fine, info, warning, and severe [1][2]. 2. guardsMethods: A list of corresponding guard method names. By default, these include: isTraceEnabled, isDebugEnabled, isInfoEnabled, isWarnEnabled, isErrorEnabled, and isLoggable [1][2]. Implementation Requirements The rule maintains a mapping between these two lists. When configuring these properties, the number of log levels must correspond to the number of guard methods [3][4]. Specifically: - For every logLevel, a guardMethod must be specified [3]. - If the number of provided logLevels exceeds the number of provided guardsMethods, the last guard method provided is reused for the remaining log levels [3]. - It is not possible to specify logLevels without also specifying guardMethods [3]. Version History The GuardLogStatement rule was introduced in PMD 5.1.0 [5][6]. It remains a standard rule in modern versions of PMD, categorized under Best Practices [1][2]. Configuration of these properties is typically handled within a ruleset XML file [1][2]. Example Configuration
Citations:
- 1: https://pmd.github.io/pmd/pmd_rules_java_bestpractices.html
- 2: https://docs.pmd-code.org/pmd-doc-6.55.0/pmd_rules_java_bestpractices.html
- 3: https://github.com/pmd/pmd/blob/main/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/GuardLogStatementRule.java
- 4: https://fossies.org/dox/pmd-dist-7.25.0-src/GuardLogStatementRule_8java_source.html
- 5: https://pmd.sourceforge.io/pmd-5.2.2/pmd-java/rules/java/logging-jakarta-commons.html
- 6: https://pmd.sourceforge.io/pmd-5.2.1/pmd-java/rules/java/logging-jakarta-commons.html
Keep the PMD properties enabled and deduplicate only info.
logLevels/guardsMethods are valid GuardLogStatement properties, and the commented-out PMD config still pins 7.25.0. Remove the duplicate info in the logLevels value for clarity, but leave these properties configured unless PMD is intentionally disabled.
♻️ Proposed cleanup
- <property name="logLevels" value="trace,debug,info,warn,error,log,finest,finer,fine,info,warning,severe" />
+ <property name="logLevels" value="trace,debug,info,warn,error,log,finest,finer,fine,warning,severe" />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <property name="logLevels" value="trace,debug,info,warn,error,log,finest,finer,fine,info,warning,severe" /> | |
| <!-- Adds "isEnabled" to the default list. --> | |
| <property name="guardsMethods" value="isTraceEnabled,isDebugEnabled,isInfoEnabled,isWarnEnabled,isErrorEnabled,isLoggable,isEnabled" /> | |
| <property name="logLevels" value="trace,debug,info,warn,error,log,finest,finer,fine,warning,severe" /> | |
| <!-- Adds "isEnabled" to the default list. --> | |
| <property name="guardsMethods" value="isTraceEnabled,isDebugEnabled,isInfoEnabled,isWarnEnabled,isErrorEnabled,isLoggable,isEnabled" /> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.pmd-ruleset.xml around lines 26 - 28, Update the PMD ruleset properties
near logLevels and guardsMethods by removing only the duplicate info entry from
logLevels. Keep both properties enabled and preserve the existing guard method
configuration.
| import org.checkerframework.framework.qual.TypeUseLocation; | ||
|
|
||
| /** | ||
| * Represents a value that might or might not be confidential. This is the top of the Confidential |
There was a problem hiding this comment.
"the top of the" => "the top qualifier in the"
|
This is essentially ready to merge. I have just a few comments. |
| Since \code{@NonConfidential} is the default annotation, if a programmer | ||
| forgets to annotate a sensitive source as \<@Confidential>, then there will | ||
| be an information leak that the Confidential Checker cannot warn about. | ||
| Once sources and sinks are annotated, the Confidential Checker guarantees |
There was a problem hiding this comment.
Is this saying that annotations aren't need elsewhere? I would assume that you still need to annotate method signatures.
| indicates a value whose confidentiality is unknown. | ||
| \code{@UnknownConfidential} is a supertype of \code{@Confidential} | ||
| and \code{@NonConfidential}. | ||
| \item[\refqualclass{checker/confidential/qual}{PolyConfidential}] |
There was a problem hiding this comment.
I suggest putting @PolyConfidential last, after the real type qualifiers, even though it is used more often than @BottomConfidential.
| \end{figure} | ||
|
|
||
| The Confidential Checker's \code{@Confidential} qualifier has similarities | ||
| to the Taint Checker's \code{@Tainted} qualifier. One difference is that |
There was a problem hiding this comment.
| to the Taint Checker's \code{@Tainted} qualifier. One difference is that | |
| to the Tainting Checker's \code{@Tainted} qualifier. One difference is that |
| The Confidential Checker's \code{@Confidential} qualifier has similarities | ||
| to the Taint Checker's \code{@Tainted} qualifier. One difference is that | ||
| there is no subtyping relationship between \code{@Confidential} and | ||
| \code{@NonConfidential}. However, the external behavior of these annotations |
There was a problem hiding this comment.
I don't understand what you mean by external behavior here.
| * A TreeAnnotator to enforce certain toString return type rules: | ||
| * | ||
| * <ul> | ||
| * <li>toString(@NonConfidential this) can return @NonConfidential or @Confidential String |
There was a problem hiding this comment.
This comment does not match the implemenation.
Associated with JDK pull request #224