Open
Conversation
…ed references in code as necessary
…caped SQL values as SqlSanitized.
SQL tainting qualifiers and basic tests
…and SqlSanitizedUser
…bined SqlQuoteless and SqlEvenQuotes; revised documentations
mernst
requested changes
Jun 6, 2025
Member
mernst
left a comment
mernst
left a comment
There was a problem hiding this comment.
I have just a couple last comments.
checker/src/main/java/org/checkerframework/checker/confidential/ConfidentialTransfer.java
Show resolved
Hide resolved
checker/src/main/java/org/checkerframework/checker/confidential/ConfidentialTransfer.java
Outdated
Show resolved
Hide resolved
# Conflicts: # checker/src/main/java/org/checkerframework/checker/confidential/ConfidentialTransfer.java
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new Confidential Checker to the Checker Framework that identifies sensitive information exposure (information leakage). The checker prevents confidential values from flowing to non-confidential locations, helping prevent security vulnerabilities like PII exposure in logs, UI elements, or other public-facing sinks.
Key changes:
- Implements a complete type system with
@Confidential,@NonConfidential,@UnknownConfidential, and related qualifiers - Adds extensive library annotations for logging frameworks (Log4j, SLF4J, Apache Commons), Android UI components, and Spring Security
- Includes comprehensive documentation and test cases
Reviewed Changes
Copilot reviewed 49 out of 50 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| checker-qual/src/main/java/org/checkerframework/checker/confidential/qual/*.java | Core qualifier annotations defining the confidential type hierarchy |
| checker/src/main/java/org/checkerframework/checker/confidential/*.java | Main checker implementation with type factory, visitor, and transfer functions |
| checker/src/main/java/org/checkerframework/checker/confidential/*.astub | Library annotations for popular frameworks (Spring Security, logging libraries, Android) |
| docs/manual/confidential-checker.tex | Complete documentation chapter explaining the checker's purpose and usage |
| framework/tests/all-systems/*.java | Test files with @SuppressWarnings annotations for confidential checker warnings |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request introduces a new Confidential Checker for the Checker Framework. The change adds five qualifier annotations (Confidential, NonConfidential, UnknownConfidential, PolyConfidential, and BottomConfidential) to define a type qualifier hierarchy for tracking sensitive information. Core implementation includes ConfidentialAnnotatedTypeFactory, ConfidentialChecker, ConfidentialTransfer, and ConfidentialVisitor classes. The pull request includes stub files annotating external APIs from Android, Spring Security, SLF4J, Log4j, Apache Commons Logging, and other libraries. Test infrastructure, documentation files, and test cases are also provided. Existing all-systems test files receive suppression annotations for confidential-related warnings. 🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@checker/src/main/java/org/checkerframework/checker/confidential/AlertDialog.astub`:
- Line 50: Annotate the resource-typed parameters to match Android SDK: add
`@StyleRes` to the Builder constructor parameter named themeResId in the
Builder(`@UnknownConfidential` Context context, int themeResId) declaration, and
add `@LayoutRes` to the parameter named layoutResId (the method/constructor that
takes layoutResId at the later declaration around line 120); ensure you import
or qualify the `@StyleRes` and `@LayoutRes` annotations consistent with other stubs
so types compile.
In
`@checker/src/main/java/org/checkerframework/checker/confidential/ApacheLog.astub`:
- Around line 5-18: The Log interface is missing single-argument logging
overloads and level-guard methods; add void debug(Object var1), void
error(Object var1), void fatal(Object var1), void info(Object var1), void
trace(Object var1), and void warn(Object var1) alongside boolean
isDebugEnabled(), isErrorEnabled(), isFatalEnabled(), isInfoEnabled(),
isTraceEnabled(), and isWarnEnabled(); keep the existing multi-parameter
signatures (e.g., debug(Object, `@UnknownConfidential` Throwable)) unchanged and
mirror their nullability/annotation conventions for the single-parameter
versions where appropriate so type-checking in callers using Log.debug(msg) and
guard checks compiles.
In
`@checker/src/main/java/org/checkerframework/checker/confidential/Authentication.astub`:
- Around line 1-21: The stub is missing an import for GrantedAuthority which
breaks resolution of the return type for Authentication.getAuthorities(); add an
import for org.springframework.security.core.GrantedAuthority at the top of the
Authentication interface stub (the file containing interface Authentication and
method getAuthorities()) so the Collection<? extends GrantedAuthority> type
resolves correctly.
In
`@checker/src/main/java/org/checkerframework/checker/confidential/Claims.astub`:
- Around line 6-35: The Claims interface methods currently lack consistent
`@PolyConfidential` annotations causing confidentiality downgrades; update the
interface so getters have a `@PolyConfidential` receiver (e.g., add
"@PolyConfidential Claims this" to getIssuer, getSubject, getAudience,
getExpiration, getNotBefore, getIssuedAt, getId) and ensure return types and
setter parameter types are annotated as `@PolyConfidential` where they should
reflect the polymorphic confidentiality (e.g., return types for getters and
parameter types for setters like setIssuer, setSubject, setAudience, setId, and
the Map generic parameters already present), and also annotate the generic
get(String, Class<T>) appropriately to propagate `@PolyConfidential` from the
Claims receiver.
In
`@checker/src/main/java/org/checkerframework/checker/confidential/ConfidentialAnnotatedTypeFactory.java`:
- Around line 128-138: The visitMethodInvocation handling for objectToString
should defensively handle a null receiver from getReceiverType(tree) before
calling hasPrimaryAnnotation; update visitMethodInvocation so it first assigns
AnnotatedTypeMirror receiver = getReceiverType(tree) and checks receiver !=
null, then if receiver != null && receiver.hasPrimaryAnnotation(NONCONFIDENTIAL)
set type.replaceAnnotation(NONCONFIDENTIAL) else set
type.replaceAnnotation(CONFIDENTIAL). This prevents a NullPointerException while
preserving the conservative CONFIDENTIAL default when the receiver is absent.
In
`@checker/src/main/java/org/checkerframework/checker/confidential/ConfidentialTransfer.java`:
- Around line 111-114: The getValueAnnotation(CFValue cfValue) method can NPE
when cfValue is null (getValueOfSubNode may return null); add a null check at
the start of getValueAnnotation that returns null (or a sensible default) if
cfValue is null, otherwise call
qualHierarchy.findAnnotationInHierarchy(cfValue.getAnnotations(),
atypeFactory.UNKNOWN_CONFIDENTIAL); this prevents cfValue.getAnnotations() from
being invoked on a null reference.
In
`@checker/src/main/java/org/checkerframework/checker/confidential/ExpiringMap.astub`:
- Around line 3-10: The file references TimeUnit in the ExpiringMap.put method
but lacks its import; add the import for java.util.concurrent.TimeUnit to the
top of the file so the signature public V put(`@PolyConfidential` K key,
`@PolyConfidential` V value, long duration, TimeUnit timeUnit); in class
ExpiringMap resolves correctly.
In
`@checker/src/main/java/org/checkerframework/checker/confidential/HttpServletResponse.astub`:
- Around line 1-18: The HttpServletResponse stub declares package
jakarta.servlet.http but extends the wrong ServletResponse interface from
javax.servlet; update the import to use jakarta.servlet.ServletResponse so the
interface HttpServletResponse extends the Jakarta namespace parent (modify the
import of ServletResponse and any references to javax.servlet.ServletResponse to
jakarta.servlet.ServletResponse in the HttpServletResponse interface
declaration), ensuring the unique symbol HttpServletResponse now extends
jakarta.servlet.ServletResponse.
In
`@checker/src/main/java/org/checkerframework/checker/confidential/Log4jLogger.astub`:
- Around line 33-35: The Log4jLogger.astub methods (e.g., error, warn, info,
debug, trace, fatal, log, traceEntry) currently annotate payload parameters like
Object, Supplier, MessageSupplier, and Throwable with `@UnknownConfidential` which
permits confidential data into logs; remove the `@UnknownConfidential` annotation
from all such payload parameters so they default to `@NonConfidential` (i.e., no
annotation) across every log-level family and helper methods referenced (lines
noted in the review) to enforce sensitive sinks; update every overloaded
signature (including varargs and Supplier/MessageSupplier variants) in
Log4jLogger.astub to use unannotated payload parameter types.
In
`@checker/src/main/java/org/checkerframework/checker/confidential/UsernamePasswordAuthenticationToken.astub`:
- Line 5: The file contains an unused import org.springframework.util.Assert in
the UsernamePasswordAuthenticationToken.astub stub; remove that import line so
the stub no longer imports Assert (locate the import statement for
org.springframework.util.Assert in the UsernamePasswordAuthenticationToken.astub
file and delete it).
In `@docs/CHANGELOG.md`:
- Around line 260-262: Remove the duplicated sentence describing the
SqlQuotesChecker from the changelog entry so only a single description for
SqlQuotesChecker remains; locate the repeated sentence that exactly repeats the
prior line in the CHANGELOG entry for "SqlQuotesChecker" and delete the
redundant copy, leaving the original sentence intact.
- Around line 6-7: The changelog entry "New Confidential Checker to identify
sensitive information exposure." is outside any release section; move this
sentence under a proper release heading (e.g., add or use an "## [Unreleased]"
or the appropriate version heading) so it appears as a bullet or sentence within
that section and matches surrounding entries' format; ensure the heading syntax
(## or ###) and list/bullet style match existing changelog conventions and that
there are no duplicate standalone lines left outside sections.
In `@docs/manual/confidential-checker.tex`:
- Around line 110-113: Fix the subject–verb agreement in the concatenation rule
sentence that references \code{`@Confidential`} and \code{`@NonConfidential`}:
change the plural verb phrase "are most narrowly typed as follows" to the
singular "is most narrowly typed as follows" so the sentence reads correctly for
the singular subject "Concatenation of \code{`@Confidential`} and
\code{`@NonConfidential`} values"; update the sentence in the paragraph that
introduces the concatenation typing rule (the line containing "Concatenation of
\code{`@Confidential`} and \code{`@NonConfidential`} values results...")
accordingly.
In `@docs/manual/hevea.sty`:
- Around line 37-41: The fallback aliases for \url, \oneurl and \footurl
incorrectly use \let to point a 1-arg interface to 2-arg macros (\ahref,
\ahrefurl, \footahref); replace those lets with 1-arg wrapper definitions so the
single-argument behavior is retained. Concretely, instead of \let\url\ahref (and
similar), define \url#1{\ahref{`#1`}{`#1`}}, \oneurl#1{\ahrefurl{`#1`}{`#1`}} and
\footurl#1{\footahref{`#1`}{`#1`}} so the visible text equals the URL when the
url/hyperref package is absent while still delegating to the existing
\ahref/\ahrefurl/\footahref implementations.
- Around line 73-79: hevea.sty declares a bgcolor environment that uses colorbox
but does not declare the color package dependency; to make the style
self-contained, add a RequirePackage{xcolor} immediately after the existing
RequirePackage{comment} declaration so the bgcolor environment and \colorbox
have the needed package available when hevea.sty is loaded in other documents.
- Around line 16-17: The \@ifundefined call currently has only two branches
which causes \makeatletter to be consumed as the false branch; update the call
that defines ifimagen (the \@ifundefined{\ifimagen}{\newif\ifimagen\imagenfalse}
invocation) to include an explicit empty third argument for the false branch
(i.e., add {} as the third parameter) so \makeatletter is not accidentally taken
as the false branch and the conditional behaves correctly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/manual/figures/confidential.svgis excluded by!**/*.svg
📒 Files selected for processing (39)
checker-qual/src/main/java/org/checkerframework/checker/confidential/qual/BottomConfidential.javachecker-qual/src/main/java/org/checkerframework/checker/confidential/qual/Confidential.javachecker-qual/src/main/java/org/checkerframework/checker/confidential/qual/NonConfidential.javachecker-qual/src/main/java/org/checkerframework/checker/confidential/qual/PolyConfidential.javachecker-qual/src/main/java/org/checkerframework/checker/confidential/qual/UnknownConfidential.javachecker/src/main/java/org/checkerframework/checker/confidential/AbstractAuthenticationTargetUrlRequestHandler.astubchecker/src/main/java/org/checkerframework/checker/confidential/AlertDialog.astubchecker/src/main/java/org/checkerframework/checker/confidential/AndroidLog.astubchecker/src/main/java/org/checkerframework/checker/confidential/ApacheLog.astubchecker/src/main/java/org/checkerframework/checker/confidential/Authentication.astubchecker/src/main/java/org/checkerframework/checker/confidential/Claims.astubchecker/src/main/java/org/checkerframework/checker/confidential/ConfidentialAnnotatedTypeFactory.javachecker/src/main/java/org/checkerframework/checker/confidential/ConfidentialChecker.javachecker/src/main/java/org/checkerframework/checker/confidential/ConfidentialTransfer.javachecker/src/main/java/org/checkerframework/checker/confidential/ConfidentialVisitor.javachecker/src/main/java/org/checkerframework/checker/confidential/Cookie.astubchecker/src/main/java/org/checkerframework/checker/confidential/ExpiringMap.astubchecker/src/main/java/org/checkerframework/checker/confidential/HttpServletResponse.astubchecker/src/main/java/org/checkerframework/checker/confidential/JwtParser.astubchecker/src/main/java/org/checkerframework/checker/confidential/Log4jLogger.astubchecker/src/main/java/org/checkerframework/checker/confidential/PasswordEncoder.astubchecker/src/main/java/org/checkerframework/checker/confidential/Slf4jLogger.astubchecker/src/main/java/org/checkerframework/checker/confidential/UserDetails.astubchecker/src/main/java/org/checkerframework/checker/confidential/UsernamePasswordAuthenticationToken.astubchecker/src/test/java/org/checkerframework/checker/test/junit/ConfidentialTest.javachecker/tests/confidential/ConfidentialConcatenation.javachecker/tests/confidential/ConfidentialToString.javachecker/tests/confidential/SimpleConfidential.javadocs/CHANGELOG.mddocs/manual/advanced-features.texdocs/manual/confidential-checker.texdocs/manual/hevea.stydocs/manual/manual.texframework/tests/all-systems/Issue577.javaframework/tests/all-systems/Issue6282.javaframework/tests/all-systems/MultipleUnions.javaframework/tests/all-systems/UnionCrash.javaframework/tests/all-systems/UnionTypes.javaframework/tests/all-systems/Unions.java
checker/src/main/java/org/checkerframework/checker/confidential/AlertDialog.astub
Show resolved
Hide resolved
checker/src/main/java/org/checkerframework/checker/confidential/ApacheLog.astub
Show resolved
Hide resolved
checker/src/main/java/org/checkerframework/checker/confidential/Authentication.astub
Show resolved
Hide resolved
checker/src/main/java/org/checkerframework/checker/confidential/Claims.astub
Show resolved
Hide resolved
...rc/main/java/org/checkerframework/checker/confidential/ConfidentialAnnotatedTypeFactory.java
Show resolved
Hide resolved
|
Hi @mernst, I worked on a sensitivity/confidential data checker internally last year at my workplace using the Checker Framework, mainly focused on preventing sensitive data from being logged or exposed through APIs, and I would love to contribute to this Confidential Checker PR. I noticed there hasn’t been activity for a few weeks, and I would love to help move this forward. I can help with implementation, tests, or edge cases based on what I learned while building our internal checker. Would you be open to collaboration on this? I’m happy to help with implementation, tests, or documentation wherever needed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@checker/tests/confidential/ConfidentialConcatenation.java`:
- Line 1: The comment "// NonConfidential <: Confidential" is misleading; remove
or replace it with an accurate note that both `@NonConfidential` and `@Confidential`
are siblings annotated with `@SubtypeOf`(UnknownConfidential.class) and therefore
do not have a direct subtype relationship — only `@BottomConfidential` is a
subtype of both; update or delete the comment accordingly and reference the
qualifier annotations (NonConfidential, Confidential, UnknownConfidential,
BottomConfidential) when editing.
In `@checker/tests/confidential/SimpleConfidential.java`:
- Around line 10-13: The call in nonConfidentialRef passes a `@NonConfidential`
String to executeConfidential(`@Confidential` String) and is missing the
expected-error annotation; update the test by adding the same expected error
marker used elsewhere (e.g., "// :: error: [argument]") on the
executeConfidential(s) invocation so the checker expects the incorrect
assignment between `@NonConfidential` and `@Confidential` in nonConfidentialRef.
In
`@framework/src/main/java/org/checkerframework/common/value/ValueAnnotatedTypeFactory.java`:
- Around line 311-321: The canonicalAnnotation method incorrectly calls
super.canonicalAnnotation(anno) before handling `@MinLen`, which causes the
superclass aliasing (addAliasedTypeAnnotation(MinLen.class, BOTTOMVAL)) to turn
MinLen into BottomVal and skip your special handling; fix this by checking
AnnotationUtils.areSameByName(anno, MINLEN_NAME) and, if true, computing int
from = getMinLenValue(anno) and returning createArrayLenRangeAnnotation(from,
Integer.MAX_VALUE) before invoking super.canonicalAnnotation(anno) (or return
that result immediately) so MinLen is converted to ArrayLenRange as intended.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0264e7fa-e691-4c35-ba0f-fa0c313ce86a
📒 Files selected for processing (4)
checker/tests/confidential/ConfidentialConcatenation.javachecker/tests/confidential/ConfidentialToString.javachecker/tests/confidential/SimpleConfidential.javaframework/src/main/java/org/checkerframework/common/value/ValueAnnotatedTypeFactory.java
framework/src/main/java/org/checkerframework/common/value/ValueAnnotatedTypeFactory.java
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
checker/tests/confidential/SimpleConfidential.java (1)
10-14:⚠️ Potential issue | 🟠 MajorFix expected-error placement in
nonConfidentialRef.Line 11 marks the wrong call as an error.
executeNonConfidential(s)should be valid, while Line 13 (executeConfidential(s)) is the one that needs// :: error: [argument].Proposed fix
void nonConfidentialRef(`@NonConfidential` String s) { - // :: error: [argument] executeNonConfidential(s); + // :: error: [argument] executeConfidential(s); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/tests/confidential/SimpleConfidential.java` around lines 10 - 14, The error annotation in nonConfidentialRef is on the wrong call; move the `// :: error: [argument]` comment from the `executeNonConfidential(s)` invocation to the `executeConfidential(s)` invocation so that `executeNonConfidential(s)` is allowed and `executeConfidential(s)` is flagged; update the comment placement in the method `nonConfidentialRef` referencing the calls to `executeNonConfidential` and `executeConfidential`.checker/src/main/java/org/checkerframework/checker/confidential/Claims.astub (1)
7-35:⚠️ Potential issue | 🔴 CriticalPropagate
@PolyConfidentialconsistently across Claims accessors/mutators.On Line 7 and Lines 9-35, unannotated returns/params default to
@NonConfidential, which can downgrade values flowing through a@PolyConfidential Claimsreceiver.Proposed fix
public interface Claims extends Map<@PolyConfidential String, `@PolyConfidential` Object>, ClaimsMutator<Claims> { - String getIssuer(); + `@PolyConfidential` String getIssuer(`@PolyConfidential` Claims this); - Claims setIssuer(String var1); + `@PolyConfidential` Claims setIssuer(`@PolyConfidential` String var1); `@PolyConfidential` String getSubject(`@PolyConfidential` Claims this); - Claims setSubject(String var1); + `@PolyConfidential` Claims setSubject(`@PolyConfidential` String var1); `@PolyConfidential` String getAudience(`@PolyConfidential` Claims this); - Claims setAudience(String var1); + `@PolyConfidential` Claims setAudience(`@PolyConfidential` String var1); `@PolyConfidential` Date getExpiration(`@PolyConfidential` Claims this); - Claims setExpiration(Date var1); + `@PolyConfidential` Claims setExpiration(`@PolyConfidential` Date var1); `@PolyConfidential` Date getNotBefore(`@PolyConfidential` Claims this); - Claims setNotBefore(Date var1); + `@PolyConfidential` Claims setNotBefore(`@PolyConfidential` Date var1); `@PolyConfidential` Date getIssuedAt(`@PolyConfidential` Claims this); - Claims setIssuedAt(Date var1); + `@PolyConfidential` Claims setIssuedAt(`@PolyConfidential` Date var1); `@PolyConfidential` String getId(`@PolyConfidential` Claims this); - Claims setId(String var1); + `@PolyConfidential` Claims setId(`@PolyConfidential` String var1); - <T> T get(String var1, Class<T> var2); + <T> `@PolyConfidential` T get(`@PolyConfidential` Claims this, String var1, Class<T> var2); }What are the exact method signatures in the current io.jsonwebtoken.Claims API for getIssuer, setIssuer, getSubject, setSubject, getAudience, setAudience, getExpiration, setExpiration, getNotBefore, setNotBefore, getIssuedAt, setIssuedAt, getId, setId, and <T> get(String, Class<T>)?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/confidential/Claims.astub` around lines 7 - 35, The Claims accessor/mutator signatures must propagate `@PolyConfidential` when called on a `@PolyConfidential` receiver: update getIssuer, setIssuer, getSubject, setSubject, getAudience, setAudience, getExpiration, setExpiration, getNotBefore, setNotBefore, getIssuedAt, setIssuedAt, getId, setId, and the generic <T> get(String, Class<T>) to include `@PolyConfidential` on the receiver and on returned values and/or parameters as appropriate (e.g., annotate return types like `@PolyConfidential` String or `@PolyConfidential` Date for getters, annotate setter parameters or receiver with `@PolyConfidential` where the flow should be preserved, and annotate the receiver for the generic get(String, Class<T>) to ensure polymorphic confidentiality propagation).docs/CHANGELOG.md (1)
6-7:⚠️ Potential issue | 🟡 MinorMove this entry under a release heading.
Line 6 is still a standalone entry outside any version/unreleased section, so it won’t be grouped with release notes.
Proposed fix
+## Unreleased + +### User-visible changes + -New Confidential Checker to identify sensitive information exposure. +New Confidential Checker to identify sensitive information exposure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/CHANGELOG.md` around lines 6 - 7, The standalone changelog line "New Confidential Checker to identify sensitive information exposure." must be moved under an existing release heading (for example the "## Unreleased" or a specific "## [vX.Y.Z] - YYYY-MM-DD" section) so it is grouped with other release notes; open CHANGELOG.md, cut that line and paste it beneath the appropriate heading (or create an "Unreleased" heading if none exists), keeping the same wording and formatting as other entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@checker/src/main/java/org/checkerframework/checker/confidential/Claims.astub`:
- Around line 7-35: The Claims accessor/mutator signatures must propagate
`@PolyConfidential` when called on a `@PolyConfidential` receiver: update getIssuer,
setIssuer, getSubject, setSubject, getAudience, setAudience, getExpiration,
setExpiration, getNotBefore, setNotBefore, getIssuedAt, setIssuedAt, getId,
setId, and the generic <T> get(String, Class<T>) to include `@PolyConfidential` on
the receiver and on returned values and/or parameters as appropriate (e.g.,
annotate return types like `@PolyConfidential` String or `@PolyConfidential` Date
for getters, annotate setter parameters or receiver with `@PolyConfidential` where
the flow should be preserved, and annotate the receiver for the generic
get(String, Class<T>) to ensure polymorphic confidentiality propagation).
In `@checker/tests/confidential/SimpleConfidential.java`:
- Around line 10-14: The error annotation in nonConfidentialRef is on the wrong
call; move the `// :: error: [argument]` comment from the
`executeNonConfidential(s)` invocation to the `executeConfidential(s)`
invocation so that `executeNonConfidential(s)` is allowed and
`executeConfidential(s)` is flagged; update the comment placement in the method
`nonConfidentialRef` referencing the calls to `executeNonConfidential` and
`executeConfidential`.
In `@docs/CHANGELOG.md`:
- Around line 6-7: The standalone changelog line "New Confidential Checker to
identify sensitive information exposure." must be moved under an existing
release heading (for example the "## Unreleased" or a specific "## [vX.Y.Z] -
YYYY-MM-DD" section) so it is grouped with other release notes; open
CHANGELOG.md, cut that line and paste it beneath the appropriate heading (or
create an "Unreleased" heading if none exists), keeping the same wording and
formatting as other entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d7761d62-c8e8-4ef2-8be5-3c8546a04c7b
📒 Files selected for processing (9)
checker/src/main/java/org/checkerframework/checker/confidential/Authentication.astubchecker/src/main/java/org/checkerframework/checker/confidential/Claims.astubchecker/src/main/java/org/checkerframework/checker/confidential/ExpiringMap.astubchecker/src/main/java/org/checkerframework/checker/confidential/UsernamePasswordAuthenticationToken.astubchecker/tests/confidential/ConfidentialConcatenation.javachecker/tests/confidential/SimpleConfidential.javadocs/CHANGELOG.mddocs/manual/confidential-checker.texdocs/manual/sql-quotes-checker.tex
|
I'm unsure if this is intended design but I just wanted to clarify. Currently the Confidential checker disallows calling toString() on Consider below test case in This is failing because any method invocation on confidential object is not allowed instead of assignment check. This case should pass (but it is failing because of the same above issue) Also, methods like trim(), substring(), concat(), replace(), etc., should likely return a value with the same confidentiality as the receiver (polymorphic qualifier). On the other hand, methods like length(), isEmpty(), and hashCode() do not reveal the underlying content and could reasonably be treated as It might make sense for the Confidential Checker to provide a JDK stub file. I’d be happy to help contribute if this aligns with the intended design of the Confidential Checker. |
Member
|
@Abhishek1152 I agree with your analysis, and we would gratefully accept a pull request to fix the issue that you pointed out. Thank you! |
|
Hi @mernst, I have raised PR for checker-framework and jdk against the author's commit. Please take a look and let me know if it aligns with you. A quick query, should we consider adding checks around hardcoded string literals? This can potentially prevent leaks in VCS. Eg. |
|
Hi @mernst, just a gentle follow-up on this. I’ve submitted PRs to address the issue we discussed and added tests to verify propagation. Could you please take a look when you have some time? I’d be happy to make any changes if needed. Also, please let me know if you would prefer this work to be submitted as a PR directly against the typetools:master instead of on top of the existing PR. Along with that, is there a reason why we have a diamond hierarchy for Confidential and decided to manually override in commonAssignmentCheck. Instead of linear hierarchy Thank you |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Associated with JDK pull request #224