Skip to content
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

CORS Security Patch Fix on files docker-compose.yml and config/application.rb #47

Open
wants to merge 4 commits into
base: development
Choose a base branch
from

Conversation

epineto
Copy link

@epineto epineto commented Dec 10, 2024

Title: "Security Patch Fix: Addressing CORS Vulnerability for OnTrack Application"


Summary:
This PR addresses a Cross-Origin Resource Sharing (CORS) vulnerability in the OnTrack application caused by the use of the Access-Control-Allow-Origin: * header. The current configuration poses significant security risks by allowing unrestricted cross-origin access.


Impacts:

  • Unauthorized Access:
    Any website can interact with the API, potentially leading to data leakage or abuse.
  • Increased Attack Surface:
    Vulnerable to cross-origin attacks and other malicious activities.
  • Compliance Risks:
    Violates security and privacy standards.

Remediation:

  • Preferred Option: Restrict Access-Control-Allow-Origin:
    • Implement a restrictive CORS policy using the DF_ALLOWED_ORIGINS environment variable for flexibility.
    • Update backend configurations in:
      • Docker: /doubtfire-api/docker-compose.yml
      • Rails: /doubtfire-api/config/application.rb

Configuration Updates:

  1. Docker:

    • Add the following environment variable:
      DF_ALLOWED_ORIGINS: "http://localhost:4200,https://example.com"
    • Notes:
      • The DF_ALLOWED_ORIGINS variable must reflect the exact URLs where the OnTrack app will be accessed (e.g., production, staging, or development environments).
      • Failure to update this variable correctly will result in inaccessibility for valid clients.
  2. Rails:

    • Add middleware configuration in application.rb:
      config.middleware.insert_before Warden::Manager, Rack::Cors do
        allow do
          origins ENV['DF_ALLOWED_ORIGINS'].split(',')
          resource '*', headers: :any, methods: %i[get post put delete options]
        end
      end

Files docker-compose.yml and config/application.rb have been updated

@washyking
Copy link

This Pr addresses a big security issue by improving the CORS configuration. Restricting the allowed origin is very wise.
I tested this using a curl request. See image below.
This shows me sending from the accepted 4200 url as you can see its accepted cors

accept

This details me sending with a bad origin url "malicious" as you can see the cors does not show up

fail

Good job on the task.

@epineto
Copy link
Author

epineto commented Dec 11, 2024

Thanks @washyking for conducting tests and also being very diligent!

Copy link

@nodogx nodogx left a comment

Choose a reason for hiding this comment

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

The code logic looks really good. I have tried to get the CORS headers as well for an unauthorised website, which it doesn't give. Which shows it works as intended :)

Unauthorised Website

Screenshot 2024-12-13 at 12 37 35 am

Authorised Website

Headers:

Screenshot 2024-12-13 at 12 36 02 am

@aNebula aNebula self-requested a review December 21, 2024 05:04
@aNebula
Copy link

aNebula commented Dec 21, 2024

Hi @epineto - the changes look good to me, well done. To make lasting contributions, please open upstream pull request with these changes against doubtfire-lsm/doubtfire-api, 8.0.x branch

@epineto
Copy link
Author

epineto commented Dec 21, 2024

Hi @aNebula, sure, will do! Thanks.

@epineto
Copy link
Author

epineto commented Dec 28, 2024

Hi @epineto - the changes look good to me, well done. To make lasting contributions, please open upstream pull request with these changes against doubtfire-lsm/doubtfire-api, 8.0.x branch

Hi @aNebula , PR submitted as requested: doubtfire-lms#452

Thanks, Epi

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