Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(sessions): Clean old expired sessions #11770

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

kiblik
Copy link
Contributor

@kiblik kiblik commented Feb 8, 2025

I witnessed a situation when session table django_session increased to a size of 3.43GB with 10M tuples. It was the largest table in the whole DefectDojo.

After investigation, I found out that until the user does not log out proactively, the session is not removed even if it is expired:

Clearing the session store
... If you’re using the database backend, the django_session database table will grow. ...
... If the user logs out manually, Django deletes the row. But if the user does not log out, the row never gets deleted. ...
Django does not provide automatic purging of expired sessions. Therefore, it’s your job to purge expired sessions on a regular basis. Django provides a clean-up management command for this purpose: clearsessions. It’s recommended to call this command on a regular basis, for example as a daily cron job.
...
https://docs.djangoproject.com/en/5.1/topics/http/sessions/#clearing-the-session-store

As soon I ran ./manage.py clearsessions, the number of tuples jumped from 10M to 44.

So Inspired by one blog, I implemented weekly clean-up.

Why "dirty SQL" migration if you already added cronjob?

Great question. When I ran clearsessions, it killed my Pod on OOM (multiple times with a gradual increase of memory limit). When I disabled the limiter for memory, Pod was willing to consume 9GB of memory and it was running over 3 hours. This procedure is not possible to execute in a regular migration Job. The reason for the long and memory-consuming procedure is the current implementation of clearsessions:

https://github.com/django/django/blob/0bac41fc7e4a842e8d20319cba31cc645501c245/django/contrib/sessions/management/commands/clearsessions.py#L16

https://github.com/django/django/blob/0bac41fc7e4a842e8d20319cba31cc645501c245/django/contrib/sessions/backends/db.py#L192

Not sure what is the real reason why ....filter(...).delete() consumes that much memory and takes that much time (fetching all data before removal? or many SQL logs?) but as soon as I performed raw cleanup (used in migration) on the same dataset (restored from backups) removal jumped for 3 hours to 2 or 3 minutes.

@github-actions github-actions bot added New Migration Adding a new migration file. Take care when merging. settings_changes Needs changes to settings.py based on changes in settings.dist.py included in this PR labels Feb 8, 2025
@kiblik kiblik force-pushed the clear_session branch 2 times, most recently from 0da3305 to 778605b Compare February 8, 2025 14:37
@kiblik kiblik marked this pull request as ready for review February 10, 2025 20:54
Copy link

DryRun Security Summary

The pull request introduces a session cleanup system using Celery for Django applications, with minor security considerations around SQL handling but no critical vulnerabilities identified.

Expand for full summary

This PR implements a new session cleanup mechanism across multiple files, adding a Celery task and configuration for periodically clearing expired Django sessions.

Security Findings:

  1. Potential SQL Injection Risk in dojo/db_migrations/0222_clean_old_sessions.py: The SQL statement uses a direct string, which could potentially be vulnerable to SQL injection, though mitigated by Django's typical sanitization.
  2. Session Management Implications: The changes aim to reduce attack surface by removing stale session data and preventing potential database bloat.

No critical security vulnerabilities were identified in the implementation.

Code Analysis

We ran 9 analyzers against 3 files and 0 analyzers had findings. 9 analyzers had no findings.

View PR in the DryRun Dashboard.

Copy link
Contributor

@mtesauro mtesauro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Migration Adding a new migration file. Take care when merging. settings_changes Needs changes to settings.py based on changes in settings.dist.py included in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants