mirror of
https://github.com/ansible/awx.git
synced 2026-10-29 22:03:20 -02:30
Pratyush BhandariandCursor
9fdfc95d37
* AAP-78694 - Add CleanTextMixin to AWX resource serializers Applies django-ansible-base's CleanTextMixin to the AWX serializers listed in AAP-78694: Organization, User, Team, Credential(+Create), CredentialType, Project, JobTemplate, WorkflowJobTemplate, Inventory, Host, Group, InventorySource, InstanceGroup, Instance, ExecutionEnvironment, NotificationTemplate, Schedule, AdHocCommand, WorkflowJobTemplateNode, WorkflowJobTemplateNodeCreateApproval, JobLaunch, and WorkflowJobLaunch. Adds a local PromptFieldCleanTextMixin for serializers whose model derives from LaunchTimeConfigBase, where scm_branch/limit/job_tags/ skip_tags are NullablePromptPseudoField descriptors invisible to the mixin's normal model-field auto-discovery. Per-serializer excluded_fields/name_fields overrides are documented inline where a field's existing behavior would otherwise conflict with Tier 1/2 validation (Jinja2-templated fields, secret fields, and existing colon-containing name/hostname syntax). Signed-off-by: Pratyush Bhandari <prbhanda@redhat.com> * AAP-78694 - Cover CleanTextMixin gaps in Copy, Bulk, and Job Detail paths Three write paths were found to bypass the Tier 1/2 validation applied elsewhere for the same resources: - CopySerializer (used by the Copy button/endpoint for EE, Project, Credential, JobTemplate, WorkflowJobTemplate, NotificationTemplate) accepted a user-supplied name with zero validation. Adds a new PlainSerializerCleanTextMixin for plain serializers.Serializer subclasses with no backing model. - BulkJobNodeSerializer's validate() intentionally skips LaunchConfigurationBaseSerializer.validate() (it assumes unified_job_template/inventory/execution_environment are model instances, but bulk nodes pass raw PKs to avoid a per-node DB lookup), which meant it also skipped CleanTextMixin. Adds PromptFieldCleanTextMixin._run_clean_text_validation() so the Tier 1/2 checks still run despite that skip. BulkJobLaunchSerializer and WorkflowJobNodeSerializer are also covered. - JobSerializer (backing the narrow JobDetail PUT/PATCH window while a job's status is "new") had no coverage at all. Confirmed live against a running controller that a Copy with a malicious name previously succeeded (201) where the equivalent PATCH on the same resource was already rejected (400); both are rejected consistently after this change. Signed-off-by: Pratyush Bhandari <prbhanda@redhat.com> * AAP-78694 - Fix NotificationTemplateSerializer validate() bypass validate() returned early without calling super().validate() whenever notification_configuration was absent from the request (e.g. a PATCH that only changes name/description), which skipped CleanTextMixin entirely on that path. Same bug class as the BulkJobNodeSerializer MRO-skip already fixed, caught by code review. Confirmed live: a PATCH with only a malicious name (no notification_configuration) previously succeeded; now rejected with 400, while a legitimate name-only PATCH still succeeds. Signed-off-by: Pratyush Bhandari <prbhanda@redhat.com> * AAP-78694 - Add CleanTextMixin to the previously out-of-scope API-only serializers LabelSerializer, CredentialInputSourceSerializer, and WorkflowApprovalTemplateSerializer were originally scoped out ("no UI exposure") but are now in scope per updated ticket guidance. WorkflowApprovalTemplateSerializer backs the update path for the same model WorkflowJobTemplateNodeCreateApprovalSerializer already protects on create -- without this, an approval name rejected at creation could be rewritten to anything via PATCH. Confirmed live: Label create and WorkflowApprovalTemplate update both reject a malicious name (400) and accept a valid one. CredentialInputSourceSerializer verified at the code level only (plain CharField input_field_name, no custom validators; metadata is a JSONSchemaField holding vault paths/keys, not secret values) -- setting up a live external-credential-type chain in this environment wasn't done. Signed-off-by: Pratyush Bhandari <prbhanda@redhat.com> * AAP-78694 - Correct HostSerializer name_fields demotion comment The comment claimed the Tier 1 -> Tier 2 demotion was needed for "hostname:port" syntax, but that colon is stripped from attrs['name'] in validate() before it reaches CleanTextMixin -- that case never needed the demotion. What actually needs it: names with any colon count other than exactly 1, which _get_host_port_from_name passes through untouched -- most notably IPv6 addresses, which its own comment says it explicitly does not parse. Caught by code review. Confirmed live: an IPv6-style host name (2 colons) is accepted unmodified (201). Signed-off-by: Pratyush Bhandari <prbhanda@redhat.com> * AAP-78694 - Validate non-secret notification_configuration keys Replaced the blanket notification_configuration exclusion with a precise excluded_json_keys covering only the actual secret fields. Unlike Credential.inputs (admin-definable custom types with arbitrary secret fields), notification_type is a fixed models.CharField(choices) -- every backend class ships in awx/main/notifications/ and is known at code-authoring time, so the secret key set can be a static list rather than resolved per-instance. Enumerated every init_parameters entry with type "password" across all 8 backends (email/irc/webhook password, slack/pagerduty token, twilio account_token, grafana grafana_key, awssns aws_secret_access_key/aws_session_token) and confirmed none of those names are used for a non-secret value in a different backend. Previously-unvalidated, UI-exposed fields like webhook url and slack channels are now covered. Confirmed live: a javascript: webhook url and a <script>-tagged slack channel are now rejected (400), while a weird-looking token/password value is still accepted unchanged. Signed-off-by: Pratyush Bhandari <prbhanda@redhat.com> * AAP-78694 - Exclude extra_data on launch-config serializers extra_data (WorkflowJobTemplateNode/WorkflowJobNode/Schedule) plays the same role as extra_vars elsewhere and is submitted in the same two formats -- a dict, or a raw YAML/JSON string (parse_yaml_or_json handles both). Its model field is JSONBlob, whose get_internal_type() reports "TextField" for legacy DB-migration reasons, so CleanTextMixin silently skipped dict payloads (not a str) while string payloads hit Tier 2's template-injection blocklist and incorrectly rejected legitimate Jinja variable references -- a real regression once enforcement is on, caught by code review. First attempt put the exclusion on the shared LaunchConfigurationBaseSerializer base and silently did nothing: CleanTextMixin's own excluded_fields = frozenset() default sits earlier in the MRO than that base class for every subclass (mixin is first in the bases tuple), so the base class's value was always shadowed. Verified with a standalone MRO test before settling on setting excluded_fields directly on each leaf serializer (WorkflowJobTemplateNodeSerializer, WorkflowJobNodeSerializer, ScheduleSerializer), matching every other exclusion in this file. Confirmed live on both WorkflowJobTemplateNodeSerializer and ScheduleSerializer: a YAML-string extra_data with a Jinja reference is accepted (previously 400), while other Tier 1/2 fields on the same serializers are still rejected as expected. Signed-off-by: Pratyush Bhandari <prbhanda@redhat.com> * AAP-78694 - Validate non-secret Credential.inputs keys dynamically Unlike NotificationTemplate.notification_configuration (fixed set of backend classes, static excluded_json_keys works), CredentialType is admin-definable via a real "Create a new custom credential type" endpoint, so the set of secret field names in Credential.inputs can't be known at class-definition time -- it varies per credential's own type, via that type's own inputs schema ("secret": true flags, exposed as CredentialType.secret_fields). validate() resolves the credential_type from attrs (create) or the existing instance (update) and sets excluded_json_keys as an instance attribute before chaining into CleanTextMixin.validate(). Setting an instance attribute rather than a class attribute sidesteps the MRO trap found while fixing extra_data on launch-config serializers: CleanTextMixin's own excluded_fields/excluded_json_keys class-level defaults sit earlier in the MRO than any shared base class further down the chain, silently shadowing it -- an instance attribute always wins regardless of MRO position, and correctly threads through CredentialSerializerCreate and its User/Team/Organization subclasses for free, since each of their validate() overrides chains through super() into this one. Falls back to excluding the whole inputs blob (prior, safe behavior) if credential_type can't be resolved. Confirmed live: malicious username (non-secret, including via PATCH with no credential_type in the payload) is rejected; a dangerous- looking password value and a real unencrypted SSH private key (secret fields) are both still accepted unchanged. Signed-off-by: Pratyush Bhandari <prbhanda@redhat.com> * Increase test coverage * Cover remaining serializer branches for Sonar gate Line coverage is already over 90%; Sonar's quality gate uses combined line+branch coverage. These unit tests exercise previously untested methods so the PR can clear the 90% new-code threshold. Signed-off-by: Pratyush Bhandari <prbhanda@redhat.com> * style(api): format uncovered serializer tests for ruff Signed-off-by: Pratyush Bhandari <prbhanda@redhat.com> Assisted-by: Cursor Grok 4.6 (SpaceXAI / Cursor) Co-authored-by: Cursor <cursoragent@cursor.com> * test(api): set Mock.name as an attribute, not a constructor kwarg unittest.mock.Mock(name=...) only sets the mock's repr name, so getattr(obj, 'name') returned another Mock and four coverage tests failed in api-test. Signed-off-by: Pratyush Bhandari <prbhanda@redhat.com> Assisted-by: Cursor Grok 4.6 (SpaceXAI / Cursor) Co-authored-by: Cursor <cursoragent@cursor.com> * test(api): keep a single call inside pytest.raises blocks Sonar python:S5918 flags exception tests that construct mocks or serializers inside the raises context. Signed-off-by: Pratyush Bhandari <prbhanda@redhat.com> Assisted-by: Cursor Grok 4.6 (SpaceXAI / Cursor) Co-authored-by: Cursor <cursoragent@cursor.com> * Add more test coverage --------- Signed-off-by: Pratyush Bhandari <prbhanda@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>