CSC/ECE 517 Spring 2024 - E2401 Implementing and testing import & export controllers: Difference between revisions
(Adding all major sections) |
(→3. Static analysis fixing: Add image of errors) |
||
| Line 29: | Line 29: | ||
=== 3. Static analysis fixing === | === 3. Static analysis fixing === | ||
As aforementioned, the previous team's project occurred at a time before GitHub Actions verified many aspects of the code using static analysis. This resulted in many, many errors being brought forth when we copied the previous team's code. Most severe were a handful of security vulnerabilities that existed in E1923's code. All the found security vulnerabilities existed in import_file_controller.rb. The first, involved constantize. The error existed because constantize was being run on user provided input, therefore, a malicious actor could provide unintended classes to the import method and receive information about these classes and maybe even have these other classes added to the website's database. To fix this error, we created a simple ALLOWED_MODELS whitelist array that includes the strings of all the permitted class types. We then call constantize on the match one of these strings to avoid the GitHub vulnerability message. This way, only the classes we desire to be imported will actually be allowed by the allowed_model method. The second vulnerability, involved the eval method. Upon further research, this is a '''terrible''' method to use in general in Ruby. The eval method will run the code of whatever is inside the string provided to it. While this is powerful, a malicious user can use this to run arbitrary code on Expertiza. The eval method was overkill for what it was really trying to do, parse a hash. We replaced the eval function with JSON.parse() which fixes this vulnerability. After the security vulnerabilities, we had 404 style errors thrown by codeclimate. Many of these changes were simple yet very tedious. For example, many suggested change double quotes to single quotes, removing trailing or excess whitespace, correcting indentation, removing unnecessary .to_s method calls, and adding spaces around the curly braces of hashes. After these changes, only 33 remain. Reviewing the GitHub Actions, we recognize that codeclimate is one the checks that is not required and we would recommend that many of the remaining suggestions are unhelpful and some may be suppressed. For example, the recommended fixes for "Use the new Ruby 1.9 hash syntax" involves changing the rocket (=>) to a colon (:), however, this change in our context will actually cause a compilation error. Furthermore, some of the refactoring suggestions are unnecessary. For example, the branch condition size was determined to high for the get_topic_attributes method in sign_up_topic.rb, however, the method is simply only adding fields if they exist and splitting this code might make it more confusing to read, not less. | As aforementioned, the previous team's project occurred at a time before GitHub Actions verified many aspects of the code using static analysis. This resulted in many, many errors being brought forth when we copied the previous team's code. Most severe were a handful of security vulnerabilities that existed in E1923's code. All the found security vulnerabilities existed in import_file_controller.rb. The first, involved constantize. The error existed because constantize was being run on user provided input, therefore, a malicious actor could provide unintended classes to the import method and receive information about these classes and maybe even have these other classes added to the website's database. To fix this error, we created a simple ALLOWED_MODELS whitelist array that includes the strings of all the permitted class types. We then call constantize on the match one of these strings to avoid the GitHub vulnerability message. This way, only the classes we desire to be imported will actually be allowed by the allowed_model method. The second vulnerability, involved the eval method. Upon further research, this is a '''terrible''' method to use in general in Ruby. The eval method will run the code of whatever is inside the string provided to it. While this is powerful, a malicious user can use this to run arbitrary code on Expertiza. The eval method was overkill for what it was really trying to do, parse a hash. We replaced the eval function with JSON.parse() which fixes this vulnerability. After the security vulnerabilities, we had 404 style errors thrown by codeclimate. Many of these changes were simple yet very tedious. For example, many suggested change double quotes to single quotes, removing trailing or excess whitespace, correcting indentation, removing unnecessary .to_s method calls, and adding spaces around the curly braces of hashes. After these changes, only 33 remain. Reviewing the GitHub Actions, we recognize that codeclimate is one the checks that is not required and we would recommend that many of the remaining suggestions are unhelpful and some may be suppressed. For example, the recommended fixes for "Use the new Ruby 1.9 hash syntax" involves changing the rocket (=>) to a colon (:), however, this change in our context will actually cause a compilation error. Furthermore, some of the refactoring suggestions are unnecessary. For example, the branch condition size was determined to high for the get_topic_attributes method in sign_up_topic.rb, however, the method is simply only adding fields if they exist and splitting this code might make it more confusing to read, not less. | ||
[[File:Team E2401 Codeclimate Errors.webp|thumb|left|Original codeclimate errors after copying and integrating E1923's code]] | |||
== Future improvements == | == Future improvements == | ||
Like our preceding team, E1923, we have not touched the exporting code. Exporting could receive a mirroring change to that of importing to allow for a similar sharing of code. Additionally, as mentioned in the static analysis fixing section, there are some codeclimate errors remaining in our final pull request. If the teaching staff believes all of these should be fixed as the automated system describes, this would be a future necessary task, and potentially a good option as a good continuation for our team into Project 4. | Like our preceding team, E1923, we have not touched the exporting code. Exporting could receive a mirroring change to that of importing to allow for a similar sharing of code. Additionally, as mentioned in the static analysis fixing section, there are some codeclimate errors remaining in our final pull request. If the teaching staff believes all of these should be fixed as the automated system describes, this would be a future necessary task, and potentially a good option as a good continuation for our team into Project 4. | ||
Revision as of 09:08, 27 March 2024
This wiki page is for the description of changes made under E2401 OSS assignment for Spring 2024, CSC/ECE 517.
Team
Mentor
- Ed Gehringer
Team Members
- Gwen Mason - cpmason@ncsu.edu
- John Nolan - jrnolan2@ncsu.edu
- Meet Patel - mpatel29@ncsu.edu
Expertiza Background
Expertiza is an open source Ruby on Rails application for teachers to manage classrooms. Expertiza is maintained by the teaching staff at NC State along with the very students who use it. Expertiza has many different models saved inside it including assignments, questions, and users. Relevant to our project, these models can be imported or exported through the website as CSV files for ease of uploading or editing many objects at once.
Description of project
Our project's description changed late into the project submission period. We were originally tasked with using the new implementation of the import_file_controller.rb created by team E1923. Their project involved consolidating all the import functions into one common functionality. Before team 1923, each model used their own import code. However, these import codes shared a lot of functionality and were ripe for refactoring. E1923's import_file_controller.rb used polymorphism to fix this duplicate code. Each model had import specific methods that the import file controller calls and receives the information necessary to create and save the respective models. We spent the early stages of the project understanding the previous team's code. Our original goal was to take several models not yet utilizing this import_file_controller.rb and extending them to use the common code.
However, later into the project we learned that unfortunately, team E1923's code was never merged into the main Expertiza repository. In 2019, the project was tagged with merge, however, it never actually was and later closed in 2021. Upon this discovery, our project's mission changed dramatically. Instead of getting other models to work with the existing import_file_controller.rb, our goal was to redo the changes of team 1923. Our goal is to get justice for the work of E1923 by getting their changes into Expertiza. However, our project was not as easy as just copying over the changes from E1923. The codebase has changed significantly over the previous 5 years which meant that much of their code and tests had to be fast forwarded to integrate into the current repository. Additionally, E1923 finished their project without the static analysis tools that currently check for styling and vulnerabilities. There were 100s of errors on GitHub Actions related to styling, test failures, and vulnerabilities that we had to correct given the scale of E1923's changes. Because of the late-term revelation, Dr. Ed graciously extended our deadline by a few days to allow our new goal to be completed.
Our changes
Our changes can be split into three main phases. All changes worked towards a common goal to have E1923's code integrated into the current repository.
1. Copying changes over
Under our mentor's advice, we could not directly merge E1923's code into the current Expertiza. This is simply because there have been so many changes over the last 5 years that the merge conflicts would be infeasible to sift through. Instead, we had to manually go file by file and make the changes matching E1923. We had to pay attention to two main diffs for each of the 47 files that E1923 changed. We first compared the changes that E1923 made to the 2019 Expertiza repo then compare the original 2019 repo against the modern repo. We had to resolve all the conflicts that arose from changes in these files throughout the years. For example, if a method was renamed during the last 5 years, we had to make sure the copied code was changed to use that new method.
2. Test fixing
Upon completion of the copying phase, we discovered that many tests were failing. Luckily, many of these tests had common causes (originally just a few compilation errors). Despite our scrupulousness, a few bugs had still crawled into our updated code. We had to add some missing methods. Eventually, we got to the bugs not caused by simple copy errors as well. Some bugs were caused by outdated tests and deeper routed issues that occurred from trying to integrate the 2019 and current codebases. We rewrote tests and had to refactor E1923's code until we were finally to have all tests fixing.
3. Static analysis fixing
As aforementioned, the previous team's project occurred at a time before GitHub Actions verified many aspects of the code using static analysis. This resulted in many, many errors being brought forth when we copied the previous team's code. Most severe were a handful of security vulnerabilities that existed in E1923's code. All the found security vulnerabilities existed in import_file_controller.rb. The first, involved constantize. The error existed because constantize was being run on user provided input, therefore, a malicious actor could provide unintended classes to the import method and receive information about these classes and maybe even have these other classes added to the website's database. To fix this error, we created a simple ALLOWED_MODELS whitelist array that includes the strings of all the permitted class types. We then call constantize on the match one of these strings to avoid the GitHub vulnerability message. This way, only the classes we desire to be imported will actually be allowed by the allowed_model method. The second vulnerability, involved the eval method. Upon further research, this is a terrible method to use in general in Ruby. The eval method will run the code of whatever is inside the string provided to it. While this is powerful, a malicious user can use this to run arbitrary code on Expertiza. The eval method was overkill for what it was really trying to do, parse a hash. We replaced the eval function with JSON.parse() which fixes this vulnerability. After the security vulnerabilities, we had 404 style errors thrown by codeclimate. Many of these changes were simple yet very tedious. For example, many suggested change double quotes to single quotes, removing trailing or excess whitespace, correcting indentation, removing unnecessary .to_s method calls, and adding spaces around the curly braces of hashes. After these changes, only 33 remain. Reviewing the GitHub Actions, we recognize that codeclimate is one the checks that is not required and we would recommend that many of the remaining suggestions are unhelpful and some may be suppressed. For example, the recommended fixes for "Use the new Ruby 1.9 hash syntax" involves changing the rocket (=>) to a colon (:), however, this change in our context will actually cause a compilation error. Furthermore, some of the refactoring suggestions are unnecessary. For example, the branch condition size was determined to high for the get_topic_attributes method in sign_up_topic.rb, however, the method is simply only adding fields if they exist and splitting this code might make it more confusing to read, not less.
Future improvements
Like our preceding team, E1923, we have not touched the exporting code. Exporting could receive a mirroring change to that of importing to allow for a similar sharing of code. Additionally, as mentioned in the static analysis fixing section, there are some codeclimate errors remaining in our final pull request. If the teaching staff believes all of these should be fixed as the automated system describes, this would be a future necessary task, and potentially a good option as a good continuation for our team into Project 4.