Skip to content

Conversation

@atharv02-git
Copy link

Base Branch: 8.0.x

Note: Making a PR to thoth-tech/doubtfire-deploy branch only for peer review purpose.

Description

  • This PR addresses Clickjacking and potential XSS vulnerabilities by adding appropriate security headers to the proxy-nginx.conf file used in production. These headers are now enforced at the outer reverse proxy layer (doubtfire-deploy) to ensure consistent protection across all services.
  • The changes follow recommendations from the AppAttack x OnTrack vulnerability report and align with security best practices for modern web applications.

Note

Kindly go through the attached documentation first inorder to understand what this fix is about in detail and how it can be tested.

What was changed:

  • Modified: production/shared-files/proxy-nginx.conf

Fixes # (Clickjacking vulnerability (AppAttack finding))

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • This change requires a documentation update

How Has This Been Tested?

  • Used browser DevTools to inspect headers returned for application responses
  • Yet to test Clickjacking Prevention in a Malicious <Iframe> Setup as listed in the report.

Testing Checklist:

  • Tested in latest Chrome
  • Needs to be tested inside a dedicated environment like kali linux inside a virtual box.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings @b0ink, @ibi420, and Iris

@atharv02-git atharv02-git changed the base branch from main to 8.0.x May 6, 2025 03:25
Copy link

@ibi420 ibi420 left a comment

Choose a reason for hiding this comment

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

This directly fixes the clickjacking vulnerability. From my test following the methods outlined in the documentation from Appattack, I was able to replicate this attack and confirm that this fix effectively stops the attack.

Copy link

@lachlan-robinson lachlan-robinson left a comment

Choose a reason for hiding this comment

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

Hey @atharv02-git, well done addressing the clickjacking vulnerability.

  • X-Frame-Options: DENY prevents the page from being framed by any site, which will mitigate clickjacking attacks as intended.
  • frame-ancestors 'none' is a more modern approach which addresses the issue as well.
  • Overall, you have addressed the clickjacking concern here with redundancy which is good.

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.
Please open an upstream PR against 9.x branch of doubtfire-deploy .
In the description add reference of the upstream of thoth-tech/doubtfire-web#322 and vice-versa.

@atharv02-git
Copy link
Author

LGTM. Please open an upstream PR against 9.x branch of doubtfire-deploy . In the description add reference of the upstream of thoth-tech/doubtfire-web#322 and vice-versa.

LGTM. Please open an upstream PR against 9.x branch of doubtfire-deploy . In the description add reference of the upstream of thoth-tech/doubtfire-web#322 and vice-versa.

Hi @aNebula,
I’ve opened the upstream Pull Request to the doubtfire-lms/doubtfire-deploy repository (9.x branch) as requested.
The PR description includes a reference to the related upstream PR for thoth-tech/doubtfire-web#322, as well as a cross-link back to the old deploy PR.

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.

4 participants