Skip to content

Conversation

@atharv02-git
Copy link

Base Branch: 8.0.x

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

Description

This PR ensures that the internal nginx.conf inside doubtfire-web does not override the security headers (e.g., X-Frame-Options, Content-Security-Policy) that are now being enforced via the outer proxy-nginx.conf file in the doubtfire-deploy repository.

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:

  • Commented out redundant security headers from doubtfire-web/nginx.conf to prevent conflict or override with headers applied at the reverse proxy layer (proxy-nginx.conf).
  • This avoids duplication and ensures centralized management of security headers at the proxy level for consistency across services.

Fixes # (Header override issues caused by multiple NGINX layers)

Type of change

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

How Has This Been Tested?

  • Validated that headers set in proxy-nginx.conf (doubtfire-deploy) reflect in browser response
  • Confirmed no duplication or override from doubtfire-web/nginx.conf
  • Ensured static files are still served correctly via inner NGINX
  • 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
  • I have requested a peer review for this change to @b0ink, @ibi420, and Iris

@atharv02-git
Copy link
Author

I have accidentally committed and pushed my package.json files as well to origin and because of that it says 3 files changed instead of actual 1 file.

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,

  • This change effectively prevents conflicting security policies from overlapping between the proxy-nginx in doubtfire-deploy.
  • You can push a commit that reverts package-lock.json and package.json to their original state to remove their changes from this PR.

@ibi420
Copy link

ibi420 commented May 16, 2025

Hello @atharv02-git, I have reviewed these changes, and I can confirm that the headers set successfully reflect in the browser's response and there is no duplication or override. The clickjacking vulnerability has been patched. I also noticed the inclusion of the package.json and package-lock.json files. I believe the inclusion of the package.json file with its current modifications will make it easier for people to build their containers on Windows, but the package-lock.json file will cause conflicts. I recommend the removal of the package-lock.json file. Thank you for the opportunity to review your work

@atharv02-git atharv02-git force-pushed the fix/clickjacking-vulnerability-fix branch from 77df952 to 0bc4513 Compare May 16, 2025 14:03
@atharv02-git
Copy link
Author

Hi @lachlan-robinson and @ibi420, in-order not to create any conflict I updated the package.json to it's original state and pushed the commit, and deleted package-lock.json in order to resolve conflicts

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.

Hi @atharv02-git,
Thanks for addressing the header duplication issue and updating the nginx.conf files. I’ve reviewed the changes and confirmed that the headers are now served correctly without conflict. The clickjacking protection is working as expected.
Also appreciate you resolving the package file conflict—this is now ready to go.
Approved.

@aNebula
Copy link

aNebula commented Jun 18, 2025

@atharv02-git you have deleted the package-lock.json from the repository - this deletes the file.
I am happy for you to open upstream PR against doubtfire-lms/doubtfire-web 9.x branch with just the nginx.conf file - do not include deletion - may be just create a new fork of 9.x and create upstream PR with that change in nginx.conf.
Add reference of your upstream doubtfire-deploy PR in the description of the upstream web PR and vice-versa.

@atharv02-git
Copy link
Author

@atharv02-git you have deleted the package-lock.json from the repository - this deletes the file.
I am happy for you to open upstream PR against doubtfire-lms/doubtfire-web 9.x branch with just the nginx.conf file - do not include deletion - may be just create a new fork of 9.x and create upstream PR with that change in nginx.conf.
Add reference of your upstream doubtfire-deploy PR in the description of the upstream web PR and vice-versa.

Hi @aNebula,

I’ve opened the upstream Pull Request to the doubtfire-lms/doubtfire-web repository (9.x branch) with only the intended changes including nginx.conf only and no deletions like package-lock.json have been included.
The PR description also references the related upstream deploy PR (doubtfire-lms/doubtfire-deploy#31) for cross-context, as advised.

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