Site Compass

Challenge

Reivernet Group hired Redwerk to help them audit one of their software products before the official release. The software in question was Site Compass, a network mapping app. Site Compass is meant for construction companies that want to improve their profitability.

Site Compass helps users manage building sites by centralizing all the needed data associated with budget planning, tendering, and asset management. The app also simplifies reporting, including generating audit reports and bills of materials.

It was crucial for Reivernet Group that an independent party performed the code review. They needed an accurate and unbiased evaluation of their product’s quality. Our scope of work included:

• Architecture Review. At this step, we were to ensure the system’s foundation was well-designed, met requirements, and could accommodate future growth.
• Database Review. Here, we ensure the database is optimized for performance, security, and data integrity.
• Code Quality Review. This part includes assessing the code readability, maintainability, consistency, and adherence to modern coding standards. We were asked to review only the backend code.
• Test Coverage Review. Here, we ensure the code is adequately tested and identify areas where additional testing is needed.
• Security Review. Our goal was to evaluate the app against OWASP’s Top 10 Web Vulnerabilities and provide mitigation strategies.

Besides reporting the found issues, we provide detailed recommendations on how to fix them and how much time is needed to refactor the code.

Solution

Site Compass is built in C# using Uno Platform, which allows iOS, Android, and Windows apps to be all in one code set. We have years of experience with all the technologies used in this project, so it was quite an easy task for us.

To speed up the process and ensure we catch every mistake, we completed the manual check with automation, using specialized tools like NDepend and PVS-Studio. Here’s a brief overview of issues we encountered at each step.

Architecture Review

The app has a layered architecture with separate presentation, code, and database layers. This approach is efficient for multi-platform solutions like Site Compass due to its shared codebase, reducing development time and cost. However, it can lead to some types or classes from the codebase becoming too generic and difficult to maintain.

As for critical issues, we found that some namespaces were mutually dependent. Circular dependencies make it difficult to modify or test one namespace without affecting the other. This tight coupling can hinder code flexibility and increase development time.

A solution to this would be moving one or several types from the low-level namespaces to the high-level one or vice versa. Another option is to use inversion of control (IoC) to introduce an abstraction layer between dependent components and promote loose coupling.

Also, we found some classes were too deep in the inheritance tree, with depth of inheritance reaching 5-8 levels. This is a deviation from the object-oriented programming that favors composition over inheritance.

Long inheritance chains are problematic because they violate the principle of encapsulation, meaning “parent” classes can expose implementation details to “children”. The latter defeats the purpose of having separate layers, as anyone can mess with the internal structure, leading to instability.

Fixing this would require analyzing tightly coupled functionalities and changing their design using composition.

Database Structure

We found no issues with the database architecture and scalability. Azure Cosmos DB offers enough flexibility to scale it and address performance issues. The only aspect that could affect the app’s performance is a scenario where the user’s region differs from the cloud region, leading to slow load times.

Backend Code Quality

We reported several critical and medium severity issues regarding the backend code quality.
We identified 38 methods with 7 to 14 parameters. That’s too many parameters. Methods with too many parameters are painful to call and might degrade performance.

A solution is to add more properties/fields to the declaring type to handle numerous states. An alternative is to provide a class or a structure dedicated to handling argument passing.

The next issue is overly big types, resulting in the so-called “god class” phenomenon. God class or god object is a single class that tries to do everything. It exerts excessive control over other classes in the system, often growing so large that it becomes responsible for performing all tasks. This results in a code that is difficult to understand, maintain, and test.

Fixing a god class requires splitting it into smaller classes with a single responsibility and well-defined boundaries. Try to maintain the interface of the god class at first and delegate calls to the new extracted classes. In the end, the god class should be a pure facade without its own logic. Then, you can keep it for convenience or throw it away and start using the new classes only.

Another issue affecting the code maintainability and testability is using non-readonly static fields. If the value never changes, make it read-only and set it directly in the static constructor or inline with its declaration. If the value changes occasionally, we should use an instance field instead. Each object will have its own “box,” avoiding shared state issues.

We also reported overly complex, potentially dead, or poorly commented methods and methods with excessively long names.

Test Coverage

Only a small percentage of code was covered with tests. Low test coverage means large portions of code remain untested, leading to bugs slipping into production.

Early and ongoing quality assurance is vital because bugs found in production are more expensive and time-consuming to fix, disrupting release schedules and requiring hotfixes. Also, inadequate initial testing makes regression testing less effective, as it’s unclear what the original behavior was in untested areas.

Security Review

Our security review included checking the code for injection vulnerabilities, such as NoSQL, LDAP, and OS injections.

We ensured there were no broken authentication occurrences, sensitive data exposure, poorly configured XML parsers, and default, incomplete, or ad hoc configurations.

Our reviewers also looked for loopholes enabling persistent threats. Those would allow attackers to remain undetected in case of a breach, compromising more systems and data.