Replace domain validation boolean with tri-state mode#7611
Merged
Conversation
…/monitor/disabled) Replace FIDES__SECURITY__DISABLE_DOMAIN_VALIDATION boolean with FIDES__SECURITY__DOMAIN_VALIDATION_MODE enum that accepts "enabled", "monitor", or "disabled". Monitor mode validates domains but logs warnings instead of blocking, giving operators a safe way to observe violations before enforcing. Default is "monitor". Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplace the binary domain-validation flag with a tri-state DomainValidationMode (enabled / monitor / disabled); update configuration, domain utilities, SaaS schemas, AuthenticatedClient, and tests to use the mode; validate_value_against_allowed_list gains a mode parameter that logs in monitor mode instead of raising. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Auth as AuthenticatedClient
participant Schema as SaaS_Schema
participant Util as domain_util
Client->>Auth: perform operation needing domain check
Auth->>Auth: read cached domain_validation_mode
Auth->>Schema: validate param / extract allowed hosts
Schema->>Util: get_domain_validation_mode()
Util-->>Schema: DomainValidationMode (enabled/monitor/disabled) rgba(0,128,0,0.5)
Schema->>Util: validate_value_against_allowed_list(value, allowed_values, param_name, mode=domain_mode)
alt mode == enabled
Util->>Schema: raise ValueError on mismatch rgba(255,0,0,0.5)
else mode == monitor
Util->>Schema: log warning on mismatch rgba(255,165,0,0.5)
Schema-->>Auth: continue (no exception)
else mode == disabled
Util-->>Schema: skip validation (no-op) rgba(128,128,128,0.5)
end
Auth-->>Client: proceed or error based on validation outcome
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Greptile SummaryThis PR replaces the Key changes:
Confidence Score: 4/5
Important Files Changed
Last reviewed commit: a815acb |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/ops/service/connectors/saas/test_authenticated_client.py (1)
293-300: Consider adding a test for monitor mode behavior.The tests cover
enabledanddisabledmodes, but there's no explicit test verifying thatmonitormode logs a warning without raising an exception. This would ensure the new logging path works correctly.📝 Example test for monitor mode
`@mock.patch`( "fides.api.service.connectors.saas.authenticated_client.get_domain_validation_mode", return_value=DomainValidationMode.monitor, ) def test_validation_monitor_mode_logs_warning(self, mock_mode, caplog): """Monitor mode should log a warning but not raise.""" client = _make_client_with_allowed_values({"domain": ["api.stripe.com"]}) with caplog.at_level("WARNING"): client._validate_request_domain("evil.example.com") # Should not raise assert "Domain validation violation (monitor mode)" in caplog.text🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ops/service/connectors/saas/test_authenticated_client.py` around lines 293 - 300, Add a new test that asserts monitor mode logs a warning but does not raise: patch get_domain_validation_mode to return DomainValidationMode.monitor, create the client via _make_client_with_allowed_values({"domain": ["api.stripe.com"]}), call client._validate_request_domain("evil.example.com") inside a caplog.at_level("WARNING") context, and assert the warning message (e.g. contains "Domain validation violation (monitor mode)") appears in caplog.text; this verifies the _validate_request_domain path for monitor mode without expecting an exception.tests/ops/schemas/connection_configuration/test_connection_secrets_saas.py (1)
149-153: Add explicitmonitor-mode coverage.These updates only exercise
enabledanddisabled, butmonitoris the new default and has different semantics: allow the value and emit a warning. Without a test here, the migration path can regress silently.Example test to cover the missing path
+import logging + `@patch`( "fides.api.schemas.connection_configuration.connection_secrets_saas.get_domain_validation_mode", return_value=DomainValidationMode.disabled, ) def test_disallowed_domain_passes_when_validation_disabled( self, mock_mode, saas_config_with_allowed_values ): """Domain validation should be skipped when disabled.""" schema = SaaSSchemaFactory(saas_config_with_allowed_values).get_saas_schema() schema.model_validate({"domain": "evil.example.com", "api_key": "sk_test_123"}) + + `@patch`( + "fides.api.schemas.connection_configuration.connection_secrets_saas.get_domain_validation_mode", + return_value=DomainValidationMode.monitor, + ) + def test_disallowed_domain_warns_in_monitor_mode( + self, mock_mode, saas_config_with_allowed_values, caplog + ): + schema = SaaSSchemaFactory(saas_config_with_allowed_values).get_saas_schema() + + with caplog.at_level(logging.WARNING): + schema.model_validate( + {"domain": "evil.example.com", "api_key": "sk_test_123"} + ) + + assert "not in the list of allowed values" in caplog.textAlso applies to: 159-162, 171-175, 182-186, 206-210, 225-229, 248-252
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ops/schemas/connection_configuration/test_connection_secrets_saas.py` around lines 149 - 153, The tests currently patch get_domain_validation_mode only for DomainValidationMode.enabled/disabled; add explicit coverage for the monitor mode by creating parallel test cases (e.g., extend test_allowed_domain_passes and the other affected tests referenced) that patch get_domain_validation_mode to return DomainValidationMode.monitor and assert the behavior: the value is allowed but a warning is emitted (check for a logged warning or returned warning flag as appropriate). Locate occurrences where get_domain_validation_mode is patched and duplicate the test logic for the monitor variant, ensuring assertions verify both acceptance of the value and emission of a warning rather than an error or silence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/ops/schemas/connection_configuration/test_connection_secrets_saas.py`:
- Around line 149-153: The tests currently patch get_domain_validation_mode only
for DomainValidationMode.enabled/disabled; add explicit coverage for the monitor
mode by creating parallel test cases (e.g., extend test_allowed_domain_passes
and the other affected tests referenced) that patch get_domain_validation_mode
to return DomainValidationMode.monitor and assert the behavior: the value is
allowed but a warning is emitted (check for a logged warning or returned warning
flag as appropriate). Locate occurrences where get_domain_validation_mode is
patched and duplicate the test logic for the monitor variant, ensuring
assertions verify both acceptance of the value and emission of a warning rather
than an error or silence.
In `@tests/ops/service/connectors/saas/test_authenticated_client.py`:
- Around line 293-300: Add a new test that asserts monitor mode logs a warning
but does not raise: patch get_domain_validation_mode to return
DomainValidationMode.monitor, create the client via
_make_client_with_allowed_values({"domain": ["api.stripe.com"]}), call
client._validate_request_domain("evil.example.com") inside a
caplog.at_level("WARNING") context, and assert the warning message (e.g.
contains "Domain validation violation (monitor mode)") appears in caplog.text;
this verifies the _validate_request_domain path for monitor mode without
expecting an exception.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82157187-6264-4073-8a27-395ec4351544
📒 Files selected for processing (9)
changelog/7611-domain-validation-monitor-mode.yamlsrc/fides/api/schemas/connection_configuration/connection_secrets_saas.pysrc/fides/api/schemas/saas/saas_config.pysrc/fides/api/service/connectors/saas/authenticated_client.pysrc/fides/api/util/domain_util.pysrc/fides/api/util/saas_util.pysrc/fides/config/security_settings.pytests/ops/schemas/connection_configuration/test_connection_secrets_saas.pytests/ops/service/connectors/saas/test_authenticated_client.py
Contributor
Author
|
Added monitor mode test coverage in both |
- Add monitor mode test coverage in both test files - Move get_domain_validation_mode from saas_util to domain_util to fix circular import Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
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 `@src/fides/api/util/domain_util.py`:
- Around line 13-17: get_domain_validation_mode currently unconditionally
returns DomainValidationMode.monitor when CONFIG.dev_mode is true; change it so
an explicitly configured mode in CONFIG.security.domain_validation_mode takes
precedence. Update get_domain_validation_mode to: if
CONFIG.security.domain_validation_mode is set (not None / explicitly provided)
return that value; otherwise, if CONFIG.dev_mode is true return
DomainValidationMode.monitor; else return CONFIG.security.domain_validation_mode
(or the default). Reference symbols: get_domain_validation_mode,
CONFIG.dev_mode, CONFIG.security.domain_validation_mode,
DomainValidationMode.monitor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac667bf9-beab-4626-afe0-c6ec492228bd
📒 Files selected for processing (7)
src/fides/api/schemas/connection_configuration/connection_secrets_saas.pysrc/fides/api/schemas/saas/saas_config.pysrc/fides/api/service/connectors/saas/authenticated_client.pysrc/fides/api/util/domain_util.pysrc/fides/api/util/saas_util.pytests/ops/schemas/connection_configuration/test_connection_secrets_saas.pytests/ops/service/connectors/saas/test_authenticated_client.py
💤 Files with no reviewable changes (1)
- src/fides/api/util/saas_util.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/ops/service/connectors/saas/test_authenticated_client.py
- src/fides/api/schemas/saas/saas_config.py
Dev mode no longer overrides an explicit disabled setting back to monitor. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
Updated |
Add field validator to lowercase the value before enum coercion, so ENABLED, Monitor, DISABLED etc. all work. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
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 `@src/fides/api/util/domain_util.py`:
- Around line 78-83: Replace the tenant-specific warning: when the code path
using the variable monitor calls logger.warning with violation_msg and a
hardcoded reference to DOMAIN_VALIDATION_MODE, change it to a generic,
non-identifying message that does not include violation_msg or raw
domain/allowlist values and does not mention a specific env value; update the
logger.warning invocation in domain_util.py (the site where monitor and
violation_msg are used) to a short, mode-agnostic message such as "Domain
validation violation detected; details suppressed by current configuration." so
no customer-specific identifiers are written to logs and avoid referencing
get_domain_validation_mode() or hardcoded '=enabled' text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64f6ac20-710b-4a44-8b92-d8ad59f5eb33
📒 Files selected for processing (1)
src/fides/api/util/domain_util.py
There was a problem hiding this comment.
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 `@src/fides/config/security_settings.py`:
- Around line 150-156: The Field for domain_validation_mode currently defaults
to DomainValidationMode.monitor which causes fresh prod installs to be
warning-only; change the default in the domain_validation_mode Field to
DomainValidationMode.enabled so enforcement is the default, and keep any
downgrade-to-monitor behavior limited to the dev-only logic in
get_domain_validation_mode(); ensure you update the Field default
(domain_validation_mode) and do not alter get_domain_validation_mode() except to
rely on it for dev-only overrides.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc57e7f0-d114-4d95-b4cd-263d8610a26e
📒 Files selected for processing (1)
src/fides/config/security_settings.py
Remove inaccurate hint about DOMAIN_VALIDATION_MODE=enabled since dev mode downgrades that to monitor anyway. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Avoids boolean trap — callers now pass the mode directly instead of encoding behavior as a boolean. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/fides/api/schemas/connection_configuration/connection_secrets_saas.py (1)
82-95: Consider hoistingget_domain_validation_mode()outside the loop.The call to
get_domain_validation_mode()is inside thefor name, value in values.items()loop, but the mode is a global configuration that won't change during iteration. Moving it before the loop (e.g., near line 54) avoids redundant calls.♻️ Suggested refactor
`@model_validator`(mode="before") `@classmethod` def required_components_supplied(cls, values: Dict) -> Dict[str, Any]: # type: ignore """Validate that the minimum required components have been supplied.""" # check required components are present required_components = [ name for name, attributes in cls.model_fields.items() if attributes.is_required() ] min_fields_present = all( values.get(component) for component in required_components ) if not min_fields_present: raise ValueError( f"{cls.__name__} must be supplied all of: [{', '.join(required_components)}]." # type: ignore ) + domain_mode = get_domain_validation_mode() + # check the types and values are consistent with the option and multivalue fields for name, value in values.items(): connector_param = cls.get_connector_param(name) if connector_param: # ... existing options/multiselect validation ... param_type = connector_param.get("param_type") allowed_values = connector_param.get("allowed_values") - domain_mode = get_domain_validation_mode() if ( param_type == "endpoint" and allowed_values is not None and len(allowed_values) > 0 and isinstance(value, str) and domain_mode != DomainValidationMode.disabled ): validate_value_against_allowed_list( value, allowed_values, name, mode=domain_mode, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fides/api/schemas/connection_configuration/connection_secrets_saas.py` around lines 82 - 95, Hoist the call to get_domain_validation_mode() out of the per-item loop so it is computed once, then reuse that stored mode inside the loop where validate_value_against_allowed_list(...) is called; specifically, compute mode = get_domain_validation_mode() before iterating over values.items() and replace the in-loop domain_mode references with the cached mode to avoid repeated calls during the for name, value in values.items() iteration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/fides/api/schemas/connection_configuration/connection_secrets_saas.py`:
- Around line 82-95: Hoist the call to get_domain_validation_mode() out of the
per-item loop so it is computed once, then reuse that stored mode inside the
loop where validate_value_against_allowed_list(...) is called; specifically,
compute mode = get_domain_validation_mode() before iterating over values.items()
and replace the in-loop domain_mode references with the cached mode to avoid
repeated calls during the for name, value in values.items() iteration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40827cd7-6241-4211-9148-3c9f3da94241
📒 Files selected for processing (4)
src/fides/api/schemas/connection_configuration/connection_secrets_saas.pysrc/fides/api/schemas/saas/saas_config.pysrc/fides/api/service/connectors/saas/authenticated_client.pysrc/fides/api/util/domain_util.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/fides/api/service/connectors/saas/authenticated_client.py
- src/fides/api/util/domain_util.py
domain_util.py was using the stdlib `logging` module while the rest of
the codebase uses `loguru`, so warnings were silently dropped. Switch to
loguru and use {} placeholder style.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
In enabled mode, the domain validation ValueError was caught by the retry_send decorator and logged as a generic NetworkError. Introduce a DomainValidationError exception with a dedicated DomainBlocked error group so the violation message surfaces in logs instead of "Unknown exception connecting to ...". Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
DomainValidationError was being wrapped in a generic ConnectionException
("Operational Error connecting to ...") which hid the actual violation
from the user. Handle it separately in retry_send to preserve the
message.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- format_value_for_toml now quotes string values whose schema type wasn't resolved (e.g. enum fields like domain_validation_mode) - Remove caplog/capsys assertion from monitor-mode test since loguru doesn't write to either; the test still verifies no exception is raised Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
RobertKeyser
approved these changes
Mar 11, 2026
…llowed_list The function defaults to monitor mode which only logs warnings. Tests expecting DomainValidationError need enabled mode to trigger raises. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Resolve import conflict in authenticated_client.py by keeping all three imports from saas_util. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This function was removed by the branch (replaced by get_domain_validation_mode in domain_util) but reintroduced during the merge with main. Fixes mypy, check_install, and collect-tests CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mfbrown
pushed a commit
that referenced
this pull request
Mar 12, 2026
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ticket ENG-2569
Description Of Changes
Replace the
FIDES__SECURITY__DISABLE_DOMAIN_VALIDATIONboolean env var with a tri-stateFIDES__SECURITY__DOMAIN_VALIDATION_MODEthat acceptsenabled,monitor, ordisabled.In monitor mode, domain violations are logged as warnings instead of blocking requests, giving operators a safe migration path: observe violations first, then flip to
enabledto enforce. The default ismonitor.Dev mode now defaults to
monitorinstead ofdisabled, so developers still see domain violation warnings during local development.Code Changes
src/fides/config/security_settings.py- AddDomainValidationModeenum, replacedisable_domain_validation: boolfield withdomain_validation_mode: DomainValidationModesrc/fides/api/util/saas_util.py- Replaceis_domain_validation_disabled()withget_domain_validation_mode()returning the enum; dev mode maps tomonitorsrc/fides/api/util/domain_util.py- Addmonitorparam tovalidate_value_against_allowed_list(); logs warning instead of raising when monitor=Truesrc/fides/api/service/connectors/saas/authenticated_client.py- Use new mode enum for runtime request domain validationsrc/fides/api/schemas/connection_configuration/connection_secrets_saas.py- Use new mode enum for secret save-time validationsrc/fides/api/schemas/saas/saas_config.py- Use new mode enum for connector template default value validation (lazy import to avoid circular dependency)tests/ops/schemas/connection_configuration/test_connection_secrets_saas.py- Update mocks to useget_domain_validation_mode+ enum valuestests/ops/service/connectors/saas/test_authenticated_client.py- Update mocks to useget_domain_validation_mode+ enum valuesSteps to Confirm
FIDES__SECURITY__DOMAIN_VALIDATION_MODE=monitorand save a disallowed domain on a SaaS connector — should succeed with a warning in the logsFIDES__SECURITY__DOMAIN_VALIDATION_MODE=enabledand repeat — should be rejected with a ValueErrorFIDES__SECURITY__DOMAIN_VALIDATION_MODE=disabledand repeat — should succeed silentlymonitor, violations should log warningsPre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works🤖 Generated with Claude Code
Summary by CodeRabbit
Changed
Tests
Changelog