Skip to content

Conversation

@ibi420
Copy link

@ibi420 ibi420 commented May 7, 2025

This pull request introduces a fix that mitigates malicious code injection in PDF files by disabling JavaScript execution during PDF viewing.

Description

This change prevents the execution of embedded JavaScript in PDF files, thereby reducing the risk of malicious code being triggered when viewing a PDF.
It addresses a security vulnerability where PDFs could be used as an attack vector.

Fixes: [Insert issue number here]

Type of Change

(Select the relevant option(s) and delete the others)

  • 🐞 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 💥 Breaking change (fix or feature that may break existing functionality)
  • 📚 Documentation update

How Has This Been Tested?

The change was tested by loading multiple PDF files (with and without embedded JavaScript) to confirm that no JavaScript is executed during rendering.
Browser console logs and alert behaviors were used to verify JS suppression.

Test Configuration:

  • Latest Chrome
  • Latest Safari
  • Latest Firefox

Developer Checklist

  • Code adheres to project style guidelines
  • Performed self-review and cleaned up the code
  • Added comments in complex sections
  • Updated relevant documentation
  • No new warnings introduced
  • I have requested a review from @returnMarcco and @theiris6

@returnMarcco
Copy link

Hi Ibi,

I can't test and review this PR until the backend changes have been verified to work on your end. Please see the conversation here for more info: thoth-tech/doubtfire-api#65

Cheers

Copy link

@theiris6 theiris6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The frontend security fixes for PDF handling have been successfully implemented.
I've verified that the PDF.js integration in the Doubtfire web component has been properly updated to disable JavaScript execution in the file viewer.
The validation step during uploads correctly rejects files containing embedded JavaScript.
Test passed - approving the frontend changes.

Copy link

@atharv02-git atharv02-git left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ibi420, I have just reviewed your work and here are my reviews:

Test on 9.x:

  1. Firstly I simulated the attack as per the steps suggested in the documentation, I was able to successfully load the malicious pdf on to the 9.x branch and can confirm the vulnerability exist.

  2. Creating a payload:
    Screenshot 2025-05-17 100258

  3. Loading the malicious payload by logging in using student_id:
    Screenshot 2025-05-17 101242

  4. Checking if receive the exact same output after logging in using convenor:
    Screenshot 2025-05-17 101452

  5. The above simulation tells that the vulnerability exists.

After pulling Ibi's feature branch:

Before proceeding further I would like to say, I was sucessfully able to pull both web and api feature branches, and was able to build the work without any new errors. I didn't run web/api independently I ran them together.

  • After rebuilding the app I simulated the exact same tests and I can confirm the changes that Ibi made on frontend by [disableJavascript]="true" tells that disabling js in the form of payload attached in a pdf, doesn't allow browser to execute the malicious payload inside the pdf.
  • Screenshot 2025-05-17 110134

Good work on fixing this vulnerability, for the changes at the backend I will be posting the review on api PR. 🫡👌

Copy link

@aNebula aNebula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
@ibi420 Please open the upstream pull request with these changes against doubtfire-lms/doubtfire-web 9.x branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants