Remove pickle from forms and Celery serializer#14791
Merged
Conversation
Pickle deserialization on attacker-controllable bytes is arbitrary code execution. Two sites used pickle: - The survey choice-question form pickled/unpickled a list of strings through MultiExampleField.compress / MultiWidgetBasic.decompress and pickle.loads in survey/views.py. Switched to json — the data is just a list of up to 6 strings. - Celery defaulted to the pickle serializer with CELERY_ACCEPT_CONTENT including pickle/yaml/msgpack so dispatch sites could pass Django model instances and a Dojo_User on the wire. Made every task argument JSON-serializable: async_delete_task takes (model_label, pk) and refetches; SLA recompute takes (sla_config_id, product_ids); per-channel notification sends run inline inside the surrounding async_create_notification task instead of dispatching an inner Celery task with model instances; user context is injected as async_user_id and resolved in the worker. Flipped DD_CELERY_TASK_SERIALIZER default to json, tightened CELERY_ACCEPT_CONTENT to ["json"], and added CELERY_RESULT_SERIALIZER = "json". Added unittests/test_no_pickle.py as a regression net (asserts no pickle imports in dojo/ and that the Celery serializer settings stay JSON-only) and unittests/test_survey_forms.py for the widget round-trip. Operator note: workers running this version reject in-flight pickled messages with ContentDisallowed. Drain the broker (\`celery -A dojo purge -f\`) or scale workers to zero before deploy. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
settings_changes
Contributor
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Maffooch
changed the title
Resolved conflicts after dev's notifications module reorganization: - dojo/tasks.py: accepted dev's re-export module (Celery tasks moved to dojo/notifications/tasks.py). - dojo/notifications/helper.py: kept dev's import structure, retained the per-channel `.run()` synchronous calls so model instances no longer cross the wire as task arguments. - dojo/notifications/tasks.py: applied the add_alerts signature change here (drop runinterval positional arg, inline timedelta). - unittests/test_survey_forms.py: dropped the per-file pickle guard to satisfy ruff PLC0415; the broader guard in test_no_pickle.py already covers the entire dojo/ tree. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
The async_user_id user resolution and refetch in async_delete_task / SLA recompute add 1-6 queries per scenario; CI auto-generated the new expected counts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Before the pickle removal, the SLA recompute task received the SLA_Configuration / Product instances by reference (Celery's sync .apply() does not serialize). The inner update function set async_updating=False on those shared instances, so the dispatcher's local self.async_updating ended up False as well. After switching the dispatch to pass IDs and refetch in the task, the inner function only resets async_updating on its refetched copies. The dispatcher's in-memory self.async_updating stayed True, so a subsequent save() on the same instance triggered the lock-revert path at SLA_Configuration.save() line 1058 and overwrote the caller's field changes (e.g. enforce_critical) with the DB values. Manifested as test_sla_expiration_date_after_sla_not_enforced failing: sla_config.enforce_critical=False was reverted to True on save. Reset async_updating on the in-memory caller instances after dispatch returns to keep them consistent with the post-task DB state. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
blakeaowens
approved these changes
May 4, 2026
Collaborator
|
Wrapped each of the four per-channel .run() calls (Slack, MSTeams, Mail, Webhooks) in try/except Exception with logger.exception(...) for the failed channel. A failure in one channel will now log a traceback and let the remaining channels proceed, matching the prior behavior. |
rossops
approved these changes
May 4, 2026
paulOsinski
approved these changes
May 4, 2026
Contributor
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Contributor
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
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
5 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.
Summary
MultiExampleField/MultiWidgetBasic) anddojo/survey/views.py— the payload is a list of up to 6 strings, JSON-native.async_delete_tasktakes(model_label, pk)and refetches; SLA recompute takes(sla_config_id, product_ids); per-channel notification sends run inline inside the surroundingasync_create_notificationtask instead of dispatching an inner Celery task with model instances; user context is injected asasync_user_idand resolved in the worker viaDojo_User.objects.filter(pk=...).first().DD_CELERY_TASK_SERIALIZERdefault tojson, tightenCELERY_ACCEPT_CONTENTto["json"](drops pickle/msgpack/yaml), and pinCELERY_RESULT_SERIALIZER = "json". Drop the now-unneededtimedeltaarg from theadd-alertsbeat schedule entry.unittests/test_no_pickle.py(regression net: walksdojo/forimport pickleand asserts JSON-only Celery settings) andunittests/test_survey_forms.py(widget round-trip).Why
pickle.loads()on attacker-controllable bytes is arbitrary code execution. Removing pickle from both sites eliminates the class of risk and aligns the Celery configuration with the JSON-only default that Celery has shipped for years.Operator note
Workers running this version reject in-flight pickled messages from the previous version with
ContentDisallowed. Drain the broker (`celery -A dojo purge -f`) or scale workers to zero before deploying. Beat-driven tasks (e.g. `add_alerts`) self-heal on the next tick.Test plan
🤖 Generated with Claude Code