starred/netbox
1
0
Fork 0
mirror of https://github.com/netbox-community/netbox.git synced 2026-01-11 21:10:29 +01:00
Code Issues 273 Packages Projects Releases 10 Wiki Activity

Underscores in model names results in broken permissions #8160

New Issue
Closed
opened 2025-12-29 20:33:12 +01:00 by adam · 13 comments
adam commented 2025-12-29 20:33:12 +01:00
Owner
Copy Link

Originally created by @Dominic-Walther on GitHub (Jun 4, 2023).

Originally assigned to: @arthanson on GitHub.

NetBox version

3.5.3

Python version

3.9

Steps to Reproduce

  1. Create a model containing an underscore in the name
  2. Log in as non-superuser with appropriate permissions

Expected Behavior

User can perform whatever actions they have permissions for

Observed Behavior

Permissions for non-superusers are broken.

Issue:
In commit 85e932bfc1#, a bug was introduced in the splitting of permission names. Specifically in the resolve_permission() function:

[-]        return name.split('.')[1].split('_')[0]
[+]        app_label, codename = name.split('.')
[+]        action, model_name = codename.rsplit('_', 1)

By using .rsplit() instead of .split() on for example model BGP_PE with permission "view_bgp_pe" results in action="view_bgp" and model_name="pe", neither of which is valid.

Originally created by @Dominic-Walther on GitHub (Jun 4, 2023). Originally assigned to: @arthanson on GitHub. ### NetBox version 3.5.3 ### Python version 3.9 ### Steps to Reproduce 1. Create a model containing an underscore in the name 2. Log in as non-superuser with appropriate permissions ### Expected Behavior User can perform whatever actions they have permissions for ### Observed Behavior Permissions for non-superusers are broken. Issue: In commit https://github.com/netbox-community/netbox/commit/85e932bfc1e6ad62c3e09e0b5a354c7c69597830#, a bug was introduced in the splitting of permission names. Specifically in the `resolve_permission()` function: ```py [-] return name.split('.')[1].split('_')[0] [+] app_label, codename = name.split('.') [+] action, model_name = codename.rsplit('_', 1) ``` By using `.rsplit()` instead of `.split()` on for example model `BGP_PE` with permission `"view_bgp_pe"` results in `action="view_bgp"` and `model_name="pe"`, neither of which is valid.
adam added the type: bugstatus: acceptedseverity: low labels 2025-12-29 20:33:12 +01:00
adam closed this issue 2025-12-29 20:33:12 +01:00
adam commented 2025-12-29 20:33:13 +01:00
Author
Owner
Copy Link

@abhi1693 commented on GitHub (Jun 4, 2023):

I dont think this qualifies as a bug in the core code. The commit you have mentioned is just refactored code. NetBox follows PEP8 conventions very strictly. I'd suggest to do the same in your plugin as well.

https://peps.python.org/pep-0008/#class-names

@abhi1693 commented on GitHub (Jun 4, 2023): I dont think this qualifies as a bug in the core code. The commit you have mentioned is just refactored code. NetBox follows PEP8 conventions very strictly. I'd suggest to do the same in your plugin as well. https://peps.python.org/pep-0008/#class-names
adam commented 2025-12-29 20:33:13 +01:00
Author
Owner
Copy Link

@Dominic-Walther commented on GitHub (Jun 4, 2023):

Refactors generally don't intend to change behaviour and nowhere in the "Plugins" documentation is this requirement mentioned.
So either it is a bug, or an error should be raised and the documentation needs updating. As it stands, this just leads to unexpected behaviour by failing silently.

@Dominic-Walther commented on GitHub (Jun 4, 2023): Refactors generally don't intend to change behaviour and nowhere in the "Plugins" documentation is this requirement mentioned. So either it is a bug, or an error should be raised and the documentation needs updating. As it stands, this just leads to unexpected behaviour by failing silently.
adam commented 2025-12-29 20:33:13 +01:00
Author
Owner
Copy Link

@ubaumann commented on GitHub (Jun 5, 2023):

Hi @abhi1693 @jeremystretch

I understand your wish to close bug reports as fast as possible, but I would appreciate it if we could discuss further if this is a bug.
As it is an undocumented change in refactoring that could lead to breaking changes, I tend to classify it as a bug.

If this is not a bug, I agree with @Dominic-Walther that this should raise an error because otherwise, unexpected behaviour could lead to security issues, and this would not be classified as a bug and more like a vulnerability. The documentation should be updated and clarified.

@ubaumann commented on GitHub (Jun 5, 2023): Hi @abhi1693 @jeremystretch I understand your wish to close bug reports as fast as possible, but I would appreciate it if we could discuss further if this is a bug. As it is an undocumented change in refactoring that could lead to breaking changes, I tend to classify it as a bug. If this is not a bug, I agree with @Dominic-Walther that this should raise an error because otherwise, unexpected behaviour could lead to security issues, and this would not be classified as a bug and more like a vulnerability. The documentation should be updated and clarified.
adam commented 2025-12-29 20:33:13 +01:00
Author
Owner
Copy Link

@kkthxbye-code commented on GitHub (Jun 5, 2023):

As it is an undocumented change in refactoring

How do you mean? The commits implementing object based permissions and the commit that you refer to as a refactor are all part of the same PR. Looking at the changes there was never a release of netbox with the code that you wish to restore, so not sure how this is a breaking change when it has never been released in the state you claim.

So it's not an undocumented change as far as I can tell, it's how object based permissions have always worked. If I misunderstood, please link the netbox release before and after the claimed undocumentated change.

That being said, it doesn't seem logical to use rsplit unless it is expected that the action can contain underscores, which I don't think is a valid expectation.

@kkthxbye-code commented on GitHub (Jun 5, 2023): > As it is an undocumented change in refactoring How do you mean? The commits implementing object based permissions and the commit that you refer to as a refactor are [all part of the same PR](https://github.com/netbox-community/netbox/pull/4705). Looking at the changes there was never a release of netbox with the code that you wish to restore, so not sure how this is a breaking change when it has never been released in the state you claim. So it's not an undocumented change as far as I can tell, it's how object based permissions have always worked. If I misunderstood, please link the netbox release before and after the claimed undocumentated change. That being said, it doesn't seem logical to use rsplit unless it is expected that the action can contain underscores, which I don't think is a valid expectation.
adam commented 2025-12-29 20:33:14 +01:00
Author
Owner
Copy Link

@ubaumann commented on GitHub (Jun 5, 2023):

My apologies. I did not check if the refactoring was in the same PR as the introduced feature. In this case, it is not a breaking change.
Still unexpected behavior, in my opinion, and it would be good to discuss it.

@ubaumann commented on GitHub (Jun 5, 2023): My apologies. I did not check if the refactoring was in the same PR as the introduced feature. In this case, it is not a breaking change. Still unexpected behavior, in my opinion, and it would be good to discuss it.
adam commented 2025-12-29 20:33:14 +01:00
Author
Owner
Copy Link

@DanSheps commented on GitHub (Jun 5, 2023):

I don't think this is a fixable problem, at least not without some not-so-insignificant code refactoring.

The issue is we are using rsplit() for this to handle bulk actions because the actions for "Bulk Edit", and "Bulk Delete", on devices for example, are:

bulk_edit_device
bulk_delete_device

This results in it being device_bulk_edit. When evaluating permissions, you get bulk_edit or bulk_delete for the action, as you should.

I can see where this goes off the rails if there is an underscore in the name. You could fix this if you switched to a regex with defined permission actions, however the issue is there is the option to have custom permissions actions assigned to objects and those are not accountable in using defined names.

@DanSheps commented on GitHub (Jun 5, 2023): I don't think this is a fixable problem, at least not without some not-so-insignificant code refactoring. The issue is we are using rsplit() for this to handle bulk actions because the actions for "Bulk Edit", and "Bulk Delete", on devices for example, are: `bulk_edit_device` `bulk_delete_device` This results in it being `device_bulk_edit`. When evaluating permissions, you get `bulk_edit` or `bulk_delete` for the action, as you should. I can see where this goes off the rails if there is an underscore in the name. You could fix this if you switched to a regex with defined permission actions, however the issue is there is the option to have custom permissions actions assigned to objects and those are not accountable in using defined names.
adam commented 2025-12-29 20:33:14 +01:00
Author
Owner
Copy Link

@kkthxbye-code commented on GitHub (Jun 5, 2023):

Good catch, forgot about bulk actions. I guess a small note in the plugin docs stating that model names can't contain underscores would be the way to go then.

Luckily the object permissions have behaved this way since they were added, so only post-2.8 and pre-2.9 plugins can possible be affected, and since that's ages ago the actual issue here is probably not that common.

@kkthxbye-code commented on GitHub (Jun 5, 2023): Good catch, forgot about bulk actions. I guess a small note in the plugin docs stating that model names can't contain underscores would be the way to go then. Luckily the object permissions have behaved this way since they were added, so only post-2.8 and pre-2.9 plugins can possible be affected, and since that's ages ago the actual issue here is probably not that common.
adam commented 2025-12-29 20:33:14 +01:00
Author
Owner
Copy Link

@Dominic-Walther commented on GitHub (Jun 5, 2023):

I'm not too fond of a name restriction since it invalidates otherwise valid Python code for what is effectively a stylistic choice and an oversight, but I'm not sure there is a clean path short of a rework and/or breaking changes either. The use of a token which is also a valid input is hard to fix retroactively given custom actions.

Sanitizing the class name could be a workaround as, since no models with underscores have ever worked properly, there is nothing to break, but I imagine that would require the sanitation to become a permanent feature even if a rework does eventually occur.

The model providing a sanitized name for itself through the Meta class could be an option, but would require proper warnings (if not provided where needed) as it's not an intuitive requirement.

@Dominic-Walther commented on GitHub (Jun 5, 2023): I'm not too fond of a name restriction since it invalidates otherwise valid Python code for what is effectively a stylistic choice and an oversight, but I'm not sure there is a clean path short of a rework and/or breaking changes either. The use of a token which is also a valid input is hard to fix retroactively given custom actions. Sanitizing the class name could be a workaround as, since no models with underscores have ever worked properly, there is nothing to break, but I imagine that would require the sanitation to become a permanent feature even if a rework does eventually occur. The model providing a sanitized name for itself through the Meta class could be an option, but would require proper warnings (if not provided where needed) as it's not an intuitive requirement.
adam commented 2025-12-29 20:33:15 +01:00
Author
Owner
Copy Link

@kkthxbye-code commented on GitHub (Jun 5, 2023):

I personally don't think any more time investment here is worth it, but if you can figure out a solution that doesn't break any existing plugins or permissions, you are free to submit a PR.

@kkthxbye-code commented on GitHub (Jun 5, 2023): I personally don't think any more time investment here is worth it, but if you can figure out a solution that doesn't break any existing plugins or permissions, you are free to submit a PR.
adam commented 2025-12-29 20:33:15 +01:00
Author
Owner
Copy Link

@DanSheps commented on GitHub (Jun 5, 2023):

We could keep it open, incase someone comes up with an option to do this without breaking the bulk actions (could be as simple as checking for the presence of "bulk" in the action and handling it accordinly).

Howver, I do think underscores in model names are a good thing to "ban", as a practice and maybe that is the way to resolve this issue is to "ban" it. As mentioned by @abhi1693, we follow PEP8:

According to PEP8, class names should be CapWords.

@DanSheps commented on GitHub (Jun 5, 2023): We could keep it open, incase someone comes up with an option to do this without breaking the bulk actions (could be as simple as checking for the presence of "bulk" in the action and handling it accordinly). Howver, I do think underscores in model names are a good thing to "ban", as a practice and maybe that is the way to resolve this issue is to "ban" it. As mentioned by @abhi1693, we follow PEP8: According to [PEP8](https://www.python.org/dev/peps/pep-0008/#class-names), class names **should** be CapWords.
adam commented 2025-12-29 20:33:15 +01:00
Author
Owner
Copy Link

@DanSheps commented on GitHub (Jun 5, 2023):

@Dominic-Walther What plugin are you using, or is this an internal plugin?

@DanSheps commented on GitHub (Jun 5, 2023): @Dominic-Walther What plugin are you using, or is this an internal plugin?
adam commented 2025-12-29 20:33:15 +01:00
Author
Owner
Copy Link

@Dominic-Walther commented on GitHub (Jun 5, 2023):

It's not really public yet.
PEP8 is all well and good, but when dealing with long acronyms such as MPLS L3 VPN it becomes hard to read in all-caps without demarcations (as PEP8 suggests) and unwieldy when spelled out. Granted, it's a niche case, but one for which I'm happy to break with convention.

@Dominic-Walther commented on GitHub (Jun 5, 2023): It's not really public yet. PEP8 is all well and good, but when dealing with long acronyms such as `MPLS L3 VPN` it becomes hard to read in all-caps without demarcations (as PEP8 suggests) and unwieldy when spelled out. Granted, it's a niche case, but one for which I'm happy to break with convention.
adam commented 2025-12-29 20:33:16 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (Jun 14, 2023):

To summarize the above, it's not possible to accommodate the proposal because Django convention dictates that an underscore is used to separate the permission action (which may be custom) from the model name. For example, a permission named

myplugin.foo_x_bar

Could indicate the foo_x action for a model named Bar, or the foo action for a model named X_Bar. AFAICT, the Django documentation does not address this conflict (likely because it's an exceedingly rare circumstance).

could be as simple as checking for the presence of "bulk" in the action and handling it accordingly

This would not suffice as plugin models can introduce their own custom actions, which would be impossible to predict.

@jeremystretch commented on GitHub (Jun 14, 2023): To summarize the above, it's not possible to accommodate the proposal because Django convention dictates that an underscore is used to separate the permission action (which may be custom) from the model name. For example, a permission named `myplugin.foo_x_bar` Could indicate the `foo_x` action for a model named `Bar`, or the `foo` action for a model named `X_Bar`. AFAICT, the Django documentation does not address this conflict (likely because it's an exceedingly rare circumstance). > could be as simple as checking for the presence of "bulk" in the action and handling it accordingly This would not suffice as plugin models can introduce their own custom actions, which would be impossible to predict.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
beta
breaking change
complexity: high
complexity: low
complexity: medium
needs milestone
netbox
pending closure
plugin candidate
pull-request
Mirrored from GitHub Pull Request
 severity: high
severity: low
severity: medium
status: accepted
status: backlog
status: blocked
status: duplicate
status: needs owner
status: needs triage
status: revisions needed
status: under review
topic: GraphQL
topic: Internationalization
topic: OpenAPI
topic: UI/UX
topic: cabling
topic: event rules
topic: htmx navigation
topic: industrialization
topic: migrations
topic: plugins
topic: scripts
topic: templating
topic: testing
type: bug
type: deprecation
type: documentation
type: feature
type: housekeeping
type: translation
No Label severity: low status: accepted type: bug
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
adam
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/netbox#8160
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?