[AAP-77215] Add CleanTextMixin to DAB concrete model serializers- #4 - #1118
Open
vidyanambiar wants to merge 10 commits into
Open
[AAP-77215] Add CleanTextMixin to DAB concrete model serializers- #4#1118vidyanambiar wants to merge 10 commits into
vidyanambiar wants to merge 10 commits into
Conversation
Wire CleanTextMixin into the five DAB-owned serializers so that Authenticator, AuthenticatorMap, OAuth2Application, OAuth2AccessToken, and RoleDefinition endpoints reject unsafe text input (Tier 1 name allowlist, Tier 2 dangerous-pattern blocklist) with grandfathering for unchanged values on update. Fix broken super().validate() chains in AuthenticatorSerializer and AuthenticatorMapSerializer so the mixin's validate() is reachable. Set excluded_fields on AuthenticatorMapSerializer for the organization, role, and team fields which legitimately accept template expansion syntax. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> fix: resolve black and isort formatting issues Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Add excluded_json_keys for authenticator configuration sub-keys Exclude encrypted sub-keys (SECRET, BIND_PASSWORD, SP_PRIVATE_KEY) and structured pass-through data (ADDITIONAL_UNVERIFIED_ARGS) from CleanTextMixin's JSONField scan on AuthenticatorSerializer. Non-excluded string sub-keys like NAME are still validated as defense-in-depth. Add code comments on both AuthenticatorSerializer.excluded_json_keys and AuthenticatorMapSerializer.excluded_fields explaining the rationale and referencing the validation doc. Document the full exclusion analysis in docs/lib/validation.md. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> fix: add required organization field to OAuth2Application tests OAuth2Application requires an organization on create. The three failing tests were missing this field, causing a 400 before CleanTextMixin validation could run. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Address code review findings for CleanTextMixin integration tests Add API-level test for AuthenticatorMap (POST with dangerous name asserts HTTP 400). Add team field to excluded_fields test so all three expansion fields are exercised. Add comment explaining why OAuth2Token tests omit the grandfather case. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The CleanTextMixin validation is gated behind this feature flag, so tests expecting HTTP 400 rejections need it enabled via @override_settings to actually trigger validation errors. Co-Authored-By: Claude <noreply@anthropic.com>
@override_settings as a class decorator requires Django SimpleTestCase subclasses. These are plain pytest classes, so use the pytest-django settings fixture via an autouse fixture instead. Co-Authored-By: Claude <noreply@anthropic.com>
…n docs Add test_rejects_changed_invalid_name_on_update to the OAuth2Application and RoleDefinition CleanTextMixin integration tests, mirroring the Authenticator test's coverage of changed-value rejection on update. Clarify docs/lib/validation.md: correct the claim that dict/list JSON sub-keys are skipped by CleanTextMixin (they're recursed into and validated), and document that AuthenticatorMap's organization/role/team exclusion also bypasses validation for literal, non-templated values. Rename test_excluded_fields_accept_dangerous_content to test_excluded_fields_accept_invalid_content for clarity. Assisted-by: Claude Code / Sonnet 5 (Anthropic)
… test and immutable excluded_json_keys Add test_rejects_changed_invalid_name_on_update to TestAuthenticatorMapCleanText, an HTTP-level negative-grandfather test mirroring the one already present for Authenticator, OAuth2Application, and RoleDefinition. The existing AuthenticatorMap grandfather coverage only exercised the serializer in isolation. Wrap AuthenticatorSerializer.excluded_json_keys in MappingProxyType, matching CleanTextMixin's own default, so the shared class attribute can't be mutated in place across serializer instances/requests. Assisted-by: Claude Code / Sonnet 5 (Anthropic)
The docs/lib/validation.md code sample for AuthenticatorSerializer's excluded_json_keys still showed the old plain-dict form after the prior commit switched the actual code to MappingProxyType. Since this sample is the template future plugin authors copy, the stale example would have silently reintroduced the mutable-dict pattern. Assisted-by: Claude Code / Sonnet 5 (Anthropic)
…ields
excluded_fields exempted these CharFields from CleanTextMixin entirely,
so literal dangerous values (not just template-expansion syntax) bypassed
validation and flowed unsanitized into Organization/Team creation on
every login. Gate the exemption on has_expansion() instead so only
genuine {% for_attr_value() %} expansion values skip validation.
Also adds a test locking in that PEM-formatted certificate config values
(SP_PUBLIC_CERT, IDP_X509_CERT) still pass Tier 2 validation, and fixes
a tautological assertion in the AuthenticatorMap expansion-field test.
Addresses the code review finding carried across iterations 1-4 of PR ansible#4.
Assisted-by: Claude Code / Sonnet 5 (Anthropic)
…ect comment The comment claimed description is not PATCH-writable, but it is — OAuth2TokenViewSet is a full ModelViewSet and description is not in read_only_fields. Adds HTTP-level grandfather and rejection tests matching the pattern used for the other four models. Addresses PR ansible#4 review finding on test_clean_text_integration.py:77. Co-Authored-By: Claude <noreply@anthropic.com>
The doc previously implied literal content is always validated in
expansion fields, but mixed values containing both literal text and
{% for_attr_value() %} syntax skip validation entirely. This is
intentional per the SDP scope exclusion for Jinja2 template fields.
Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughCleanTextMixin is integrated into authentication, OAuth2, and RBAC serializers. Authenticator configuration exclusions and AuthenticatorMap expansion handling preserve supported structured values. New integration tests cover creation and update validation. ChangesClean text validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The serializers now reject unsafe names and descriptions while preserving supported template fields and unchanged update values; no actionable merge-blocking risk remains after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## devel #1118 +/- ##
=======================================
Coverage 94.79% 94.79%
=======================================
Files 259 259
Lines 14710 14720 +10
Branches 2271 2272 +1
=======================================
+ Hits 13944 13954 +10
Misses 766 766
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
daphnemaeve
approved these changes
Aug 31, 2026
Member
Author
Code Review: AAP-77215 Add CleanTextMixin to DAB concrete model serializersVerdict: READY_FOR_HUMAN_REVIEW No Critical or Major findings. 3 Minor findings, none blocking:
Verification results (MRO/ Generated with AI assistance: Claude Code / Sonnet 5 (Anthropic) |
… test and JSON sub-key comment Add an HTTP-level admin_api_client.patch() grandfather test for AuthenticatorMap, mirroring the pattern used for Authenticator, OAuth2Application, OAuth2AccessToken, and RoleDefinition. The existing test exercised the serializer in isolation only; rename it to test_grandfather_unchanged_name_on_update_at_serializer_level and drop its unused map_serializer fixture parameter. Correct the excluded_json_keys comment on AuthenticatorSerializer: nested dict/list sub-keys are not skipped by CleanTextMixin's JSON scan, they're recursed into and their string leaves are still validated under Tier 2. Assisted-by: Claude Code / Sonnet 5 (Anthropic)
|
DVCS PR Check Results: PR appears valid (JIRA key(s) found) |
|
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
2 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.
Description
Dependencies
This PR builds on top of #1087 (merged), which introduced the CleanTextMixin, Tier 1 name validator, and Tier 2 free-text validator.
JIRA: https://redhat.atlassian.net/browse/AAP-77215
What is being changed?
Wire
CleanTextMixin(#1087) into DAB's five concrete model serializers so that Authenticator, AuthenticatorMap, OAuth2Application, OAuth2AccessToken, andRoleDefinition endpoints reject unsafe text input with HTTP 400.
AuthenticatorSerializer— Tier 1 validation onnameAuthenticatorMapSerializer— Tier 1 onname, withexcluded_fieldsfororganization/role/team(template expansion syntax)OAuth2ApplicationSerializer— Tier 1 onname, Tier 2 ondescriptionOAuth2TokenSerializer— Tier 2 ondescriptionRoleDefinitionSerializer— Tier 1 onname, Tier 2 ondescriptionWhy is this change needed?
Addresses AAP-77215 under the parent epic AAP-74584 (DAB
Input Validation and Sanitization). CAP-1040 identified that free-text API fields accept unsafe input (XSS, shell injection, control characters). Validation is
enforced at the serializer layer so all clients (UI, API, CLI) get consistent rejection.
How does this change address the issue?
CleanTextMixinis placed first in the MRO of each serializer, hooking intovalidate()viasuper()chainingAuthenticatorSerializer,AuthenticatorMapSerializer) had brokensuper().validate()chains (return datainstead ofreturn super().validate(data)) — fixed as a prerequisiteAuthenticatorMapSerializersetsexcluded_fields = frozenset({'organization', 'role', 'team'})because these fields accept{% for_attr_value() %}template expansion syntax
AuthenticatorUpdateSerializer,RoleDefinitionDetailSerializer) inherit the mixin automaticallyType of Change
Self-Review Checklist
Related PRs
ATF Tests
ATF tests: https://gitlab.cee.redhat.com/ansible/testing/platform-services-test-suite/-/merge_requests/188
Testing Instructions
Prerequisites
pip install -e ".[all]")Steps to Test
Manual Testing
Please follow these instructions: https://docs.google.com/document/d/1wfGlKk4BWULIhUB9jPCw_M65NIetfzUHWUQCEWkC-wE/edit?usp=sharing
Tests
Run the existing mixin unit tests:
pytest test_app/tests/lib/serializers/test_clean_text_mixin.py -v
pytest test_app/tests/lib/utils/test_validation.py -v
Run the new integration tests for each app:
pytest test_app/tests/authentication/test_clean_text_integration.py -v
pytest test_app/tests/oauth2_provider/test_clean_text_integration.py -v
pytest test_app/tests/rbac/api/test_clean_text_integration.py -v
Run the existing serializer/view tests to verify no regressions:
pytest test_app/tests/authentication/ -v
pytest test_app/tests/oauth2_provider/ -v
pytest test_app/tests/rbac/ -v
Verify AuthenticatorMap template expansion fields still work:
pytest test_app/tests/authentication/serializers/test_authenticator_map.py::TestAuthenticatorMapEscapeSequence -v
Expected Results
<script>alert(1)</script>) rejected with HTTP 400 on create$(rm -rf /)) rejected with HTTP 400 on create{% for_attr_value() %}) still accepted in organization/role/team fieldsAdditional Context
Dependencies
This PR builds on top of #1087, which introduced the
CleanTextMixin, Tier 1 name validator, andTier 2 free-text validator. That PR must be merged first — the commits from it appear in this branch's history.
Required Actions
Key design decisions
excluded_fieldsfor AuthenticatorMap template fieldsorganization,role,teamaccept{% for_attr_value() %}expansion syntax which matches thesuper().validate()chainsAuthenticatorSerializerandAuthenticatorMapSerializerreturneddatawithout callingsuper().validate(), silentlyModelSerializer.validate()is a passthrough.Summary by CodeRabbit
New Features
Documentation
Tests