Cross-Site Request Forgery Lab Completed #4496
Conversation
WalkthroughAdds user progress models and migration, registers them in Django admin, integrates progress updates into task detail and submission flows, seeds CSRF lab tasks via a management command, and makes task payload labels/placeholders dynamic by lab type. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser
participant Server as Django Views
participant DB as Models/DB
participant Admin as Django Admin
User->>Browser: Open Dashboard
Browser->>Server: GET /dashboard
Server->>DB: Fetch Labs + UserLabProgress
DB-->>Server: Labs + progress%
Server-->>Browser: Render dashboard with progress
User->>Browser: Open Task Detail
Browser->>Server: GET /labs/:lab/tasks/:task
Server->>DB: get_or_create(UserLabProgress), fetch UserTaskProgress
DB-->>Server: Progress records
Server-->>Browser: Render task (dynamic label), task_completed
User->>Browser: Submit Answer/Payload
Browser->>Server: POST /submit (theory/simulation)
Server->>DB: Evaluate, update/create UserTaskProgress (attempts, completed, timestamps)
Server->>DB: update/create UserLabProgress (last_accessed, completed_at if 100%)
DB-->>Server: Updated records
Server-->>Browser: JSON { correct, message, task_completed }
Admin->>Server: View Progress Lists
Server->>DB: Query UserTaskProgress/UserLabProgress
DB-->>Server: Results with computed progress
Server-->>Admin: Admin tables
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Out-of-scope changes
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (14)
website/templates/task_detail.html (1)
126-136: Avoid brittle name-based conditionals; centralize label/placeholder mapping in the view (DRY).Hard-coding
lab.namecomparisons in the template is fragile (renames, i18n) and duplicates logic between label and placeholder. Prefer passing precomputedpayload_labelandpayload_placeholderfrom the view (ideally keyed by an invariant like slug/type).Apply this diff to simplify the template:
- <label for="payload" class="block text-sm font-medium text-gray-700 mb-2"> - {% if lab.name == "SQL Injection" %} - Enter your SQL injection payload: - {% elif lab.name == "Cross-Site Scripting (XSS)" %} - Enter your XSS payload: - {% elif lab.name == "Cross-Site Request Forgery" %} - Enter your CSRF attack code: - {% else %} - Enter your payload: - {% endif %} - </label> + <label for="payload" class="block text-sm font-medium text-gray-700 mb-2"> + {{ payload_label }} + </label>- placeholder="{% if lab.name == 'SQL Injection' %}Enter your SQL injection payload here...{% elif lab.name == 'Cross-Site Scripting (XSS)' %}Enter your XSS payload here...{% elif lab.name == 'Cross-Site Request Forgery' %}Enter your CSRF attack code here...{% else %}Enter your payload here...{% endif %}"></textarea> + placeholder="{{ payload_placeholder }}"></textarea>And in
task_detailview (outside this hunk), compute and pass these:# website/views/Simulation.py (in task_detail, before building context) label_map = { "SQL Injection": "Enter your SQL injection payload:", "Cross-Site Scripting (XSS)": "Enter your XSS payload:", "Cross-Site Request Forgery": "Enter your CSRF attack code:", } placeholder_map = { "SQL Injection": "Enter your SQL injection payload here...", "Cross-Site Scripting (XSS)": "Enter your XSS payload here...", "Cross-Site Request Forgery": "Enter your CSRF attack code here...", } payload_label = label_map.get(lab.name, "Enter your payload:") payload_placeholder = placeholder_map.get(lab.name, "Enter your payload here...") # add to context: "context": { # ... "payload_label": payload_label, "payload_placeholder": payload_placeholder, }If available, prefer a stable
lab.slugortask.task_typeover display names.Please verify that your existing Labs have names/slugs that match the mapping keys to avoid regressions.
Also applies to: 141-141
website/models.py (1)
2586-2589: Consider memoizing or precomputing progress to avoid repeated queries in str.
__str__callscalculate_progress_percentage()which hits the DB. In admin lists or logs, this can cause N+1 queries. Options:
- Cache progress per instance call, or
- Annotate counts in queries that display these strings, or
- Store progress denormalized and update on relevant events.
website/management/commands/create_csrf_tasks.py (1)
9-17: Optional: wrap seeding in a transaction for atomicity.If any create/update fails mid-loop, you can end up with a partial set. Wrapping the operations in a single
transaction.atomic()ensures all-or-nothing.website/views/Simulation.py (4)
68-70: Bump last_accessed when viewing a task.You create/get
UserLabProgressbut never save, solast_accessedwon’t update after the first creation. Update it on task view.Apply this diff:
- user_lab_progress, created = UserLabProgress.objects.get_or_create(user=request.user, lab=lab) + user_lab_progress, created = UserLabProgress.objects.get_or_create(user=request.user, lab=lab) + # Update last access timestamp + user_lab_progress.last_accessed = timezone.now() + user_lab_progress.save(update_fields=["last_accessed"])
141-147: Make simulation payload matching more user-friendly (normalize whitespace).Exact string equality is brittle for HTML/JS payloads. Normalize by stripping all whitespace and lowercasing both sides to reduce false negatives.
Apply this diff:
simulation_config = content.simulation_config expected_payload = None if "success_payload" in simulation_config: expected_payload = simulation_config["success_payload"] - is_correct = user_payload.strip().lower() == expected_payload.strip().lower() + # Normalize by removing all whitespace and case + user_clean = "".join(user_payload.split()).lower() + expected_clean = "".join(expected_payload.split()).lower() + is_correct = user_clean == expected_clean else: is_correct = False- "user_cleaned": user_payload.strip().lower() if expected_payload else "N/A", - "expected_cleaned": expected_payload.strip().lower() if expected_payload else "N/A", + "user_cleaned": user_clean if expected_payload else "N/A", + "expected_cleaned": expected_clean if expected_payload else "N/A",Also applies to: 168-170
112-125: Also record lab access/completion timestamps after theory submissions.
- Update
last_accessedon the user’s lab progress for every submission.- Set
completed_atwhen the lab reaches 100% for the first time.Apply this diff:
# Create or update lab progress - user_lab_progress, lab_created = UserLabProgress.objects.get_or_create(user=request.user, lab=lab) + user_lab_progress, lab_created = UserLabProgress.objects.get_or_create(user=request.user, lab=lab) + # Bump last access timestamp + user_lab_progress.last_accessed = timezone.now() + # If lab is now complete, set completed_at once + if user_lab_progress.calculate_progress_percentage() == 100 and not user_lab_progress.completed_at: + user_lab_progress.completed_at = timezone.now() + user_lab_progress.save(update_fields=["last_accessed", "completed_at"]) + else: + user_lab_progress.save(update_fields=["last_accessed"])
149-162: Mirror lab progress timestamp updates for simulation submissions.Keep lab-level last access/current completion consistent across both branches.
Apply this diff:
# Create or update lab progress - user_lab_progress, lab_created = UserLabProgress.objects.get_or_create(user=request.user, lab=lab) + user_lab_progress, lab_created = UserLabProgress.objects.get_or_create(user=request.user, lab=lab) + user_lab_progress.last_accessed = timezone.now() + if user_lab_progress.calculate_progress_percentage() == 100 and not user_lab_progress.completed_at: + user_lab_progress.completed_at = timezone.now() + user_lab_progress.save(update_fields=["last_accessed", "completed_at"]) + else: + user_lab_progress.save(update_fields=["last_accessed"])website/admin.py (2)
773-779: Optimize admin list views with select_related.Listing
UserTaskProgresscan incur N+1 queries for user/task/lab. Addlist_select_relatedto reduce DB hits.Apply this diff:
class UserTaskProgressAdmin(admin.ModelAdmin): list_display = ("user", "task", "completed", "attempts", "completed_at", "last_attempt_at") list_filter = ("completed", "task__lab", "task__task_type", "completed_at") search_fields = ("user__username", "task__name", "task__lab__name") readonly_fields = ("created_at", "updated_at") raw_id_fields = ("user", "task") + list_select_related = ("user", "task", "task__lab")
781-792: Same for UserLabProgress admin to avoid N+1 queries.Add
list_select_relatedfor user/lab, sinceget_progresswill still query for counts; this at least reduces FKs overhead.Apply this diff:
class UserLabProgressAdmin(admin.ModelAdmin): list_display = ("user", "lab", "get_progress", "started_at", "completed_at", "last_accessed") list_filter = ("lab", "completed_at", "started_at") search_fields = ("user__username", "lab__name") readonly_fields = ("created_at", "updated_at") raw_id_fields = ("user", "lab") + list_select_related = ("user", "lab")website/migrations/0246_add_user_progress_models.py (5)
21-23: “last_accessed” will update on any save, not just user accessUsing auto_now=True ties last_accessed to every save (e.g., toggling completed flags), which may not represent “access”. Consider making it nullable and explicitly updating when a user opens/views a lab.
Apply this change if you want explicit access tracking:
-("last_accessed", models.DateTimeField(auto_now=True)), +("last_accessed", models.DateTimeField(blank=True, null=True)),Then set last_accessed in the view/service layer when a user actually accesses the lab.
52-52: “last_attempt_at” should reflect attempts onlyauto_now=True will refresh on any save, not necessarily an attempt. Prefer explicit updates when attempts increment.
Suggested adjustment:
-("last_attempt_at", models.DateTimeField(auto_now=True)), +("last_attempt_at", models.DateTimeField(blank=True, null=True)),Then set last_attempt_at only when a submission/attempt is recorded.
39-44: Prefer explicit UniqueConstraint over unique_together (forward-looking)unique_together works but is legacy. UniqueConstraint provides better naming and extensibility.
Minimal change in this migration:
options={ "verbose_name": "User Lab Progress", - "verbose_name_plural": "User Lab Progress", - "unique_together": {("user", "lab")}, + "verbose_name_plural": "User Lab Progress", }, ), @@ options={ "verbose_name": "User Task Progress", - "verbose_name_plural": "User Task Progress", - "unique_together": {("user", "task")}, + "verbose_name_plural": "User Task Progress", }, ),Add constraints after the CreateModel operations:
# Insert these entries into the `operations` list after the two CreateModel blocks migrations.AddConstraint( model_name="userlabprogress", constraint=models.UniqueConstraint( fields=("user", "lab"), name="uniq_user_lab_progress_user_lab" ), ), migrations.AddConstraint( model_name="usertaskprogress", constraint=models.UniqueConstraint( fields=("user", "task"), name="uniq_user_task_progress_user_task" ), ),Also applies to: 71-76
40-41: Nit: Adjust plural labels for admin readabilityCurrent verbose_name_plural mirrors singular. Consider a clearer plural form.
For example:
-"verbose_name_plural": "User Lab Progress", +"verbose_name_plural": "User Lab Progress records", -"verbose_name_plural": "User Task Progress", +"verbose_name_plural": "User Task Progress records",Also applies to: 72-73
53-53: Storing free-form “user_answer” may include sensitive data—plan for handlingDepending on the lab, answers can contain tokens, headers, or URLs with secrets. Define a retention and protection strategy (sanitization, encryption-at-rest, and/or expiring old answers).
Options:
- Sanitize before save (strip cookies/tokens).
- Use an encrypted field (e.g., a Fernet-encrypted TextField) where regulation requires.
- Add a scheduled job to purge old user_answer values after N days.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (6)
website/admin.py(2 hunks)website/management/commands/create_csrf_tasks.py(1 hunks)website/migrations/0246_add_user_progress_models.py(1 hunks)website/models.py(1 hunks)website/templates/task_detail.html(1 hunks)website/views/Simulation.py(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
website/admin.py (1)
website/models.py (3)
UserLabProgress(2553-2588)UserTaskProgress(2529-2550)calculate_progress_percentage(2570-2580)
website/management/commands/create_csrf_tasks.py (1)
website/models.py (4)
Labs(2458-2483)TaskContent(2510-2526)Tasks(2486-2507)update_total_tasks(2468-2475)
website/views/Simulation.py (1)
website/models.py (13)
Labs(2458-2483)TaskContent(2510-2526)Tasks(2486-2507)UserLabProgress(2553-2588)UserTaskProgress(2529-2550)calculate_progress_percentage(2570-2580)save(75-78)save(220-232)save(1124-1151)save(1259-1262)save(1503-1525)save(2222-2225)save(2640-2651)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (5)
website/views/Simulation.py (1)
26-32: LGTM: safe fallback for per-lab progress on dashboard.Gracefully handles missing
UserLabProgressand surfaces a numeric progress value. Looks good.website/admin.py (1)
794-796: Admin registrations look good.Progress models are properly registered with lean, useful columns and filters.
website/migrations/0246_add_user_progress_models.py (3)
14-77: LGTM: Migration structure and relationships look consistentDependencies, FK relations, and uniqueness semantics are sensible for per-user lab/task progress tracking. No blockers from the migration’s shape.
27-27: No changes needed for model labels in migrationsThe investigation shows that your model classes are declared as
class Labs(models.Model)andclass Tasks(models.Model)inwebsite/models.py. Accordingly, the migration strings:
to="website.labs"in0246_add_user_progress_models.pyto="website.tasks"in0246_add_user_progress_models.py(and the identical references in
0244_tasks_taskcontent.py) correctly match the lowercase plural model names. No updates are required.
18-18: No changes needed: DEFAULT_AUTO_FIELD is AutoField and migrations use AutoField
The project’s DEFAULT_AUTO_FIELD is set todjango.db.models.AutoField, and all existing migrations—including this one—useAutoFieldfor primary keys. This migration is consistent with the established convention.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
website/management/commands/create_sql_injection_tasks.py (2)
339-344: Avoid overriding manual activation state (is_active) on every runIncluding is_active=True in defaults will force-activate tasks each time this command runs, potentially undoing intentional admin toggles. Since the model default is already True, you can omit it here to preserve existing state while still creating new tasks as active.
Apply this diff to stop forcing is_active:
defaults={ "name": task_data["name"], "description": task_data["description"], "task_type": task_data["task_type"], - "is_active": True, },
367-371: Make the log message reflect updates, not existenceupdate_or_create updates the row when created is False, so “already exists” is misleading. Prefer “Updated” (or “Ensured/updated”) to reflect the actual behavior. Also, this is a successful outcome, not a warning.
Apply this diff:
- else: - self.stdout.write(self.style.WARNING(f'Task "{task.name}" already exists')) + else: + self.stdout.write(self.style.SUCCESS(f'Updated task: "{task.name}"'))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (3)
website/management/commands/create_csrf_tasks.py(1 hunks)website/management/commands/create_sql_injection_tasks.py(1 hunks)website/models.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- website/management/commands/create_csrf_tasks.py
- website/models.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
website/management/commands/create_sql_injection_tasks.py (1)
website/models.py (1)
Tasks(2486-2507)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (2)
website/management/commands/create_sql_injection_tasks.py (2)
336-344: Correct pivot to update_or_create keyed by (lab, order)This aligns with the model’s unique_together constraint and makes the seeding idempotent. Moving name into defaults ensures renames propagate cleanly.
334-377: Consistency Check Passed: SQLi and CSRF Seeders AlignBoth
create_sql_injection_tasks.pyandcreate_csrf_tasks.pyuse:
Tasks.objects.update_or_create(lab=…, order=…, defaults={ "name":…, "description":…, "task_type":…, "is_active": True })TaskContent.objects.update_or_create(task=task, defaults=…)No drift detected between the two commands. If you intentionally want to force
is_active=True, everything is consistent; otherwise, feel free to remove that field from the defaults in both seeders.
Fixes: #4492
Summary by CodeRabbit
New Features
Admin
Database/Migrations
Management Commands