Skip to content

Conversation

@miguelappleton
Copy link
Contributor

@miguelappleton miguelappleton commented Jun 17, 2025

My main changes involve 3 different fixes for these insecure uses of readObject:

  • when the object was a String, I replaced writeObject and readObject with writeUTF and readUTF;

  • when the object was a custom type, I added it to an ObjectInputFilter, which is passed into the ObjectInputStream, to only allow those types:

    • I had to create a custom filter class because ObjectInputStream does some stream header reading on its constructor, and you can only set an ObjectInputFilter when the stream has not read anything, so I had to do it via this custom class, as the filter could not be set after class instantiation;
  • for extraCacheKey I could not add it to the filter as is, because the object was a Serializable, and if I allow the ObjectInputStream to accept Serializable objects, the danger of injection attacks still remains. However I saw that extraCacheKey was just a CacheKey object that was being unnecessarily converted (or "extended") to its parent type Serializable, so I just made it "remain" as a CacheKey through the whole process and then filtered that custom type, similarly to the ones on the above step.

I also addressed (in a separate commit) some of the SonarQube changes suggested for the files I altered (and some other files that I had altered but ended up not altering), but I wasn't super thorough and basically only did the changes which I believed would not cause problems elsewhere.

@miguelappleton miguelappleton force-pushed the PPP-5720 branch 2 times, most recently from c57e378 to 5875d3f Compare June 17, 2025 11:21
@miguelappleton
Copy link
Contributor Author

miguelappleton commented Jun 17, 2025

From running a test workflow without these changes, I can see that 19 of the 24 failed tests are failing regardless of the changes in this PR.

The ones that are currently failing due to this PR are the following:

  • pt.webdetails.cda.filetests.CompoundQueryTest.testCompoundUnion: filter can not be set after an object has been read;
  • pt.webdetails.cda.filetests.SqlTest.testSqlQueryCache: filter can not be set after an object has been read;
  • pt.webdetails.cda.filetests.SqlTest.testStringArrayParameter: filter can not be set after an object has been read;
  • pt.webdetails.cda.filetests.SqlTest.testFormulaCacheSql: filter can not be set after an object has been read;
  • pt.webdetails.cda.utils.ParameterTest.testParamNumericArraySerialization: filter status: REJECTED.

(Which makes sense because they relate to the addition of the ObjectInputFilter)

Currently working on addressing these.

@buildguy

This comment has been minimized.

@hitachivantarasonarqube

This comment has been minimized.

@buildguy

This comment has been minimized.

@miguelappleton miguelappleton force-pushed the PPP-5720 branch 2 times, most recently from 6ed11ac to 6a3b74d Compare June 24, 2025 17:39
@hitachivantarasonarqube

This comment has been minimized.

@buildguy

This comment has been minimized.

@hitachivantarasonarqube

This comment has been minimized.

@buildguy

This comment has been minimized.

@hitachivantarasonarqube

This comment has been minimized.

@buildguy

This comment has been minimized.

@hitachivantarasonarqube

This comment has been minimized.

@buildguy

This comment has been minimized.

@hitachivantarasonarqube

This comment has been minimized.

@buildguy

This comment has been minimized.

@hitachivantarasonarqube

This comment has been minimized.

@buildguy

This comment has been minimized.

Copy link

@befc befc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ✅

@hitachivantarasonarqube

This comment has been minimized.

@buildguy

This comment has been minimized.

@hitachivantarasonarqube
Copy link

Failed

  • 67.80% Coverage on New Code (is less than 80.00%)

Analysis Details

0 Issues

  • Bug 0 Bugs
  • Vulnerability 0 Vulnerabilities
  • Code Smell 0 Code Smells

Coverage and Duplications

  • Coverage 67.80% Coverage (47.40% Estimated after merge)
  • Duplications 0.00% Duplicated Code (2.10% Estimated after merge)

Project ID: pentaho:cda-plugin

View in SonarQube

@buildguy
Copy link

👍 Frogbot scanned this pull request and did not find any new security issues.

Note:

Frogbot also supports Contextual Analysis, Secret Detection, IaC and SAST Vulnerabilities Scanning. This features are included as part of the JFrog Advanced Security package, which isn't enabled on your system.


@buildguy
Copy link

✅ Build finished in 16m 11s

Build command:

mvn clean verify -B -e -Daudit -Djs.no.sandbox

👌 All tests passed!

Tests run: 240, Failures: 0, Skipped: 0    Test Results


ℹ️ This is an automatic message

@webdetails webdetails deleted a comment from hitachivantarasonarqube bot Jun 27, 2025
@webdetails webdetails deleted a comment from buildguy Jun 27, 2025
@webdetails webdetails deleted a comment from buildguy Jun 27, 2025
@webdetails webdetails deleted a comment from hitachivantarasonarqube bot Jun 27, 2025
@webdetails webdetails deleted a comment from hitachivantarasonarqube bot Jun 27, 2025
@webdetails webdetails deleted a comment from buildguy Jun 27, 2025
@webdetails webdetails deleted a comment from hitachivantarasonarqube bot Jun 27, 2025
@miguelappleton miguelappleton merged commit 2b05622 into webdetails:master Jun 27, 2025
1 of 2 checks passed
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