Skip to content

Add security warning to mailbox docs #149295

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.

Sign up for GitHub

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

Open
StanFromIreland wants to merge 1 commit into
base: main
Choose a base branch
from
+5 −0
Open

Add security warning to mailbox docs #149295

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Doc/library/mailbox.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ accessing and manipulating on-disk mailboxes and the messages they contain.
:class:`~email.message.Message` class with format-specific state and behavior.
Supported mailbox formats are Maildir, mbox, MH, Babyl, and MMDF.

.. warning::

The :mod:`!mailbox` module assumes full control over input parameters and the
underlying mailbox storage; it does not protect against untrusted paths or
externally modified data.

@bitdancer bitdancer May 2, 2026

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.

Well, you could probably add a variation on this warning to every module in the stdlib that operates on files, so at the moment I'm not convinced. I haven't read over the security issues, though, so we'll see. I don't think the message as is is all that useful, though. I certainly don't know what it is trying to tell me.

I suggest we hold on this for a bit for further discussion.

@gpshead gpshead Jun 7, 2026

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.

The message is passively trying to say: We will not treat these problems in mailbox as security issues because it is nobody's priority to attempt to maintain mailbox for such purposes.

We could just say that instead.

Realistically: Why do we even have this module in the stdlib at all?

@StanFromIreland StanFromIreland Jun 7, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The message is passively trying to say: We will not treat these problems in mailbox as security issues because it is nobody's priority to attempt to maintain mailbox for such purposes.

We could just say that instead.

Exactly! Hmm, we imply the same for various other modules and APIs but generally use the fancy wording.

Realistically: Why do we even have this module in the stdlib at all?

I think the AA batteries we included have expanded to D batteries. ;-)

@gpshead gpshead Jun 7, 2026

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.

fwiw mailbox is a leaf node module (nothing else in the stdlib depends on it AFAICT) so while I wouldn't actually go so far as to say something as snarky as my "isn't a priority" in the docs, wording like your proposal laying out security expectations is good to have.

for this specific module, given the reports we've been receiving on PSRT maybe we could be more specific and offer some advice to users. The security reports that we're not considering serious have been around the mailbox files/directory-trees being on attacker controlled file systems. Not about the contents of an individual email stored within. "externally modified data" in your phrasing is maybe a bit vague and could imply message contents are involved?

on the slippery slops docs warning topic: We have a general theme of these messages being spread out throughout our docs near the APIs they apply to. Messaging like this is intended to meet the user where they're at when looking up how to use something.

Our other security what is and isn't an issue docs like the devguide and security.md can also point to things in a more "do or do not report x/y/z" manner for specific packages.

@StanFromIreland StanFromIreland Jun 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fwiw mailbox is a leaf node module (nothing else in the stdlib depends on it AFAICT)

Oh indeed. It really should have been a PyPI module, if only it had existed at the time.

Not about the contents of an individual email stored within. "externally modified data" in your phrasing is maybe a bit vague and could imply message contents are involved?

Maybe:

... it does not protect against mailbox files or directory trees controlled or modified by untrusted users.

Or something like:

... it does not protect against untrusted paths or externally modified mailbox files or directory trees.

Messaging like this is intended to meet the user where they're at when looking up how to use something.

In this case, the warning applies to the entire module, so I think putting warnings under each API would be too duplicative.

@gpshead gpshead Jun 7, 2026 •
edited
Loading

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. 

.. warning::

   The :mod:`!mailbox` module is designed for mailboxes under the sole
   control of the application and the local user.  It is **not** hardened
   for use in hostile environments.  As such, the following are not treated
   as security vulnerabilities:

   * Passing untrusted input to :mod:`!mailbox` APIs.  Values such as
     message keys and folder names are used as filesystem paths, so
     unsanitized input may read, write, or delete files outside the
     mailbox.  Sanitizing these inputs is the caller's responsibility;
     at a minimum, reject any value containing ``".."``, ``"/"``,
     ``"\"``, or ``":"``.

   * Using a mailbox whose files or directories reside on a filesystem
     that other users can modify.  Symlink and other races there can
     redirect mailbox operations to arbitrary files.

Maybe? though that also seems verbose. (that's from me going back and forth with Claude a few times given our desire to clarify the reports at hand and wanting to make the advice somewhat actionable)

We might accept PRs helping flag some such inputs via ValueError's in the future but the important thing is we're not claiming to be comprehensive.

@StanFromIreland StanFromIreland Jun 7, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It is certainly more detailed, although “As such, the following are not treated as security vulnerabilities:” feels too specific. I think the docs should explicitly state the general (lack of a) support boundary.

Also, I think we should avoid suggestions like "at a minimum, reject any value containing "..", "/", "\", or ":"." It is not a complete cross-platform path-safety rule and could become misleading.

@gpshead gpshead Jun 7, 2026

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.

yeah, agreed. even listing the characters to avoid screams "so why isn't that a ValueError already?"... the reason likely being nobody proactively maintains the module. and avoiding characters instead of specifying what is allowed is backwards... MH is integer keyed message ids by definition for example.

@StanFromIreland StanFromIreland Jun 7, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Suggested change
externally modified data.
externally modified mailbox files or directory trees.

Seeing the complications adding suggestions introduces, WDYT about this? Based on usage on GitHub (which overall is tiny!), almost all users are using the module for small little local scripts, and won't care about such considerations anyhow.


.. seealso::

Expand Down
Loading