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

Proposal: Get rid of all serialization in Rhino #704

Open
gbrail opened this issue Jun 6, 2020 · 8 comments · May be fixed by #727
Open

Proposal: Get rid of all serialization in Rhino #704

gbrail opened this issue Jun 6, 2020 · 8 comments · May be fixed by #727
Labels
Continuations Potentially Breaking Change Issues that could break backwards compatibility roadmap security
Projects

Comments

@gbrail
Copy link
Collaborator

gbrail commented Jun 6, 2020

Rhino tries to make all JavaScript objects and other objects Serializable, because it's always done that.

I think that we should consider getting rid of Serialization support for a future release for a few reasons:

  1. There is a pretty big category of security holes that are created when things are Serializable.
  2. Keeping things Serializable is a maintenance burden and makes it harder for us to change internal structures.
  3. Others in the Java community are considering removing it from Java some day.
  4. Most other JavaScript engines handle this using JSON.

Can people who actually Serialize and de-serialize Rhino objects please comment here on whether you are using Serialization, and how?

@rbri
Copy link
Collaborator

rbri commented Jun 6, 2020

HtmlUnit does not use serialization of JavaScript objects.
Everything that gives us more freedom to change the code base is very welcome.

@gbrail
Copy link
Collaborator Author

gbrail commented May 3, 2021

I'd actually still like to do this -- serialization is being more and more generally acceptable as being terrible for interoperability and security. Unless someone has code that will break I am going to create a new PR soon.

@p-bakker p-bakker linked a pull request Jul 2, 2021 that will close this issue
@p-bakker p-bakker added security Potentially Breaking Change Issues that could break backwards compatibility labels Jul 5, 2021
@michbarsinai
Copy link

Combined with continuations, serialization is a super-powerful tool, as it allows a single machine to run virtually unlimited amount of threads. This is by capturing and serializing continuations, and storing them in a DB. Think of a server that runs multiple processes based on external events. I've heard of at least one system that's doing this using Rhino. I know there was also an experimental project that used serialized continuations to move a computation between servers, but I'm not sure if it ever reached production.

In BPjs we depend on serialized continuations for re-trying computations.

I of course understand the complications serializations add, but given the power they give to Rhino, I'd be happy to see them stay.

@p-bakker p-bakker added this to To do in v2.0.0 via automation Oct 14, 2021
@p-bakker p-bakker moved this from To do to To Discuss in v2.0.0 Oct 14, 2021
@p-bakker
Copy link
Collaborator

p-bakker commented Oct 14, 2021

@gbrail based on the discussion here and in the Next steps for Rhino after the v1.7.14 release discussion, it looks like serialization, at least in the context of Continuations is something that is still desirable.

Not being an expert (or even a rookie in this area), but what our options? Do we have to keep serialization indefinetly, or are there other options?

Based on your initial coming in this issue:

and makes it harder for us to change internal structures.

Can we eliminate this hurdle by saying that we dont support loading serialized stuff with other version as with which they were written?

Most other JavaScript engines handle this using JSON.

I don't think other engines support continuations, which I assume require also storing the callstack, but would that not also be something we could offer as JSON? Not sure how would work with POJOs though.

@jgomer2001
Copy link

Combined with continuations, serialization is a super-powerful tool

Agree 100%

I don't think other engines support continuations

Agree as well.
Except for Kotlin coroutines and Rhino, I think there are no other means to support the concept of resumable computations in the JVM.

Currently we are incubating a project consisting of a framework for web flows. At the heart of it we have Rhino serializable continuations. They allow us to restore the state of a given flow regardless the end-user hits a different node (server) after every form submission (think of multiple java containers with the same stateless webapp deployed).

Having such a bad reputation, I tried to see if I could switch from Java serialization to a different serialization mechanism (like kryo) but I couldn't get any far: I noticed several core classes provide their custom readObject and writeObject methods so that introduces a "logical" dependency on how I would have to do serialization with a different tool.

I wonder if in the future you would like to take an approach for serialization so it isn't coupled to the Java serialization API. Maybe this would release some of the pain that @gbrail regarded at the beginning of this issue (numeral 2)

PS: if you have any suggestion for me (so that I can use my own serialization tool), let me know. Ideally I need to support serialization of arbitrary objects that hold flows' state, ie. I don't know the Classes a priori.

@michbarsinai
Copy link

@jgomer2001 in BPjs, we subclassed ScriptableOutputStream, and replace objects with object stubs before they go into the stream. We do the reverse process on de-serializations.

See here:

https://github.com/bThink-BGU/BPjs/blob/2650ad1d588d1aaf83740b1b1be2d0d3dc84bda5/src/main/java/il/ac/bgu/cs/bp/bpjs/bprogramio/BPJSStubOutputStream.java#L48

Credit for the original idea goes to @szegedi, which used it in Spring JSFlow (https://github.com/szegedi/spring-web-jsflow/blob/master/src/main/java/org/szegedi/spring/web/jsflow/support/FlowStateSerializer.java)

@jgomer2001
Copy link

Pretty interesting, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Continuations Potentially Breaking Change Issues that could break backwards compatibility roadmap security
Projects
v2.0.0
To Discuss
Development

Successfully merging a pull request may close this issue.

6 participants