don't follow symlinks or trust another user's file in FS.open

[netliomax25-code]

1. With cache=True the cache path is derived only from the grammar, the options and the version strings, so <tmpdir>/.lark_cache_<user>_<sha256>_3_12.tmp is predictable to anyone else with access to the temp directory.
2. A file another user leaves at that path reaches pickle.load in lark.py:377 before the sha256 line is compared on 378, and since that digest sits in the same file it is a cache key rather than an authenticity check, so the load is arbitrary code execution as whoever runs the parser.
3. Saving the cache followed a symlink left at the same path and overwrote whatever it pointed at.

Sent the non-atomicwrites branch of FS.open through an O_NOFOLLOW open that also refuses a file it doesn't own and keeps the cache at 0600, which is roughly what the atomicwrites branch gets from mkstemp today. A refused cache just falls through to the existing handlers, so the grammar is parsed normally. Three tests in tests/test_cache.py cover the load path, the save path and the file mode; they fail on master and pass here, and the rest of the suite is green with and without atomicwrites installed.

[MegaIng]

Is this standard practice in other projects using cache files?

Would it be easier to put the cache files somewhere else?

[netliomax25-code]

Both fair questions.

1. On prior art: it's what CPython does for its own temp files. tempfile.mkstemp opens with O_RDWR|O_CREAT|O_EXCL|O_NOFOLLOW at mode 0600 (tempfile.py, _text_openflags and _mkstemp_inner). Lark already inherits that on one path: with the atomic_cache extra installed, FS.open hands writes to atomicwrites.atomic_write, which creates the file through mkstemp and renames it into place, so that branch is already 0600 and already symlink-proof. This PR mostly gives the plain-open() fallback the same properties.

2. The gap that made me file it is that reads never reach that branch. FS.open only routes to atomicwrites when "w" in mode, so 'rb' is a plain open() either way, and that's the call feeding pickle.load on lark.py:377 (the sha256 line is compared on 378, after that load). I re-checked on master just now: with atomicwrites installed, a pickle planted at the predictable cache path still runs on load; without the extra, the save path also follows a symlink and overwrites whatever it points at. On this branch both are refused and the grammar is parsed normally.

   The exposure is platform-dependent, which may be why it isn't obvious: on macOS gettempdir() is a per-user /var/folders/.../T at 0700, but on Linux and CI it's /tmp at 1777.

3. On moving the files, yes, and I'd say that's the better fix if you want to go that way. A directory only the user can write to removes the cause instead of defending against it, and it's the norm: pip, black, pre-commit, Cython and huggingface_hub all default to a per-user cache dir, and matplotlib only falls back to the temp dir when that's unwritable, and even then uses mkdtemp at 0700 and warns. Two things to weigh: lark has no runtime dependencies, so it'd mean hand-rolling XDG_CACHE_HOME/~/.cache plus %LOCALAPPDATA% rather than taking platformdirs, and it changes the default cache location, so files already in the temp dir get orphaned and the new directory wants creating at 0700.

The two aren't exclusive, and they're not the same size of change. I'm happy to convert this PR to a per-user cache directory, or leave it as the narrow fix and do the relocation separately. Whichever you'd rather carry.

Validation on the branch: full suite green with and without atomicwrites installed (1315 tests), CI passing.

[netliomax25-code]

any preference?

[MegaIng]

I think we should consider doing both. The change from this PR on it's own is a good idea already.

[MegaIng]

The current implementation does not refuse to open files that have incorrect permissions for reading, it just changes them. Is this intentional?

And don't worry about cache invalidation, a new lark version is going to invalidate all caches anyway.

[netliomax25-code]

1. On the read path it wasn't intentional, and you're right that it's the wrong default. Reads only checked ownership, so a file we own but that group or others can write could have been tampered with by another user and we'd still pickle.load it. Changing perms there doesn't help either, since the bad contents are already on disk.

2. Fixed it to refuse instead: on read, _open_private now rejects a cache whose group/other bits are set (st_mode & 0o077) rather than trusting or rewriting it. A refused cache falls through to the existing handler and the grammar is rebuilt, and since 0600 is what we write, only stale or planted files get refused. Good point on invalidation too, so I didn't add any migration for older 0644 caches; they'll just be rebuilt.

The write path still tightens to 0600 rather than refusing, which seems right since it truncates and we own it.

Added test_load_refuses_group_or_world_accessible alongside the existing three. Full suite green with and without atomicwrites installed.

On the relocation: agreed, I'll keep this as the narrow fix and open the per-user cache directory as a separate PR.

[MegaIng]

@erezsh I can't approve workflows AFAICT.

[erezsh]

@MegaIng I changed your role from “triage” to “write". Let me know if it still doesn't work.

[MegaIng]

@erezsh Works, thank you. I am going to leave merging to you, but I think this PR is ready.

[erezsh]

Thanks, I haven't had time to look into it yet, but I'll do it soon.

[erezsh]

Fable had some comments, and at a glance they seem right. I would appreciate if you could address them.

1. O_TRUNC runs before the ownership check — _open_private, lark/utils.py:319 on the branch. In the non-atomicwrites write path the open truncates the target and only then fstats and refuses it. I reproduced it: a group-writable cache owned by nobody went from 16 bytes → 0 bytes, then PermissionError: ... belongs to another user. For a legitimately shared explicit cache="path" that means we destroy a colleague's cache and then don't replace it. Fix: drop O_TRUNC from the flags and call os.ftruncate(fd, 0) after the checks pass.

2. Refusals surface as ERROR + full traceback. The new PermissionErrors land in except Exception: logger.exception(...) at lark/lark.py:393, so an intentional "won't read this" looks like a crash. Concretely, anyone with an explicit cache="path" file written by the current release (0644 under default umask) gets a traceback on first load after upgrading — before this PR the same situation (version mismatch) fell through silently. (cache=True is unaffected since the version is part of the filename.) Suggest catching PermissionError separately in lark.py and logging a one-line warning, or raising a dedicated exception lark.py can recognise.

3. The read check is stricter than the threat. st.st_mode & 0o077 refuses group/world-readable files, but only write bits let another user tamper; 0o022 matches the stated rationale ("could have been tampered with"). More important: on filesystems that don't honour fchmod (vfat, WSL /mnt/c, some Docker bind mounts, NFS with squash) files report 0777/0755, so an explicit cache="path" there becomes permanently non-functional — reparsed on every Lark() with a traceback each time under the current logging. That's the scenario where point 2 turns from cosmetic to painful.

4. No test coverage of _open_private on Windows.

(I'm not sure 2 is a bad thing, but worth keeping in mind.)