Skip to content

Conversation

@theiris6
Copy link

@theiris6 theiris6 commented Aug 15, 2025

Description

Upstream PR: thoth-tech#67
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

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

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.

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

If you have any questions, please contact @macite or @jakerenzella.

macite and others added 3 commits April 4, 2025 21:47
- 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
Added authentication check for /settings/privacy endpoint
@macite
Copy link
Member

macite commented Aug 15, 2025

Why are these settings considered sensitive?

@macite macite closed this Aug 15, 2025
@theiris6
Copy link
Author

Why are these settings considered sensitive?

I marked this as a vulnerability because the previous behavior exposed internal system configuration data to unauthenticated users. The /api/settings endpoint allowed anyone to see which integrations (TurnItIn, D2L, Overseer, etc.) were enabled.

This is considered sensitive because:

Attack Surface Discovery: Knowing which integrations are enabled gives an attacker reconnaissance information (e.g., which third-party services to target or spoof).

Exploitation Risk: An attacker could craft targeted phishing or exploit known vulnerabilities in those integrations based on this info.

Security Best Practice: According to OWASP’s API Security Top 10 : API3:2023 Excessive Data Exposure
, exposing unnecessary internal data to unauthenticated users is a security risk because it aids attackers even if the data seems noncritical.

So while the fix also resolves a functional bug (unauthorized access), it directly mitigates an information disclosure vulnerability that was flagged in the security audit. That’s why I classified it as a security sensitive change.

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.

2 participants