Skip to content

Commit 64432ae

Browse files
committed
fix: address security vulnerabilities and code quality issues
Security fixes: - Fix open redirect vulnerability in segment financial info views by validating return_url parameter with url_has_allowed_host_and_scheme() - Add null check for obj.segment in SegmentFinancialInfoView to prevent AttributeError when segment is missing API improvements: - Replace hardcoded URL strings with DRF reverse() calls in segment financial info serializer for proper URL generation - Fix NoReverseMatch error by using correct URL pattern name 'plugins-api:cesnet_service_path_plugin-api:segment-detail' - Remove redundant permission checks in SegmentSerializer.get_financial_info() Code quality improvements: - Fix typo: rename SegmnetCircuitMappingViewSet to SegmentCircuitMappingViewSet across all files (urls.py, __init__.py, views/segment_circuit_mapping.py) - Move get_currency_choices and get_default_currency imports to module level in segment_financial_info form to follow Python best practices - Remove duplicate '{% load plugins %}' statement from segment.html template - Fix inconsistent typing: change lowercase 'list' to 'List' in GraphQL schema for segment_financial_info_list field Documentation: - Add clarifying note in README.md explaining that EUR in example config is an override while CZK is the fallback default currency All changes maintain backward compatibility and improve code maintainability.
1 parent 6248c0d commit 64432ae

File tree

10 files changed

+35
-37
lines changed

10 files changed

+35
-37
lines changed

README.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -404,6 +404,8 @@ PLUGINS_CONFIG = {
404404
}
405405
```
406406

407+
*Note: This example shows EUR as the configured default currency. If not configured, the application will use CZK as the fallback default.*
408+
407409
**Configuration options:**
408410
- `currencies`: List of (code, name) tuples for available currencies
409411
- `default_currency`: Default currency code to use when creating new financial records

cesnet_service_path_plugin/api/serializers/segment.py

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -98,21 +98,14 @@ def get_financial_info(self, obj):
9898
return None
9999

100100
# Check if user has permission to view financial info
101-
has_financial_view_perm = request.user.has_perm("cesnet_service_path_plugin.view_segmentfinancialinfo")
102-
103-
# Check if user has permission to view financial info
104-
if not has_financial_view_perm:
101+
if not request.user.has_perm("cesnet_service_path_plugin.view_segmentfinancialinfo"):
105102
return None
106103

107-
# Try to get financial info if user has permission
108-
financial_info = None
109-
if has_financial_view_perm:
110-
financial_info = getattr(obj, "financial_info", None)
111-
104+
# Fetch and serialize financial info if user has permission
105+
financial_info = getattr(obj, "financial_info", None)
112106
if financial_info:
113107
return SegmentFinancialInfoSerializer(financial_info, context=self.context).data
114-
else:
115-
return None
108+
return None
116109

117110
def update(self, instance, validated_data):
118111
"""Handle file upload during update"""

cesnet_service_path_plugin/api/serializers/segment_financial_info.py

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
# cesnet_service_path_plugin/api/serializers/segment_financial_info.py
22
from netbox.api.serializers import NetBoxModelSerializer
33
from rest_framework import serializers
4+
from rest_framework.reverse import reverse
45

56
from cesnet_service_path_plugin.models import Segment, SegmentFinancialInfo
67

@@ -71,13 +72,17 @@ def get_segment_detail(self, obj):
7172
request = self.context.get("request")
7273

7374
if request:
74-
# API context - build absolute URI
75-
segment_url = request.build_absolute_uri(
76-
f"/api/plugins/cesnet-service-path-plugin/segments/{obj.segment.id}/"
75+
# API context - build absolute URI using reverse
76+
segment_url = reverse(
77+
"plugins-api:cesnet_service_path_plugin-api:segment-detail",
78+
kwargs={"pk": obj.segment.id},
79+
request=request,
7780
)
7881
else:
7982
# Non-API context (e.g., form validation) - use relative URL
80-
segment_url = f"/api/plugins/cesnet-service-path-plugin/segments/{obj.segment.id}/"
83+
segment_url = reverse(
84+
"plugins-api:cesnet_service_path_plugin-api:segment-detail", kwargs={"pk": obj.segment.id}
85+
)
8186

8287
return {
8388
"id": obj.segment.id,

cesnet_service_path_plugin/api/urls.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
router.register("segments", views.SegmentViewSet)
88
router.register("service-paths", views.ServicePathViewSet)
99
router.register("service-path-segment-mappings", views.ServicePathSegmentMappingViewSet)
10-
router.register("segment-circuit-mappings", views.SegmnetCircuitMappingViewSet)
10+
router.register("segment-circuit-mappings", views.SegmentCircuitMappingViewSet)
1111
router.register("segment-financial-info", views.SegmentFinancialInfoViewSet)
1212

1313

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
from .segment import SegmentViewSet
2-
from .segment_circuit_mapping import SegmnetCircuitMappingViewSet
2+
from .segment_circuit_mapping import SegmentCircuitMappingViewSet
33
from .segment_financial_info import SegmentFinancialInfoViewSet
44
from .service_path import ServicePathViewSet
55
from .service_path_segment_mapping import ServicePathSegmentMappingViewSet
66

77
__all__ = [
88
"SegmentFinancialInfoViewSet",
99
"SegmentViewSet",
10-
"SegmnetCircuitMappingViewSet",
10+
"SegmentCircuitMappingViewSet",
1111
"ServicePathSegmentMappingViewSet",
1212
"ServicePathViewSet",
1313
]

cesnet_service_path_plugin/api/views/segment_circuit_mapping.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
from cesnet_service_path_plugin.api.serializers import SegmentCircuitMappingSerializer
66

77

8-
class SegmnetCircuitMappingViewSet(NetBoxModelViewSet):
8+
class SegmentCircuitMappingViewSet(NetBoxModelViewSet):
99
metadata_class = ContentTypeMetadata
1010
queryset = models.SegmentCircuitMapping.objects.all()
1111
serializer_class = SegmentCircuitMappingSerializer

cesnet_service_path_plugin/forms/segment_financial_info.py

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
from utilities.forms.rendering import FieldSet
55

66
from cesnet_service_path_plugin.models import Segment, SegmentFinancialInfo
7+
from cesnet_service_path_plugin.models.segment_financial_info import get_currency_choices, get_default_currency
78

89

910
class SegmentFinancialInfoForm(NetBoxModelForm):
@@ -36,8 +37,6 @@ def __init__(self, *args, **kwargs):
3637
super().__init__(*args, **kwargs)
3738

3839
# Dynamically set currency choices from the model's get_currency_choices
39-
from cesnet_service_path_plugin.models.segment_financial_info import get_currency_choices, get_default_currency
40-
4140
self.fields["charge_currency"].choices = get_currency_choices()
4241
self.fields["charge_currency"].initial = get_default_currency()
4342

cesnet_service_path_plugin/graphql/schema.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,11 @@
44
import strawberry_django
55

66
from .types import (
7-
SegmentType,
87
SegmentCircuitMappingType,
98
SegmentFinancialInfoType,
10-
ServicePathType,
9+
SegmentType,
1110
ServicePathSegmentMappingType,
11+
ServicePathType,
1212
)
1313

1414

@@ -21,7 +21,7 @@ class CesnetServicePathQuery:
2121
segment_circuit_mapping_list: List[SegmentCircuitMappingType] = strawberry_django.field()
2222

2323
segment_financial_info: SegmentFinancialInfoType = strawberry_django.field()
24-
segment_financial_info_list: list[SegmentFinancialInfoType] = strawberry_django.field()
24+
segment_financial_info_list: List[SegmentFinancialInfoType] = strawberry_django.field()
2525

2626
service_path: ServicePathType = strawberry_django.field()
2727
service_path_list: List[ServicePathType] = strawberry_django.field()

cesnet_service_path_plugin/templates/cesnet_service_path_plugin/segment.html

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
{% load tabs %}
77
{% load i18n %}
88
{% load perms %}
9-
{% load plugins %}
109
{% load custom_filters %}
1110
{% load render_table from django_tables2 %}
1211

cesnet_service_path_plugin/views/segment_financial_info.py

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
from django.shortcuts import redirect
2+
from django.utils.http import url_has_allowed_host_and_scheme
23
from netbox.views import generic
34
from utilities.views import register_model_view
45

@@ -16,6 +17,9 @@ class SegmentFinancialInfoView(generic.ObjectView):
1617

1718
def get(self, request, *args, **kwargs):
1819
obj = self.get_object(**kwargs)
20+
# Check if segment exists before redirecting
21+
if obj.segment is None:
22+
return redirect("/")
1923
# Redirect to the parent segment's detail view
2024
return redirect(obj.segment.get_absolute_url())
2125

@@ -32,13 +36,11 @@ def get_return_url(self, request, obj=None):
3236
"""
3337
# Check if return_url is in request
3438
if return_url := request.GET.get("return_url") or request.POST.get("return_url"):
35-
return return_url
39+
# Validate the return_url to prevent open redirect
40+
if url_has_allowed_host_and_scheme(return_url, allowed_hosts={request.get_host()}, require_https=True):
41+
return return_url
3642

37-
# Otherwise return to the segment detail
38-
if obj and obj.segment:
39-
return obj.segment.get_absolute_url()
40-
41-
# Fallback to the default behavior
43+
# Return safe default if validation fails or no return_url provided
4244
return super().get_return_url(request, obj)
4345

4446

@@ -52,11 +54,9 @@ def get_return_url(self, request, obj=None):
5254
"""
5355
# Check if return_url is in request
5456
if return_url := request.GET.get("return_url") or request.POST.get("return_url"):
55-
return return_url
56-
57-
# Otherwise return to the segment detail
58-
if obj and obj.segment:
59-
return obj.segment.get_absolute_url()
57+
# Validate the return_url to prevent open redirect
58+
if url_has_allowed_host_and_scheme(return_url, allowed_hosts={request.get_host()}, require_https=True):
59+
return return_url
6060

61-
# Fallback to the default behavior
61+
# Return safe default if validation fails or no return_url provided
6262
return super().get_return_url(request, obj)

0 commit comments

Comments
 (0)