Skip to content

Validate allowed_values in ConnectorRunner fixture setup#7577

Merged
Linker44 merged 2 commits intomainfrom
enforce-allowed-values-in-connector-runner
Mar 5, 2026
Merged

Validate allowed_values in ConnectorRunner fixture setup#7577
Linker44 merged 2 commits intomainfrom
enforce-allowed-values-in-connector-runner

Conversation

@Linker44
Copy link
Contributor

@Linker44 Linker44 commented Mar 5, 2026

Description Of Changes

Integration tests using ConnectorRunner were silently bypassing allowed_values validation for SaaS connector secrets. Two issues compounded this:

  1. _connection_config() calls ConnectionConfig.create() directly, which never runs the Pydantic schema validator that enforces allowed_values.
  2. Even if the schema were invoked, FIDES__DEV_MODE=True is set in all docker-compose test environments, which causes is_domain_validation_disabled() to return True and skip validation anyway.

The fix calls validate_value_against_allowed_list directly in _connection_config() before persisting the ConnectionConfig. This bypasses the dev mode gate intentionally — the goal is to catch misconfigured test secrets at fixture setup time with a clear error, not to test the production validation path (which is already covered by unit tests).

Code Changes

  • tests/ops/integration_tests/saas/connector_runner.py - Validate endpoint-type connector params with allowed_values against the provided secrets before creating the ConnectionConfig

Steps to Confirm

  1. Run any integration test that uses ConnectorRunner with a connector that has allowed_values (e.g. Stripe) — test setup should succeed with a valid domain (api.stripe.com)
  2. Temporarily set the Stripe domain secret to an invalid value (e.g. evil.example.com) and confirm the fixture fails immediately with a validation error rather than failing later during the HTTP request

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:
    • No UX review needed
  • Followup issues:
    • No followup issues
  • Database migrations:
    • No migrations
  • Documentation:
    • No documentation updates required

Summary by CodeRabbit

  • Tests
    • Strengthened runtime validation for connector configuration parameters, particularly for endpoint values, to ensure compliance with system requirements and improve configuration reliability.

Integration tests using ConnectorRunner bypassed the SaaS schema
allowed_values validation because _connection_config calls
ConnectionConfig.create directly, skipping the Pydantic model validator.
Additionally, FIDES__DEV_MODE=True in docker-compose disables domain
validation in the schema anyway.

Call validate_value_against_allowed_list directly in _connection_config
so connector fixtures fail fast with a clear error if a secret domain
does not match the connector's allowed_values, regardless of dev mode.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@vercel
Copy link
Contributor

vercel bot commented Mar 5, 2026

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

Project Deployment Actions Updated (UTC)
fides-plus-nightly Ready Ready Preview, Comment Mar 5, 2026 6:46pm
1 Skipped Deployment
Project Deployment Actions Updated (UTC)
fides-privacy-center Ignored Ignored Mar 5, 2026 6:46pm

Request Review

@coderabbitai
Copy link

coderabbitai bot commented Mar 5, 2026

📝 Walkthrough

Walkthrough

Added runtime validation for connector endpoint parameters within the connection config initialization. The enhancement iterates through connector parameters and validates string values of type "endpoint" against their respective allowed values lists using an existing validation function.

Changes

Cohort / File(s) Summary
Connector Parameter Validation
tests/ops/integration_tests/saas/connector_runner.py
Added runtime validation logic to verify connector endpoint parameters against allowed values during connection config initialization.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • Vagoasdf

Poem

🐰 A rabbit hops through code with glee,
Validating endpoints, wild and free,
Eight little lines of logic bright,
Keep those parameters safe and right! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and clearly describes the main change: adding validation for allowed_values in the ConnectorRunner fixture setup, which matches the core objective of the PR.
Description check ✅ Passed The pull request description covers all required sections: description of changes, code changes, steps to confirm, and pre-merge checklist with appropriate checkmarks.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch enforce-allowed-values-in-connector-runner

Comment @coderabbitai help to get the list of available commands and usage tips.

@Linker44 Linker44 self-assigned this Mar 5, 2026
@Linker44 Linker44 marked this pull request as ready for review March 5, 2026 18:20
@Linker44 Linker44 requested a review from a team as a code owner March 5, 2026 18:20
@Linker44 Linker44 requested review from Vagoasdf and galvana and removed request for a team and galvana March 5, 2026 18:20
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 5, 2026

Greptile Summary

This PR adds early allowed_values validation for endpoint-type SaaS connector params in the ConnectorRunner fixture setup, catching misconfigured secrets at fixture creation time instead of silently allowing them through (due to FIDES__DEV_MODE=True bypassing Pydantic schema validation in test environments).

Key observations:

  • The new import validate_value_against_allowed_list is correctly placed at the top of the file (line 47), consistent with the project's conventions.
  • The isinstance(value, str) guard correctly skips validation for None (absent secret) and external reference dicts, matching the design intent.
  • The and allowed_values short-circuit correctly avoids passing None to a function typed as List[str], and is consistent with how validate_value_against_allowed_list itself handles empty/null lists (it returns early for an empty list).
  • The string literal "endpoint" is used consistently throughout the codebase without a dedicated enum, so the new usage is idiomatic.
  • validate_value_against_allowed_list raises a ValueError, which will correctly propagate through the fixture chain to fail test setup with a clear, actionable error message.

Confidence Score: 5/5

  • This PR is safe to merge — it adds a test-only validation guard with no production code changes.
  • The change is confined to a single test helper file, touches only ~7 lines, and calls an already-tested utility (validate_value_against_allowed_list) that is well-covered by existing unit tests. No production logic is modified, no DB schema changes are introduced, and the logic correctly handles all edge cases (missing secrets, non-string values, None vs [] for allowed_values).
  • No files require special attention.

Last reviewed commit: dcfb42d

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/ops/integration_tests/saas/connector_runner.py`:
- Around line 386-387: The current branch in connector_runner.py that handles
endpoint params skips validation when value is not a string; change it to fail
fast: in the block that checks param_type == "endpoint" and allowed_values, if
value is not None and not isinstance(value, str) raise a clear error (ValueError
or TypeError) referencing the param name so invalid types (dict/int) fail
immediately; if value is a string, continue to call
validate_value_against_allowed_list(name, value, allowed_values) as before.
Ensure you update the same conditional that currently uses isinstance(value,
str) and keep references to param_type, allowed_values, name, value, and
validate_value_against_allowed_list so the check is enforced for non-string
endpoint secrets.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 63d264d7-734e-4578-93c6-0af22aafe69d

📥 Commits

Reviewing files that changed from the base of the PR and between e7fa470 and dcfb42d.

📒 Files selected for processing (1)
  • tests/ops/integration_tests/saas/connector_runner.py

@Linker44 Linker44 added the run unsafe ci checks Runs fides-related CI checks that require sensitive credentials label Mar 5, 2026
@Linker44 Linker44 added this pull request to the merge queue Mar 5, 2026
Merged via the queue into main with commit faa52ee Mar 5, 2026
57 of 58 checks passed
@Linker44 Linker44 deleted the enforce-allowed-values-in-connector-runner branch March 5, 2026 19:57
mfbrown pushed a commit that referenced this pull request Mar 12, 2026
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run unsafe ci checks Runs fides-related CI checks that require sensitive credentials

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants