check imap attachment symlink on the resolved destination path

[Samin061]

ImapHook writes each downloaded attachment under local_output_directory, but _is_symlink passed the bare attachment name to os.path.islink, so it inspected a path relative to the process working directory rather than the file that gets opened. A symlink planted at the real destination slipped past the check and the following open(..., "wb") wrote through it, so an attacker-controlled attachment name and payload could overwrite the link target. Resolve the destination with _correct_path before the symlink check so the file actually being written is the one inspected.

---

Was generative AI tooling used to co-author this PR?

• Yes (please specify the tool below)

[SameerMesiah97]

Looks good.

[Vamsi-klu]

Confirmed the root cause. _create_file opens _correct_path(name, local_output_directory) in "wb" mode, but the old _is_symlink called os.path.islink(name) on the bare attachment name, which resolves relative to the process CWD rather than the real write target. A symlink planted at the actual destination was never inspected, so the subsequent open() would follow it and overwrite the link target. Routing the check through _correct_path makes islink inspect exactly the path that gets written, so the check and the write are now consistent. The regression test is genuine: os.path.islink is asserted to be called with "test_directory/symlink" instead of the bare "symlink", so reverting the source line fails the assertion.

Two non-blocking notes for hardening this path further. (1) A check-then-open TOCTOU window still exists between islink and open() -- os.open with O_NOFOLLOW would close it. (2) islink here only inspects the final path component; a name containing a separator plus a pre-planted symlinked parent directory would still be followed (the escaping guard only blocks "../"). For context, PR #67075 previously proposed this same check plus the O_NOFOLLOW open but was auto-closed as a stale draft, so its approach may be worth folding in.

[Samin061]

Good notes. I folded in the O_NOFOLLOW open, so the write itself refuses a symlink planted in the check-then-open window rather than relying only on the islink pre-check. It falls back to a no-op where the flag doesn't exist (Windows), which lines up with the existing islink caveat there.

You're right that the parent-directory case is separate: O_NOFOLLOW only guards the final component, so a name like sub/evil where sub is a pre-planted symlink still slips past both the islink check and the ../ escaping guard. Closing that means resolving the real path and verifying it stays under local_output_directory, which is a broader change than this fix. I'd rather keep this PR scoped to the race and handle the containment check as a follow-up, unless you'd prefer it folded in here.

[Vamsi-klu]

Hey, thanks for the reply and addressing the feedback. I guess you have merge conflicts in this branch. Could you also resolve this?

[Samin061]

Rebased onto latest main and resolved the conflict. Main had grown the overwrite handling in _create_file, so I folded the O_NOFOLLOW opener into that single method instead of the duplicate the merge left behind, and updated the two overwrite tests to expect opener=ANY. Full imap hook suite is green (37 passed).

[shahar1]

Could you please indicate in the PR body whether was an AI usage in this PR to comply with the guidelines? Thank you!

[Samin061]

Sure thing. I leaned on an assistant only a little here, mostly for research and sanity-checking the race window, but the fix and the tests are my own, so I left the co-author box unchecked to reflect that. Happy to spell it out in the body if you'd prefer it stated explicitly.

[jroachgolf84]

LGTM - make sure to update the PR body like @shahar1 mentioned ;)

[potiuk]

Thanks — this is the most security-relevant fix in the current provider batch, and it fixes two distinct problems.

The check was looking at the wrong path. _is_symlink(name) called os.path.islink(name) on the bare attachment name — resolved against the process CWD — while the file is actually written to local_output_directory/name. So a symlink planted at the destination was never detected; the guard was effectively inert. Checking self._correct_path(name, local_output_directory) is the fix.

And the check-then-open race is closed too. Even a correct islink() can be raced by planting a symlink between the check and the open(). Passing an opener that ORs in O_NOFOLLOW means the kernel refuses to follow, so the window doesn't matter. getattr(os, "O_NOFOLLOW", 0) degrading to a no-op on Windows is the right compromise, and the comment explains the race rather than just describing the code.

Worth being explicit about severity: unlike most path-quoting fixes in this area, attachment names here come from email — genuinely untrusted, attacker-influenced input, not Dag-author configuration. So this isn't a robustness nicety; it's a real symlink-follow issue. Worth considering whether it merits a backport.

I also checked the adjacent path handling and it holds up: _correct_path concatenates rather than using os.path.join, so an absolute name like /etc/passwd becomes test_directory//etc/passwd instead of escaping, and _is_escaping_current_directory covers ../. No neighbouring hole here.

On the tests: strengthening test_download_mail_attachments_with_symlink from call_count == 1 to assert_called_once_with("test_directory/symlink") is the assertion that actually pins the fix, and extracting the opener to verify O_NOFOLLOW is in the flags is a neat way to test that without a real filesystem.

---

Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting