Vamsi-klu commented on PR #69194: URL: https://github.com/apache/airflow/pull/69194#issuecomment-4912281302
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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
