Skip to content

Conversation

@tvandort
Copy link
Contributor

@tvandort tvandort commented Dec 16, 2025

Ticket ENG-2192

Description Of Changes

Adds good practice security headers to Admin UI & Fides API.

Code Changes

Adds the following headers on routes:

  • X-Content-Type-Options
  • Strict-Transport-Security
  • Content-Security-Policy
  • X-Frame-Options

Steps to Confirm

  1. set FIDES__SECURITY__HEADERS_MODE="recommended"
  2. run fides after exporting the admin UI to serve it from port 8080
  3. browse around
  4. check the network panel to see that headers are applied

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
    • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
    • Updates unreleased work already in Changelog, no new entry necessary
  • UX feedback:
    • All UX related changes have been reviewed by a designer
    • No UX review needed
  • Followup issues:
    • Followup issues created
    • No followup issues
  • Database migrations:
    • Ensure that your downrev is up to date with the latest revision on main
    • Ensure that your downgrade() migration is correct and works
      • If a downgrade migration is not possible for this change, please call this out in the PR description!
    • No migrations
  • Documentation:
    • Documentation complete, PR opened in fidesdocs
    • Documentation issue created in fidesdocs
    • If there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
    • No documentation updates required

@vercel
Copy link

vercel bot commented Dec 16, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Review Updated (UTC)
fides-plus-nightly Ignored Ignored Preview Jan 5, 2026 10:16pm
fides-privacy-center Ignored Ignored Jan 5, 2026 10:16pm

@tvandort tvandort force-pushed the ENG-2192-security-headers branch from 1014390 to decc150 Compare December 16, 2025 20:36
@codecov
Copy link

codecov bot commented Dec 17, 2025

Codecov Report

❌ Patch coverage is 85.71429% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.17%. Comparing base (cfbf98a) to head (b2badd6).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
src/fides/api/util/security_headers.py 84.21% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7134      +/-   ##
==========================================
- Coverage   87.17%   87.17%   -0.01%     
==========================================
  Files         535      536       +1     
  Lines       35330    35371      +41     
  Branches     4113     4120       +7     
==========================================
+ Hits        30800    30835      +35     
- Misses       3639     3643       +4     
- Partials      891      893       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tvandort tvandort force-pushed the ENG-2192-security-headers branch 3 times, most recently from ab3f5f4 to ee621ed Compare December 18, 2025 16:29
@tvandort tvandort force-pushed the ENG-2192-security-headers branch from 5d2649e to d9517e8 Compare January 5, 2026 21:05
@tvandort tvandort marked this pull request as ready for review January 5, 2026 21:28
@tvandort tvandort requested a review from a team as a code owner January 5, 2026 21:28
@tvandort tvandort requested review from galvana and removed request for a team January 5, 2026 21:28
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 5, 2026

Greptile Summary

Adds configurable security headers (X-Content-Type-Options, Strict-Transport-Security, Content-Security-Policy, X-Frame-Options) to Fides API responses via new middleware. The feature is controlled by FIDES__SECURITY__HEADERS_MODE config setting (defaults to "none").

Key changes:

  • New SecurityHeadersMiddleware applies headers based on path-matching rules
  • Headers apply to all routes, with CSP/X-Frame-Options excluded from /api/* and /health endpoints
  • Configuration uses literal type union for type-safe mode selection

Critical issues found:

  • The is_exact_match() function has a bug that will crash when regex doesn't match (accessing .string on None)
  • Module-level config evaluation freezes header mode at import time, preventing runtime config changes
  • Regex pattern for excluding API/health endpoints may not work correctly due to missing anchor
  • Test expectations don't align with "exact match" function semantics

Confidence Score: 2/5

  • Not safe to merge - contains critical bugs that will cause runtime failures
  • The implementation has a critical bug in is_exact_match() that will raise AttributeError when paths don't match patterns, and the module-level config evaluation prevents runtime configuration changes. These are blocking issues.
  • src/fides/api/util/security_headers.py requires fixes for the is_exact_match() bug and config evaluation timing, and tests/api/util/test_security_headers.py needs corrected test expectations and integration tests

Important Files Changed

Filename Overview
src/fides/config/security_settings.py Added headers_mode configuration field with safe defaults
src/fides/api/app_setup.py Added middleware registration for security headers
src/fides/api/util/security_headers.py Implemented security headers middleware with pattern matching logic - has a critical regex matching bug
tests/api/util/test_security_headers.py Basic unit tests for helper functions - missing integration tests for actual header application

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

4 files reviewed, 6 comments

Edit Code Review Agent Settings | Greptile


from fides.config import CONFIG

apply_recommended_headers = CONFIG.security.headers_mode == "recommended"
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Module-level config evaluation will freeze the value at import time. If headers_mode changes at runtime, this won't reflect the change. Move this check inside the middleware dispatch method.

Suggested change
apply_recommended_headers = CONFIG.security.headers_mode == "recommended"
# Remove this line - the check should be done at request time in the middleware

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is intended to make sure we don't access the config object on each request.

@tvandort
Copy link
Contributor Author

tvandort commented Jan 5, 2026

@grepileai review again pls

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