Skip to content

Keep the cache fallback in the temporary directory out of other users' reach - #431

Merged
o-nnerb merged 2 commits into
mainfrom
cache-dir-ownership
Oct 18, 2026
Merged

o-nnerb merged 2 commits into
mainfrom
cache-dir-ownership

Conversation

@o-nnerb

@o-nnerb o-nnerb commented Oct 18, 2026

Copy link
Copy Markdown
Member

What

When the disk cache has no per-user directory to go to, it no longer uses a directory in the shared temporary directory that another user could have made first. (Audit E12, the /tmp fallback.)

The cache lives in Library/Caches on Darwin and in $XDG_CACHE_HOME or $HOME/.cache on Linux. Only a process with neither (no HOME, no XDG_CACHE_HOME: a launch daemon, some CI runners and containers) fell back to the temporary directory, to <tmp>/com.request-dl-nio.Swift.Cache: a path every user can predict, in a directory every user can write to. A user who got there first could create that directory, which made it theirs, and then rename it from under the process whenever they liked (shown with two uids on Linux). The entries are encrypted and authenticated (EncryptionKey), so what that could do was cost the victim its cache and read the names of the directories, which encode the URLs; this closes the door instead of relying on the key.

The change

The fallback is now Internals' FilePath.sharedCachesDirectory, resolved once per process:

  1. <tmp>/request-dl-nio-<euid>, if FilePath.ownedDirectory(named:under:owner:) trusts it. It creates the directory owner only when it is not there, and trusts it when
    • it is a real directory (lstat, so a symbolic link someone planted is not followed), owned by the process's user, that group and others cannot even read; and
    • nobody but that user or root can rename it away: the parent is owned by one of them, and is not writable by group or others, or has the sticky bit (which is what the temporary directory of an ordinary machine has).
  2. <tmp>/request-dl-nio-<random 64 bits>, created owner only with mkdir (which fails if the name exists), if the first is not trusted. It lasts as long as the process does: nobody else could have made it first, and the cache does not survive a restart in this case.
  3. The temporary directory itself, where there is no lstat to ask (Windows), as it always was.

A directory that fails the check is left alone, not repaired or removed: it is someone else's.

What changes for whom

  • Darwin, and Linux with HOME or XDG_CACHE_HOME: nothing.
  • Linux without either: the cache moves from <tmp>/com.request-dl-nio.Swift.Cache/<suite> to <tmp>/request-dl-nio-<euid>/com.request-dl-nio.Swift.Cache/<suite>. The entries under the old path are not read again and nothing removes them (the same thing that happened when the cache moved out of /tmp for everyone else; that part of E12 is still open).
  • Android (POSIX, but not os(Linux), so no XDG directory is looked for): the same as Linux without HOME. The temporary directory of an app is its own, so the check passes and the only difference is the request-dl-nio-<euid> level.
  • Windows: nothing (no lstat to ask; the temporary directory is per user there).

Tests

InternalsCacheDirectoryTests, with who owns what as a parameter (two users cannot be had in a test):

  • created owner only when absent; used when it is the owner's already;
  • not used when it belongs to someone else, when group or others can get into it (0755, 0750, 0770, 0707), or when a symbolic link is there;
  • not used when the parent lets others rename it (0777, no sticky bit); used when it is world-writable and sticky (1777); not used when the parent is missing;
  • the exclusive directory is new every time and owner only, and nil without a parent;
  • the shared fallback is not the temporary directory, is owner only and is the same every time.

Checked by weakening each check (the sticky/permission rule on the parent, the owner-only rule, and lstat for stat): the tests for the parent, for the mode and for the symbolic link fail.

macOS passes locally; Linux, Android and the static SDK (musl) are for CI: the code is POSIX and uses Darwin, Glibc, Musl or Android for mkdir, lstat and geteuid.

🤖 Generated with Claude Code

…' reach

Without HOME or XDG_CACHE_HOME the disk cache used <tmp>/com.request-dl-nio.Swift.Cache,
a path any user can predict, in a directory any user can write to. It now uses
<tmp>/request-dl-nio-<euid> when that is a real directory of the process's user that
nobody else can read or rename away, and otherwise a directory with an unguessable
name that lasts as long as the process.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@codecov

codecov Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.64%. Comparing base (6fcd41d) to head (e884564).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #431      +/-   ##
==========================================
+ Coverage   94.63%   94.64%   +0.01%     
==========================================
  Files         464      465       +1     
  Lines       19367    19423      +56     
==========================================
+ Hits        18327    18382      +55     
- Misses       1040     1041       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

The fallback chain is a function of its own now (sharedCachesDirectory(under:)),
the unguessable part of a name can be given, and the tests cover a parent that
is a file, a directory that cannot be made, a name that is taken, every name
taken, and a fallback that finds nothing.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@o-nnerb
o-nnerb merged commit 9438e2c into main Oct 18, 2026
19 of 20 checks passed
@o-nnerb
o-nnerb deleted the cache-dir-ownership branch October 18, 2026 23:17
o-nnerb added a commit that referenced this pull request Oct 19, 2026
…es and events on a server, cancelled image load (#435)

## What

**The tests the audit listed as not written (V5).** Tests only, no
source change.

| Audit | Test | What it shows |
|---|---|---|
| **N3** | `CacheReplayTests`:
`readPiece_whenTheFileCannotBeRead_isUnreadable` (mode `000`, skipped as
root) and `readPiece_whenADirectoryTookTheFilesPlace_isUnreadable` | An
I/O error that is not a missing file is as unreadable as a removed file:
a miss, not a failure |
| **N1** | `InternalsURLSessionUploadFileTests`:
`write_whenCancelledWhileSpilling_throwsCancellationAndLeavesNoFileOpen`
and
`write_whenCancelledBeforeTheThreshold_throwsCancellationWithoutTouchingDisk`
| A request body drained slowly (a wait that throws `CancellationError`)
and cancelled while it is written to its temporary file: the stream is
closed from a cancelled task and nothing leaks |
| **L1** |
`AsyncBytesServerTests.lines_whenTheReaderPauses_theServerIsHeldBack`
(every executor) | A reader that stops after one line of a 48 MiB body
stops draining the socket: the server sees a stalled connection, the
body is not read into memory to keep up |
| **L1** |
`AsyncBytesServerTests.events_whenALineNeverEnds_failsAndTheTransferIsCancelled`
(every executor) | A response that never ends a line fails with
`AsyncBytesItemTooLargeError` once it is over the limit, and the
connection is closed (the server's writes fail), long before the body
could have been read |
| **N10** |
`RDLImageLoaderNetworkTests.cancellingTheOnlyCaller_stopsTheDownloadOnTheWire`
| A real request cancelled through `RDLImageLoader`: the server holds a
128 MiB download after 1 MiB, the only call is cancelled, the server is
let go, and the body did not reach the client (`isBodyComplete ==
false`, `bodyLength < length`) |

## Checked

- **N1:** closing the stream without `Internals.uncancellable` in
`FileStreamBuffer.close()` makes the first N1 test end the process with
`Fatal error: Leaking file descriptor`, which is what N1 fixed.
Restored.
- **N10:** not cancelling the download when the last call leaves makes
the test fail (`isBodyComplete` is `true`). Restored.
- **L1, readers:** without the server's stall detection the test fails
at its bound. (The `events` test and the second N3 test were not
mutation-checked: the first has no cheap way to fail without an
unbounded wait, and the second shares its shape with the existing "file
gone" test.)
- macOS full suite and the URLSession-only build pass locally. `swift
format lint --strict` is clean on the directories touched. The new
network tests run on macOS here; Linux and the simulators are for CI.

## Not in this pull request

- **N1, the other close sites** (the URL helpers `create`, `readData`,
`write`): they need a hook between opening and closing the file, since
`openFile` fails first in a cancelled task. Unchanged.
- **N7:** already covered by
`subscript_whenAnotherInstanceWritesAfterAMiss_findsTheEntryOnceTheIntervalPassed`,
which uses a second `DiskStorage` over the same directory for the other
process (the index is in the process, so the directory is the only
shared state).
- **I2:** covered by `RedirectErrorTests` on every executor since #421.
- **S5:** the other-uid part needs root; the rule is parameterized by
owner in #431.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: brennobemoura <37243584+brennobemoura@users.noreply.github.com>
Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant