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

2026-01-20 09:51:08 +01:00
parent 492f8b4e90
commit 18fac6e8b9
17 changed files with 540 additions and 653 deletions
--- a/code-review.md
+++ b/code-review.md
@@ -0,0 +1,512 @@
+# 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)
+```python
+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)
+```python
+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)
+```python
+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)
+```python
+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)
+```python
+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)
+```python
+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)
+```python
+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)
+```python
+@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)
+```python
+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)
+```python
+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)
+```python
+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)
+```python
+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)
+```html
+{% 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)
+```html
+<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)
+```html
+<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)
+```python
+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)
+```python
+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)
+```python
+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)
+```python
+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)
+```python
+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)
+```python
+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)
+```python
+"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)
+```python
+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)
+```python
+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)
+1. Set up proper SECRET_KEY via environment variable
+2. Configure DEBUG=False explicitly
+3. Remove wildcard from ALLOWED_HOSTS default
+4. Add rate limiting to all endpoints
+5. Configure session expiry
+6. Fix XSS vulnerability in template
+7. Switch to PostgreSQL database
+
+### Short-term (1-2 Weeks)
+1. Add database constraints for date validation
+2. Wrap import command in transactions
+3. Add missing verbose_name to models
+4. Fix code style violations
+5. Add test coverage for critical paths
+6. Move inline JavaScript to external files
+
+### Long-term (1 Month)
+1. Implement PostgreSQL full-text search
+2. Add comprehensive test suite
+3. Set up CSP headers properly
+4. Implement comprehensive authentication security
+5. Add performance monitoring
+6. Document all security configurations
+
+---
+
+## POSITIVE FINDINGS
+
+1. **Good project organization** - Clear app structure following Django conventions
+2. **Proper CSRF handling** - X-CSRFToken header properly implemented
+3. **Input validation** - Comment length limits and dangerous pattern checks in place
+4. **Good German localization** - German field names and verbose texts throughout
+5. **Django admin integration** - Well-configured admin interface
+6. **Management commands** - Useful import/export functionality
+7. **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*