Skip to content

Clean the session files upon unpackling Error in FileSession clean up#2012

Open
jiangwen365 wants to merge 2 commits into
cherrypy:mainfrom
jiangwen365:patch-2
Open

Clean the session files upon unpackling Error in FileSession clean up#2012
jiangwen365 wants to merge 2 commits into
cherrypy:mainfrom
jiangwen365:patch-2

Conversation

@jiangwen365

@jiangwen365 jiangwen365 commented Dec 24, 2023 •
edited by webknjaz
Loading

Copy link
Copy Markdown

I've seen clean failures in my production error log files due to UnpicklingError: invalid load key, '\x00'. I suspect the session file was created by attackers. This would 1) break the cleansing thread, leaving many expired sessions not cleaned up, also 2) I tend to think it's not good/safe to ignore and keep these "bad" session files. Below is the original traceback info in the error log.

Traceback (most recent call last):
  File "C:\Program Files (x86)\Python311-32\Lib\site-packages\cherrypy\process\plugins.py", line 518, in run
    self.function(*self.args, **self.kwargs)
  File "C:\Program Files (x86)\Python311-32\Lib\site-packages\cherrypy\lib\sessions.py", line 586, in clean_up
    contents = self._load(path)
               ^^^^^^^^^^^^^^^^
  File "C:\Program Files (x86)\Python311-32\Lib\site-packages\cherrypy\lib\sessions.py", line 520, in _load
    return pickle.load(f)
           ^^^^^^^^^^^^^^
_pickle.UnpicklingError: invalid load key, '\x00'.

What kind of change does this PR introduce?

  • bug fix
  • feature
  • docs update
  • tests/coverage improvement
  • refactoring
  • other

What is the related issue number (starting with #)

What is the current behavior? (You can also link to an open issue here)
UnpicklingError: invalid load key, '\x00' would break the cleansing thread, leaving many expired sessions not cleaned up, also I tend to think it's not good/safe to ignore and keep these "bad" session files.

What is the new behavior (if this is a feature change)?
If pickling a session file is throwing pickling error, then just delete that file.

Other information:

Checklist:

  • I think the code is well written
  • I wrote good commit messages
  • I have squashed related commits together after the changes have been approved
  • Unit tests for the changes exist
  • Integration tests for the changes exist (if applicable)
  • I used the same coding conventions as the rest of the project
  • The new code doesn't generate linter offenses
  • Documentation reflects the changes
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences

jiangwen365 and others added 2 commits December 24, 2023 16:31
@jiangwen365
Clean the session files upon unpacking Error
5f10f25 
I've seen clean failures in the error log files due to UnpicklingError: invalid load key, '\x00'.
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
911370b 
for more information, see https://pre-commit.ci
@jiangwen365 jiangwen365 changed the title Clean the session files upon unpacking Error Clean the session files upon unpackling Error Dec 24, 2023
@jiangwen365 jiangwen365 changed the title Clean the session files upon unpackling Error Clean the session files upon unpackling Error in FileSession clean up Dec 24, 2023
webknjaz
webknjaz reviewed Dec 26, 2023
View reviewed changes
Comment thread cherrypy/lib/sessions.py
# Session expired: deleting it
os.unlink(path)

except pickle.UnpicklingError:

@webknjaz webknjaz Dec 26, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you add a test for this?

@jiangwen365 jiangwen365 Dec 26, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I wish I am able to write a test for this - but with limited skill, I couldn't figure out how to write one - if possible could you please provide some guidance and thoughts?

@webknjaz webknjaz Dec 30, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure either. It would probably be an integration test that sets up a server with sessions, makes a query (which results in a session file appearing on disk), then corrupts that file by writing arbitrary data into it (a random byte might be enough), followed by tearing down the server and that should raise the exception. Maybe, it'd be easier (or more stable) with calling clean_up() and checking that the file disappeared from disk. Additionally, it would be wise to use pytest-mocker's mocker.spy() on the _load() method to verify that the exception was raised, indeed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@webknjaz webknjaz webknjaz left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jiangwen365 @webknjaz