Skip to content

Prevent IMAP hook from silently overwriting attachments with identical filenames#67899

Closed
lohitkolluri wants to merge 1 commit into
apache:mainfrom
lohitkolluri:fix/65870-imap-attachment-overwrite
Closed

Prevent IMAP hook from silently overwriting attachments with identical filenames#67899
lohitkolluri wants to merge 1 commit into
apache:mainfrom
lohitkolluri:fix/65870-imap-attachment-overwrite

Conversation

@lohitkolluri

@lohitkolluri lohitkolluri commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Problem

When multiple emails contain attachments with the same filename, download_mail_attachments silently overwrites the previous file. There is no way to preserve all attachments without manually renaming files after download.

Solution

overwrite parameter

Added an overwrite parameter to download_mail_attachments (default True for backward compatibility). When set to False, files with colliding names get a counter suffix (e.g. _1, _2) instead of being overwritten.

Atomic writes

The unique-filename path uses exclusive-create mode ("xb") instead of checking os.path.exists() before writing, eliminating a TOCTOU race condition where concurrent operations could still collide.

In-batch duplicate tracking

A seen dict tracks generated filenames within a single download_mail_attachments call, preventing the counter suffix from colliding with a real attachment name in the same batch.

Changes

  • imap.py: Added overwrite parameter (default True), atomic "xb" mode in the non-overwrite path, _unique_filename helper, and seen dict for in-batch duplicate tracking
  • test_imap.py: 11 new tests covering defaults, duplicates, filesystem collisions, generated-name collisions, FileExistsError retry, and the backward-compatible overwrite path

closes: #65870


Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (DeepSeek V4 Flash)

Generated-by: Claude Code (DeepSeek V4 Flash) following the guidelines


Note

✅ Ready for review · @lohitkolluri@potiuk · 2026-06-12 13:30 UTC

Thanks @lohitkolluri — all checks are green and this PR is marked ready for maintainer review. The ball is with the maintainers now; a maintainer will take the next look.

Automated triage — may be imperfect.

@boring-cyborg

boring-cyborg Bot commented Jun 2, 2026

Copy link
Copy Markdown

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our prek-hooks will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example Dag that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

@lohitkolluri
lohitkolluri force-pushed the fix/65870-imap-attachment-overwrite branch from 8b38111 to cfba11b Compare June 2, 2026 18:27
@lohitkolluri

lohitkolluri commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

@eladkal The CI failures in this run are unrelated to the PR changes — both failures are from Docker Hub being unreachable during the initial cleanup step (docker: Error response from daemon: Get "https://registry-1.docker.io/v2/": net/http: request canceled while waiting for connection). This is a transient infrastructure issue on the GitHub Actions runners, not a code issue. The branch has been updated with latest main to re-trigger CI.

@lohitkolluri lohitkolluri changed the title fix(imap): Add create_unique_filenames option to prevent attachment overwrites fix(imap): Rename create_unique_filenames to overwrite, use atomic xb mode Jun 2, 2026
@lohitkolluri lohitkolluri changed the title fix(imap): Rename create_unique_filenames to overwrite, use atomic xb mode fix(imap): Prevent IMAP hook from silently overwriting attachments with identical filenames Jun 2, 2026
@lohitkolluri
lohitkolluri force-pushed the fix/65870-imap-attachment-overwrite branch from 450374a to b8979f4 Compare June 2, 2026 18:50
@potiuk

potiuk commented Jun 9, 2026

Copy link
Copy Markdown
Member

@lohitkolluri A few things need addressing before review — see our Pull Request quality criteria.

  • Provider tests. See docs.

No rush.

Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you.

@lohitkolluri
lohitkolluri force-pushed the fix/65870-imap-attachment-overwrite branch from 3a444bc to 225ab29 Compare June 10, 2026 09:22
@potiuk potiuk added the ready for maintainer review Set after triaging when all criteria pass. label Jun 12, 2026
@lohitkolluri
lohitkolluri force-pushed the fix/65870-imap-attachment-overwrite branch from 225ab29 to 8191379 Compare July 1, 2026 20:40
@lohitkolluri lohitkolluri changed the title fix(imap): Prevent IMAP hook from silently overwriting attachments with identical filenames Prevent IMAP hook from silently overwriting attachments with identical filenames Jul 2, 2026
@lohitkolluri
lohitkolluri force-pushed the fix/65870-imap-attachment-overwrite branch from 8191379 to f61fdad Compare July 2, 2026 19:15
…l filenames

Added an `overwrite` parameter (default `True` for backward compatibility)
to `download_mail_attachments`. When set to `False`, files with colliding
names get a counter suffix (e.g. `_1`, `_2`) instead of being overwritten.

- Switched to atomic `"xb"` mode for the unique-filename path to
  eliminate a TOCTOU race condition
- Added in-batch duplicate tracking so generated counter suffixes
  don't collide with real attachment names
- Added tests covering defaults, duplicates, collisions, and
  FileExistsError retry

Signed-off-by: Lohit Kolluri <lohitkolluri@gmail.com>
@lohitkolluri
lohitkolluri force-pushed the fix/65870-imap-attachment-overwrite branch from f61fdad to 8ed8bb2 Compare July 2, 2026 19:15
@lohitkolluri

Copy link
Copy Markdown
Contributor Author

@potiuk Updated the PR title CI should be green now can you rerun the CI.

@vincbeck

vincbeck commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Fixed in #68838

@vincbeck vincbeck closed this Jul 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:imap ready for maintainer review Set after triaging when all criteria pass.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

IMAP hook silently overwrites attachments with identical filenames

3 participants