adebaumann/vgui-cicd

Fork 0

Files

Adrian A. Baumann 18fac6e8b9

SonarQube Scan / SonarQube Trigger (push) Has been cancelled

Details

Code reviewed; Package versions updated to latest (incl. Django 6)

2026-01-20 09:51:08 +01:00

20 KiB

Raw Blame History

Code Review: vgui-cicd Django Project

Date: January 20, 2026 Reviewer: AI Code Review Project Path: /home/adebaumann/development/vgui-cicd

1. PROJECT STRUCTURE

Overview

The project follows Django conventions with a clear app structure:

VorgabenUI - Main project settings, URLs, WSGI/ASGI
dokumente - Core document and Vorgabe models (315 lines)
abschnitte - Text section models and utilities
stichworte - Keyword/stichwort models
referenzen - Reference models (MPTT-based)
rollen - Role models
pages - General pages and views
diagramm_proxy - Diagram caching functionality

Issues Found

Minor - settings-docker.py: The settings-docker.py file has duplicate AUTH_PASSWORD_VALIDATORS definitions (lines 92-105 and 183-199), which is redundant.

File: /home/adebaumann/development/vgui-cicd/VorgabenUI/settings-docker.py (lines 92-105 and 183-199)

2. SETTINGS REVIEW

Critical Issues

1. Fallback SECRET_KEY in production (/home/adebaumann/development/vgui-cicd/VorgabenUI/settings.py, lines 27-47)

SECRET_KEY = os.environ.get('VORGABENUI_SECRET')
if not SECRET_KEY:
    is_build_env = any([...])
    debug_mode = ...
    if debug_mode or is_build_env:
        SECRET_KEY = 'dev-fallback-key-for-local-debugging-only-not-for-production-use-12345'

Issue: Even though there's a check for build environments, the hardcoded fallback key creates a significant security risk if the environment variable is not properly set in production
Recommendation: The fallback should NEVER be enabled, even in development - require the environment variable to be set

2. DEBUG mode default (/home/adebaumann/development/vgui-cicd/VorgabenUI/settings.py, line 24)

DEBUG = os.environ.get('DEBUG', 'True').lower() in ('true', '1', 'yes', 'on')

Issue: DEBUG defaults to True, which could expose sensitive information if environment variables are misconfigured
Recommendation: Require explicit setting of DEBUG to False in production

Major Issues

3. ALLOWED_HOSTS with wildcard (/home/adebaumann/development/vgui-cicd/VorgabenUI/settings.py, line 50)

ALLOWED_HOSTS = os.environ.get('DJANGO_ALLOWED_HOSTS', "10.128.128.144,localhost,127.0.0.1,*").split(",")

Issue: Default includes * which allows any host - dangerous in production
Recommendation: Default should not include wildcard; require explicit configuration

4. No rate limiting on authentication (/home/adebaumann/development/vgui-cicd/VorgabenUI/settings.py)

Issue: No login throttling or rate limiting configured for authentication endpoints
Recommendation: Add django-axes or similar for brute-force protection

5. SQLite in production (/home/adebaumann/development/vgui-cicd/VorgabenUI/settings.py, lines 109-114)

DATABASES = {
    'default': {
        'ENGINE': 'django.db.backends.sqlite3',
        'NAME': BASE_DIR / 'data/db.sqlite3',
    }
}

Issue: SQLite is used as default database - not suitable for production with concurrent access
Recommendation: Configure PostgreSQL or other production-ready database

Minor Issues

6. TIME_ZONE mismatch (/home/adebaumann/development/vgui-cicd/VorgabenUI/settings.py, lines 139-145)

Issue: LANGUAGE_CODE = 'de-ch' but TIME_ZONE = 'UTC' - timezone should probably be 'Europe/Zurich' for Swiss deployment
File: /home/adebaumann/development/vgui-cicd/VorgabenUI/settings.py (lines 141)

3. MODELS REVIEW

Major Issues

1. Missing verbose_name on models - Several models are missing proper verbose_name in their Meta class:

Stichwort (/home/adebaumann/development/vgui-cicd/stichworte/models.py, lines 4-11) - missing verbose_name
Referenz (/home/adebaumann/development/vgui-cicd/referenzen/models.py, lines 6-27) - missing verbose_name
Rolle (/home/adebaumann/development/vgui-cicd/rollen/models.py, lines 5-11) - missing verbose_name
Person (/home/adebaumann/development/vgui-cicd/dokumente/models.py, lines 22-30) - missing verbose_name
Thema (/home/adebaumann/development/vgui-cicd/dokumente/models.py, lines 32-39) - missing verbose_name

2. BooleanField with blank=True but no default (/home/adebaumann/development/vgui-cicd/dokumente/models.py, line 52)

aktiv = models.BooleanField(blank=True)

Issue: BooleanField should have explicit default=False for clarity
Recommendation: Add default=False

3. No database constraints for date validation (/home/adebaumann/development/vgui-cicd/dokumente/models.py, lines 96-97)

gueltigkeit_von = models.DateField()
gueltigkeit_bis = models.DateField(blank=True,null=True)

Issue: No database-level constraint ensuring gueltigkeit_bis >= gueltigkeit_von
Recommendation: Add constraint validation or override save() method

Minor Issues

4. Inconsistent naming in foreign key fields - Some models use plural related names inconsistently:

autoren and pruefende on Dokument use plural (correct)
Could consider singular related_name for consistency where applicable

5. Missing related_name on some ManyToMany fields (/home/adebaumann/development/vgui-cicd/dokumente/models.py, line 289)

autoren = models.ManyToManyField(Person)  # Missing related_name

Recommendation: Add related_name='changelog_entries' for clarity

4. VIEWS REVIEW

Critical Issues

1. No CSRF protection on comment endpoints (/home/adebaumann/development/vgui-cicd/dokumente/views.py, lines 321-377)

@require_POST
@login_required
def add_vorgabe_comment(request, vorgabe_id):
    # No @csrf_exempt but also no CSRF token verification in the view

Issue: The view uses @require_POST but doesn't verify CSRF tokens for the JSON endpoint
Recommendation: Add @csrf_exempt ONLY if intentionally bypassing, or ensure CSRF is handled via the X-CSRFToken header (which is done in the template)

Note: Looking at line 368, the template sends 'X-CSRFToken': getCookie('csrftoken'), so this is actually properly handled. Not a bug.

2. XSS in comment display (/home/adebaumann/development/vgui-cicd/dokumente/views.py, lines 305-306)

escaped_text = escape(comment.text).replace('\n', '<br>')

Issue: Using escape() is good, but line breaks are converted to <br> which could still be exploited
Note: The dangerous_patterns check at lines 339-343 provides some protection
Recommendation: Consider using a more robust HTML sanitization library

3. No input validation on comment length (/home/adebaumann/development/vgui-cicd/dokumente/views.py, line 335)

if len(text) > 2000:  # Reasonable length limit

Issue: This is actually properly implemented with a 2000 character limit
Status: OK

Major Issues

4. Referenz view lacks error handling (/home/adebaumann/development/vgui-cicd/referenzen/views.py, lines 11-19)

def detail(request, refid):
    referenz_item = Referenz.objects.get(id=refid)

Issue: DoesNotExist exception not caught - will return 500 error instead of 404
Recommendation: Use get_object_or_404 for consistency

Minor Issues

5. Search view allows complex regex patterns (/home/adebaumann/development/vgui-cicd/pages/views.py, lines 36-70)

Issue: The validation is good but the groupby usage at line 54 could fail if data is not properly sorted
Recommendation: Add explicit ordering before groupby

6. No rate limiting on search (/home/adebaumann/development/vgui-cicd/pages/views.py)

Issue: Search endpoint could be abused for DoS
Recommendation: Add rate limiting

5. URL CONFIGURATION

Minor Issues

1. No URL namespace for include (/home/adebaumann/development/vgui-cicd/VorgabenUI/urls.py, lines 31-35)

path('dokumente/', include("dokumente.urls")),
path('stichworte/', include("stichworte.urls")),

Issue: No app_name namespace defined in included apps
Recommendation: Add app_name = 'dokumente' to dokumente/urls.py for cleaner reversals

2. Inconsistent trailing slashes - Most URLs have trailing slashes but not all - ensure consistency

6. TEMPLATES REVIEW

Critical Issues

1. XSS vulnerability in standard_detail.html (/home/adebaumann/development/vgui-cicd/dokumente/templates/standards/standard_detail.html, lines 163-164)

{% for ref in vorgabe.referenzpfade %}
{{ ref|safe }}{% if not forloop.last %}, {% endif %}

Issue: Using |safe on reference paths could allow XSS if malicious content is stored
Recommendation: Use escape filter and handle line breaks separately

2. JavaScript in template without CSP (/home/adebaumann/development/vgui-cicd/dokumente/templates/standards/standard_detail.html, lines 249-424)

Issue: Inline JavaScript in template
Note: CSP headers are set in the view (line 316-317), but inline scripts violate strict CSP
Recommendation: Move JavaScript to external file

Major Issues

3. Missing ARIA labels and roles - Several accessibility issues:

Base template (/home/adebaumann/development/vgui-cicd/pages/templates/base.html) has navigation but missing aria-label on some elements
The mobile navigation could use better ARIA attributes

4. Missing alt attributes on images (/home/adebaumann/development/vgui-cicd/pages/templates/base.html, lines 39-41)

<img src="{% static 'swiss/img/logo-CH.svg' %}"
     onerror="this.onerror=null; this.src='{% static 'swiss/img/logo-CH.png' %}'"
     alt="Zur Startseite" />

Issue: alt is present but could be more descriptive
Status: Acceptable

Minor Issues

5. Hardcoded URLs in templates (/home/adebaumann/development/vgui-cicd/dokumente/templates/standards/incomplete_vorgaben.html, line 21)

<a href="/autorenumgebung/dokumente/vorgabe/{{ item.vorgabe.id }}/change/"

Issue: Hardcoded admin URL instead of using URL reversal
Recommendation: Use {% url 'admin:dokumente_vorgabe_change' item.vorgabe.id %}

7. FORMS REVIEW

Minor Issues

1. No dedicated form classes - Most form handling is done via Django admin forms or directly in views

Recommendation: Consider creating explicit ModelForm classes for views that accept user input (e.g., comment form)

2. VorgabeForm in admin could have more validation (/home/adebaumann/development/vgui-cicd/dokumente/admin.py, lines 95-107)

Issue: The form only validates Thema is required
Recommendation: Add validation for date ranges and conflicts

8. MANAGEMENT COMMANDS

Major Issues

1. import-document command lacks error recovery (/home/adebaumann/development/vgui-cicd/dokumente/management/commands/import-document.py, lines 288-349)

for v in vorgaben_data:
    try:
        thema = Thema.objects.get(name=v["thema"])
    except Thema.DoesNotExist:
        self.stdout.write(self.style.WARNING(...))
        continue  # Silently skips vorgabe

Issue: If one Vorgabe fails, the entire command may leave partial data
Recommendation: Use transaction.atomic() to ensure atomicity

2. No progress indicator for large imports (/home/adebaumann/development/vgui-cicd/dokumente/management/commands/import-document.py)

Issue: For large files, no progress shown
Recommendation: Add progress output

Minor Issues

3. export_json command hardcodes "Standard IT-Sicherheit" (/home/adebaumann/development/vgui-cicd/dokumente/management/commands/export_json.py, line 30)

result = {
    "Vorgabendokument": {
        "Typ": "Standard IT-Sicherheit",

Issue: Typ is hardcoded instead of using dokument.dokumententyp.name
Recommendation: Use actual dokumententyp

9. TESTS REVIEW

Major Issues

1. No tests for diagram caching - The diagramm_proxy module has no test coverage

Recommendation: Add tests for diagram_cache.py

2. No tests for referenzen views - The tree view and detail view have no test coverage

Recommendation: Add tests for referenzen/views.py

3. No tests for authentication security - Missing tests for:

Brute-force protection
Session management
Password policy enforcement

Minor Issues

4. Test file organization - test_json.py should be part of tests.py or in a proper test package structure

Status: Acceptable but could be improved

5. Tests rely on hardcoded paths (/home/adebaumann/development/vgui-cicd/dokumente/tests.py, lines 1165-1168)

self.assertContains(response, 'href="/autorenumgebung/dokumente/vorgabe/2/change/"')

Issue: Uses hardcoded URL paths instead of URL reversal
Recommendation: Use reverse('admin:dokumente_vorgabe_change', args=[vorgabe.pk])

10. SECURITY REVIEW

Critical Issues

1. No rate limiting on any endpoint - All views lack rate limiting

Recommendation: Add django-ratelimit or similar

2. Diagram cache potentially vulnerable to DoS (/home/adebaumann/development/vgui-cicd/diagramm_proxy/diagram_cache.py, lines 24-67)

def get_cached_diagram(diagram_type, diagram_content):
    content_hash = compute_hash(diagram_content)
    cache_path = get_cache_path(diagram_type, content_hash)

    if default_storage.exists(cache_path):
        return cache_path

    # Generate diagram via POST request
    url = f"{KROKI_UPSTREAM}/{diagram_type}/svg"

Issue: No validation on diagram size or content - could lead to DoS via large diagrams
Recommendation: Add size limits and timeout

Major Issues

3. CSRF trusted origins only for HTTPS (/home/adebaumann/development/vgui-cicd/VorgabenUI/settings.py, line 104)

CSRF_TRUSTED_ORIGINS=["https://vorgabenportal.knowyoursecurity.com"]

Issue: Only one origin configured - ensure this covers all deployment URLs
Recommendation: Make configurable via environment variable

4. No session expiry configuration (/home/adebaumann/development/vgui-cicd/VorgabenUI/settings.py)

Issue: Sessions don't expire
Recommendation: Set SESSION_COOKIE_AGE and SESSION_SAVE_EVERY_REQUEST

11. CODE STYLE COMPLIANCE (AGENTS.md)

Violations Found

1. Import order inconsistent (/home/adebaumann/development/vgui-cicd/dokumente/views.py, lines 1-16)

Imports are not strictly ordered (stdlib, Django, local apps)
Example: import parsedatetime is placed after Django imports

2. Missing German verbose_name on models (as noted in Section 3)

3. Function naming (/home/adebaumann/development/vgui-cicd/dokumente/models.py, line 101)

def Vorgabennummer(self):  # Should be vorgabennummer (snake_case)

Issue: Method name uses PascalCase instead of snake_case per AGENTS.md guidelines

4. Typo in error message (/home/adebaumann/development/vgui-cicd/dokumente/models.py, line 213)

"Geltungsdauer übeschneidet sich"  # Should be "überschneidet sich"

12. PERFORMANCE ISSUES

Major Issues

1. N+1 query potential in search (/home/adebaumann/development/vgui-cicd/pages/views.py, lines 53-68)

qs = VorgabeKurztext.objects.filter(inhalt__icontains=suchbegriff)...

Issue: Uses icontains which cannot use database indexes effectively
Recommendation: Consider PostgreSQL full-text search for better performance

2. No select_related in referenzen views (/home/adebaumann/development/vgui-cicd/referenzen/views.py, lines 7-8)

def tree(request):
    referenz_items = Referenz.objects.all()

Issue: No prefetch_related for related data
Recommendation: Add prefetch_related for referenzerklaerung_set and unterreferenzen

SUMMARY

Critical Issues (Must Fix)

#	Issue	Location	Recommendation
1	SECRET_KEY fallback	`VorgabenUI/settings.py:27-47`	Never enable fallback, require env var
2	DEBUG defaults to True	`VorgabenUI/settings.py:24`	Require explicit False for production
3	No rate limiting	All views	Add django-ratelimit
4	Session never expires	`VorgabenUI/settings.py`	Set SESSION_COOKIE_AGE
5	XSS via `\|safe` filter	`standard_detail.html:163-164`	Use `escape` filter

Major Issues (Should Fix)

#	Issue	Location	Recommendation
1	SQLite database	`VorgabenUI/settings.py:109-114`	Use PostgreSQL for production
2	ALLOWED_HOSTS wildcard	`VorgabenUI/settings.py:50`	Remove `*` from default
3	Missing date constraint	`dokumente/models.py:96-97`	Add validation for date ranges
4	Import not atomic	`import-document.py:288-349`	Wrap in transaction.atomic()
5	Missing test coverage	Multiple modules	Add tests for untested code

Minor Issues (Nice to Fix)

#	Issue	Location
1	Code style violations	`dokumente/views.py:1-16`
2	Typo: "übeschneidet"	`dokumente/models.py:213`
3	Missing ARIA labels	`base.html`
4	Hardcoded URLs in templates	`incomplete_vorgaben.html:21`
5	Duplicate AUTH_PASSWORD_VALIDATORS	`settings-docker.py:92-105, 183-199`

RECOMMENDED ACTIONS

Immediate (Before Production)

Set up proper SECRET_KEY via environment variable
Configure DEBUG=False explicitly
Remove wildcard from ALLOWED_HOSTS default
Add rate limiting to all endpoints
Configure session expiry
Fix XSS vulnerability in template
Switch to PostgreSQL database

Short-term (1-2 Weeks)

Add database constraints for date validation
Wrap import command in transactions
Add missing verbose_name to models
Fix code style violations
Add test coverage for critical paths
Move inline JavaScript to external files

Long-term (1 Month)

Implement PostgreSQL full-text search
Add comprehensive test suite
Set up CSP headers properly
Implement comprehensive authentication security
Add performance monitoring
Document all security configurations

POSITIVE FINDINGS

Good project organization - Clear app structure following Django conventions
Proper CSRF handling - X-CSRFToken header properly implemented
Input validation - Comment length limits and dangerous pattern checks in place
Good German localization - German field names and verbose texts throughout
Django admin integration - Well-configured admin interface
Management commands - Useful import/export functionality
Referenzen tree structure - MPTT implementation for hierarchical data

APPENDIX: Additional Issues Detected by LSP

The following issues were detected by the Language Server Protocol (LSP) analyzer:

dokumente/models.py

Line	Issue
14, 26, 36, 274	`__str__` method return type mismatch - returns `CharField` instead of `str`
70	Unknown attribute `vorgaben` on `Dokument`
102	Unknown attribute `nummer` on `ForeignKey`
106, 114	Unknown attribute `strftime` on `DateField`
137, 144, 196	Unknown attribute `objects` on `type[Vorgabe]`
173	Unknown attribute `thema_id` on `Vorgabe`
248, 254, 260, 266	`Meta` class overrides incompatible parent class
282	`VorgabenTable.Meta` incompatible with `Vorgabe.Meta`
294	Unknown attribute `nummer` on `ForeignKey`
314	Unknown attributes `username` and `Vorgabennummer` on `ForeignKey`

dokumente/views.py

Line	Issue
22, 133, 265	Unknown attribute `objects` on `type[Dokument]`
94	Unknown attribute `objects` on `type[Vorgabe]`
285	Argument type `str` cannot be assigned to parameter `content` of type `bytes`
345, 412, 440	Unknown attribute `objects` on `type[VorgabeComment]`

abschnitte/models.py

Line	Issue
6	`__str__` method return type mismatch
18	Argument type `Literal[0]` cannot be assigned to parameter `default`

referenzen/models.py

Line	Issue
23	`__str__` method return type mismatch
26	`Referenz.Meta` overrides incompatible parent `MPTTModel.Meta`
32	`Referenzerklaerung.Meta` overrides incompatible parent `Textabschnitt.Meta`

rollen/models.py

Line	Issue
8	`__str__` method return type mismatch
15	`RollenBeschreibung.Meta` overrides incompatible parent `Textabschnitt.Meta`

End of Code Review

20 KiB Raw Blame History

Code Review: vgui-cicd Django Project

1. PROJECT STRUCTURE

Overview

Issues Found

2. SETTINGS REVIEW

Critical Issues

Major Issues

Minor Issues

3. MODELS REVIEW

Major Issues

Minor Issues

4. VIEWS REVIEW

Critical Issues

Major Issues

Minor Issues

5. URL CONFIGURATION

Minor Issues

6. TEMPLATES REVIEW

Critical Issues

Major Issues

Minor Issues

7. FORMS REVIEW

Minor Issues

8. MANAGEMENT COMMANDS

Major Issues

Minor Issues

9. TESTS REVIEW

Major Issues

Minor Issues

10. SECURITY REVIEW

Critical Issues

Major Issues

11. CODE STYLE COMPLIANCE (AGENTS.md)

Violations Found

12. PERFORMANCE ISSUES

Major Issues

SUMMARY

Critical Issues (Must Fix)

Major Issues (Should Fix)

Minor Issues (Nice to Fix)

RECOMMENDED ACTIONS

Immediate (Before Production)

Short-term (1-2 Weeks)

Long-term (1 Month)

POSITIVE FINDINGS

APPENDIX: Additional Issues Detected by LSP

dokumente/models.py

dokumente/views.py

abschnitte/models.py

referenzen/models.py

rollen/models.py

20 KiB

Raw Blame History