Skip to content

Harden IMAP hook against symlink-followed attachment writes - #67075

Closed
Lougarou wants to merge 1 commit into
apache:mainfrom
Lougarou:did/imap-symlink-hardening
Closed

Harden IMAP hook against symlink-followed attachment writes#67075
Lougarou wants to merge 1 commit into
apache:mainfrom
Lougarou:did/imap-symlink-hardening

Conversation

@Lougarou

@Lougarou Lougarou commented May 17, 2026

Copy link
Copy Markdown
Contributor

ImapHook._create_file wrote downloaded attachments with a plain
open(file_path, "wb"), which follows a symlink at the destination. The
existing _is_symlink guard in _create_files only inspects the bare
attachment name relative to the current working directory — not the real
joined output path (file_path) that is actually written to.

This hardens _create_file:

  • it re-checks os.path.islink on the real joined target and refuses to write
    through a symlink there, matching the existing _is_symlink rejection style;
  • it opens via os.open with O_NOFOLLOW (feature-gated with
    getattr(os, "O_NOFOLLOW", 0), so it is a no-op on Windows, which lacks the
    flag) to also close the check-to-open TOCTOU window.

The open flags are O_WRONLY | O_CREAT | O_TRUNC — deliberately without
O_EXCL — so the hook's existing overwrite-on-redownload behaviour is
preserved; the file mode is 0o600. Legitimate downloads (including
redownload/overwrite) behave exactly as before; only writes that would pass
through a symlink are now refused.

test_imap.py is updated: the download_mail_attachments tests now patch
os.open / os.fdopen instead of the builtin open, the symlink test's
islink call-count expectation is relaxed (it is now legitimately consulted in
two places), and two tests are added — one pinning that a symlink at the real
joined target blocks the write, one pinning the O_NOFOLLOW / no-O_EXCL flags.


Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)

Generated-by: Anthropic Claude (Claude Code) following the guidelines


  • Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
  • For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
  • When adding dependency, check compliance with the ASF 3rd Party License Policy.
  • For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

Important

🛠️ Maintainer triage note for @Lougarou · by @potiuk · 2026-06-18 13:57 UTC

Paused pending your next update — this PR has been inactive for ~30 days, so it's been moved to draft to keep the review queue clear:

  • Rebase on the latest main, address any new failures, and mark it Ready for review when you pick it back up — no rush.
  • See the Pull Request quality criteria.

The ball is in your court — you've been assigned to this PR.

Automated triage — may be imperfect; a maintainer takes the next look.

@potiuk potiuk added the ready for maintainer review Set after triaging when all criteria pass. label May 19, 2026
@potiuk
potiuk marked this pull request as draft June 18, 2026 13:59
@potiuk

potiuk commented Jul 2, 2026

Copy link
Copy Markdown
Member

Note

🗑️ Closing stale draft · by @potiuk · 2026-07-02 18:53 UTC

This draft has been inactive for ~13 days. Closing to keep the queue tidy — no judgment on the change itself. Please reopen (or open a fresh PR) whenever you pick it back up, and a maintainer will be glad to take a look.

Automated triage — may be imperfect.

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.

2 participants