22745 - Enforce object permissions on Script REST API write operations

[arthanson]

Closes: #22745

Fixes a permissions gap in the Scripts REST API where DELETE/PUT/PATCH on /api/extras/scripts/{id}/ succeeded for view_script-only users and read-only tokens, hard-deleting or mutating scripts.

Script records are created and modified by uploading their module (/api/extras/scripts/upload/), so the script endpoint is now read-only apart from execution: GET on the list and detail routes, and POST on the detail route to run a script. Running a script enforces the run action on the object and read-only tokens are rejected with a 403.

[arthanson]

Claude issues fixed.

[jeremystretch]

@claude /review Only report on issues of potential concern; we don't need commentary on anything that doesn't require further consideration.

[jeremystretch]

Please see Claude's findings above. At least items 1 and 2 should be addressed IMO.

[github-actions]

Claude finished @arthanson's task in 9m 38s —— View job

---

Review: Enforce object permissions on Script REST API write operations

• Gather PR diff and context
• Review extras/api/routers.py + urls.py
• Review extras/api/views.py (permission enforcement)
• Review netbox/api/viewsets/__init__.py change
• Review test coverage
• Post findings

The core approach is sound: ScriptViewSet is now (ListModelMixin, RetrieveModelMixin, BaseViewSet) per the earlier review, so PUT/PATCH/DELETE are dropped by get_method_map() (405) rather than being permission-checked, run is a first-class routed action (so it finally appears in the OpenAPI schema instead of the old extend_schema_view(update=...) workaround), and the isnumeric() → isdecimal() + int() guard fixes a genuine 500. I verified the OPTIONS path works as claimed (BulkOperationMetadata.determine_actions() clones the request as POST, so keying get_serializer() on request.method is correct), and that BaseViewSet.get_queryset()'s prefetch resolution tolerates ScriptSerializer's non-model fields (result, vars, description).

Concerns below, roughly in order of significance. I was not able to run the test suite in this environment (no configured database), so findings are from reading.

---

1. ScriptRouter hard-codes one ViewSet and temporarily mutates shared route state

netbox/extras/api/routers.py:24-34 rebinds self.routes for the duration of the super().get_routes() call. Two issues:

• Silent routing loss on subclassing. The mapping is keyed on the router, not the ViewSet. A ScriptViewSet subclass registered on a plain NetBoxRouter (a plugin, or a future urls.py that doesn't use ScriptRouter) loses POST/run with no error — the detail route just stops accepting POST. test_script_viewset_subclass covers the subclass-on-ScriptRouter case, which is the direction that already works.
• The mutation is observable. router.urls is a lazily-computed property; if URLConf loading is triggered at request time under a threaded server, a concurrent get_routes() for another viewset on the same router instance can see the patched detail route. The blast radius is currently bounded (DRF's get_method_map() drops mappings for methods the viewset doesn't implement, and no other extras viewset defines run), but it's latent.

Both go away if the mapping is declared on the ViewSet and applied generically — which also answers the earlier "nothing inherent to custom scripts about this" comment, removes the routers → views import, and needs no mutation because SimpleRouter.get_routes() returns concrete Routes before get_method_map() filtering:

# netbox/netbox/api/routers.py
class NetBoxRouter(DefaultRouter):
    def get_routes(self, viewset):
        routes = super().get_routes(viewset)
        if extra := getattr(viewset, 'detail_route_mapping', None):
            routes = [
                route._replace(mapping={**route.mapping, **extra})
                if isinstance(route, Route) and route.detail else route
                for route in routes
            ]
        return routes

# ScriptViewSet
detail_route_mapping = {'post': 'run'}

extras/api/routers.py, extras/api/urls.py's router swap, and most of test_api_routers.py then disappear. Fix this →

2. HTTP_ACTIONS.get() makes the permission restriction fail open

netbox/netbox/api/viewsets/__init__.py:93 — switching [] → .get() correctly turns a TRACE request from a 500 into a 405, but it now conflates "known method, no restriction needed" (OPTIONS → None) with "method I've never heard of" → skip restrict() entirely. Not exploitable today (router mappings only use GET/HEAD/POST/PUT/PATCH/DELETE), but a plugin action bound to an unusual method (@action(methods=['trace'])) would bypass object-level permissions silently. Fail closed instead — e.g. raise MethodNotAllowed when request.method not in HTTP_ACTIONS, keeping None as the explicit "no restriction" marker.

3. PR description no longer matches the behavior (and this is a breaking API change on main)

The description says the write operations "now enforce the matching model permission and token write-ability, returning 403 when unauthorized." As implemented, PUT/PATCH/DELETE return 405 (endpoints removed), and run without run_script returns 404 (object filtered out by restrict() — consistent with test_render_without_permission, so this is the right convention, just not what the description says). Worth updating the description, and worth an explicit call-out that PUT/PATCH/DELETE on /api/extras/scripts/{id}/ are being removed — that's a breaking change for a patch-release branch, even though the removed behavior was the bug.

Related, for confirmation rather than as a defect: ScriptModuleViewSet is Create+Update only, so with script DELETE gone there is now no REST API path to remove a stale Script row (is_executable=False, retained for its jobs). That matches the UI (no Script delete view exists), but the class comment at netbox/extras/api/views.py:311 — "created, modified, and deleted through their module" — overstates what the module endpoint actually supports.

4. Third copy of the token-write check

netbox/extras/api/views.py:383 now duplicates logic that already exists in TokenPermissions._verify_write_permission() and TokenWritePermission. The inline version is deliberately different — it passes through session auth, whereas TokenWritePermission raises PermissionDenied for any non-token auth (asserted by test_render_token_write_enabled), meaning ConfigTemplateViewSet.render and RenderConfigMixin.render_config reject session-authenticated callers. So there are now two divergent answers to "is this caller allowed to write?" in the same file. Consider making TokenWritePermission pass through non-token auth and reusing it via get_permissions() for action == 'run', matching the established pattern at views.py:244 and extras/api/mixins.py:82 — one place to change if token semantics ever move.

5. Test coverage gaps

• Every new run test uses the numeric PK, but the documented interface is the dotted form (/api/extras/scripts/example.MyReport/, docs/customization/custom-scripts.md:536). That path exercises lookup_value_regex = '[^/]+' against the newly-added detail-route POST mapping and the format-suffix patterns — exactly the interaction most likely to regress. test_run_script_format_suffix covers the suffix with a PK; a POST by <module>.<name> would close the gap.
• Nothing covers GET /api/extras/scripts/ (list). The base-class change newly routes list through BaseViewSet.get_queryset()/filter_queryset(); it resolves fine (prefetch ['module'], non-model serializer fields skipped via FieldDoesNotExist), but it's untested.

Minor

test_route_templates_not_mutated asserts on router.routes[2] — positional indexing into DRF's route templates is brittle. Selecting the detail Route by isinstance(r, Route) and r.detail would survive a DRF reshuffle.

---

· branch 22745-script

[arthanson]

Per Claude review - fixed 2, 3 and 5.

1 - was deliberate, the generic fix required a meta attribute and was for all routes fixing something that is only used by scripts that will be replaced in the future. So this isolates the change and allows easy removal / deprecation in the future.

4 - it is deliberately different and same for 1 it makes it an isolated and easier to remove in the future / limiting the blast radius of the change.