SEP-1656: Restrict task hook paths to an allow-listed module namespace - #1307
Open
marcuscruz-percona wants to merge 15 commits into
Open
SEP-1656: Restrict task hook paths to an allow-listed module namespace#1307marcuscruz-percona wants to merge 15 commits into
marcuscruz-percona wants to merge 15 commits into
Conversation
…mespace A hook path is imported and invoked by the tasks service, so resolve_hook() now validates the path before anything else: it must be a well-formed "module:function" pair naming a public callable under one of the roots in the new TasksSettings.HOOK_MODULE_ALLOWLIST. The check runs ahead of the _RESOLVED cache lookup, so a path that reached the cache before the allow-list narrowed still fails closed. The error subclasses ValueError so the existing hook call sites keep treating a bad path as a skipped enrichment rather than a failure. The setting is read at startup and deliberately not runtime-overridable -- widening the namespace live would itself be a privilege-escalation path.
The task factories inherit both hook fields as optional strings, so polyfactory filled them with random values roughly half the time -- which would fail the incoming write-boundary allow-list wherever a built task is revalidated as a TaskWrite. Pin them to None. Rewrite the placeholder hook paths in the affected suites to conforming values. The resolver checks the allow-list before the cache, so even a path patched directly into _RESOLVED has to conform.
Both hook fields were accepted as free-form strings on the two authenticated task write surfaces, so any caller who could create or update a task could name an arbitrary importable callable for the tasks service to invoke. Validate them on TaskWrite, which turns a denied path into a 422 before it is ever persisted. The validator lives on TaskWrite rather than TaskBase on purpose: TaskBase also backs Task and TaskResponse, and a validator there would make read-back of a row whose stored path predates the allow-list fail.
The form backfill rebuilds a TaskWrite from an already-persisted row, outside the try/except blocks guarding the reconstructor and the stamp. Now that TaskWrite constrains hook paths, a row predating that constraint would abort the whole run instead of being skipped.
…allow-list The TaskWrite the form backfill builds is a carrier, not an update body: stamp_form_input writes only to write.data, and that dict -- not the envelope -- is what the caller persists. Copying the two hook-path fields onto it therefore bought nothing and made the new allow-list validator reachable from a read path, so any row whose stored path predates the constraint lost its backfill. Rows carrying a pre-rename app.sep.plugins.* path are the real case, not a hypothetical one. Leave the hook fields unset on the carrier and drop the ValidationError skip that was papering over the failure.
…tting The field docstring claimed the allow-list is read at startup, but validate_hook_path re-reads tasks_settings on every call. What actually holds the guarantee is that the field is not exposed for override, so say that instead, and mark it not_overridable_field rather than relying on the unmarked-top-level-field default -- widening the namespace at runtime would itself be a privilege-escalation path.
…-list The main merge brought in TestDispatchSeam and TestDispatchFailureCarveOut, written before the allow-list landed and so still naming a bare pkg:rec recorder. Five tests were failing on the branch as a result.
…ejections The rejected-path list and the pair of hook field names were retyped in three test modules, and neither write-boundary module exercised the empty module part or empty function part the resolver already rejects. Hoist both corpora to the tasks conftest, add the missing malformed shapes, and let PUT run the full corpus rather than a single arbitrary callable. Group the new route tests under a test class, annotate the new signatures, and correct a factory docstring still naming a model that no longer exists.
marcuscruz-percona
requested review from
a team,
maxbube,
peter-o-addo and
yyyyyyyan
as code owners
August 7, 2026 19:05
There was a problem hiding this comment.
Pull request overview
This PR closes a security hole in the Tasks service where per-task hook fields (alert_detail_builder, run_result_recorder) could be set to arbitrary importable callables (e.g., os:system) and then imported/invoked by the service. It adds an allow-list for hook module roots and enforces it both at the request validation boundary and again at invocation time.
Changes:
- Added
TASKS.HOOK_MODULE_ALLOWLIST(defaulting to("app.sep.apps",)) and made it explicitly not runtime-overridable. - Implemented hook-path validation + a dedicated
HookPathNotAllowedError, and re-checked hook paths inresolve_hook()before cache lookup / import. - Updated and expanded tests and factories to reflect the allow-listed namespace and to verify rejection/acceptance behavior across models and routes.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
app/tasks/hook_resolver.py |
Adds allow-list enforcement, structured validation, logging, and fail-closed behavior ahead of cache/import. |
app/tasks/models.py |
Enforces hook-path validation on TaskWrite only (write boundary), preserving read-back compatibility. |
app/tasks/config.py |
Introduces HOOK_MODULE_ALLOWLIST and marks it not runtime-overridable. |
app/sep/apps/framework/form_backfill.py |
Avoids validating/copying legacy hook-path values during backfill stamping to preserve eligibility. |
settings.yaml |
Documents/configures the default allow-list root under TASKS.HOOK_MODULE_ALLOWLIST. |
changelog.d/SEP-1656.security.md |
Adds a security changelog fragment describing the vulnerability closure. |
tests/app/tasks/test_hook_resolver.py |
Adds focused validation and allow-list tests, including cache-poisoning and “reject before import”. |
tests/app/tasks/test_models.py |
Adds TaskWrite allow-list validation tests plus “read-back legacy path succeeds” checks. |
tests/app/tasks/test_routes.py |
Adds route-level 422 enforcement tests for both create and update, and acceptance of shipped hook constants. |
tests/app/tasks/test_alert_hooks.py |
Updates cached hook paths to the allow-listed namespace and keeps “swallow errors” behavior covered. |
tests/app/tasks/test_run_result.py |
Updates recorder hook paths and cache keys to conform to the allow-listed namespace. |
tests/app/tasks/test_config.py |
Verifies default allow-list and confirms it is not runtime-overridable. |
tests/app/tasks/conftest.py |
Centralizes hook-path fields and rejected-path fixtures for parametrized tests. |
tests/app/sep/apps/framework/test_spec.py |
Updates framework stamping tests to use allow-listed hook paths. |
tests/app/sep/apps/framework/test_apps.py |
Updates app-to-tasks POST stamping assertions to use allow-listed hook paths. |
tests/app/sep/apps/framework/test_form_backfill.py |
Adds coverage ensuring backfill still works for rows with legacy (pre-allow-list) stored hook paths. |
tests/app/factories.py |
Pins hook-path fields to None to prevent random factory values from tripping new TaskWrite validation. |
marcuscruz-percona
added
qa in progress
The assertion read caplog.text, whose handler pytest attaches to the root logger. LOGGING_CONFIG declares the root logger, so applying it clears root's handlers and removes that capturing handler, and it also sets propagate=False on the app logger, so the record would not reach root anyway. Whether the config had been applied yet depended on which other tests shared the xdist worker, so the test passed in isolation and failed intermittently in CI with an empty caplog.text. Attach the capturing handler to the emitting logger instead, which the logging config never reconfigures.
Co-authored-by: marcuscruz-percona <272857389+marcuscruz-percona@users.noreply.github.com>
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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
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
alert_detail_builderandrun_result_recorderwere free-form"module:function"strings onTaskWrite, persisted onto theTaskrow and later resolved throughimportliband invoked. Any authenticated caller who could create or update a task could name an arbitrary importable callable (os:system,builtins:eval) and have the tasks service run it at alert-build or terminal-status time. Both fields are now constrained to an allow-listed module namespace.TaskWritefield validator rejects the value with a 422 before it is persisted, andresolve_hook()re-checks beforeimportlib.import_module— ahead of the_RESOLVEDcache lookup — so a path that reached the database by any other route fails closed at invoke time instead of being imported. Malformed pairs (no:, empty module part, empty function part, non-identifier, private or dunder attribute) are rejected too, so the resolver no longer raises a bareValueErrormid-dispatch.TaskWriteunder it, so an admin-only gate would break task creation for every non-admin user of every app. Legitimate hook values are never caller-supplied —TaskExecutionAppstamps them from static class attributes — so a namespace allow-list admits every real hook and rejects every arbitrary callable.TASKS.HOOK_MODULE_ALLOWLISTholds the roots, defaulting toapp.sep.appsand settable fromsettings.yamlor the environment. It is markednot_overridable_field: widening the namespace through the settings API at runtime would itself be a privilege-escalation path.No schema change and no migration — the column type is unchanged and every hook value in the tree already conforms.
Tested
alert_detail_builderis stamped, stored, and resolves on alert buildrun_result_recorderresolves and records the runPOST /api/tasks/withalert_detail_builder: "os:system"→ 422 naming the field and the allow-listed roots; confirmed the rejection is loggedPUT /api/tasks/{name}, and forrun_result_recorderTASKS.HOOK_MODULE_ALLOWLISTto an extra root insettings.yaml, restarted, confirmed a hook under it resolvesPATCHofTASKS.HOOK_MODULE_ALLOWLIST; confirmed it is rejected as not overridableGETsucceeds and the legacy-form backfill still stamps itChecklist
make test)make run-pre-commit)make makemigrations) — N/A, no model schema changechangelog.d/if the change is user-facing (make changelog-add), or confirmed N/A