Skip to content

ENG-3001: Protect default_oauth_client#7720

Merged
tvandort merged 2 commits intomainfrom
ENG-3001-default-client-fix
Mar 20, 2026
Merged

ENG-3001: Protect default_oauth_client#7720
tvandort merged 2 commits intomainfrom
ENG-3001-default-client-fix

Conversation

@tvandort
Copy link
Copy Markdown
Contributor

@tvandort tvandort commented Mar 20, 2026

The seeded default_oauth_client is a system-internal FK owner for default
policies and should not be visible or modifiable via the OAuth client API.

Ticket ENG-3001

Description Of Changes

Hides a default client that is generated for policies in seed data. Deleting this could cause problems we so want to prevent it from being changed by clients.

Code Changes

  • Filter for default client on OAuth endpoints

Steps to Confirm

  1. calling /api/v1/oauth/client not longer returns any results on a fresh db.

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

The seeded default_oauth_client is a system-internal FK owner for default
policies and should not be visible or modifiable via the OAuth client API.

- Add _is_system_client() predicate covering both root client and default_oauth_client
- Update _get_client_or_error and _get_client_or_none to use the predicate
- Filter default_oauth_client from list_clients results

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@tvandort tvandort requested a review from a team as a code owner March 20, 2026 19:35
@tvandort tvandort requested review from thabofletcher and removed request for a team March 20, 2026 19:35
@vercel
Copy link
Copy Markdown
Contributor

vercel bot commented Mar 20, 2026

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

2 Skipped Deployments
Project Deployment Actions Updated (UTC)
fides-plus-nightly Ignored Ignored Preview Mar 20, 2026 8:52pm
fides-privacy-center Ignored Ignored Mar 20, 2026 8:52pm

Request Review

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 20, 2026

Greptile Summary

This PR protects the seeded default_oauth_client (used as an internal FK owner for default policies) from being visible or modifiable via the OAuth client API by introducing an _is_system_client helper and applying filters across all relevant endpoints. The approach is sound, but there is a critical SQL correctness issue in the list_clients filter.

Key changes:

  • Introduces _is_system_client(client_id, client) to centralize the logic for identifying system-internal clients (root client or default_oauth_client).
  • Refactors _get_client_or_error and _get_client_or_none to use the new helper, ensuring single/direct client lookups also hide the protected client.
  • Adds a .filter(ClientDetail.fides_key != DEFAULT_OAUTH_CLIENT_KEY) to list_clients to exclude the seeded client from paginated listings.

Issue found:

  • The list_clients filter on the nullable fides_key column using != is incorrect SQL: NULL != 'default_oauth_client' evaluates to NULL (unknown) in standard SQL, not TRUE. This means all regular user-created clients — which have fides_key = NULL — will also be excluded from the list, effectively breaking the entire client listing endpoint. The filter needs to explicitly allow NULL values, e.g. using or_(ClientDetail.fides_key.is_(None), ClientDetail.fides_key != DEFAULT_OAUTH_CLIENT_KEY).

Confidence Score: 2/5

  • Not safe to merge — the list_clients endpoint will silently return no user-created clients due to a SQL NULL-comparison bug.
  • The protective intent is correct and the _is_system_client helper is well-structured. However, the fides_key != DEFAULT_OAUTH_CLIENT_KEY filter on a nullable column will exclude all clients where fides_key IS NULL (which includes every regular user-created client), completely breaking the GET /oauth/client listing endpoint. This needs to be fixed before merging.
  • src/fides/api/v1/endpoints/oauth_endpoints.py — specifically the list_clients filter at line 219.

Important Files Changed

Filename Overview
src/fides/api/v1/endpoints/oauth_endpoints.py Adds _is_system_client helper and filters the default OAuth client from all CRUD endpoints. Contains a critical SQL bug in list_clients where filtering a nullable fides_key column with != excludes NULL-keyed (regular) clients from results.

Last reviewed commit: "Protect default_oaut..."

query = (
ClientDetail.query(db=db)
.filter(ClientDetail.id != CONFIG.security.oauth_root_client_id)
.filter(ClientDetail.fides_key != DEFAULT_OAUTH_CLIENT_KEY)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 NULL fides_key clients excluded from list

In SQL, NULL != 'default_oauth_client' evaluates to NULL (unknown), not TRUE. Because fides_key is defined as nullable=True in the ClientDetail model, any user-created client that has no fides_key (i.e., fides_key IS NULL) will fail this filter condition and be silently excluded from the paginated results.

In practice, regular clients created through the create_client endpoint do not set a fides_key, so they will all have fides_key = NULL, meaning this endpoint would return an empty list for all non-system clients — breaking the entire listing functionality.

The fix is to explicitly allow NULL values:

Suggested change
.filter(ClientDetail.fides_key != DEFAULT_OAUTH_CLIENT_KEY)
.filter(or_(ClientDetail.fides_key.is_(None), ClientDetail.fides_key != DEFAULT_OAUTH_CLIENT_KEY))

Note: or_ would also need to be imported from sqlalchemy (it is not currently imported in this file).

@tvandort tvandort enabled auto-merge March 20, 2026 21:04
@tvandort tvandort added this pull request to the merge queue Mar 20, 2026
Merged via the queue into main with commit 3d8d1a6 Mar 20, 2026
56 of 57 checks passed
@tvandort tvandort deleted the ENG-3001-default-client-fix branch March 20, 2026 21:18
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