Added Labs: Under Security Labs Added More Labs #4628
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRefactored the task detail template to replace nested per-lab conditionals with explicit blocks for multiple security lab types and unified MCQ/simulation result rendering and submission flow. Added a new Django management command to seed six security labs and their tasks within a single atomic transaction. Changes
Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Knowledge base: Disabled due to 📒 Files selected for processing (2)
🧰 Additional context used🪛 Ruff (0.14.4)website/management/commands/seed_all_security_lab.py10-10: Unused method argument: (ARG002) 10-10: Unused method argument: (ARG002) ⏰ 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)
🔇 Additional comments (3)
Tip 📝 Customizable high-level summaries are now available!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example:
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (16)
website/templates/task_detail.html (1)
274-341: Client-side handlers are well-implemented.The JavaScript submission handlers correctly:
- Include CSRF tokens in fetch requests
- Handle errors gracefully
- Provide visual feedback for success/failure
- Prevent default form submission
One minor suggestion: consider replacing
alert()calls with inline error messages for better UX consistency with the existing result displays.Optional: Replace alert() with inline error messages:
.catch(error => { console.error('Error:', error); - alert('An error occurred. Please try again.'); + const resultDiv = document.getElementById('mcq-result'); + const messageDiv = document.getElementById('result-message'); + resultDiv.classList.remove('hidden'); + messageDiv.textContent = 'An error occurred. Please try again.'; + messageDiv.className = 'text-sm font-medium text-red-600'; });website/management/commands/create_file_upload_tasks.py (3)
8-8: Unused method parameters.The
argsandkwargsparameters are standard in Django management commands but unused here. While this is a minor nitpick, you can clean it up for consistency.- def handle(self, *args, **kwargs): + def handle(self, *args, **options):Or simply acknowledge they're unused by the Django framework pattern.
222-222: Unused unpacked variable.The
task_contentvariable fromupdate_or_create()is never used. Prefix it with an underscore to indicate it's intentionally ignored.- task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data) + _task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data)
1-237: Consider extracting common command logic.All six new lab seed commands share nearly identical structure (get lab, upsert tasks, update total). Consider creating a base class or helper function to reduce duplication and improve maintainability.
Example base class approach:
class BaseLabTaskCommand(BaseCommand): lab_name: str tasks_data: list def handle(self, *args, **options): try: lab = Labs.objects.get(name=self.lab_name) except Labs.DoesNotExist: self.stdout.write(self.style.ERROR(f"{self.lab_name} lab not found. Please run create_initial_labs first.")) return for task_data in self.tasks_data: task, created = Tasks.objects.update_or_create(...) # ... rest of common logic lab.update_total_tasks() self.stdout.write(self.style.SUCCESS(f"{self.lab_name} lab setup complete with {lab.total_tasks} tasks"))website/management/commands/create_ssrf_tasks.py (3)
8-8: Unused method parameters.Same issue as in other command files -
argsandkwargsare unused.
238-238: Unused unpacked variable.Prefix
task_contentwith underscore to indicate it's intentionally unused.- task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data) + _task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data)
27-27: Ambiguous quotation marks in strings.Lines 27 and 59 use RIGHT SINGLE QUOTATION MARK (') instead of APOSTROPHE ('). While this won't break functionality, it can cause encoding issues in some contexts.
Replace ambiguous quotes:
- Line 27:
Server-Side Request Forgery (SSRF)?should use'- Line 59:
user's browsershould use'Also applies to: 59-59
website/management/commands/create_open_redirect_tasks.py (2)
8-8: Unused method parameters.The
argsandkwargsparameters are unused.
227-227: Unused unpacked variable.Prefix
task_contentwith underscore.- task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data) + _task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data)website/management/commands/create_broken_auth_tasks.py (2)
8-8: Unused method parameters.The
argsandkwargsparameters are unused.
327-327: Unused unpacked variable.Prefix
task_contentwith underscore.- task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data) + _task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data)website/management/commands/create_data_exposure_tasks.py (2)
8-8: Unused method parameters.The
argsandkwargsparameters are unused.
269-269: Unused unpacked variable.Prefix
task_contentwith underscore.- task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data) + _task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data)website/management/commands/create_idor_tasks.py (3)
8-8: Unused method parameters.The
argsandkwargsparameters are unused.
180-180: Unused unpacked variable.Prefix
task_contentwith underscore.- task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data) + _task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data)
63-63: Ambiguous quotation marks in strings.Lines 63, 69, and 82 use RIGHT SINGLE QUOTATION MARK (') instead of APOSTROPHE (').
Replace with standard apostrophes for better encoding compatibility.
Also applies to: 69-69, 82-82
📜 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 (8)
website/management/commands/create_broken_auth_tasks.py(1 hunks)website/management/commands/create_data_exposure_tasks.py(1 hunks)website/management/commands/create_file_upload_tasks.py(1 hunks)website/management/commands/create_idor_tasks.py(1 hunks)website/management/commands/create_initial_tasks.py(1 hunks)website/management/commands/create_open_redirect_tasks.py(1 hunks)website/management/commands/create_ssrf_tasks.py(1 hunks)website/templates/task_detail.html(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.0)
website/management/commands/create_file_upload_tasks.py
8-8: Unused method argument: args
(ARG002)
8-8: Unused method argument: kwargs
(ARG002)
222-222: Unpacked variable task_content is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
website/management/commands/create_data_exposure_tasks.py
8-8: Unused method argument: args
(ARG002)
8-8: Unused method argument: kwargs
(ARG002)
269-269: Unpacked variable task_content is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
website/management/commands/create_idor_tasks.py
8-8: Unused method argument: args
(ARG002)
8-8: Unused method argument: kwargs
(ARG002)
63-63: String contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF001)
69-69: String contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF001)
82-82: String contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF001)
180-180: Unpacked variable task_content is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
website/management/commands/create_broken_auth_tasks.py
8-8: Unused method argument: args
(ARG002)
8-8: Unused method argument: kwargs
(ARG002)
327-327: Unpacked variable task_content is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
website/management/commands/create_open_redirect_tasks.py
8-8: Unused method argument: args
(ARG002)
8-8: Unused method argument: kwargs
(ARG002)
227-227: Unpacked variable task_content is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
website/management/commands/create_ssrf_tasks.py
8-8: Unused method argument: args
(ARG002)
8-8: Unused method argument: kwargs
(ARG002)
27-27: String contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF001)
59-59: String contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF001)
238-238: Unpacked variable task_content is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
⏰ 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 (3)
website/management/commands/create_initial_tasks.py (1)
36-71: LGTM! Clean expansion of lab definitions.The new lab entries follow the established pattern and provide clear, educational descriptions. The estimated times and ordering are consistent with the existing structure.
website/templates/task_detail.html (2)
146-244: LGTM! Per-lab payload inputs are well-organized.The conditional rendering of lab-specific payload inputs with contextual placeholders is a good UX pattern. The fallback generic textarea ensures all labs are covered.
48-48: No XSS risk: theory_content is admin-only
theory_content is only modifiable via Django admin or management commands, so rendering it with|safedoes not expose untrusted input.
github-project-automation
bot
moved this from Backlog
to Ready
in 📌 OWASP BLT Project Board
There was a problem hiding this comment.
Pull Request Overview
This pull request adds six new security labs to the platform: Broken Authentication, IDOR, File Upload Vulnerabilities, Sensitive Data Exposure, Open Redirect, and SSRF. Each lab includes comprehensive theory and simulation tasks with MCQ questions and interactive payload testing.
Key Changes:
- Added 6 new security vulnerability labs with educational content
- Enhanced task detail template with dynamic per-lab payload inputs and improved UI
- Created Django management commands to populate labs with theory and simulation tasks
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| website/templates/task_detail.html | Enhanced with lab-specific payload inputs, improved layout, and refactored JavaScript handlers |
| website/management/commands/create_initial_tasks.py | Added 6 new lab definitions with descriptions and estimated completion times |
| website/management/commands/create_broken_auth_tasks.py | Complete task set for Broken Authentication lab covering default credentials, session fixation, JWT issues, and brute force |
| website/management/commands/create_idor_tasks.py | IDOR lab tasks covering theory and simulation scenarios for object reference vulnerabilities |
| website/management/commands/create_file_upload_tasks.py | File upload vulnerability tasks including web shell uploads, MIME type bypasses, and path traversal |
| website/management/commands/create_data_exposure_tasks.py | Sensitive data exposure lab with theory and simulations for various data leak scenarios |
| website/management/commands/create_open_redirect_tasks.py | Open redirect vulnerability lab covering phishing scenarios and bypass techniques |
| website/management/commands/create_ssrf_tasks.py | SSRF lab tasks including internal access, redirection chains, and filter bypasses |
Nachiket-Roy
force-pushed
the
feature/added-more-labs
branch
from
3698c10 to
68ca240
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
website/templates/task_detail.html (1)
274-341: Move inline JS to a static file and harden fetch handling.Extract to
website/static/js/task_detail.js(defer or module import) for CSP, caching, and maintainability. Also checkresponse.okand guard JSON parsing.Apply this diff to improve robustness inline (if extraction deferred):
- fetch("{% url 'submit_answer' lab.id task.id %}", { + fetch("{% url 'submit_answer' lab.id task.id %}", { method: 'POST', headers: { 'X-CSRFToken': document.querySelector('[name=csrfmiddlewaretoken]').value, 'Content-Type': 'application/x-www-form-urlencoded', }, body: 'answer=' + encodeURIComponent(selectedAnswer) }) - .then(response => response.json()) + .then(async (response) => { + if (!response.ok) { + const text = await response.text().catch(() => ''); + throw new Error(`HTTP ${response.status}: ${text.slice(0,200)}`); + } + const ct = response.headers.get('content-type') || ''; + if (!ct.includes('application/json')) throw new Error('Unexpected response type'); + return response.json(); + })Mirror the same pattern for the simulation fetch block.
🧹 Nitpick comments (25)
website/templates/task_detail.html (4)
60-67: Make MCQ option values explicit; avoid relying on first character.Using
value="{{ option.0 }}"assumes options start with “A)”, “B)”, etc. This is brittle for i18n or different option formats. Prefer explicit (value, text) pairs.Apply this diff (template side), expecting
content.mcq_optionsto be a list of 2-tuples like("A", "Whitelist …"):-{% for option in content.mcq_options %} +{% for value, text in content.mcq_options %} <label class="flex items-center p-3 border border-gray-200 rounded-lg hover:bg-gray-50 cursor-pointer"> <input type="radio" name="answer" - value="{{ option.0 }}" + value="{{ value }}" class="h-4 w-4 text-[#e74c3c] focus:ring-[#e74c3c] border-gray-300"> - <span class="ml-3 text-sm text-gray-900">{{ option }}</span> + <span class="ml-3 text-sm text-gray-900">{{ text }}</span> </label> {% endfor %}If changing seed data isn’t feasible now, at least split on
') 'defensively in the template to extract the leading key.
145-244: Replace long{% elif %}chain onlab.namewith config-driven rendering.String-matching on display names is fragile (rename/i18n breaks it). Drive inputs from a stable code/slug or from
content.simulation_config(e.g.,input_type,placeholder).Minimal diff showing slug usage:
-{% if lab.name == "SQL Injection" %} +{% if lab.slug == "sqli" %} ... -{% elif lab.name == "Cross-Site Scripting (XSS)" %} +{% elif lab.slug == "xss" %} ...Better: include in
simulation_config:-<form id="simulation-form"> +<form id="simulation-form" data-input-type="{{ content.simulation_config.input_type|default:'textarea' }}" + data-placeholder="{{ content.simulation_config.placeholder|default:'Enter your payload here...' }}"> ... -{% else %} - ... generic textarea ... -{% endif %} +{% if content.simulation_config.input_type == 'input' %} + <input id="payload" name="payload" type="text" ... placeholder="{{ content.simulation_config.placeholder }}"> +{% else %} + <textarea id="payload" name="payload" rows="4" ... placeholder="{{ content.simulation_config.placeholder }}"></textarea> +{% endif %}This removes the brittle name checks and reduces template size.
269-269: Remove empty layout stub.
<div class="mt-8 flex justify-between"></div>is empty and can be removed.
285-291: CSRF token lookup should target the relevant form.
document.querySelector('[name=csrfmiddlewaretoken]')returns the first token; safer to scope to the form in case multiple forms exist in future.- 'X-CSRFToken': document.querySelector('[name=csrfmiddlewaretoken]').value, + 'X-CSRFToken': form.querySelector('[name=csrfmiddlewaretoken]').value,And for simulation:
- 'X-CSRFToken': document.querySelector('[name=csrfmiddlewaretoken]').value, + 'X-CSRFToken': e.currentTarget.querySelector('[name=csrfmiddlewaretoken]').value,Also applies to: 318-324
website/management/commands/create_file_upload_tasks.py (3)
8-8: Silence unused parameters.
handle(self, *args, **kwargs)doesn’t useargs/kwargs. Rename to_args/_kwargsto satisfy Ruff ARG002.- def handle(self, *args, **kwargs): + def handle(self, *_args, **_kwargs):
222-222: Prefix unused variable fromupdate_or_create.
task_contentisn’t used. Prefix with underscore to satisfy Ruff RUF059.- task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data) + _task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data)
8-16: Wrap seeding in a transaction for atomicity.If any step fails midway, you can leave partially-updated tasks. Use
transaction.atomic()to commit all-or-nothing.+from django.db import transaction ... - def handle(self, *_args, **_kwargs): + def handle(self, *_args, **_kwargs): + with transaction.atomic(): try: upload_lab = Labs.objects.get(name="File Upload Vulnerabilities") except Labs.DoesNotExist: self.stdout.write( self.style.ERROR("File Upload Vulnerabilities lab not found. Please run create_initial_labs first.") ) return ... - upload_lab.update_total_tasks() - self.stdout.write( - self.style.SUCCESS(f"File Upload Vulnerabilities lab setup complete with {upload_lab.total_tasks} tasks") - ) + upload_lab.update_total_tasks() + self.stdout.write(self.style.SUCCESS( + f"File Upload Vulnerabilities lab setup complete with {upload_lab.total_tasks} tasks"))Also applies to: 196-237, 234-237
website/management/commands/create_open_redirect_tasks.py (3)
8-8: Silence unused parameters.- def handle(self, *args, **kwargs): + def handle(self, *_args, **_kwargs):
227-227: Prefix unused variable fromupdate_or_create.- task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data) + _task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data)
8-14: Guard withtransaction.atomic()for all-or-nothing seeding.+from django.db import transaction ... - def handle(self, *_args, **_kwargs): - try: + def handle(self, *_args, **_kwargs): + with transaction.atomic(): + try: or_lab = Labs.objects.get(name="Open Redirect") - except Labs.DoesNotExist: + except Labs.DoesNotExist: self.stdout.write(self.style.ERROR("Open Redirect lab not found. Please run create_initial_labs first.")) return ... - or_lab.update_total_tasks() - self.stdout.write(self.style.SUCCESS(f"Open Redirect lab setup complete with {or_lab.total_tasks} tasks")) + or_lab.update_total_tasks() + self.stdout.write(self.style.SUCCESS(f"Open Redirect lab setup complete with {or_lab.total_tasks} tasks"))Also applies to: 202-239
website/management/commands/create_ssrf_tasks.py (4)
8-8: Silence unused parameters.- def handle(self, *args, **kwargs): + def handle(self, *_args, **_kwargs):
238-238: Prefix unused variable fromupdate_or_create.- task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data) + _task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data)
25-54: Normalize curly quotes to ASCII for copy-paste fidelity.Strings include
’(U+2019). Replace with'to avoid issues in rendered HTML and linters (RUF001).Example:
-<p>... user’s privileges ...</p> +<p>... user's privileges ...</p>Apply similarly across theory content blocks.
Also applies to: 57-63
8-18: Wrap entire seed intransaction.atomic().+from django.db import transaction ... - def handle(self, *_args, **_kwargs): - try: + def handle(self, *_args, **_kwargs): + with transaction.atomic(): + try: ssrf_lab = Labs.objects.get(name="Server-Side Request Forgery (SSRF)") - except Labs.DoesNotExist: + except Labs.DoesNotExist: self.stdout.write( self.style.ERROR( "Server-Side Request Forgery (SSRF) lab not found. Please run create_initial_labs first." ) ) return ... - ssrf_lab.update_total_tasks() - self.stdout.write(self.style.SUCCESS(f"SSRF lab setup complete with {ssrf_lab.total_tasks} tasks")) + ssrf_lab.update_total_tasks() + self.stdout.write(self.style.SUCCESS(f"SSRF lab setup complete with {ssrf_lab.total_tasks} tasks"))Also applies to: 208-251
website/management/commands/create_broken_auth_tasks.py (3)
8-8: Silence unused parameters.- def handle(self, *args, **kwargs): + def handle(self, *_args, **_kwargs):
327-327: Prefix unused variable fromupdate_or_create.- task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data) + _task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data)
8-18: Usetransaction.atomic()to ensure all-or-nothing task seeding.+from django.db import transaction ... - def handle(self, *_args, **_kwargs): - try: + def handle(self, *_args, **_kwargs): + with transaction.atomic(): + try: auth_lab = Labs.objects.get(name="Broken Authentication") - except Labs.DoesNotExist: + except Labs.DoesNotExist: self.stdout.write( self.style.ERROR( "Broken Authentication lab not found. Please run create_initial_labs first." ) ) return ... - auth_lab.update_total_tasks() - self.stdout.write(self.style.SUCCESS(f"Broken Authentication lab setup complete with {auth_lab.total_tasks} tasks")) + auth_lab.update_total_tasks() + self.stdout.write(self.style.SUCCESS( + f"Broken Authentication lab setup complete with {auth_lab.total_tasks} tasks"))Also applies to: 302-340
website/management/commands/create_idor_tasks.py (4)
8-8: Silence lint for unused args/kwargs.Rename to underscore-prefixed to appease Ruff ARG002.
- def handle(self, *args, **kwargs): + def handle(self, *_args, **_kwargs):
180-180: Prefix unused variable to satisfy Ruff RUF059.The returned object isn’t used.
- task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data) + _task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data)
1-3: Wrap seeding in a transaction for consistency.Atomic ensures all tasks/content are applied together; avoids partial state if an error occurs mid‑loop.
-from django.core.management.base import BaseCommand +from django.core.management.base import BaseCommand +from django.db import transaction @@ - for task_data in tasks_data: - task, created = Tasks.objects.update_or_create( + with transaction.atomic(): + for task_data in tasks_data: + task, created = Tasks.objects.update_or_create( lab=idor_lab, order=task_data["order"], defaults={ "name": task_data["name"], "description": task_data["description"], "task_type": task_data["task_type"], "is_active": True, }, - ) + ) @@ - task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data) + _task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data) @@ - idor_lab.update_total_tasks() - self.stdout.write(self.style.SUCCESS(f"IDOR lab setup complete with {idor_lab.total_tasks} tasks")) + idor_lab.update_total_tasks() + idor_lab.refresh_from_db(fields=["total_tasks"]) + self.stdout.write(self.style.SUCCESS(f"IDOR lab setup complete with {idor_lab.total_tasks} tasks"))Also applies to: 155-193
192-193: Ensure printed total reflects DB value.If update_total_tasks saves to DB, refresh the in‑memory object before printing.
- idor_lab.update_total_tasks() - self.stdout.write(self.style.SUCCESS(f"IDOR lab setup complete with {idor_lab.total_tasks} tasks")) + idor_lab.update_total_tasks() + idor_lab.refresh_from_db(fields=["total_tasks"]) + self.stdout.write(self.style.SUCCESS(f"IDOR lab setup complete with {idor_lab.total_tasks} tasks"))website/management/commands/create_data_exposure_tasks.py (4)
8-8: Silence lint for unused args/kwargs.Rename to underscore‑prefixed names.
- def handle(self, *args, **kwargs): + def handle(self, *_args, **_kwargs):
269-269: Prefix unused variable to satisfy Ruff RUF059.Discard the object; keep the created flag.
- task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data) + _task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data)
1-3: Make seeding atomic.Apply a single transaction for consistent writes across tasks and contents.
-from django.core.management.base import BaseCommand +from django.core.management.base import BaseCommand +from django.db import transaction @@ - for task_data in tasks_data: - task, created = Tasks.objects.update_or_create( + with transaction.atomic(): + for task_data in tasks_data: + task, created = Tasks.objects.update_or_create( lab=sde_lab, order=task_data["order"], defaults={ "name": task_data["name"], "description": task_data["description"], "task_type": task_data["task_type"], "is_active": True, }, - ) + ) @@ - task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data) + _task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data) @@ - sde_lab.update_total_tasks() - self.stdout.write(self.style.SUCCESS(f"Sensitive Data Exposure lab setup complete with {sde_lab.total_tasks} tasks")) + sde_lab.update_total_tasks() + sde_lab.refresh_from_db(fields=["total_tasks"]) + self.stdout.write(self.style.SUCCESS(f"Sensitive Data Exposure lab setup complete with {sde_lab.total_tasks} tasks"))Also applies to: 239-282
281-282: Refresh before printing the total.Guarantees the printed value matches DB state.
- sde_lab.update_total_tasks() - self.stdout.write(self.style.SUCCESS(f"Sensitive Data Exposure lab setup complete with {sde_lab.total_tasks} tasks")) + sde_lab.update_total_tasks() + sde_lab.refresh_from_db(fields=["total_tasks"]) + self.stdout.write(self.style.SUCCESS(f"Sensitive Data Exposure lab setup complete with {sde_lab.total_tasks} tasks"))
📜 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 (8)
website/management/commands/create_broken_auth_tasks.py(1 hunks)website/management/commands/create_data_exposure_tasks.py(1 hunks)website/management/commands/create_file_upload_tasks.py(1 hunks)website/management/commands/create_idor_tasks.py(1 hunks)website/management/commands/create_initial_tasks.py(1 hunks)website/management/commands/create_open_redirect_tasks.py(1 hunks)website/management/commands/create_ssrf_tasks.py(1 hunks)website/templates/task_detail.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- website/management/commands/create_initial_tasks.py
🧰 Additional context used
🪛 Ruff (0.14.1)
website/management/commands/create_idor_tasks.py
8-8: Unused method argument: args
(ARG002)
8-8: Unused method argument: kwargs
(ARG002)
63-63: String contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF001)
69-69: String contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF001)
82-82: String contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF001)
180-180: Unpacked variable task_content is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
website/management/commands/create_data_exposure_tasks.py
8-8: Unused method argument: args
(ARG002)
8-8: Unused method argument: kwargs
(ARG002)
269-269: Unpacked variable task_content is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
website/management/commands/create_open_redirect_tasks.py
8-8: Unused method argument: args
(ARG002)
8-8: Unused method argument: kwargs
(ARG002)
227-227: Unpacked variable task_content is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
website/management/commands/create_ssrf_tasks.py
8-8: Unused method argument: args
(ARG002)
8-8: Unused method argument: kwargs
(ARG002)
27-27: String contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF001)
59-59: String contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF001)
238-238: Unpacked variable task_content is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
website/management/commands/create_file_upload_tasks.py
8-8: Unused method argument: args
(ARG002)
8-8: Unused method argument: kwargs
(ARG002)
222-222: Unpacked variable task_content is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
website/management/commands/create_broken_auth_tasks.py
8-8: Unused method argument: args
(ARG002)
8-8: Unused method argument: kwargs
(ARG002)
327-327: Unpacked variable task_content is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
⏰ 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/management/commands/create_file_upload_tasks.py (1)
196-206: LGTM on idempotent upsert pattern.Using
update_or_createkeyed by(lab, order)with clear defaults is correct and repeatable for seeds.website/management/commands/create_open_redirect_tasks.py (1)
203-212: LGTM on idempotent upsert pattern.website/management/commands/create_ssrf_tasks.py (1)
209-218: LGTM on idempotent upsert pattern.website/management/commands/create_broken_auth_tasks.py (1)
303-312: LGTM on consistent upsert pattern and clear task metadata.website/management/commands/create_idor_tasks.py (1)
1-193: No issues identified—all model contracts and lab names verified.The code correctly uses lab names, task types, and TaskContent fields that are properly defined in the Django models. Labs.update_total_tasks() is called, and the conditional logic for populating theory versus simulation content aligns with the Tasks.TASK_TYPES choices.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
website/management/commands/create_idor_tasks.py (3)
61-61: Replace curly quotes with straight apostrophes.The curly quote in "it's" triggers RUF001. This issue was flagged in a previous review and remains unresolved.
Apply this diff:
- <p>If changing these identifiers reveals unauthorized information, it's an IDOR vulnerability.</p> + <p>If changing these identifiers reveals unauthorized information, it's an IDOR vulnerability.</p>
67-67: Replace curly quotes with straight apostrophes.The curly quote in "user B's" triggers RUF001. This issue was flagged in a previous review and remains unresolved.
Apply this diff:
- <p>If user A can access user B's data by changing the ID, the endpoint lacks proper access control.</p> + <p>If user A can access user B's data by changing the ID, the endpoint lacks proper access control.</p>
80-80: Replace curly quotes with straight apostrophes.The curly quote in "user's" triggers RUF001. This issue was flagged in a previous review and remains unresolved.
Apply this diff:
- "description": "Try to access another user's profile by modifying the user_id parameter.", + "description": "Try to access another user's profile by modifying the user_id parameter.",
🧹 Nitpick comments (5)
website/management/commands/create_idor_tasks.py (3)
1-3: Consolidate imports from the same module.Lines 1 and 3 import from the same module. Combine them for cleaner code.
Apply this diff:
-from django.core.management.base import BaseCommand from website.models import Labs, TaskContent, Tasks -from django.core.management.base import CommandError +from django.core.management.base import BaseCommand, CommandError
12-15: Consider exception chaining for traceback preservation.While the current error handling is functional, using
raise ... from excpreserves the exception chain for debugging.Apply this diff:
- except Labs.DoesNotExist: + except Labs.DoesNotExist as exc: raise CommandError( "Insecure Direct Object Reference (IDOR) lab not found. Please run create_initial_labs first." - ) + ) from exc
178-178: Prefix unused variable with underscore.
task_contentis unpacked but never used. Signal this intent to silence RUF059.Apply this diff:
- task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data) + _task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data)website/management/commands/create_data_exposure_tasks.py (2)
9-15: Good fix! CommandError is now raised as requested in the past review.The exception handling properly fails fast with a non-zero exit. One minor improvement: consider adding exception chaining to distinguish this from errors in exception handling itself.
Apply this diff to add proper exception chaining:
except Labs.DoesNotExist: - raise CommandError( + raise CommandError( "Sensitive Data Exposure lab not found. Please run create_initial_labs first." - ) + ) from NoneNote: The static analysis warning about unused
argsandkwargsparameters is a false positive—these are part of Django's management command signature.
237-267: Prefix unused variable with underscore.The task creation logic is sound and uses
update_or_createcorrectly for idempotency. However, thetask_contentvariable on line 267 is unpacked but never used—onlycontent_createdis needed.Apply this diff:
- task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data) + _, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data)
📜 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 (2)
website/management/commands/create_data_exposure_tasks.py(1 hunks)website/management/commands/create_idor_tasks.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.1)
website/management/commands/create_idor_tasks.py
9-9: Unused method argument: args
(ARG002)
9-9: Unused method argument: kwargs
(ARG002)
13-15: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
13-15: Avoid specifying long messages outside the exception class
(TRY003)
178-178: Unpacked variable task_content is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
website/management/commands/create_data_exposure_tasks.py
9-9: Unused method argument: args
(ARG002)
9-9: Unused method argument: kwargs
(ARG002)
13-15: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
13-15: Avoid specifying long messages outside the exception class
(TRY003)
267-267: Unpacked variable task_content is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
⏰ 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 (3)
website/management/commands/create_data_exposure_tasks.py (3)
1-8: LGTM! Command structure and imports are appropriate.The imports include
CommandErrorwhich is properly used later in the exception handling, and the command structure follows Django conventions.
17-235: Well-structured and comprehensive task definitions.The tasks_data structure is thorough and educational, with a good mix of theory and simulation exercises. The HTML content is clean, MCQ questions are relevant, and simulation configs include helpful hints and clear success criteria.
269-282: LGTM! Output and finalization are well-implemented.The output messages clearly distinguish between creation and updates, and the final call to
update_total_tasks()ensures the lab's metadata stays current.
Nachiket-Roy
force-pushed
the
feature/added-more-labs
branch
from
3ffce5b to
dd63aba
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (5)
website/management/commands/create_open_redirect_tasks.py (1)
1-240: Same code duplication issue as other seed commands.This file has the identical structural duplication discussed in
create_file_upload_tasks.py. A base class refactor would eliminate this repetition.website/management/commands/create_ssrf_tasks.py (1)
1-251: Same code duplication issue as other seed commands.This file has the identical structural duplication discussed in
create_file_upload_tasks.py. A base class refactor would eliminate this repetition.website/management/commands/create_idor_tasks.py (1)
1-191: Same code duplication issue as other seed commands.This file has the identical structural duplication discussed in
create_file_upload_tasks.py. A base class refactor would eliminate this repetition.website/management/commands/create_broken_auth_tasks.py (1)
1-340: Same code duplication issue as other seed commands.This file has the identical structural duplication discussed in
create_file_upload_tasks.py. A base class refactor would eliminate this repetition.website/management/commands/create_data_exposure_tasks.py (1)
1-282: Same code duplication issue as other seed commands.This file has the identical structural duplication discussed in
create_file_upload_tasks.py. A base class refactor would eliminate this repetition.
🧹 Nitpick comments (2)
website/management/commands/create_file_upload_tasks.py (2)
8-8: Clean up unused parameters.The
argsandkwargsparameters are required by Django's command signature but unused here. Prefix with underscore to indicate they're intentionally unused.- def handle(self, *args, **kwargs): + def handle(self, *_args, **_kwargs):
222-222: Unused variable can be replaced with underscore.The
task_contentvariable is unpacked but never used.- task_content, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data) + _, content_created = TaskContent.objects.update_or_create(task=task, defaults=content_data)
📜 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 (8)
website/management/commands/create_broken_auth_tasks.py(1 hunks)website/management/commands/create_data_exposure_tasks.py(1 hunks)website/management/commands/create_file_upload_tasks.py(1 hunks)website/management/commands/create_idor_tasks.py(1 hunks)website/management/commands/create_initial_tasks.py(1 hunks)website/management/commands/create_open_redirect_tasks.py(1 hunks)website/management/commands/create_ssrf_tasks.py(1 hunks)website/templates/task_detail.html(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.3)
website/management/commands/create_file_upload_tasks.py
8-8: Unused method argument: args
(ARG002)
8-8: Unused method argument: kwargs
(ARG002)
222-222: Unpacked variable task_content is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
website/management/commands/create_open_redirect_tasks.py
8-8: Unused method argument: args
(ARG002)
8-8: Unused method argument: kwargs
(ARG002)
227-227: Unpacked variable task_content is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
website/management/commands/create_ssrf_tasks.py
8-8: Unused method argument: args
(ARG002)
8-8: Unused method argument: kwargs
(ARG002)
27-27: String contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF001)
59-59: String contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF001)
238-238: Unpacked variable task_content is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
website/management/commands/create_data_exposure_tasks.py
9-9: Unused method argument: args
(ARG002)
9-9: Unused method argument: kwargs
(ARG002)
13-15: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
13-15: Avoid specifying long messages outside the exception class
(TRY003)
267-267: Unpacked variable task_content is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
website/management/commands/create_broken_auth_tasks.py
8-8: Unused method argument: args
(ARG002)
8-8: Unused method argument: kwargs
(ARG002)
327-327: Unpacked variable task_content is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
website/management/commands/create_idor_tasks.py
9-9: Unused method argument: args
(ARG002)
9-9: Unused method argument: kwargs
(ARG002)
13-15: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
13-15: Avoid specifying long messages outside the exception class
(TRY003)
178-178: Unpacked variable task_content is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
⏰ 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 (4)
website/management/commands/create_initial_tasks.py (1)
36-71: LGTM! Well-structured lab entries.The six new lab definitions are clear, well-described, and properly sequenced. The estimated times and descriptions align well with the complexity of each security topic.
website/management/commands/create_idor_tasks.py (1)
13-15: Good use of CommandError for CI/automation.This command correctly raises
CommandErrorinstead of returning, ensuring proper exit codes. This is the pattern the other seed commands should follow.website/templates/task_detail.html (1)
1-342: Well-structured template refactor with clear per-lab inputs.The template reorganization improves clarity by:
- Separating theory, MCQ, and simulation sections into distinct cards
- Providing lab-specific payload inputs with appropriate placeholders
- Maintaining proper CSRF protection and error handling
The per-lab conditionals (lines 146-244) are appropriately tailored to each lab's requirements, with textareas for complex payloads and text inputs for simpler ones.
website/management/commands/create_data_exposure_tasks.py (1)
13-15: Good use of CommandError for CI/automation.This command correctly raises
CommandErrorinstead of returning, ensuring proper exit codes. This is the pattern the other seed commands should follow.
|
@DonnieBLT Could you please review it when you get time and let me know if it’s ready to be merged, or if any further changes are needed? Also, should I continue keeping this branch updated with periodic rebases until the PR is merged, or will that be handled during merge? Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
website/management/commands/create_ssrf_tasks.py (1)
16-16: Fix curly quote to satisfy linter.Line 16 contains a RIGHT SINGLE QUOTATION MARK (') that triggers RUF001. Replace with a straight apostrophe.
Apply this diff:
- <p>SSRF is a vulnerability that allows attackers to trick a server into making unintended HTTP requests. The attacker controls the target URL or request destination and uses the server's privileges to access internal systems or sensitive data.</p> + <p>SSRF is a vulnerability that allows attackers to trick a server into making unintended HTTP requests. The attacker controls the target URL or request destination and uses the server's privileges to access internal systems or sensitive data.</p>
🧹 Nitpick comments (8)
website/management/commands/base_lab_seeder.py (2)
11-12: Consider adding exception chaining for better error context.When re-raising an exception during exception handling, using
from Nonemakes it explicit that this is intentional error conversion rather than a secondary error.Apply this diff:
except Labs.DoesNotExist: - raise CommandError(f"{self.lab_name} lab not found. Please run create_initial_tasks first.") + raise CommandError(f"{self.lab_name} lab not found. Please run create_initial_tasks first.") from None
5-6: Consider annotating mutable class attributes withClassVar.The linter flags
tasks_dataas a mutable class attribute. While each subclass overrides this, adding type annotations improves clarity and satisfies the linter.Apply this diff:
+from typing import ClassVar + class LabSeederCommand(BaseCommand): lab_name = None # Override in subclass - tasks_data = [] # Override in subclass + tasks_data: ClassVar[list] = [] # Override in subclasswebsite/management/commands/create_open_redirect_tasks.py (1)
7-174: Consider addingClassVarannotation for consistency.Similar to the base class, the
tasks_dataclass attribute would benefit from a type annotation.Apply this diff:
+from typing import ClassVar from .base_lab_seeder import LabSeederCommand class Command(LabSeederCommand): help = "Creates Open Redirect lab tasks" lab_name = "Open Redirect" - tasks_data = [ + tasks_data: ClassVar[list] = [ # ... existing task definitions ... ]website/management/commands/create_ssrf_tasks.py (1)
8-195: Consider addingClassVarannotation for consistency.The
tasks_dataclass attribute would benefit from a type annotation, similar to other seeder commands.Apply this diff:
+from typing import ClassVar from .base_lab_seeder import LabSeederCommand class Command(LabSeederCommand): help = "Creates Server-Side Request Forgery (SSRF) lab tasks with theory and simulation content" lab_name = "Server-Side Request Forgery (SSRF)" - tasks_data = [ + tasks_data: ClassVar[list] = [ # ... existing task definitions ... ]website/management/commands/create_data_exposure_tasks.py (1)
7-208: Consider addingClassVarannotation for consistency.The
tasks_dataclass attribute would benefit from a type annotation, consistent with the pattern across seeder commands.Apply this diff:
+from typing import ClassVar from .base_lab_seeder import LabSeederCommand class Command(LabSeederCommand): help = "Creates Sensitive Data Exposure lab tasks" lab_name = "Sensitive Data Exposure" - tasks_data = [ + tasks_data: ClassVar[list] = [ # ... existing task definitions ... ]website/management/commands/create_file_upload_tasks.py (1)
7-173: Consider addingClassVarannotation for consistency.The
tasks_dataclass attribute would benefit from a type annotation to align with best practices.Apply this diff:
+from typing import ClassVar from .base_lab_seeder import LabSeederCommand class Command(LabSeederCommand): help = "Creates File Upload Vulnerabilities lab tasks" lab_name = "File Upload Vulnerabilities" - tasks_data = [ + tasks_data: ClassVar[list] = [ # ... existing task definitions ... ]website/management/commands/create_idor_tasks.py (1)
7-136: Consider addingClassVarannotation for consistency.The
tasks_dataclass attribute would benefit from a type annotation to match the pattern across all seeder commands.Apply this diff:
+from typing import ClassVar from .base_lab_seeder import LabSeederCommand class Command(LabSeederCommand): help = "Creates Insecure Direct Object Reference (IDOR) lab tasks" lab_name = "Insecure Direct Object Reference (IDOR)" - tasks_data = [ + tasks_data: ClassVar[list] = [ # ... existing task definitions ... ]website/management/commands/create_broken_auth_tasks.py (1)
7-276: Consider addingClassVarannotation for consistency.The
tasks_dataclass attribute would benefit from a type annotation, maintaining consistency across all seeder commands.Apply this diff:
+from typing import ClassVar from .base_lab_seeder import LabSeederCommand class Command(LabSeederCommand): help = "Creates Broken Authentication / Authentication Bypass lab tasks" lab_name = "Broken Authentication" - tasks_data = [ + tasks_data: ClassVar[list] = [ # ... existing task definitions ... ]
📜 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 (7)
website/management/commands/base_lab_seeder.py(1 hunks)website/management/commands/create_broken_auth_tasks.py(1 hunks)website/management/commands/create_data_exposure_tasks.py(1 hunks)website/management/commands/create_file_upload_tasks.py(1 hunks)website/management/commands/create_idor_tasks.py(1 hunks)website/management/commands/create_open_redirect_tasks.py(1 hunks)website/management/commands/create_ssrf_tasks.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.3)
website/management/commands/create_file_upload_tasks.py
7-173: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
website/management/commands/base_lab_seeder.py
6-6: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
8-8: Unused method argument: args
(ARG002)
8-8: Unused method argument: kwargs
(ARG002)
12-12: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
12-12: Avoid specifying long messages outside the exception class
(TRY003)
website/management/commands/create_open_redirect_tasks.py
7-174: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
website/management/commands/create_ssrf_tasks.py
8-195: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
48-48: String contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF001)
website/management/commands/create_broken_auth_tasks.py
7-276: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
website/management/commands/create_data_exposure_tasks.py
7-208: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
website/management/commands/create_idor_tasks.py
7-136: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ 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 (7)
website/management/commands/base_lab_seeder.py (1)
8-57: LGTM! Well-structured base class.The extraction of common seeding logic into this base class successfully eliminates ~1,200 lines of duplication across the six task seeder commands. The separation of concerns (_build_content_data, _log_task_status) makes the code maintainable and testable.
website/management/commands/create_open_redirect_tasks.py (1)
1-174: Well-structured Open Redirect lab tasks.The task progression from introduction through exploitation to prevention is logical and comprehensive. The use of LabSeederCommand eliminates the error handling concerns from past reviews.
website/management/commands/create_ssrf_tasks.py (1)
4-195: Comprehensive SSRF lab structure.The seven tasks cover the essential SSRF concepts from basic attacks to advanced bypasses and prevention techniques. The simulation configs provide practical, hands-on learning scenarios.
website/management/commands/create_data_exposure_tasks.py (1)
1-208: Excellent data exposure lab coverage.The nine tasks effectively span the entire data exposure lifecycle: understanding risks, transport security, multiple simulation scenarios, and remediation. The past error handling concern was properly addressed.
website/management/commands/create_file_upload_tasks.py (1)
1-173: Great file upload lab structure.The refactoring to use LabSeederCommand successfully addressed the ~1,200 lines of duplication flagged in past reviews. The seven tasks provide comprehensive coverage from basic concepts to advanced bypass techniques.
website/management/commands/create_idor_tasks.py (1)
1-136: Well-designed IDOR lab.The five tasks efficiently cover IDOR fundamentals: introduction, common scenarios, practical exploits, and prevention. The past issues with error handling and curly quotes were properly resolved.
website/management/commands/create_broken_auth_tasks.py (1)
1-276: Exceptionally comprehensive authentication lab.The twelve tasks provide thorough coverage of authentication vulnerabilities, from basic default credentials through advanced JWT forgery and brute-force scenarios. This is the most detailed lab in the set, appropriately reflecting the complexity of authentication security.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
website/templates/task_detail.html (1)
126-207: Refactor template conditionals to use atypefield instead of fragile name string matching.The review concern is valid. All 10 labs referenced in the template (
SQL Injection,XSS,CSRF,Command Injection,Broken Authentication,IDOR,File Upload Vulnerabilities,Sensitive Data Exposure,Open Redirect,SSRF) are created increate_initial_tasks.pyviaLabs.objects.get_or_create(name=...). However, hardcoded string matching onlab.namecreates a maintainability risk:
- If any lab name is changed, renamed for i18n, or accidentally misspelled in the seeder, the template conditionals fail silently, falling back to generic input
- Adding new labs requires edits in two places (seeder + template)
- The Lab model has no
type,slug, or alternative identifier fieldRecommended solution: Add a
typefield to the Lab model (e.g.,type = models.CharField(max_length=50, choices=[('sql_injection', 'SQL Injection'), ...])) and update template conditionals to uselab.typeinstead oflab.name. This decouples the UI from database name strings and prevents silent failures.
🧹 Nitpick comments (3)
website/management/commands/base_lab_seeder.py (2)
10-10: Prefix unused parameters with underscore.The
argsandkwargsparameters are not used. Prefix them with underscores to indicate they are intentionally unused.Apply this diff:
- def handle(self, *args, **kwargs): + def handle(self, *_args, **_kwargs):Based on static analysis.
10-14: Validate class attributes before processing.If a subclass forgets to override
lab_nameortasks_data, the error messages may be unclear. Consider adding validation at the start ofhandleto provide better error messages.Add validation after line 10:
def handle(self, *args, **kwargs): if not self.lab_name: raise CommandError("lab_name must be set in the subclass") if not self.tasks_data: raise CommandError("tasks_data must be defined in the subclass") try: lab = Labs.objects.get(name=self.lab_name) except Labs.DoesNotExist as err: raise CommandError( f"{self.lab_name} lab not found. Please run create_initial_tasks first." ) from errwebsite/templates/task_detail.html (1)
233-299: Move JavaScript to a separate file. Per the previous review (line 234 comment), embedding substantial JavaScript in the template reduces maintainability and reusability. Extract the MCQ and simulation submission handlers to a separate.jsfile and include it via<script src="..."></script>.This refactor would also:
- Enable browser caching of the script
- Allow linting and minification tools to process the code
- Improve template readability
- Enable testing/unit testing of the handlers
📜 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 (8)
website/management/commands/base_lab_seeder.py(1 hunks)website/management/commands/create_broken_auth_tasks.py(1 hunks)website/management/commands/create_data_exposure_tasks.py(1 hunks)website/management/commands/create_file_upload_tasks.py(1 hunks)website/management/commands/create_idor_tasks.py(1 hunks)website/management/commands/create_open_redirect_tasks.py(1 hunks)website/migrations/0165_add_badge_icons.py(1 hunks)website/templates/task_detail.html(4 hunks)
🧰 Additional context used
🪛 Ruff (0.14.3)
website/management/commands/create_broken_auth_tasks.py
8-277: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
website/management/commands/create_idor_tasks.py
8-137: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
website/management/commands/create_file_upload_tasks.py
8-174: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
website/management/commands/base_lab_seeder.py
8-8: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
10-10: Unused method argument: args
(ARG002)
10-10: Unused method argument: kwargs
(ARG002)
14-14: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
14-14: Avoid specifying long messages outside the exception class
(TRY003)
website/management/commands/create_data_exposure_tasks.py
8-209: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
website/management/commands/create_open_redirect_tasks.py
8-175: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ 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/migrations/0165_add_badge_icons.py (1)
96-96: Minor refactor is correct but seems unrelated to PR objectives.The change from
shutil.copytoshutil.copyfileis technically appropriate here. Sincemedia_icon_pathis a complete file path (line 90) and badge icons don't require metadata preservation,copyfileis suitable and slightly more efficient.However, this migration deals with badge icons and appears unrelated to the PR's stated objective of adding security labs. Consider whether this change should be in a separate PR for clarity.
website/management/commands/base_lab_seeder.py (2)
36-44: LGTM!The content data building logic correctly handles both theory and simulation task types, with appropriate defaults for missing fields.
46-55: LGTM!The logging logic is clear and provides useful feedback about task and content creation/update status.
website/templates/task_detail.html (2)
27-27: SVG self-closing tag is correct.
120-220: Template structure is logically sound. The MCQ and simulation flows are properly separated, result areas are appropriately hidden/shown via JavaScript, and CSRF handling is consistent throughout.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (7)
website/management/commands/base_lab_seeder.py(1 hunks)website/management/commands/create_broken_auth_tasks.py(1 hunks)website/management/commands/create_data_exposure_tasks.py(1 hunks)website/management/commands/create_file_upload_tasks.py(1 hunks)website/management/commands/create_idor_tasks.py(1 hunks)website/management/commands/create_open_redirect_tasks.py(1 hunks)website/management/commands/create_ssrf_tasks.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- website/management/commands/create_idor_tasks.py
- website/management/commands/create_data_exposure_tasks.py
🧰 Additional context used
🪛 Ruff (0.14.3)
website/management/commands/create_ssrf_tasks.py
50-50: String contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF001)
website/management/commands/base_lab_seeder.py
12-12: Unused method argument: args
(ARG002)
12-12: Unused method argument: kwargs
(ARG002)
16-16: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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/management/commands/base_lab_seeder.py (1)
1-57: LGTM! Excellent refactoring.The base class successfully eliminates ~1,200 lines of duplication across the lab seeder commands. The ClassVar annotations address RUF012, exception chaining preserves debug context, and the separation of concerns (content building, status logging) is clean.
Note: The static analysis flags for unused
args/kwargsare false positives—Django's BaseCommand API requires this signature. The TRY003 hint about message length is a minor style preference; the current message is clear and appropriately descriptive.website/management/commands/create_open_redirect_tasks.py (1)
1-177: LGTM! Well-structured lab seeder.The command correctly extends
LabSeederCommand, defines appropriate ClassVar annotations, and provides comprehensive Open Redirect lab content covering theory, simulations, and defensive practices. All past review concerns have been addressed.website/management/commands/create_ssrf_tasks.py (1)
1-197: Overall structure is solid.The command correctly extends
LabSeederCommandand provides comprehensive SSRF lab content. ClassVar annotations are in place, and the base class pattern is properly followed. Just fix the curly quote flagged above.website/management/commands/create_file_upload_tasks.py (1)
1-176: LGTM! Comprehensive file upload lab content.The command properly extends
LabSeederCommand, includes appropriate ClassVar annotations, and provides well-structured tasks covering file upload vulnerabilities, validation techniques, and defensive practices. All past review concerns have been addressed.website/management/commands/create_broken_auth_tasks.py (1)
1-279: LGTM! Thorough authentication lab content.The command correctly extends
LabSeederCommand, properly annotatestasks_datawith ClassVar, and provides extensive broken authentication content covering default credentials, session management, JWT vulnerabilities, password reset flaws, and brute-force defenses. All past review concerns have been resolved.
Nachiket-Roy
force-pushed
the
feature/added-more-labs
branch
from
996757a to
ac774f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
website/management/commands/create_sql_injection_tasks.py (1)
8-324: Apply the Ruff suggestion: annotate mutable class attribute withClassVar.Mutable class attributes like
tasks_datashould be annotated withtyping.ClassVarto clearly indicate they're class-level (not instance-level) and to prevent potential sharing issues across instances.Apply this diff:
+from typing import ClassVar + from .base_lab_seeder import LabSeederCommand class Command(LabSeederCommand): help = "Creates SQL Injection lab tasks with theory and simulation content" lab_name = "SQL Injection" - tasks_data = [ + tasks_data: ClassVar[list[dict]] = [ { "name": "Introduction to SQL Injection",As per static analysis hints.
🧹 Nitpick comments (3)
website/templates/task_detail.html (3)
126-200: Well-refactored per-lab payload input blocks.The explicit
{% elif %}structure replaces the deeply nested conditionals, improving readability and maintainability. Each lab has clear, contextual labels and helpful placeholder examples. This addresses the prior review feedback on nested conditionals.One architectural note: the lab name string matching (e.g.,
lab.name == "SQL Injection") is brittle and depends on exact lab name consistency. Consider using a slug or ID-based approach for future robustness, but this is acceptable for now.
234-266: MCQ handler is solid; minor UX improvement available.The MCQ submission handler properly validates input, includes CSRF protection, and handles errors. Result styling based on
data.correctis clean and clear.Minor consideration: the submit button is not disabled during the fetch, which allows users to click multiple times and send duplicate requests. This is not a critical issue (server-side validation should prevent duplicate processing), but disabling the button until the response returns would improve UX.
268-299: Simulation handler is well-structured with unified payload submission.The simulation form handler properly validates and trims payload input, maintains CSRF security, and handles errors gracefully. The unified result rendering (lines 286-293) is clean and consistent with MCQ styling.
Same minor UX consideration as MCQ: the submit button lacks disabled state during the async request, allowing potential duplicate submissions. This is not critical but could be improved.
📜 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 (15)
website/management/commands/base_lab_seeder.py(1 hunks)website/management/commands/create_broken_auth_tasks.py(1 hunks)website/management/commands/create_commands_injection_tasks.py(1 hunks)website/management/commands/create_csrf_tasks.py(1 hunks)website/management/commands/create_data_exposure_tasks.py(1 hunks)website/management/commands/create_file_upload_tasks.py(1 hunks)website/management/commands/create_idor_tasks.py(1 hunks)website/management/commands/create_initial_tasks.py(1 hunks)website/management/commands/create_open_redirect_tasks.py(1 hunks)website/management/commands/create_sql_injection_tasks.py(7 hunks)website/management/commands/create_ssrf_tasks.py(1 hunks)website/management/commands/create_xss_tasks.py(1 hunks)website/management/commands/seed_all_labs.py(1 hunks)website/migrations/0165_add_badge_icons.py(1 hunks)website/templates/task_detail.html(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- website/management/commands/create_file_upload_tasks.py
🚧 Files skipped from review as they are similar to previous changes (4)
- website/management/commands/create_idor_tasks.py
- website/management/commands/create_initial_tasks.py
- website/migrations/0165_add_badge_icons.py
- website/management/commands/create_open_redirect_tasks.py
🧰 Additional context used
🪛 Ruff (0.14.4)
website/management/commands/base_lab_seeder.py
12-12: Unused method argument: args
(ARG002)
12-12: Unused method argument: kwargs
(ARG002)
16-16: Avoid specifying long messages outside the exception class
(TRY003)
website/management/commands/seed_all_labs.py
20-20: Unused method argument: args
(ARG002)
20-20: Unused method argument: kwargs
(ARG002)
website/management/commands/create_commands_injection_tasks.py
8-153: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
website/management/commands/create_sql_injection_tasks.py
8-324: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
website/management/commands/create_csrf_tasks.py
8-178: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
22-22: String contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF001)
website/management/commands/create_xss_tasks.py
8-186: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ 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 (4)
website/templates/task_detail.html (2)
27-27: SVG self-closing tag is a good formatting improvement.The refactored SVG path using self-closing syntax is valid and improves template consistency.
233-300: JavaScript handlers are secure and well-structured.Both MCQ and simulation handlers follow consistent patterns for CSRF protection, validation, and error handling. The unified result rendering improves maintainability. The fetch-based approach is modern and appropriate.
One note on the past review feedback about embedded JavaScript: while a separate file would enhance modularity, the current approach is acceptable given the context-specific nature of Django URL tags and CSRF token injection.
website/management/commands/create_sql_injection_tasks.py (2)
1-6: LGTM! Clean refactoring to data-driven approach.The transition from
BaseCommandtoLabSeederCommandwith declarative class attributes (lab_name,help) is a good architectural improvement that promotes code reuse and consistency across lab seeders.
83-278: No issues found - simulation_config variations are intentionally supported.Verification confirms the system is designed for flexible
simulation_configstructures. The view consumer inwebsite/views/Simulation.pyonly requires thesuccess_payloadfield to function; all other fields are optional metadata. All SQL injection simulation tasks (lines 83, 138, 199, 259) correctly includesuccess_payload, and the additional fields liketable_structure,blind_type,database_type, andtime_functionare inert but intentional for frontend hints and task documentation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
website/management/commands/create_csrf_tasks.py (1)
1-10: LGTM! Previous issues resolved.The ClassVar annotation has been properly added and imports are correct. The curly apostrophe issue on line 24 has also been fixed.
website/management/commands/create_xss_tasks.py (1)
1-10: LGTM! ClassVar annotation properly added.The previous lint issue has been resolved with the correct type annotation.
🧹 Nitpick comments (2)
website/management/commands/create_sql_injection_tasks.py (2)
80-98: Standardize result field naming across simulations.Line 96 uses
expected_resultwhile other simulations in this file (lines 152, 211, 272) usesuccess_criteria. Pick one field name and use it consistently throughout the file.- "expected_result": "Successfully logged in as admin without knowing the password", + "success_criteria": "Successfully logged in as admin without knowing the password",
198-220: Consider removing the uniqueexpected_approachfield.Line 218 introduces an
expected_approachfield not present in any other lab's simulation config. Unless this field serves a specific purpose consumed by the base seeder or frontend, consider removing it for consistency or adding it to other relevant simulations.
📜 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 (4)
website/management/commands/create_commands_injection_tasks.py(1 hunks)website/management/commands/create_csrf_tasks.py(1 hunks)website/management/commands/create_sql_injection_tasks.py(7 hunks)website/management/commands/create_xss_tasks.py(1 hunks)
⏰ 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
Nachiket-Roy
force-pushed
the
feature/added-more-labs
branch
from
8be3080 to
81adab5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (5)
website/management/commands/create_commands_injection_tasks.py(1 hunks)website/management/commands/create_csrf_tasks.py(1 hunks)website/management/commands/create_sql_injection_tasks.py(7 hunks)website/management/commands/create_xss_tasks.py(1 hunks)website/management/commands/seed_all_labs.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- website/management/commands/create_sql_injection_tasks.py
🧰 Additional context used
🪛 Ruff (0.14.4)
website/management/commands/seed_all_labs.py
20-20: Unused method argument: args
(ARG002)
20-20: Unused method argument: kwargs
(ARG002)
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
website/management/commands/seed_all_labs.py (2)
14-14: Consider matching the order of imports for consistency.The order of commands in
ALL_LABSdoesn't match the import order. While this doesn't affect functionality, aligning them would improve readability and make it easier to verify completeness.Apply this diff if you'd like to match the import order:
-ALL_LABS = [BrokenAuth, DataExposure, FileUpload, IDOR, OpenRedirect, SSRF, XSS, CSRF, SQL, CI] +ALL_LABS = [BrokenAuth, CI, CSRF, DataExposure, FileUpload, IDOR, OpenRedirect, SQL, SSRF, XSS]
21-26: Consider adding error handling to continue seeding if one lab fails.If any seeder's
handle()method raises an exception, the entire seeding process stops and remaining labs won't be seeded. Consider wrapping each seeder call in a try-except block to log failures and continue with remaining labs.Apply this diff to add error handling:
def handle(self, *_args, **_kwargs): for Seeder in ALL_LABS: self.stdout.write(self.style.WARNING(f"Seeding: {Seeder.lab_name}")) - seeder = Seeder() - seeder.stdout = self.stdout - seeder.stderr = self.stderr - seeder.handle() + try: + seeder = Seeder() + seeder.stdout = self.stdout + seeder.stderr = self.stderr + seeder.handle() + except Exception as e: + self.stdout.write(self.style.ERROR(f"Failed to seed {Seeder.lab_name}: {e}")) + continue self.stdout.write(self.style.SUCCESS("All labs seeded successfully"))
📜 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 (1)
website/management/commands/seed_all_labs.py(1 hunks)
⏰ 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 (3)
website/management/commands/seed_all_labs.py (3)
1-12: LGTM!The import structure is clean and consistent. All lab seeder commands are imported with descriptive aliases.
20-26: Past review concern has been addressed.The stdout and stderr are now properly forwarded to each seeder instance (lines 24-25), which resolves the AttributeError that would occur when child seeders call
self.stdout.write(). The unused parameters are also correctly prefixed with underscores.
22-22: All seeder classes define thelab_nameattribute—no changes required.Verification confirms that all 10 imported seeder classes in
seed_all_labs.pydefine thelab_nameattribute at line 8. The code at line 22 is safe to accessSeeder.lab_namefor every class in theALL_LABSlist.
Nachiket-Roy
force-pushed
the
feature/added-more-labs
branch
from
c57fb0d to
160fb87
Compare
github-project-automation
bot
moved this from Ready
to Done
in 📌 OWASP BLT Project Board
2 participants
Added more labs in security labs section closes #4784
To seed labs and their tasks simply run :
python manage.py seed_all_security_labSummary by CodeRabbit
Refactor
Chores