Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump xstream from 1.4.17 to 1.4.18 #1917

Conversation

dependabot[bot]
Copy link
Contributor

@dependabot dependabot bot commented on behalf of github Aug 23, 2021

Bumps xstream from 1.4.17 to 1.4.18.

Commits

Dependabot compatibility score

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

  • @dependabot rebase will rebase this PR
  • @dependabot recreate will recreate this PR, overwriting any edits that have been made to it
  • @dependabot merge will merge this PR after your CI passes on it
  • @dependabot squash and merge will squash and merge this PR after your CI passes on it
  • @dependabot cancel merge will cancel a previously requested merge and block automerging
  • @dependabot reopen will reopen this PR if it is closed
  • @dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
  • @dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

Bumps [xstream](https://github.com/x-stream/xstream) from 1.4.17 to 1.4.18.
- [Release notes](https://github.com/x-stream/xstream/releases)
- [Commits](https://github.com/x-stream/xstream/commits)

---
updated-dependencies:
- dependency-name: com.thoughtworks.xstream:xstream
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
@dependabot dependabot bot added Priority 1: Must Highest priority. A release cannot be made if this issue isn’t resolved. Status: In Progress Use to signal this issue is actively worked on. Target: 4.5.4 Type: Dependency Upgrade Use to signal an issue that adjusts the project’s dependencies. Typically used by dependabot only. labels Aug 23, 2021
@dependabot dependabot bot added this to the Release 4.5.4 milestone Aug 23, 2021
@smcvb smcvb self-assigned this Aug 23, 2021
Copy link
Member

@smcvb smcvb left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

With version 1.4.18 of XStream, unspecified types are no longer
supported by default in XStream (as described in their release notes
here: https://x-stream.github.io/changes.html#1.4.18). To keep XStream
as the default serializer, at least Axon's types should be included by
default. Users should also be able to disable this default if required.

#1917
- Update code style of touched files
- Fix warnings
- Fix typos

#1917
Copy link
Contributor

@lfgcampos lfgcampos left a comment

Choose a reason for hiding this comment

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

LGTM

Remove unused eq() invocation

#1917
@sonarcloud
Copy link

sonarcloud bot commented Aug 23, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@sanderino666
Copy link

Hi all,

Just as a heads up. Xstream changed the security mechanism in 1.4.18 due to a CVE and it is causing regression in projects using Axon and Spring.

x-stream/xstream#264
spring-projects/spring-framework#27342

Might be interesting to check out these discussions as well.

@abuijze
Copy link
Member

abuijze commented Sep 2, 2021

Hi @sanderino666 ,

thanks for the heads-up. We are aware of the discussions around XStream's security changes. Axon doesn't do much in terms of XStream configuration (basically leaving it to defaults). Basically, the default has changed from "allow all unless specified" to "reject all unless specified". That means the configuration should also be modified.

We're currently discussing how we can make the impact of this change minimal. Not upgrading is not an option, since security should be everyone's no1 priority. Serializers are components at the edge of an application, and simply need to be secure. So we prefer to go along with the "keep it closed by default", but that would indeed mean a breaking change for existing users...

@sanderino666
Copy link

Hi @abuijze,

Not upgrading is not an option, since security should be everyone's no1 priority

Completely agree! I can easily reproduce the issue so if you need me to test on a snapshot, let me know,

Adjust the XStreamSerializer to no longer set a default XStream instance
Also deprecate the defaultSerializer method, stating the security
warning shared by XStream.

#1917
Add a xstream version property and use it for the messaging package.
Also update the autoconfiguration package to have an optional XStream
dependency

#1917
Remove the default XStreamSerializer from the JPA and JDBC storage
engine implementations. The tests should be adjusted to use a secure
XStreamSerializer. The Javadoc should be adjusted accordingly.

#1917
Remove the default XStreamSerializer from the QuartzDeadlineManager and
QuartzEventScheduler

#1917
Remove the default XStreamSerializer from the JpaSagaStore and
JdbcSagaStore implementations. Adjusts the tests to use a secured
version of XStream on an XStreamSerializer. Update the JavaDoc
accordingly.

#1917
Set a secured XStreamSerializer through the TestSerializer

#1917
Set a secured XStreamSerializer through the TestSerializer

#1917
Set an XStream instance for the XStreamSerializer. Using the
XStreamSerializer default configuration to secure axon components is
sufficient.

#1917
Add more default aliases to the AbstractXStreamSerializer, for queries,
results and deadlines.

#1917
Remove the default XStreamSerializer from the AxonServerEventScheduler
and AxonServerEventStore. Also ensure a working XStreamSerializer is
provided through the TestSerializer for every test class utilizing
serialization.

#1917
Update the used XStreamSerializer to ascertain that it uses a dedicated
XStream instance.

#1917
Introduce an XStreamAutoConfiguration, that checks for the ComponentScan
 annotated beans to deduce what the base packages and classes are to add
  to the security context of an XStream instance. Use this XStream
 instance when constructing the XStreamSerializer, if it has been
 configured to be used.

#1917
To isolate the logic and to simplify testing, move the ComponentScan
searching logic from the XStreamAutoConfiguration to a
XStreamSecurityTypeUtility.

#1917
@smcvb smcvb requested review from smcvb, m1l4n54v1c, lfgcampos, saratry and abuijze and removed request for smcvb September 29, 2021 08:51
Add test case that validates spring boot application properties are also
 taken into account.

#1917
Add builders tests to please coverage

#1917
…n/axon-4.5.x/com.thoughtworks.xstream-xstream-1.4.18

# Conflicts:
#	modelling/src/main/java/org/axonframework/modelling/saga/repository/jpa/JpaSagaStore.java
Copy link
Contributor

@lfgcampos lfgcampos left a comment

Choose a reason for hiding this comment

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

There are a couple of other tests where we could use a TestSerializer as well but I saw all of that as a nit.

Little thing to not forget, as we talked, is to have a dedicated section on our release notes about the 'braking changes' we are forced to make because of it.

smcvb and others added 5 commits September 29, 2021 13:12
…rTest.java

Co-authored-by: Lucas Campos <lfgcampos@gmail.com>
Introduce TestSerializer to be used for legacy tests.

#1917
…thoughtworks.xstream-xstream-1.4.18' into dependabot/maven/axon-4.5.x/com.thoughtworks.xstream-xstream-1.4.18
To not give a breaking change to users, we return the XStreamSerializer.
 Whenever the default is used though, we throw a warning stating the
 users should set the security context consciously.

#1917
Allow all types on the defaultSerializer, by allowing all implementation
 of Object. Log a warning for the user that this isn't secure at all!

#1917
@sonarcloud
Copy link

sonarcloud bot commented Oct 1, 2021

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

69.4% 69.4% Coverage
0.3% 0.3% Duplication

@smcvb
Copy link
Member

smcvb commented Oct 1, 2021

Sonar complains about unreached new loglines. As we're not in the habit of validating log lines, we will ignore the coverage level warning.

Furthermore, @abuijze and I verbally reviewed this pull request. Hence, approval from his end is added to this resolution.

@smcvb smcvb merged commit dec0827 into axon-4.5.x Oct 1, 2021
@smcvb smcvb added Status: Resolved Use to signal that work on this issue is done. and removed Status: In Progress Use to signal this issue is actively worked on. labels Oct 1, 2021
@dependabot dependabot bot deleted the dependabot/maven/axon-4.5.x/com.thoughtworks.xstream-xstream-1.4.18 branch October 1, 2021 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CVSS: High Priority 1: Must Highest priority. A release cannot be made if this issue isn’t resolved. Status: Resolved Use to signal that work on this issue is done. Type: Dependency Upgrade Use to signal an issue that adjusts the project’s dependencies. Typically used by dependabot only.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants