Skip to content

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

Merged
san-zrl merged 4 commits into
cbomkit:mainfrom
sachin9058:investigate/detection-location-339
Jun 1, 2026
Merged

test(java): verify occurrence locations for issue #339#430
san-zrl merged 4 commits into
cbomkit:mainfrom
sachin9058:investigate/detection-location-339

Conversation

@sachin9058

@sachin9058 sachin9058 commented May 23, 2026

Copy link
Copy Markdown
Contributor

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

Copilot AI left a comment

Copy link
Copy Markdown

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

san-zrl commented May 26, 2026

Copy link
Copy Markdown
Contributor

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

san-zrl commented May 30, 2026

Copy link
Copy Markdown
Contributor

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
@san-zrl

san-zrl commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Thanks @sachin9058 for adding the the line number test to PreciseLocationTest! This is indeed helpful as it demonstrates that the detection part produces correct line numbers. When analysing a CBOM issue #339 still persists which means that problem must be somewhere else.

The GuavaHashingTest and the corresponding test file do not help to gain additional insight. Please remove them and I'll accept the PR.

Signed-off-by: Sachin Kumar <sachinkumar905846@gmail.com>
@sachin9058 sachin9058 force-pushed the investigate/detection-location-339 branch from 2ec5eb1 to b93fc46 Compare June 1, 2026 08:42
@sachin9058

Copy link
Copy Markdown
Contributor Author

@san-zrl Thanks for the review.

I've removed GuavaHashingTest and the corresponding fixture file as requested. The PR now focuses on the occurrence line-number assertions added to PreciseIssueLocationTest.

CI is green and the updated test continues to verify that the detection layer produces the expected DetectionContext locations.

@san-zrl san-zrl 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.

Hi @sachin9058 - Thanks a lot of contributing to this project.

@san-zrl san-zrl merged commit 546058e into cbomkit:main Jun 1, 2026
2 checks passed
@sachin9058 sachin9058 deleted the investigate/detection-location-339 branch June 1, 2026 09:09
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