Sec38 div issues #8000
Sec38 div issues #8000
Conversation
The update to Passay 1.6.0 required adapting to the change Java API. IQSS/dataverse-security#38
The password validator test spit out an error. To identify the failing example, the test was ported to JUnit5 parameterized test. The test example has been changed, as the library detects another error. The error message has been validated and found correct. IQSS/dataverse-security#38
This fixes an potential security issue due to an outdated dependency (commons-beanutils). The test needed to be fixed and revealed that formerly email addresses with trailing whitespace were accepted. The update solved this error, too. IQSS/dataverse-security#38
|
FWIW - I was doing some light testing yesterday with the 1.27 tika and haven't found any problems. |
The old version 3.1.1 had issues with updated deps using classes in version 55 (Java 11). See https://issues.apache.org/jira/browse/MDEP-613 (Although going for 3.1.2 would be sufficient, better go for recent version)
…rse-security#38 The XOM library had been changed to com.iom7.xom before, trying to achieve updated dependencies. It had been declared as a direct dependency, which it is not - the library is nowhere used in the Dataverse codebase. Instead, it is used by sword2-server and others. As the original authors of XOM added never versions, so this commit removes the direct dependency and adds a newer version to <dependencyManagement>. This has a sideeffect: now the xercesImpl library version is defined by Apache Tika (depending on it) instead of Xom (which is a sub-dependency, making the relying version there loose against Tika). This update of xercesImpl is necessary as the old version (the code used <=2.8.0) contains security issues fixed in 2.12.1, which Tika depends on. It does not seem necessary to add xercesImpl to <dependencyManagement> yet. That might change in the future to avoid drifting versions... See IQSS/dataverse-security#38
…lasspath for JUL logging
…e Commons HttpClient The future update of sword2-server will drop the dependency to commons-httpclient. We rely on that as a huge technical debt (see comment in pom.xml) and been using it ever since via transitive dependencies. Re-adding as a direct dependency to not break our codebase when updating sword2-server.
[pull] develop from IQSS:develop
Some other tests might rely on the BrandingUtil to actually do things. This refactoring ensures proper mocking and cleaning up after the tests ran, trying to ensure better atomicity. It enables easy setup and teardown of mocks to be used in other tests. It might be necessary to expose the underlying mocked objects for deeper testing. Left for later exercise if ever necessary. This refactoring also - makes a few tests parameterized to be easier to read, - enables much easier identification of failing test examples and - removes static ordering now unnecessary.
- This refactoring moves these tests from JUnit 4 to 5. - This tests setups mocking for BrandingUtil via BrandingUtilTest to be atomically usable. - Removed star import.
This will be used to write better unit tests for generated XML to not rely on perfect String comparison and fail because of negligible whitespace changes. Also move org.skyscreamer.jsonassert to bottom of deps, inline with other test deps.
- Use JUnit5 parameterized tests for examples to simplify test code - Create DOMs via JSoup and compare HTML with XMLUnit diff - Remove fixed ordering via proper atomization - Update to latest JSoup version
- This refactoring moves these tests from JUnit 4 to 5. - The code has been much simplified by moving common parts functions, static fields etc. Let people concentrate on writing tests and produce less noise. - Tried to use Jakarta JSON-B. Our DTO classes are not compatible as-is, thus sticking with Google Gson for now. - Test have seen a speedup of ~700ms. Might become even faster by caching the mapped JSON objects and handing out clones via commons-lang serialization. Left as exercise for future devs. - Also fixes some future XML serialization issues seen with the SWORD2 lib update.
|
Folks I'm on a 2 weeks vacation now. |
There was a problem hiding this comment.
I made a few comments but I don't see anything problematic w.r.t. the security updates, so I'd suggest moving ahead with this rather than waiting for any changes (since @poikilotherm is away) and making any response to the comments a new PR(s). I did suggest updating the duracloud version in the pom.xml but I can do that in a separate PR.
|
|
||
| init(); | ||
| final PasswordData passwordData = PasswordData.newInstance(password, String.valueOf(passwordModificationTime.getTime()), null); | ||
| // final PasswordData passwordData = PasswordData.newInstance(password, "username", null); | ||
| final PasswordData passwordData = new PasswordData(password); |
There was a problem hiding this comment.
passwordModificationTime is no longer used - did the change drop a time check that should be restored? Or should the param get dropped from the methods?
There was a problem hiding this comment.
This time check never had any real effect of checking a time - the time has been passed as a String value for the username property of PasswordData.newInstance(). Please see https://repo1.maven.org/maven2/org/passay/passay/1.1.0/passay-1.1.0-javadoc.jar (couldn't find a published browser readable version...)
|
|
||
| // then | ||
| String xml = XmlPrinter.prettyPrintXml(byteArrayOutputStream.toString(StandardCharsets.UTF_8)); | ||
| XmlAssert.assertThat(xml).isInvalid(); |
There was a problem hiding this comment.
We're testing that DDIExportUtil is still broken? Or am I misunderstanding something?
It looks like the earlier version of the test was disabled with an if(false) statement, but replacing that with something that verifies a bug seems very odd. I don't know the history here, but if this is still a valid use case, I'd suggest making this test work for correct behavior and then skipping it for now with a new issue to get someone to look at fixing DDIExporter.
There was a problem hiding this comment.
The test wasn't a test. It simply did nothing, as you pointed out.
I linked to #3648 from the commit message, too - this is a more complex problem.
As in good testing principles, a test should test for a certain behaviour. As the current behaviour is producing invalid XML, at least we will notice when this changes. If you folks would rather see the test disabled, that's just an annotation away. Whatever you prefer, I'm with you 😄
| XmlAssert.assertThat(xml).isInvalid(); | ||
| logger.severe("DDIExporterTest.testExportDataset() creates XML but it's invalid. Fixing in DDIExportUtil required."); | ||
| } | ||
|
|
There was a problem hiding this comment.
Why was the testCitation test dropped? Is that a mistake or is that test no longer valid/useful?
There was a problem hiding this comment.
Please see the full commit message of 67a2f50
Shortly after, more and extensive testing had been added by @pkiraly in a distinct place.
| <commons.logging.version>1.2</commons.logging.version> | ||
| <commons.lang3.version>3.12.0</commons.lang3.version> | ||
| <httpcomponents.client.version>4.5.13</httpcomponents.client.version> | ||
| <apache.httpcomponents.client.version>4.5.13</apache.httpcomponents.client.version> | ||
| <apache.httpcomponents.core.version>4.4.14</apache.httpcomponents.core.version> |
There was a problem hiding this comment.
minor point - apache.httpcomponents.[http]core.version to match the real package name?
There was a problem hiding this comment.
Whatever you folks see as a better fit. Feel free to change.
| @@ -584,7 +617,7 @@ | |||
| <dependency> | |||
| <groupId>org.apache.commons</groupId> | |||
| <artifactId>commons-compress</artifactId> | |||
| <version>1.19</version> | |||
| <!-- no version here as managed by <dependencyManagement> above! --> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>org.duracloud</groupId> | |||
There was a problem hiding this comment.
duracloud needs to go to 7.0.0 (common and storeclient - both at 4.4.6 now) per IQSS/dataverse-security#35. Can that be included here?
There was a problem hiding this comment.
Actually it will need to be 7.1.0+ which is not yet released. See https://github.com/IQSS/dataverse-security/issues/35#issuecomment-894187477
| // 5 or more numbers in a row | ||
| Arguments.of(2, "ma02138", goodStrength20, maxLength, 6, dictionary, numberOfCharactersDefault, characterRulesDefault, numConsecutiveDigitsAllowed), | ||
| // 5 or more numbers in a row, but multiple times | ||
| Arguments.of(3, "ma8312002138", goodStrength20, maxLength, 6, dictionary, numberOfCharactersDefault, characterRulesDefault, numConsecutiveDigitsAllowed) |
There was a problem hiding this comment.
Is it understood why this has 3 errors instead of 2 with the new version of passay?
There was a problem hiding this comment.
Yes. There was a bug in the older Passay version, which wasn't identifying multiple occurences of more than x numbers in a row. (It's 10 digits here, which is 2x error of 5 numbers) This made a +1 for this test necessary.
|
Made a few quick comments - still on vacation but wanted to unblock people. |
|
@poikilotherm I know that above in testing you'd said should be ok with existing unit and integration tests but I do like to better understand the scope of changes and to do some selected exploratory testing regardless of the automated tests. So, if I understand correctly, any functionality currently using XML, eg. exports, api endpoint inputs and outputs, harvesting, should be checked, whether through automated tests and/or manually? Additionally, running the automated tests should pass seeing they have been extensively updated, is this correct or can you provide a better overview of impacted functionality needing to be retested? |
|
Hey @poikilotherm, I hope you enjoy your vacation! Thanks for the PR! I'll move this back to Community Dev for now, and we can revisit it when you're back. |
|
Hi @kcondon - I'm back. I already updated the branch with latest Regarding testing:
Hope this helps...(?) |
What this PR does / why we need it:
This PR fixes several security reportings. Please see linked issue for details.
Which issue(s) this PR closes:
Closes IQSS/dataverse-security#38
Special notes for your reviewer:
There are a lot of moving parts in this one. This is due to the fact that it's moving a lot of Java XML tech in the background by updating old deps.
I tried to add as much context information to commit messages for the individual pieces as possible.
Starting with the XML stuff, it was a bit like Domino: stones started to fall once the first had been pushed.
BrandingUtilunit test been run before, failing when executed on their own. Refactored LOADS of tests (not just the ones I absolutely had too) to be more atomic. Not perfect but better.Suggestions on how to test this:
Should be all set with our usual unit and integration tests.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Nope.
Is there a release notes update needed for this change?:
Nope.
Additional documentation:
None yet.