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

Reduce Complexity, Improve Security #41

Open
7 tasks
Z-Kris opened this issue Nov 4, 2024 · 0 comments
Open
7 tasks

Reduce Complexity, Improve Security #41

Z-Kris opened this issue Nov 4, 2024 · 0 comments
Assignees
Labels
documentation Improvements or additions to documentation

Comments

@Z-Kris
Copy link
Contributor

Z-Kris commented Nov 4, 2024

Summary

Reduce the complexity and duplicate code in RSProt by reducing most of the code in most of the modules into a single shared module across all OSRS revisions. Each revision that is currently being added, adds an extra ~53,000 lines to the repository. The vast majority of this is unchanged, and will always remain unchanged. It creates a problem of having to modify multiple source sets whenever a refactoring or bug fix is performed though, rather than just changing it in one centralized place.

Goals

  • Reduce most of the code into a single shared module (model data classes, API, helpers)
  • Support encoding and decoding in both directions, allowing for programmatic testing
  • Support RSProx directly without needing to duplicate any code

Motivation

As we add more and more revisions to the project, one thing has become evident: It is increasingly more difficult to do any project-wide refactoring or bug fixes. Any change we do in one module has to be back-ported to all the others, which increases the time taken to do any fixes, and significantly increases the probability of a mistake being made in the back-porting, leading to obscure bugs that go by unnoticed until someone makes use of the bugged older revisions. It has also led me to skip doing some non-crucial changes to older revisions, leaving the older revisions less performant, or worse to use overall.

The RSProx project also suffers from similar problems - every bit of code is manually copied over from RSProt and just duplicated, instead of directly depending on the original source of code. The primary reason why we couldn't just directly depend on RSProt sources is that RSProt did not support decoding the server packets, so those decoders were created on RSProx's side instead. This leads into an annoying work-flow every revision, having to do half the work in RSProt, then switch projects, copy classes, refactor them to be compatible and furthermore write decoders for the missing parts. Instead of this complicated process, we should migrate the decoders for server packets over to RSProt instead as an opt-in feature. The big benefit here is that it will allow us to programmatically test server packets, encoding them in the test and using said decoders to decode it back into the original data, and ensure the values all align up. This is a valuable piece of information that currently has to be obtained by manual verification by myself after every revision.

The other issue, however, is client packets. Currently, neither project creates encoders for client packets, as neither one of them has a direct use for it. However, as with server packets, having functions to go both directions would allow us to programmatically test everything. Due to the benefits of this, we would introduce encoders for client packets as part of this rewrite. While this seems like a lot of extra effort - it's not as bad as it seems. Writing the decoders for client packets takes under an hour to do, writing the encoders would be on-par with that. The time wasted by manual validation of all the packets, however, is about three times as long as that. In the end, it would be a net-positive and decrease the risk of mistakes by manual human validation.

Description

This project will consist of multiple steps that need to be taken in a specific order.

Below is an ordered check-list of the steps that need to be taken to complete this issue. It's evident we cannot finish this project before the next revision is released, but we should prioritize it to be completed before revision 228 is released.

Info Packets

The first step we need to take is cleaning up info packets. These packets currently reside in the `model` modules. Most of the logic encoding and processing is done within the model module, with only the client-specific differences being pushed into client-specific modules. It was never my intent to do logic in model modules, but it turned out that way.

In order to reduce the model modules down to a single shared module between all OSRS revisions, we need to get rid of any logic processing classes related to info packets in these models. The bulk of info processing should be moved to a shared internal module across all OSRS revisions. The shared module would be capable of supporting the logic of every revision of OSRS, be that 221 or 226 (and beyond). Any parts that encode the data would have to be done in the modules that exist per client type instead. This does create some small amount of code duplication for player info, as the bulk of bit writing is unchanged between client types, but on the flip side, it would make the code more flexible if revision specific changes are ever done. NPC info is a good example of where the bit writing is different between clients.

Furthermore, once most of the code for info packets has been removed from the model module, we would need to create a proper API that mimics what the current model module is doing. This API would be revision-specific, allowing us to only expose functions which we want the end-user to be able to see. The aforementioned shared internal module would not be exposed by default, it is intended for revision-specific modules to depend on and cherry-pick the features that should be supported by said revision.

Model Versioning

The next step after this is to support versioned model classes. Since the introduction of mobile in 2018 (revision ~176), Jagex has had to use versioning for all their packets, instead of just editing existing ones and keeping them as is. Since then, whenever a packet was set up to be changed, a new variant was created with a _V* suffix, while retaining the older one for some period of time (or indefinitely, in some cases). This is because the clients get pushed out while the server is still on the older revision, so they need to support the same logic and functionality for at least one revision. The versioning only includes structural and logic changes to packets, not revision-specific scrambling. We must have distinct classes for all our revisions whenever such structural changes were performed to move on to the next step.

Model Modules

Once info packets have been correctly packaged, and all our logic changes are correctly versioned, we can begin the process of flattening the revision-specific modules into a single shared module across all OSRS revisions. In doing so, we reduce the codebase by ~31,000 lines for every revision. Out of this, ~11,000 is strictly used up by the info packets.

There is one small problem to consider in flattening the model. If we reduce it down to a single shared module, anyone depending on a specific revision of RSProt would see all the model classes across all revisions, including the ones which their selected revision does not support. There is no way of selectively deprecating or avoiding certain classes, not without some level of duplication. The best we can really do here is define the supported revisions in the documentation, which just might be the best and easiest solution.

Shared Modules

Once we have flattened the models, the next step is to correct the mistakes in login and JS5. The code there never really changes, we can use that to our advantage to de-duplicate it further, and follow a similar structure to how game packets are handled in general - models to a shared module, encoders and decoders to revision-specific modules. The only difference here is that the encoders and decoders would not be per-client-type, but rather just per-revision.

API Modules

The next step from here is flattening the current "API" modules, which are poorly named anyway. A better name for those modules would be "bindings", as that's the majority of what goes on in them - they just bind to Netty and handle any sort of Netty configuration. By renaming this, we open up the API module naming for a real API - containing logic such as the info packets.

Remaining Modules

There are two remaining modules - common and internal. It is currently unclear how these would be refactored, but they would very likely be done as part of one of the steps above, as they're heavily depended upon by some of the packets.

RSProx's Perspective

After the project has been restructured, we need to start migrating over the things that RSProx currently enhances RSProt with, such as the missing decoders and data classes. The end goal is to support the client packet encoders, and server packet decoders within RSProt as well. However, this is a time-consuming thing and we should have it as an opt-in process that we slowly migrate onto, rather than refactor everything in one go. Revisions 221 and 222 aren't supported by RSProx, which means the server packet decoders would have to be created for those. This is rather lengthy process, but should be supported eventually just for tests and security it provides.
@Z-Kris Z-Kris added the documentation Improvements or additions to documentation label Nov 4, 2024
@Z-Kris Z-Kris self-assigned this Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

1 participant