Skip to content

test(java): verify occurrence locations for issue #339#430

Open
sachin9058 wants to merge 3 commits into
cbomkit:mainfrom
sachin9058:investigate/detection-location-339
Open

test(java): verify occurrence locations for issue #339#430
sachin9058 wants to merge 3 commits into
cbomkit:mainfrom
sachin9058:investigate/detection-location-339

Conversation

@sachin9058
Copy link
Copy Markdown
Contributor

@sachin9058 sachin9058 commented May 23, 2026

Description

Adds regression coverage for issue #339 by verifying occurrence locations, not only that detections are produced.

The tests reproduce the Guava Hashing.java pattern that originally triggered the report: HMAC methods separated by multi-line Javadoc blocks.

Changes

  • Added explicit assertions on DetectionContext.lineNumber() in PreciseIssueLocationTest
  • Added GuavaHashingTest
  • Added GuavaHashingTestFile
  • Verified occurrence locations resolve to the crypto call sites rather than adjacent Javadoc boundaries

Verification

Executed locally:

mvn -pl java test -Dtest=PreciseIssueLocationTest,GuavaHashingTest

Result:

Tests run: 2
Failures: 0
Errors: 0
Skipped: 0

BUILD SUCCESS

The tests assert occurrence line numbers directly via:

((IAsset) node).getDetectionContext().lineNumber()

which is the same location object propagated into the generated CBOM occurrence.

Signed-off-by: Sachin Kumar <sachinkumar905846@gmail.com>
@sachin9058 sachin9058 requested a review from a team as a code owner May 23, 2026 23:21
Copilot AI review requested due to automatic review settings May 23, 2026 23:21
Copy link
Copy Markdown

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

Note

Copilot was unable to run its full agentic suite in this review.

Adds a regression test to ensure findings are reported on the exact call-site lines (not shifted to adjacent Javadoc comment boundaries), addressing issue #339.

Changes:

  • Introduces a new JUnit test that verifies reported issue locations using CheckVerifier.
  • Adds a dedicated Java test fixture file reproducing the “methods separated by multi-line Javadoc” pattern.
  • Validates detected algorithm/context values for multiple findings (HmacMD5, HmacSHA256).

Reviewed changes

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

File Description
java/src/test/java/com/ibm/plugin/rules/issues/PreciseIssueLocationTest.java Adds regression test + assertions for detected contexts/algorithms to validate precise issue locations.
java/src/test/files/rules/issues/PreciseIssueLocationTestFile.java Adds fixture code with Javadoc-separated methods and // Noncompliant markers to reproduce the location-shift bug.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread java/src/test/java/com/ibm/plugin/rules/issues/PreciseIssueLocationTest.java Outdated
Comment thread java/src/test/files/rules/issues/PreciseIssueLocationTestFile.java
Comment thread java/src/test/java/com/ibm/plugin/rules/issues/PreciseIssueLocationTest.java Outdated
@sachin9058
Copy link
Copy Markdown
Contributor Author

@san-zrl @n1ckl0sk0rtge
After investigating further, it looks like the runtime fix for #339 may already exist in previous commits. This PR focuses on adding regression coverage reproducing the original Javadoc-adjacent pattern and ensuring location reporting remains stable going forward.

Signed-off-by: Sachin Kumar <sachinkumar905846@gmail.com>
@san-zrl
Copy link
Copy Markdown
Contributor

san-zrl commented May 26, 2026

Hi @sachin9058 - This PR only confirms what we already know: We detect the correct crypto calls. Your code does not assert that the detected locations correspond to the real locations of the crypto calls in the code. #339 is not solved yet.

Reproducing this is cumbersome for you. You have to clone the latest versions of the main branches of sonar-cryptography, cbomkit-lib and cbomkit, compile everything locally making sure that cbomkit-lib and cbomkit use the local packages as dependencies and run cbonkit on the purl pkg:maven/com.google.guava/guava@33.0.0-jre mentioned in #339.

Fell free to explore this further. We are still interested in an explanation and a fix for the problem. Closing this for the moment.

@san-zrl san-zrl closed this May 26, 2026
@sachin9058
Copy link
Copy Markdown
Contributor Author

@san-zrl I reproduced issue #339 locally using CBOMkit against:

pkg:maven/com.google.guava/guava@33.0.0-jre

and validated the generated CBOM output through:

/api/v1/cbom/last/10

The resulting occurrences now point to the actual crypto call locations inside:

guava/src/com/google/common/hash/Hashing.java

Examples from generated CBOM:

  • MD5 → line 330
  • SHA1 → line 356
  • SHA256 → line 382
  • SHA512 → line 408

instead of pointing to the trailing Javadoc closing (*/) lines described in #339.

This confirms the refined token selection logic correctly propagates into final exported CBOM evidence locations.

@san-zrl san-zrl reopened this May 29, 2026
@san-zrl san-zrl self-assigned this May 29, 2026
@sachin9058
Copy link
Copy Markdown
Contributor Author

@san-zrl Thanks for taking another look.
After reproducing the full CBOMkit pipeline locally and validating the generated CBOM output against the Guava example from #339, I wanted to share the results in case they are useful for further investigation. Happy to help test any additional scenarios if needed.

@san-zrl
Copy link
Copy Markdown
Contributor

san-zrl commented May 30, 2026

Hi @sachin9058 -

Your PR is titled "add regression coverage for detection location". This is not quite was it does.

The name of the additional test java/src/test/java/com/ibm/plugin/rules/issues/PreciseIssueLocationTest.java. suggests that it checks the location of a detection. The test actually only checks THAT a particular detection is made (e.g., algorithm name is "HmacMD5") but not WHERE this detection is made. For the location to be tested we would need assertion on the line number of the occurrence object.

Moreover, the issue still persists. See my recent comment in #339.

Signed-off-by: Sachin Kumar <sachinkumar905846@gmail.com>
@sachin9058
Copy link
Copy Markdown
Contributor Author

@san-zrl I've updated the investigation with explicit location assertions.

Changes:

  • Added occurrence line-number assertions to PreciseIssueLocationTest.
  • Added GuavaHashingTest and GuavaHashingTestFile, which mirror the Guava Hashing.java structure that originally reproduced the issue (HMAC methods separated by multi-line Javadocs).

The tests now verify the translated occurrence location directly via DetectionContext.lineNumber() rather than only checking that detections exist.

Verification:

mvn -pl java test -Dtest=PreciseIssueLocationTest,GuavaHashingTest

Result:

Tests run: 2, Failures: 0, Errors: 0
BUILD SUCCESS

The assertions verify that the occurrence locations resolve to the crypto call sites rather than the surrounding Javadoc structure.

@sachin9058 sachin9058 changed the title test(java): add regression coverage for detection location (#339) test(java): verify occurrence locations for issue #339 May 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants