-
Notifications
You must be signed in to change notification settings - Fork 120
Fix/malicious code execution #65
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
base: 9.x
Are you sure you want to change the base?
Conversation
|
Hi Ibi, When attempting to test these changes, I have been unsuccessful in being to upload any file, including PDF file types. I tested on the Can you please test whether you are able to upload files on the Cheers |
|
@returnMarcco I have gone through it and reset my dev environment, but everything is working as expected. In order for me to help you discover the root cause of this issue, I would suggest that you look through the development logs of the back end as you start to upload a PDF. I have added loggers to the script to track each step of the sanitisation process. You should have something like this. |
|
Hey @ibi420, Thanks for the response. As mentioned in our private discussion, I will attempt to test your changes again as soon as possible and keep you updated here. Cheers |
|
Hi ibi, To try rectify the issue, I've completed the following with no success:
I'll provide a snippet of the logs that are generated when attempting to upload a PDF file via the comments section of the unit you stipulated in your walkthrough video (Object Oriented Programming - Assignment 4):
I'm going to let Nebula know that for now, I'm not able to progress on this review for now. I'm interested to see if the other reviewer for this PR finds success. Please keep me updated with any findings, and I'm always ready to get back to this review. Cheers |
theiris6
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backend security implementation for PDF handling passes all tests.
-
The server-side validation and sanitization mechanisms successfully detect and reject malicious PDFs. I've confirmed the API component properly implements the scanning for JavaScript and other malicious content embedded in uploaded files.

-
The server-side security layer provides robust protection against the identified vulnerability.
It's approved. Good work and thanks for the opportunity to review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello again @ibi420, as per my previous review on to the doubtfire-web, I can confirm the fix has been made, now let's move on to the backend fix, here are my reviews:
- The function
def sanitize_pdf()inisdefile_helper.rbwhich you created, clearly states that any malicious code injected in the form of a pdf is being sanitised and the payload free pdf is now being sent on OnTrack. - I was also able to confirm the test_cases for the function you wrote can be seen inside the development.log:

I can tell another vulnerability has been patched successfully, keep up the great work 🫡
|
LGTM. |


This pull request addresses the issue of malicious code execution in PDF files.
No new dependencies have been introduced, and this change is non-breaking. It is compatible with version 9.x.
Type of Change
How Has This Been Tested?
This fix was tested by creating a poisoned PDF and uploading it to Ontrack.
For a demo, see the video run-through:
Video demo
Developer Checklist