Passbolt API Code Review results

Passbolt uses a variety of open source technologies to underpin our software. You can see a full list of these technologies here. One of the technologies we use extensively is CakePHP, which is used to power the server side of the passbolt solution. We recently submitted passbolt for a code review from the brains behind CakePHP, CakeDC.

This audit was solely focused on the API (e.g. server side) code, and does not include a review of the Javascript code or high level design items such the API naming conventions or our use of the GPGAuth protocol.

This review was done in December, spanned across three weeks, with the code from passbolt version 2.0 development branch, based on CakePHP 3.5.

This code is available under the “development” branch on passbolt api github repository.

We are pleased to report that CakeDC did not find any critical flaws. Only three high ranking issues were found and close to 50 medium to low ranking ones. You can have a look at the full report here, or continue reading to have a quick look at the results in each area.

Security

We take security very seriously so we are probably most excited about the fact that passbolt passed the initial security review with flying colours. While no obvious security flaws were found, this does not mean that our work is done in this area. We will continue to be remain vigilant around all encryption, authentication and other security or privacy related issues.

We invite security researchers to submit any new vulnerability, findings or suggestions for improvement to [email protected].

Quality

Cakephp Conventions

Only two small issues in this area, which have been fixed even before we wrote this article.

Coding standards

Since we sniff around for coding standard issues, most issues found here were surrounding missing or mismatching of documentation blocks for functions and class signatures.

Code quality

The only high level issue in this section relates to the fact that we do not use of model aliases: we hard code the string corresponding to the model names, instead of using a function providing this name. This could lead to maintainability issues if we change the model names in the future for example.

Implementation

Framework usage

We’ve used the CakePHP framework mostly in the way the framework is intended. However, this part includes most of the recommended improvements, some are low hanging fruit, some requires a substantial investment in time.

Most notable issue relate to our design choice of using one Controller per API actions (for example, one controller with the user delete operation) instead of having all the actions related to an logical object in one file (for example, one controller with user delete, created, edit, etc.). We made this choice to facilitate and speed up testing and allow inheritance between similar actions (setting up a new account and recover an existing account share similar logic). However, this choice prevents us from leveraging some of the framework features such as RESTfull routes, therefore increasing the complexity of the file where we place our routes definitions.

Since default RESTfull routes are not always applicable to passbolt (e.g. for our most complex endpoints, such as password share that affect multiple objects) CakeDC suggested adopting another approach using Action classes (example 1, example 2). This is a reasonable tradeoff that we will be implementing in the middle/long-term future.

Separation of concern

“In general, we have not found big issues related with misuse of the framework or MVC pattern concepts.”

One notable suggested improvement would be to move some of the hardcoded access right logic (such as, you need to be an group owner to add a user to that group) into authorization adapters. This would allow us to create more flexible and configurable authorization rules in the future, so that you as the end user could potentially define some of this logic (such as: creating a new role and changing who can do what in passbolt).

Code Reuse and Modularity

CakeDC’s recommendations in this area will help us reduce the amount of boilerplate code. It would also serve to improve maintainability and readability of passbolt’s codebase and ease the learning curve for developers joining the project.

Performance

“We have not found any performance issues in the code base.”

CakeDC also let us know that they didn’t find any performance issues even though they suggested some improvements related to missing indexes in the database tables definition. We also started working on some gatling tests to allow us to monitor any new changes impact on performance.

Conclusion

We have put a lot of effort into the migration to the new version CakePHP, and we’re happy and thankful that it would appear that we didn’t mess it up. Even though this effort did delay the rollout of some of the most expected features we believe this was the right time for passbolt, as it will serve as the foundations for some of the work in the following years to come. We will continue working on implementing the recommendations of this code reviews in the course of the next few months, we will keep track of all the issues and their status here.

We’re extremely thankful to CakeDC for their thorough review and for actively sharing their experiences and knowledge of the framework with us. Jorge, Andrej, Evgeny and Larry made clear and tangible proposals to improve the API and passbolt in general. This process has been a great learning opportunity for the team.

As for the next steps we are currently opening the v2-RC1 for testing to all volunteers, while the team now focuses on implementing long overdue new features such as Tags, Import and Exports. Feel free to get in touch with us at [email protected] if you want to help with the testing!