Skip to content

Conversation

@theiris6
Copy link

@theiris6 theiris6 commented May 12, 2025

Description

This PR addresses a critical vulnerability identified in the security audit report (ref: BAC.md).
The Settings API was exposing sensitive configuration data without proper authentication, which could allow unauthorized users to access system configuration details.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Files Modified:

app/api/settings_api.rb:

  • Added helpers AuthenticationHelpers and helpers AuthorisationHelpers at the top of the class
  • Added authenticated? check to the main /settings endpoint to require authentication
  • Added authenticated? check to the /settings/privacy endpoint to require authentication
  • Created a new /settings/public endpoint that only returns non-sensitive information (externalName)
  • Moved sensitive configuration data (overseerEnabled, tiiEnabled, d2lEnabled) to be accessible only to authenticated users

app/api/api_root.rb:

  • Added AuthenticationHelpers.add_auth_to SettingsApi in the "Add auth details to all endpoints" section

  • This ensures all protected settings endpoints require proper authentication headers

API Changes:

  1. Protected Endpoint -/api/settings:
  • Now requires valid authentication headers (Username and Auth-Token)
  • Returns full settings data including sensitive integration status for authenticated users
  • Returns 419 error with authentication message for unauthenticated requests
  1. New Public Endpoint -/api/settings/public:
  • Accessible without authentication
  • Returns only non-sensitive settings (externalName)
  • Allows frontend to retrieve basic application information without authentication
  1. Protected Endpoint - /api/settings/privacy:
  • Now requires valid authentication headers
  • Contains sensitive privacy policy information that should only be accessible to authenticated users

How Has This Been Tested?

  • Verified unauthenticated requests to /api/settings return proper 419 authentication errors
  • Confirmed authenticated requests to /api/settings return complete configuration data
  • Tested that /api/settings/public is accessible without authentication
  • Validated that only non-sensitive data is returned from the public endpoint
  • Re-ran the security audit test script which now passes for this vulnerability
  • Manually verified the fix addresses the specific issue identified in the security audit report

Security Impact:

This fix prevents unauthorized users from accessing sensitive system configuration details. Previously, an attacker could determine which integrations were enabled (TurnItIn, D2L, Overseer) and potentially use this information to target specific attack vectors. The fix now ensures proper access controls are in place while still allowing the application to function normally.

Resolves security issue identified in vulnerability report BAC.md.

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, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation if appropriate
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have created or extended unit tests to address my new additions
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

TinyServal and others added 30 commits June 24, 2024 21:55
- Implement methods in task_definition model for numbas data management
- Implement routes in task_definition_api for numbas data managemnt
- Remove unused upload API in numbas_api
macite and others added 20 commits January 30, 2025 23:49
- refactor to allow different file roots
- remove need for portfolio evidence attribute, while still working with existing values
Use new environment variable to enable archive - false by default.
Add ability to auto archive units
- use archive folder when unit archived
- move submission history to archive folder when unit archived
- move submission history on task abbr change
- move submission history on username change
- delete submission history on task delete
- action is too slow for use in sidekiq
- removed from scheduled actions
- Modified app/api/settings_api.rb to require authentication for sensitive endpoints
- Created a new public endpoint for non-sensitive settings
- Added authentication requirement to privacy settings endpoint
- Added SettingsApi to the authentication helpers list in app/api/api_root.rb
- Prevents unauthorized access to system configuration
Copy link

@EkamBhullar EkamBhullar left a comment

Choose a reason for hiding this comment

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

Hey @theiris6 , PR looks good, I tried accessing the settings endpoint without authenticating and it gave me error as expected and public endpoint worked properly
image.

The PR addresses a critical security vulnerability and implements the fix in a clean, maintainable way. However, I believe,

-Basic Error Handling can be added in /settings endpoint, in case if there is any loading failure.

Also, do you think authenticated check can also be added in /settings/privacy?

@returnMarcco
Copy link

returnMarcco commented May 20, 2025

Hi @theiris6,

Can you please replace me with somebody else for this PR as at the moment, I'm unable to build the backend for any branch based on the 8.x branch. I've also sent you a private message in Teams.

Cheers

Added authentication check for /settings/privacy endpoint
@theiris6
Copy link
Author

Hi @theiris6,

Can you please replace me with somebody else for this PR as at the moment, I'm unable to build the backend for any branch based on the 8.x branch. I've also sent you a private message in Teams.

Cheers

Thank you for reviewing my PR and testing the fix! I'm glad to see that the implementation is working as expected, with the main settings endpoint properly requiring authentication while the public endpoint is accessible.

Based on your feedback, I've made both suggested improvements:

  1. Added authentication to the privacy settings endpoint.

  2. Implemented error handling for the settings endpoints to improve robustness. The application will now properly catch and log any configuration loading failures, returning an appropriate error message to the user instead of potentially crashing.

Let me know if you have any other suggestions or if these changes look good to you!

Copy link

@EkamBhullar EkamBhullar left a comment

Choose a reason for hiding this comment

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

Great Job on fixing this vulnerability, I have verified, it works properly and you have made Changes requested.

@aNebula
Copy link

aNebula commented Jun 12, 2025

LGTM.
@theiris6 please open an upstream PR with these changes and description against 9.x branch on doubtfire-lms/doubtfire-api.
This will be recorded as contribution in your public github portfolio - which can help during job applications.

@theiris6 theiris6 changed the base branch from app-attack-fixes to 9.x July 18, 2025 07:49
@theiris6 theiris6 changed the base branch from 9.x to app-attack-fixes July 18, 2025 07:49
@theiris6 theiris6 changed the base branch from app-attack-fixes to 9.x July 25, 2025 06:55
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.

7 participants