Skip to content

[AAP-78694] Add CleanTextMixin to settings serializer - #16661

Open
prat98 wants to merge 1 commit into
ansible:develfrom
prat98:AAP-78694-extension-add-cleantextmixin-to-settings
Open

prat98 wants to merge 1 commit into
ansible:develfrom
prat98:AAP-78694-extension-add-cleantextmixin-to-settings

Conversation

@prat98

@prat98 prat98 commented Sep 16, 2026

Copy link
Copy Markdown
Contributor
SUMMARY

Apply django-ansible-base CleanTextMixin validation to settings category PATCH/PUT via SettingSingletonSerializer, closing a gap where dynamic settings fields were not covered by the same unsafe-text checks used elsewhere in the API.

  • Mix in PlainSerializerCleanTextMixin on SettingSingletonSerializer so top-level CharField settings from the settings registry are validated when CLEAN_TEXT_ENABLED is on.
  • Add minimal Meta.model stand-ins (_SettingSingletonFakeModel / _SettingSingletonFakeOpts) so audit logging and OPTIONS metadata (get_field_info) have app_label / object_name without introspecting a real Django model.
  • Exclude fields that must accept non-cleaned content: CUSTOM_LOGIN_INFO (HTML), CUSTOM_LOGO (data: URIs), and all encrypted settings (passwords, tokens, PEMs).
  • Refactor validate() so category / global registry validators run first, then super().validate() chains into CleanTextMixin (previously category all validation returned early and skipped the mixin).

Known limitation: PlainSerializerCleanTextMixin only discovers top-level CharField settings; string values nested in ListField / DictField settings are not validated by this path.

Related JIRA: AAP-78694

ISSUE TYPE
  • New or Enhanced Feature
COMPONENT NAME
  • API
STEPS TO REPRODUCE AND EXTRA INFO

With clean text enforcement enabled (ansible_base.lib.serializers.mixins.get_setting returning true for the clean-text flag), PATCH a registered CharField setting under a category singleton endpoint with unsafe markup (e.g. <script>x</script>). Before this change the request could succeed; after, the API returns 400 with field errors.

Encrypted settings and CUSTOM_LOGIN_INFO remain unchanged when sent values that would fail clean-text rules.

Functional tests added in awx/conf/tests/functional/test_api.py:

  • test_setting_singleton_rejects_unsafe_char_field
  • test_setting_singleton_skips_encrypted_fields
  • test_setting_singleton_skips_custom_login_info_html
pytest awx/conf/tests/functional/test_api.py -k "setting_singleton_rejects_unsafe or setting_singleton_skips"

Summary by CodeRabbit

  • Bug Fixes
    • Settings now consistently validate text input and reject unsafe markup when clean-text enforcement is enabled.
    • Encrypted settings continue to preserve their values without being incorrectly rejected by clean-text validation.
    • Custom login information continues to support HTML content as intended.
    • Validation errors are now returned clearly for settings containing disallowed unsafe content.

@coderabbitai

coderabbitai Bot commented Sep 16, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: f9a0756b-ed5f-45fd-b6bf-7776bfccaaf2

📥 Commits

Reviewing files that changed from the base of the PR and between 4c6526d and eb619b7.

📒 Files selected for processing (1)
  • awx/conf/tests/functional/test_api.py

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

SettingSingletonSerializer now applies clean-text processing to eligible settings, excludes encrypted and custom login/logo settings, and continues parent validation. Functional tests cover rejection of unsafe regular values and preservation of excluded values.

Changes

Setting clean-text validation

Layer / File(s) Summary
Serializer clean-text integration
awx/conf/serializers.py
SettingSingletonSerializer uses PlainSerializerCleanTextMixin with fake model metadata. Login/logo settings and registry-marked encrypted settings are excluded. Validation now always calls the parent serializer.
Clean-text behavior coverage
awx/conf/tests/functional/test_api.py
Tests enable clean-text enforcement and cover unsafe regular values, encrypted settings, and CUSTOM_LOGIN_INFO.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Feature

Merge Risk: ⚪ Minimal · up to eb619

The change adds the intended clean-text enforcement for eligible settings while preserving documented exclusions, with no established merge blocker.

🚥 Pre-merge checks | ✅ 4 | ❌ 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%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding CleanTextMixin validation to the settings serializer.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@prat98 prat98 changed the title [AAP-78694] Add cleantextmixin to settings serializer [AAP-78694] Add CleanTextMixin to settings serializer Sep 16, 2026
@prat98
prat98 force-pushed the AAP-78694-extension-add-cleantextmixin-to-settings branch from 4c6526d to eb619b7 Compare September 16, 2026 21:52
@sonarqubecloud

Copy link
Copy Markdown

@aap-pde-ci-bot

Copy link
Copy Markdown

❌ Test Results - FAILED

Summary

Metric Count
Total Tests 612
✅ Passed 557
❌ Failed 4
⚠️ Errors 1
⏭️ Skipped 50
⏱️ Duration 2608.86s

Pass Rate: 91.0%

❌ Failed Tests

Test Class
test_constructed_inventory_basic_aggregation opt.test-suite.tests.inventories.test_constructed_control_plane.TestConstructedInventoryControlPlane
test_constructed_inventory_basic_aggregation opt.test-suite.tests.inventories.test_constructed_control_plane.TestConstructedInventoryControlPlane
test_jt_prompts_for_labels[1-True] opt.test-suite.tests.prompts.test_prompts_labels.TestPromptLabels
test_user_can_delete_projects_with_role[Project Admin-204-Controller] opt.test-suite.tests.rbac.test_projects.TestControllerProjectsRBAC
test_project_sync_with_galaxy_disabled opt.test-suite.tests.test_projects.TestProjectGalaxyInstall

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants