diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md new file mode 100644 index 00000000..8d08beae --- /dev/null +++ b/.claude/CLAUDE.md @@ -0,0 +1,160 @@ +# CLAUDE.md + +Guidance for Claude when working in this repository (File Browser, `filebrowser/filebrowser`). + +## Repo orientation + +- Go backend (`github.com/filebrowser/filebrowser/v2`, ecosystem `go`) + Vue frontend under `frontend/`. +- The project is in **maintenance-only mode** (see `SECURITY.md`). Prefer small, surgical, well-tested changes. +- Version scheme: `v2.63.x`. Conventional-commit messages (`fix(scope): …`, `feat: …`, `chore: …`). +- Verify with `go build ./...`, `go vet ./...`, `go test ./...`. Reuse existing test harnesses (e.g. `signToken`, `scopedUserStorage`, `handle`, `customFSUser`, `mockUserStore`). + +--- + +# Handling security advisories + +Use this playbook when asked to triage, verify, fix, or manage GitHub security advisories. +All advisory state lives on GitHub and is driven with the `gh` CLI. States are +`triage → draft → published`, plus `closed`. + +## 1. Fetch + +```bash +# List by state (also: published, draft, closed) +gh api '/repos/filebrowser/filebrowser/security-advisories?state=triage&per_page=100' \ + --jq '.[] | {ghsa_id, severity, summary, state}' + +# Full report for one advisory (read .summary and .description) +gh api /repos/filebrowser/filebrowser/security-advisories/GHSA-xxxx-xxxx-xxxx \ + --jq '.summary, "---", .description' +``` + +Always pull the **published** set too — you need it to dedup against. + +## 2. Verify each report — do NOT trust the report text + +Read the actual source **at HEAD** and reach one verdict per advisory: + +- **CONFIRMED** — defect exists at HEAD. Quote the exact `file:line`. +- **FIXED** — already patched (find the fix commit). +- **FALSE / NOT APPLICABLE** — claim is wrong, or targets a different project. +- **NOT EXPLOITABLE** — pattern exists but no code path can reach the precondition. +- **DUPLICATE** — of a published advisory, or of another triage advisory. + +Common traps — check each before accepting a report: + +- **"Incomplete fix of a prior advisory."** Read the original fix commit and confirm the *specific* + sibling code path is actually still unguarded — a prior fix may already cover a related path. +- **Wrong project.** Confirm the referenced files, symbols, and endpoints exist in this repo; reports + sometimes describe a fork. If the cited code isn't here, close as not applicable. +- **"Legacy / upgraded / imported records are affected."** Trace the field's git history and confirm that + some released version could actually produce such a record before believing it + (`git log -S'Field' -- path`, `git show `, `git tag --contains `). +- **Overlapping reports.** Two triage advisories may share one root cause — consolidate and fix once. +- **Known, intentionally-unaddressed classes.** Some areas are known and tracked but not fixed (see + `SECURITY.md`'s Known Issues); matching reports are duplicates. + +Record, per advisory: verdict, the `file:line` evidence, exploitability preconditions +(default config? platform-specific? auth required?), and the disposition. + +## 3. Fix (one commit per advisory) + +- Branch off `master` first; never commit fixes directly to `master`. +- **One commit per fix**, each referencing the GHSA in the body (`Refs GHSA-xxxx-xxxx-xxxx`). +- When several advisories share a root cause because parallel code paths diverged, **centralize** the logic + (e.g. `settings.CreateUserHome` used by signup, proxy, and hook provisioning) so they cannot drift again. +- Keep it surgical; match surrounding style. + +## 4. Add a regression test per fix + +- Reuse the existing harnesses; add a focused test asserting the fixed behavior (and that the legitimate + path still works). Commit tests alongside the fixes (`test(scope): …`, `Refs GHSA-…`). +- Run `go test ./... && go vet ./...` before finishing. + +## 5. Severity — set a CVSS vector, don't hand-assert + +Set a CVSS v3.1 vector; GitHub derives the score **and** severity from it (overriding the plain `severity` +field). Encode the real preconditions in the vector so the band is defensible: + +- Requires a specific target configuration/platform (e.g. case-insensitive FS) → **AC:H**. +- Unauthenticated vs needs an account → **PR:N / PR:L**. +- Keep it consistent with the **predecessor CVE's** rating (an incomplete-fix follow-up should not outrank + its parent). Bands: `0.1–3.9` low, `4.0–6.9` medium, `7.0–8.9` high, `9.0–10.0` critical. + +```bash +gh api -X PATCH /repos/filebrowser/filebrowser/security-advisories/GHSA-xxxx-xxxx-xxxx \ + -f cvss_vector_string='CVSS:3.1/AV:N/AC:H/PR:N/UI:N/S:U/C:H/I:H/A:H' \ + --jq '{ghsa_id, severity, score: .cvss.score, vector: .cvss.vector_string}' +``` + +## 6. Clean up the title + +Rewrite reporter titles to the concise, sentence-case, backtick-free style of the published advisories +(state the vuln; drop "Incomplete fix of…" prefixes and jargon). The title is the `summary` field: + +```bash +gh api -X PATCH .../security-advisories/GHSA-xxxx-xxxx-xxxx \ + -f summary='Proxy-auth auto-provisioning ignores createUserDir and grants the server root scope' +``` + +## 7. Rewrite the body into the standard structure + +Reporter reports arrive in whatever shape the reporter used. Before drafting, rewrite the +`description` into the sections below — **reusing the reporter's own wording wherever it is +accurate**, rather than paraphrasing it. Drop the greeting, the offer to help, and any claim the +verification in step 2 disproved. Keep sections in this order and omit the ones that don't apply: + +| Section | Contents | +| --- | --- | +| `## Summary` | What the defect is, in two or three sentences. Note the version it was reported against and the range it was verified over. | +| `## Details` | Root cause, naming `file.go`, the function, and a short quote of the **pre-fix** code. | +| `## PoC` | The reporter's reproduction steps and observed result, trimmed to the essentials. | +| `## Impact` | Who can exploit it (privilege level, preconditions) and what they get. | +| `## Patches` | Fixed version plus a link to the fix commit, and one sentence on what the fix does. Mention it if the reporter re-tested and confirmed. | +| `## Workarounds` | Real mitigations, or `None. Upgrade to vX.Y.Z.` | +| `## Out of scope` | Anything in the original report deliberately **not** treated as a vulnerability, with the reasoning. Needed whenever the advisory is narrower than the report. | +| `## References` | Fix and regression-test commit links. | + +Use `##` headings (matching the published advisories) and keep the body in the maintainer's voice — +first person ("I reproduced…") belongs only inside quoted PoC steps. + +Send it as a file so the Markdown survives shell quoting: + +```bash +jq -Rs '{description: .}' desc.md \ + | gh api -X PATCH .../security-advisories/GHSA-xxxx-xxxx-xxxx --input - +``` + +## 8. Set affected & patched versions + +The package is always `{ecosystem: "go", name: "github.com/filebrowser/filebrowser/v2"}`. + +- `vulnerable_version_range` — `<= ` (the newest tag; find it with + `git tag --list 'v2.*' --sort=-version:refname | head -1`). +- `patched_versions` — the **next** release that will actually ship the fix. It doesn't exist just + because the branch does; it still has to be cut. **When unsure which version that will be, ask.** + +Replace the placeholder versions below before sending: + +```bash +printf '%s' '{"vulnerabilities":[{"package":{"ecosystem":"go","name":"github.com/filebrowser/filebrowser/v2"},"vulnerable_version_range":"<= ","patched_versions":"","vulnerable_functions":[]}]}' \ + | gh api -X PATCH .../security-advisories/GHSA-xxxx-xxxx-xxxx --input - +``` + +## 9. Move state / close, by disposition + +```bash +gh api -X PATCH .../security-advisories/GHSA-xxxx-xxxx-xxxx -f state=draft # fixed, awaiting release +gh api -X PATCH .../security-advisories/GHSA-xxxx-xxxx-xxxx -f state=closed # duplicate / N-A / not-exploitable +``` + +- **CONFIRMED & fixed** → metadata done → **draft**. Publish after the patched release ships. +- **DUPLICATE / NOT APPLICABLE / NOT EXPLOITABLE** → **closed**. +- **DEFERRED** (confirmed but not yet fixed) → leave in **triage** with a note. +- The REST API **cannot post advisory comments** — replies to reporters (dup notice, evidence, links to + the relevant tracking issue) must be posted manually in the advisory UI. Draft the text for the maintainer. + +## 10. Release & publish + +Push the branch, open a PR, merge, tag/release the `patched_versions` you set, then publish the drafts. +Confirm outward-facing/irreversible advisory actions (close, publish) with the maintainer before doing them. diff --git a/CHANGELOG.md b/CHANGELOG.md index 6d5e61a5..b6d52298 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,65 @@ All notable changes to this project will be documented in this file. See [commit-and-tag-version](https://github.com/absolute-version/commit-and-tag-version) for commit guidelines. +## [2.63.21](https://github.com/filebrowser/filebrowser/compare/v2.63.20...v2.63.21) (2026-07-26) + +### Bug Fixes + +* **http:** canonicalize paths before checking access rules ([#6045](https://github.com/filebrowser/filebrowser/issues/6045)) ([e6d70cf](https://github.com/filebrowser/filebrowser/commit/e6d70cf24c0cd79a1787601dc99104ec7e7ca3ef)) + +### Reverts + +* Revert "chore(deps): update all non-major dependencies (#5946)" ([0dd8905](https://github.com/filebrowser/filebrowser/commit/0dd89058867f87a7bc04aa7517d21528a280e27c)), references [#5946](https://github.com/filebrowser/filebrowser/issues/5946) +## [2.63.20](https://github.com/filebrowser/filebrowser/compare/v2.63.19...v2.63.20) (2026-07-25) + +### Bug Fixes + +* use aria-selected ([67e893e](https://github.com/filebrowser/filebrowser/commit/67e893eee7ee411e166d3fcd759a87b6f0971277)) +* **users:** make the provisioned scope check atomic with the save ([fb6aeba](https://github.com/filebrowser/filebrowser/commit/fb6aeba9eae7b8eb401e0db325973781e1ffd08b)) +## [2.63.19](https://github.com/filebrowser/filebrowser/compare/v2.63.18...v2.63.19) (2026-07-25) + +### Bug Fixes + +* accessibility and security improvements ([#6033](https://github.com/filebrowser/filebrowser/issues/6033)) ([b21b124](https://github.com/filebrowser/filebrowser/commit/b21b1245ae1b57f031b2f5d787f32a17532402c0)) +* **auth:** isolate auto-provisioned proxy and hook users to their own home ([8ddd3d1](https://github.com/filebrowser/filebrowser/commit/8ddd3d1db9b9f5727d0bf96ea0e7d9a25a8692b4)) +* **http:** delete abandoned TUS uploads through the scoped filesystem ([9bd79c3](https://github.com/filebrowser/filebrowser/commit/9bd79c3aaeb4a55b0e69cf8976c4a258db2f5e06)) +* **http:** enforce declared Upload-Length on TUS uploads ([4daddec](https://github.com/filebrowser/filebrowser/commit/4daddec6f200b03a721197d8c0b4b652c994894e)) +* **http:** enforce download permission on the checksum branch ([6c69b5c](https://github.com/filebrowser/filebrowser/commit/6c69b5cd895e15b29e563200a9a6dfc27b06525e)) +* **http:** run upload hooks for directories ([#6034](https://github.com/filebrowser/filebrowser/issues/6034)) ([9b78324](https://github.com/filebrowser/filebrowser/commit/9b78324d773c790951cc6a97840c4b55f66b5f3d)) +* process --FollowExternalSymlinks ([c05c668](https://github.com/filebrowser/filebrowser/commit/c05c66814891c3cccef394162e428444b53394e4)) +* return error instead of panicking on an unreadable directory during copy ([#6020](https://github.com/filebrowser/filebrowser/issues/6020)) ([ac46cf0](https://github.com/filebrowser/filebrowser/commit/ac46cf06719575477d5125e7472037c204b3702d)) +* **storage:** reject case-folded home directory collisions ([4b8a8d7](https://github.com/filebrowser/filebrowser/commit/4b8a8d72ce554dde378b5091da74aa930ea18327)) +* **upload:** handle encoded path conflicts safely ([#6040](https://github.com/filebrowser/filebrowser/issues/6040)) ([7361d91](https://github.com/filebrowser/filebrowser/commit/7361d91ea2e8cc200a0f40ac84adcd02aedc9ed2)) +## [2.63.18](https://github.com/filebrowser/filebrowser/compare/v2.63.17...v2.63.18) (2026-07-04) + + +### Bug Fixes + +* avoid recursive conflict checks for copy and move ([#6009](https://github.com/filebrowser/filebrowser/issues/6009)) ([dfc2e88](https://github.com/filebrowser/filebrowser/commit/dfc2e887e1a19d54984a0d7e39a2a63caf73ef19)) +* deduplicate PT language ([4470288](https://github.com/filebrowser/filebrowser/commit/4470288ba1828a14453a99348fd22c62aa0b9460)) +* **preview:** keep the EPUB table-of-contents button clear of the header ([#6010](https://github.com/filebrowser/filebrowser/issues/6010)) ([aac2516](https://github.com/filebrowser/filebrowser/commit/aac25166378422135e624e305c410c54a39374fb)) + +## [2.63.17](https://github.com/filebrowser/filebrowser/compare/v2.63.16...v2.63.17) (2026-06-27) + + +### Bug Fixes + +* **auth:** reject signup when normalized home dir collides (GHSA-7rc3-g7h6-22m7) ([883a36f](https://github.com/filebrowser/filebrowser/commit/883a36f02fcb69566a8628cb47f18fdc73348387)) +* match admin share paths by owner scope ([#5992](https://github.com/filebrowser/filebrowser/issues/5992)) ([43a404c](https://github.com/filebrowser/filebrowser/commit/43a404ca69bf25553bfbbb2b446f0f53077c6302)) +* normalize recursive listing paths to forward slashes ([#6003](https://github.com/filebrowser/filebrowser/issues/6003)) ([2472fbc](https://github.com/filebrowser/filebrowser/commit/2472fbcd30502606feb11fbc8b8dc4f3803e6641)) +* preserve SRT subtitle line breaks ([#6002](https://github.com/filebrowser/filebrowser/issues/6002)) ([d9cf2f0](https://github.com/filebrowser/filebrowser/commit/d9cf2f0100d2c4892cad8e339eacca96df1aa5b6)) +* **raw:** neutralize backslashes in archive entry names (GHSA-83xp-526h-j3ww) ([8503ba6](https://github.com/filebrowser/filebrowser/commit/8503ba61ff51d48a7313896483d130eb6a5abfe0)) +* **share:** delete exact directory share on trailing-slash delete (GHSA-pp88-jhwj-5qh5) ([f30fca6](https://github.com/filebrowser/filebrowser/commit/f30fca636c1af9ef401e9a82ff60391cb3db97e1)) +* **share:** stop exposing password hash and bypass token in share API (GHSA-833g-cqhp-h72j) ([ec13054](https://github.com/filebrowser/filebrowser/commit/ec130546713c44cd24556907552ac554c7f809c9)) + +## [2.63.16](https://github.com/filebrowser/filebrowser/compare/v2.63.15...v2.63.16) (2026-06-23) + + +### Bug Fixes + +* dangling symlink, write, delete scope bugs ([64511ce](https://github.com/filebrowser/filebrowser/commit/64511ce45e3be379e965f7f4fb0929a068d5bb81)) +* restore symlink behavior as opt-in followExternalSymlinks ([a106392](https://github.com/filebrowser/filebrowser/commit/a1063925e15ef27f9d5dc26aae371bbf52af608c)) + ## [2.63.15](https://github.com/filebrowser/filebrowser/compare/v2.63.14...v2.63.15) (2026-06-13) diff --git a/README.md b/README.md index f2ed7957..cc305b56 100644 --- a/README.md +++ b/README.md @@ -3,7 +3,6 @@

[![Build](https://github.com/filebrowser/filebrowser/actions/workflows/ci.yaml/badge.svg)](https://github.com/filebrowser/filebrowser/actions/workflows/ci.yaml) -[![Go Report Card](https://goreportcard.com/badge/github.com/filebrowser/filebrowser/v2)](https://goreportcard.com/report/github.com/filebrowser/filebrowser/v2) [![Version](https://img.shields.io/github/release/filebrowser/filebrowser.svg)](https://github.com/filebrowser/filebrowser/releases/latest) File Browser provides a file managing interface within a specified directory and it can be used to upload, delete, preview and edit your files. It is a **create-your-own-cloud**-kind of software where you can just install it on your server, direct it to a path and access your files through a nice web interface. @@ -19,7 +18,7 @@ This project is a finished product which fulfills its goal: be a single binary w - It can take a while until someone gets back to you. Please be patient. - [Issues](https://github.com/filebrowser/filebrowser/issues) are meant to track bugs. Unrelated issues will be converted into [discussions](https://github.com/filebrowser/filebrowser/discussions). - The priority is triaging issues, addressing security issues and reviewing pull requests meant to solve bugs. -- No new features are planned. Pull requests for new features are not guaranteed to be reviewed. +- No new features are planned. Pull requests for new features will not be reviewed. Please read [@hacdias' personal reflection](https://hacdias.com/2026/03/11/filebrowser/) on the project status. diff --git a/SECURITY.md b/SECURITY.md index 490a9bea..4d8bb2a8 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -2,25 +2,32 @@ ## Supported Versions -Use this section to tell people about which versions of your project are -currently being supported with security updates. +| Version | Supported | +| ------- | --------- | +| 2.x | ✅ | +| < 2.0 | ❌ | -| Version | Supported | -| ------- | ------------------ | -| 2.x | :white_check_mark: | -| < 2.0 | :x: | +## Before Reporting + +This project is in maintenance-only mode. To avoid duplicates, first check the [existing advisories](https://github.com/filebrowser/filebrowser/security/advisories) and open issues, and confirm: + +- **It concerns this project, not a fork.** Reports about code, features, or endpoints that don't exist here belong to the relevant fork. +- **It isn't an already-known class** that remains unaddressed: + - Command execution, runner, and hooks (opt-in, disabled by default) — [#5199](https://github.com/filebrowser/filebrowser/issues/5199) + - Session and JWT handling — [#5216](https://github.com/filebrowser/filebrowser/issues/5216) + +Reports covering these are likely to be closed as duplicates. ## Reporting a Vulnerability -Vulnerabilities with critical impact should be reported on the [Security](https://github.com/filebrowser/filebrowser/security) page of this repository, which is a private way of communicating vulnerabilities to maintainers. This project is in maintenance-only mode and it can take a while until someone gets back to you. +- **Critical:** report privately via the [Security](https://github.com/filebrowser/filebrowser/security) page. +- **Non-critical:** open a public issue so the community can help; we'll label it as a security issue. -If it is not a critical vulnerability, please open an issue and we will categorize it as a security issue. By giving visibility, we can get more help from the community at fixing such issues. +Please include, where possible: -When reporting an issue, where possible, please provide at least: +- The commit the issue was found at +- A plaintext proof of concept (no binaries) +- Steps to reproduce +- Recommended remediation, if any -* The commit version the issue was identified at -* A proof of concept (plaintext; no binaries) -* Steps to reproduce -* Your recommended remediation(s), if any. - -The File Browser team is a volunteer-only effort, and may reach back out for clarification. +We're a volunteer effort, so responses can take a while, and we may reach out for clarification. diff --git a/auth/hook.go b/auth/hook.go index 60c75461..6653cd9b 100644 --- a/auth/hook.go +++ b/auth/hook.go @@ -68,7 +68,7 @@ func (a *HookAuth) Auth(r *http.Request, usr users.Store, stg *settings.Settings case "block": return nil, os.ErrPermission case "pass": - u, err := a.Users.Get(a.Server.Root, a.Cred.Username) + u, err := a.Users.Get(a.Server.Root, a.Server.FollowExternalSymlinks, a.Cred.Username) if err != nil || !users.CheckPwd(a.Cred.Password, u.Password) { return nil, os.ErrPermission } @@ -129,7 +129,7 @@ func (a *HookAuth) GetValues(s string) { // SaveUser updates the existing user or creates a new one when not found func (a *HookAuth) SaveUser() (*users.User, error) { - u, err := a.Users.Get(a.Server.Root, a.Cred.Username) + u, err := a.Users.Get(a.Server.Root, a.Server.FollowExternalSymlinks, a.Cred.Username) if err != nil && !errors.Is(err, fberrors.ErrNotExist) { return nil, err } @@ -157,15 +157,16 @@ func (a *HookAuth) SaveUser() (*users.User, error) { } u = a.GetUser(d) - userHome, err := a.Settings.MakeUserDir(u.Username, u.Scope, a.Server.Root) + // A scope explicitly returned by the hook takes precedence over the + // automatic per-user home directory derivation. + _, explicitScope := a.Fields.Values["user.scope"] + derivedScope, err := a.Settings.CreateUserHome(u, a.Server.Root, explicitScope) if err != nil { - return nil, fmt.Errorf("user: failed to mkdir user home dir: [%s]", userHome) + return nil, err } - u.Scope = userHome - log.Printf("user: %s, home dir: [%s].", u.Username, userHome) + log.Printf("user: %s, home dir: [%s].", u.Username, u.Scope) - err = a.Users.Save(u) - if err != nil { + if err := a.Users.SaveProvisioned(u, derivedScope); err != nil { return nil, err } } else if p := !users.CheckPwd(a.Cred.Password, u.Password); len(a.Fields.Values) > 1 || p { diff --git a/auth/hook_test.go b/auth/hook_test.go index 4b0112b1..10819759 100644 --- a/auth/hook_test.go +++ b/auth/hook_test.go @@ -5,6 +5,9 @@ import ( "path/filepath" "runtime" "testing" + + "github.com/filebrowser/filebrowser/v2/settings" + "github.com/filebrowser/filebrowser/v2/users" ) // writeHookScript writes a POSIX shell script to a temp file and returns its @@ -86,3 +89,68 @@ fi t.Fatalf("expected action %q, got %q", "auth", action) } } + +// newHookAuth builds a HookAuth for a freshly provisioned user with the given +// parsed hook fields (hook.action=auth is added automatically). +func newHookAuth(store *mockUserStore, s *settings.Settings, srv *settings.Server, username string, fields map[string]string) *HookAuth { + fields["hook.action"] = "auth" + return &HookAuth{ + Users: store, + Settings: s, + Server: srv, + Cred: hookCred{Username: username, Password: "a-strong-password"}, + Fields: hookFields{Values: fields}, + } +} + +// With CreateUserDir enabled and no explicit scope from the hook, a provisioned +// hook user must receive its own home directory rather than the server root. +func TestHookSaveUserCreateUserDirIsolatesScope(t *testing.T) { + t.Parallel() + + store := &mockUserStore{users: make(map[string]*users.User)} + srv := &settings.Server{Root: t.TempDir()} + s := &settings.Settings{ + Key: []byte("key"), + CreateUserDir: true, + UserHomeBasePath: "/users", + Defaults: settings.UserDefaults{ + Scope: ".", + Perm: users.Permissions{Create: true}, + }, + } + + u, err := newHookAuth(store, s, srv, "alice", map[string]string{}).SaveUser() + if err != nil { + t.Fatalf("SaveUser error: %v", err) + } + if u.Scope != "/users/alice" { + t.Errorf("hook user without explicit scope: expected /users/alice, got %q", u.Scope) + } +} + +// A scope explicitly returned by the hook takes precedence over the automatic +// per-user home directory derivation. +func TestHookSaveUserRespectsExplicitScope(t *testing.T) { + t.Parallel() + + store := &mockUserStore{users: make(map[string]*users.User)} + srv := &settings.Server{Root: t.TempDir()} + s := &settings.Settings{ + Key: []byte("key"), + CreateUserDir: true, + UserHomeBasePath: "/users", + Defaults: settings.UserDefaults{ + Scope: ".", + Perm: users.Permissions{Create: true}, + }, + } + + u, err := newHookAuth(store, s, srv, "teamlead", map[string]string{"user.scope": "/shared/team"}).SaveUser() + if err != nil { + t.Fatalf("SaveUser error: %v", err) + } + if u.Scope != "/shared/team" { + t.Errorf("explicit hook scope should win, got %q", u.Scope) + } +} diff --git a/auth/json.go b/auth/json.go index 2284dc7f..be8ad1dc 100644 --- a/auth/json.go +++ b/auth/json.go @@ -55,7 +55,7 @@ func (a JSONAuth) Auth(r *http.Request, usr users.Store, _ *settings.Settings, s } } - u, err := usr.Get(srv.Root, cred.Username) + u, err := usr.Get(srv.Root, srv.FollowExternalSymlinks, cred.Username) hash := dummyHash if err == nil { diff --git a/auth/none.go b/auth/none.go index c9381a83..30ea7129 100644 --- a/auth/none.go +++ b/auth/none.go @@ -15,7 +15,7 @@ type NoAuth struct{} // Auth uses authenticates user 1. func (a NoAuth) Auth(_ *http.Request, usr users.Store, _ *settings.Settings, srv *settings.Server) (*users.User, error) { - return usr.Get(srv.Root, uint(1)) + return usr.Get(srv.Root, srv.FollowExternalSymlinks, uint(1)) } // LoginPage tells that no auth doesn't require a login page. diff --git a/auth/proxy.go b/auth/proxy.go index 57eddd4a..5550f102 100644 --- a/auth/proxy.go +++ b/auth/proxy.go @@ -20,7 +20,7 @@ type ProxyAuth struct { // Auth authenticates the user via an HTTP header. func (a ProxyAuth) Auth(r *http.Request, usr users.Store, setting *settings.Settings, srv *settings.Server) (*users.User, error) { username := r.Header.Get(a.Header) - user, err := usr.Get(srv.Root, username) + user, err := usr.Get(srv.Root, srv.FollowExternalSymlinks, username) if errors.Is(err, fberrors.ErrNotExist) { return a.createUser(usr, setting, srv, username) } @@ -50,15 +50,12 @@ func (a ProxyAuth) createUser(usr users.Store, setting *settings.Settings, srv * user.Perm.Execute = false user.Commands = []string{} - var userHome string - userHome, err = setting.MakeUserDir(user.Username, user.Scope, srv.Root) - if err != nil { + var derivedScope bool + if derivedScope, err = setting.CreateUserHome(user, srv.Root, false); err != nil { return nil, err } - user.Scope = userHome - err = usr.Save(user) - if err != nil { + if err = usr.SaveProvisioned(user, derivedScope); err != nil { return nil, err } diff --git a/auth/proxy_test.go b/auth/proxy_test.go index 86fb7102..5d9b5ace 100644 --- a/auth/proxy_test.go +++ b/auth/proxy_test.go @@ -2,6 +2,7 @@ package auth import ( "net/http" + "strings" "testing" fberrors "github.com/filebrowser/filebrowser/v2/errors" @@ -13,7 +14,7 @@ type mockUserStore struct { users map[string]*users.User } -func (m *mockUserStore) Get(_ string, id interface{}) (*users.User, error) { +func (m *mockUserStore) Get(_ string, _ bool, id interface{}) (*users.User, error) { if v, ok := id.(string); ok { if u, ok := m.users[v]; ok { return u, nil @@ -22,14 +23,33 @@ func (m *mockUserStore) Get(_ string, id interface{}) (*users.User, error) { return nil, fberrors.ErrNotExist } -func (m *mockUserStore) Gets(_ string) ([]*users.User, error) { return nil, nil } -func (m *mockUserStore) Update(_ *users.User, _ ...string) error { return nil } +func (m *mockUserStore) GetByScope(scope string) (*users.User, error) { + for _, u := range m.users { + if strings.EqualFold(u.Scope, scope) { + return u, nil + } + } + return nil, fberrors.ErrNotExist +} + +func (m *mockUserStore) Gets(_ string, _ bool) ([]*users.User, error) { return nil, nil } +func (m *mockUserStore) Update(_ *users.User, _ ...string) error { return nil } func (m *mockUserStore) Save(user *users.User) error { m.users[user.Username] = user return nil } + +func (m *mockUserStore) SaveProvisioned(user *users.User, derivedScope bool) error { + if derivedScope { + if _, err := m.GetByScope(user.Scope); err == nil { + return fberrors.ErrExist + } + } + return m.Save(user) +} + func (m *mockUserStore) Delete(_ interface{}) error { return nil } -func (m *mockUserStore) LastUpdate(_ uint) int64 { return 0 } +func (m *mockUserStore) LastUpdate(_ uint) int64 { return 0 } func TestProxyAuthCreateUserRestrictsDefaults(t *testing.T) { t.Parallel() @@ -77,3 +97,46 @@ func TestProxyAuthCreateUserRestrictsDefaults(t *testing.T) { t.Error("auto-provisioned proxy user should retain Create permission from defaults") } } + +// With CreateUserDir enabled, two distinct proxy-authenticated users must each +// receive their own home directory instead of both inheriting the server root. +func TestProxyAuthCreateUserDirIsolatesScope(t *testing.T) { + t.Parallel() + + store := &mockUserStore{users: make(map[string]*users.User)} + srv := &settings.Server{Root: t.TempDir()} + s := &settings.Settings{ + Key: []byte("key"), + AuthMethod: MethodProxyAuth, + CreateUserDir: true, + UserHomeBasePath: "/users", + Defaults: settings.UserDefaults{ + Scope: ".", + Perm: users.Permissions{Create: true}, + }, + } + + auth := ProxyAuth{Header: "X-Remote-User"} + provision := func(name string) *users.User { + req, _ := http.NewRequest(http.MethodGet, "/", http.NoBody) + req.Header.Set("X-Remote-User", name) + u, err := auth.Auth(req, store, s, srv) + if err != nil { + t.Fatalf("Auth(%q) error: %v", name, err) + } + return u + } + + alice := provision("alice") + bob := provision("bob") + + if alice.Scope == "/" || bob.Scope == "/" { + t.Fatalf("provisioned users inherited the server root: alice=%q bob=%q", alice.Scope, bob.Scope) + } + if alice.Scope == bob.Scope { + t.Fatalf("distinct users must get distinct scopes, both got %q", alice.Scope) + } + if alice.Scope != "/users/alice" { + t.Errorf("expected /users/alice, got %q", alice.Scope) + } +} diff --git a/cmd/cmd_test.go b/cmd/cmd_test.go index e4b45c47..91a9c632 100644 --- a/cmd/cmd_test.go +++ b/cmd/cmd_test.go @@ -5,6 +5,10 @@ import ( "github.com/samber/lo" "github.com/spf13/cobra" + "github.com/spf13/pflag" + + "github.com/filebrowser/filebrowser/v2/auth" + "github.com/filebrowser/filebrowser/v2/settings" ) // TestEnvCollisions ensures that there are no collisions in the produced environment @@ -33,3 +37,25 @@ func testEnvCollisions(t *testing.T, cmd *cobra.Command) { t.Errorf("Found duplicate environment variable keys for command %q: %v", cmd.Name(), duplicates) } } + +// TestGetSettingsFollowExternalSymlinks ensures that the followExternalSymlinks +// flag is persisted to the server config when set via "config set". +func TestGetSettingsFollowExternalSymlinks(t *testing.T) { + flags := pflag.NewFlagSet("test", pflag.ContinueOnError) + addConfigFlags(flags) + + if err := flags.Parse([]string{"--followExternalSymlinks"}); err != nil { + t.Fatal(err) + } + + set := &settings.Settings{AuthMethod: auth.MethodJSONAuth} + ser := &settings.Server{} + + if _, err := getSettings(flags, set, ser, &auth.JSONAuth{}, false); err != nil { + t.Fatal(err) + } + + if !ser.FollowExternalSymlinks { + t.Error("expected FollowExternalSymlinks to be persisted as true") + } +} diff --git a/cmd/config.go b/cmd/config.go index e3bb2b86..b292f01d 100644 --- a/cmd/config.go +++ b/cmd/config.go @@ -229,6 +229,7 @@ func printSettings(ser *settings.Server, set *settings.Settings, auther auth.Aut fmt.Fprintf(w, "\tThumbnails Enabled:\t%t\n", ser.EnableThumbnails) fmt.Fprintf(w, "\tResize Preview:\t%t\n", ser.ResizePreview) fmt.Fprintf(w, "\tType Detection by Header:\t%t\n", ser.TypeDetectionByHeader) + fmt.Fprintf(w, "\tFollow External Symlinks:\t%t\n", ser.FollowExternalSymlinks) fmt.Fprintln(w, "\nTUS:") fmt.Fprintf(w, "\tChunk size:\t%d\n", set.Tus.ChunkSize) @@ -313,6 +314,8 @@ func getSettings(flags *pflag.FlagSet, set *settings.Settings, ser *settings.Ser case "disableImageResolutionCalc": ser.ImageResolutionCal, err = flags.GetBool(flag.Name) ser.ImageResolutionCal = !ser.ImageResolutionCal + case "followExternalSymlinks": + ser.FollowExternalSymlinks, err = flags.GetBool(flag.Name) // Settings flags from [addConfigFlags] case "signup": diff --git a/cmd/root.go b/cmd/root.go index 13139a7e..1d34f866 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -13,6 +13,7 @@ import ( "os" "os/signal" "path/filepath" + "strings" "syscall" "time" @@ -111,6 +112,7 @@ func addServerFlags(flags *pflag.FlagSet) { flags.Bool("disableExec", true, "disables Command Runner feature") flags.Bool("disableTypeDetectionByHeader", false, "disables type detection by reading file headers") flags.Bool("disableImageResolutionCalc", false, "disables image resolution calculation by reading image files") + flags.Bool("followExternalSymlinks", false, "follow symlinks whose target is outside the user scope (unsafe)") } var rootCmd = &cobra.Command{ @@ -353,6 +355,10 @@ func getServerSettings(v *viper.Viper, st *storage.Storage) (*settings.Server, e server.EnableExec = !v.GetBool("disableExec") } + if v.IsSet("followExternalSymlinks") { + server.FollowExternalSymlinks = v.GetBool("followExternalSymlinks") + } + if isAddrSet && isSocketSet { return nil, errors.New("--socket flag cannot be used with --address, --port, --key nor --cert") } @@ -369,6 +375,25 @@ func getServerSettings(v *viper.Viper, st *storage.Storage) (*settings.Server, e log.Println("WARNING: read https://github.com/filebrowser/filebrowser/issues/5199") } + if server.FollowExternalSymlinks { + log.Println("WARNING: Following external symlinks enabled!") + log.Println("WARNING: Symlinks pointing outside a user's scope will be followed,") + log.Println("WARNING: which can expose files outside that scope. Only enable this if") + log.Println("WARNING: you fully understand and trust the contents of every user scope.") + } + + if set, err := st.Settings.Get(); err == nil && set.Signup { + scope := strings.TrimSpace(set.Defaults.Scope) + scopeIsRoot := scope == "" || scope == "." || scope == "/" + + if !set.CreateUserDir && scopeIsRoot { + log.Println("WARNING: Signup is enabled without createUserDir and the default scope is") + log.Println("WARNING: the server root, so every self-registered user can read, modify and") + log.Println("WARNING: delete all files File Browser serves, including other users' files.") + log.Println("WARNING: Enable createUserDir, or set a default scope other than the root.") + } + } + return server, nil } @@ -446,19 +471,20 @@ func quickSetup(v *viper.Viper, s *storage.Storage) error { } ser := &settings.Server{ - BaseURL: v.GetString("baseURL"), - Port: v.GetString("port"), - Log: v.GetString("log"), - TLSKey: v.GetString("key"), - TLSCert: v.GetString("cert"), - Address: v.GetString("address"), - Root: v.GetString("root"), - TokenExpirationTime: v.GetString("tokenExpirationTime"), - EnableThumbnails: !v.GetBool("disableThumbnails"), - ResizePreview: !v.GetBool("disablePreviewResize"), - EnableExec: !v.GetBool("disableExec"), - TypeDetectionByHeader: !v.GetBool("disableTypeDetectionByHeader"), - ImageResolutionCal: !v.GetBool("disableImageResolutionCalc"), + BaseURL: v.GetString("baseURL"), + Port: v.GetString("port"), + Log: v.GetString("log"), + TLSKey: v.GetString("key"), + TLSCert: v.GetString("cert"), + Address: v.GetString("address"), + Root: v.GetString("root"), + TokenExpirationTime: v.GetString("tokenExpirationTime"), + EnableThumbnails: !v.GetBool("disableThumbnails"), + ResizePreview: !v.GetBool("disablePreviewResize"), + EnableExec: !v.GetBool("disableExec"), + TypeDetectionByHeader: !v.GetBool("disableTypeDetectionByHeader"), + ImageResolutionCal: !v.GetBool("disableImageResolutionCalc"), + FollowExternalSymlinks: v.GetBool("followExternalSymlinks"), } err = s.Settings.SaveServer(ser) diff --git a/cmd/rules.go b/cmd/rules.go index bdb1d1cf..f3e00bb6 100644 --- a/cmd/rules.go +++ b/cmd/rules.go @@ -36,7 +36,7 @@ func runRules(st *storage.Storage, cmd *cobra.Command, usersFn func(*users.User) } if id != nil { var user *users.User - user, err = st.Users.Get("", id) + user, err = st.Users.Get("", false, id) if err != nil { return err } diff --git a/cmd/users_export.go b/cmd/users_export.go index 9bbec6d8..768c381f 100644 --- a/cmd/users_export.go +++ b/cmd/users_export.go @@ -15,7 +15,7 @@ var usersExportCmd = &cobra.Command{ path to the file where you want to write the users.`, Args: jsonYamlArg, RunE: withStore(func(_ *cobra.Command, args []string, st *store) error { - list, err := st.Users.Gets("") + list, err := st.Users.Gets("", false) if err != nil { return err } diff --git a/cmd/users_find.go b/cmd/users_find.go index 09bc8d47..fc91af86 100644 --- a/cmd/users_find.go +++ b/cmd/users_find.go @@ -36,14 +36,14 @@ var findUsers = withStore(func(_ *cobra.Command, args []string, st *store) error if len(args) == 1 { username, id := parseUsernameOrID(args[0]) if username != "" { - user, err = st.Users.Get("", username) + user, err = st.Users.Get("", false, username) } else { - user, err = st.Users.Get("", id) + user, err = st.Users.Get("", false, id) } list = []*users.User{user} } else { - list, err = st.Users.Gets("") + list, err = st.Users.Gets("", false) } if err != nil { diff --git a/cmd/users_import.go b/cmd/users_import.go index 73effca6..0db3704f 100644 --- a/cmd/users_import.go +++ b/cmd/users_import.go @@ -40,7 +40,7 @@ list or set it to 0.`, } for _, user := range list { - err = user.Clean("") + err = user.Clean("", false) if err != nil { return err } @@ -52,7 +52,7 @@ list or set it to 0.`, } if replace { - oldUsers, userImportErr := st.Users.Gets("") + oldUsers, userImportErr := st.Users.Gets("", false) if userImportErr != nil { return userImportErr } @@ -76,7 +76,7 @@ list or set it to 0.`, } for _, user := range list { - onDB, err := st.Users.Get("", user.ID) + onDB, err := st.Users.Get("", false, user.ID) // User exists in DB. if err == nil { @@ -88,7 +88,7 @@ list or set it to 0.`, // with the new username. If there is, print an error and cancel the // operation if user.Username != onDB.Username { - if conflictuous, err := st.Users.Get("", user.Username); err == nil { + if conflictuous, err := st.Users.Get("", false, user.Username); err == nil { return usernameConflictError(user.Username, conflictuous.ID, user.ID) } } diff --git a/cmd/users_update.go b/cmd/users_update.go index e9a484fc..12484342 100644 --- a/cmd/users_update.go +++ b/cmd/users_update.go @@ -43,9 +43,9 @@ options you want to change.`, user *users.User ) if id != 0 { - user, err = st.Users.Get("", id) + user, err = st.Users.Get("", false, id) } else { - user, err = st.Users.Get("", username) + user, err = st.Users.Get("", false, username) } if err != nil { return err diff --git a/files/case.go b/files/case.go new file mode 100644 index 00000000..24f06bb2 --- /dev/null +++ b/files/case.go @@ -0,0 +1,34 @@ +package files + +import ( + "path/filepath" + "runtime" + "strings" + + "github.com/spf13/afero" +) + +// CaseInsensitive reports whether the filesystem backing root treats file names +// case-insensitively, as NTFS, APFS, HFS+, exFAT and CIFS do. It creates a +// probe file and looks it up under a different case, mirroring how git detects +// core.ignoreCase. +// +// It probes rather than inferring from runtime.GOOS because neither direction +// holds: macOS can be formatted case-sensitively, and a Linux host commonly +// serves a case-insensitive mount. When the root cannot be written to, it falls +// back to the host's usual default rather than assuming case-sensitivity, since +// a read-only root is still exposed to the disclosure this guards against. +func CaseInsensitive(fs afero.Fs, root string) bool { + probe, err := afero.TempFile(fs, root, "fb-case-probe-") + if err != nil { + return runtime.GOOS == "windows" || runtime.GOOS == "darwin" + } + + name := probe.Name() + probe.Close() + defer fs.Remove(name) //nolint:errcheck + + dir, base := filepath.Split(name) + _, err = fs.Stat(filepath.Join(dir, strings.ToUpper(base))) + return err == nil +} diff --git a/files/case_test.go b/files/case_test.go new file mode 100644 index 00000000..8feb708c --- /dev/null +++ b/files/case_test.go @@ -0,0 +1,44 @@ +package files + +import ( + "strings" + "testing" + + "github.com/spf13/afero" +) + +// MemMapFs keys its entries by exact name, so it is case-sensitive. +func TestCaseInsensitiveReportsCaseSensitiveFs(t *testing.T) { + fs := afero.NewMemMapFs() + if err := fs.MkdirAll("/root", 0o755); err != nil { + t.Fatal(err) + } + + if CaseInsensitive(fs, "/root") { + t.Error("CaseInsensitive() = true for a case-sensitive filesystem; want false") + } + assertNoProbeLeft(t, fs, "/root") +} + +// Whatever the verdict, the probe file must not be left behind in the user's +// data directory. +func TestCaseInsensitiveRemovesProbe(t *testing.T) { + root := t.TempDir() + fs := afero.NewOsFs() + + t.Logf("CaseInsensitive(%s) = %v", root, CaseInsensitive(fs, root)) + assertNoProbeLeft(t, fs, root) +} + +func assertNoProbeLeft(t *testing.T, fs afero.Fs, root string) { + t.Helper() + entries, err := afero.ReadDir(fs, root) + if err != nil { + t.Fatal(err) + } + for _, e := range entries { + if strings.HasPrefix(e.Name(), "fb-case-probe-") { + t.Errorf("probe file left behind: %s", e.Name()) + } + } +} diff --git a/files/file_test.go b/files/file_test.go index b5a8b37f..45b114f6 100644 --- a/files/file_test.go +++ b/files/file_test.go @@ -92,6 +92,125 @@ func TestScopedFs(t *testing.T) { t.Fatalf("expected in-scope symlink target to be accessible, got %v", err) } }) + + // Regression for the dangling-symlink write escape (GHSA-8wc8-hf36-mjh9 / + // GHSA-fh54-6rfh-r8f3): a symlink whose target does not exist yet must not be + // followed for writes. Previously within() validated the link's in-scope + // parent directory, so OpenFile(O_CREATE) dereferenced the link and created + // the file at its out-of-scope target. + t.Run("write through a dangling escaping symlink is rejected", func(t *testing.T) { + base := t.TempDir() + scope := filepath.Join(base, "scope") + outside := filepath.Join(base, "outside") + for _, d := range []string{scope, outside} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatal(err) + } + } + outsideTarget := filepath.Join(outside, "created.txt") // does not exist yet + if err := os.Symlink(outsideTarget, filepath.Join(scope, "evil")); err != nil { + t.Skipf("cannot create symlink: %v", err) + } + fs := NewScopedFs(afero.NewOsFs(), scope) + + f, err := fs.OpenFile("/evil", os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0o644) + if err == nil { + _ = f.Close() + t.Fatal("VULNERABLE: write through a dangling escaping symlink was allowed") + } + if !os.IsPermission(err) { + t.Fatalf("expected permission error, got %v", err) + } + if _, statErr := os.Stat(outsideTarget); statErr == nil { + t.Fatal("VULNERABLE: file was created outside the scope") + } + }) + + // A dangling *relative* symlink that lives under an escaping directory + // symlink must be resolved against the link's real directory, not its lexical + // parent. Otherwise the symlinked ancestor can shift the computed target back + // into scope while the real OS write lands outside it. + t.Run("write through a dangling relative symlink under a symlinked dir is rejected", func(t *testing.T) { + base := t.TempDir() + scope := filepath.Join(base, "scope") + outside := filepath.Join(base, "outside") + for _, d := range []string{scope, outside} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatal(err) + } + } + // An escaping directory symlink inside the scope: /scope/m -> /base/outside. + if err := os.Symlink(outside, filepath.Join(scope, "m")); err != nil { + t.Skipf("cannot create symlink: %v", err) + } + // A relative dangling symlink inside the escaping dir whose target, + // resolved against the real directory (/base/outside), is /base/escaped — + // outside the scope. + if err := os.Symlink("../escaped", filepath.Join(outside, "evil")); err != nil { + t.Skipf("cannot create symlink: %v", err) + } + fs := NewScopedFs(afero.NewOsFs(), scope) + + f, err := fs.OpenFile("/m/evil", os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0o644) + if err == nil { + _ = f.Close() + t.Fatal("VULNERABLE: write through a dangling relative symlink under a symlinked dir was allowed") + } + if !os.IsPermission(err) { + t.Fatalf("expected permission error, got %v", err) + } + if _, statErr := os.Stat(filepath.Join(base, "escaped")); statErr == nil { + t.Fatal("VULNERABLE: file was created outside the scope") + } + }) + + // Regression for the symlink-following delete escape (GHSA-hq4g-mpch-f9vp / + // GHSA-fmm7-x4gx-8jhr): Remove/RemoveAll used to skip guard(), so RemoveAll + // followed a symlinked ancestor escaping the scope and deleted an + // out-of-scope file. + t.Run("RemoveAll through an escaping symlink is rejected", func(t *testing.T) { + base := t.TempDir() + scope := filepath.Join(base, "scope") + outside := filepath.Join(base, "outside") + for _, d := range []string{scope, outside} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatal(err) + } + } + victim := filepath.Join(outside, "victim.txt") + if err := os.WriteFile(victim, []byte("keep"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Symlink(outside, filepath.Join(scope, "link")); err != nil { + t.Skipf("cannot create symlink: %v", err) + } + fs := NewScopedFs(afero.NewOsFs(), scope) + + if err := fs.RemoveAll("/link/victim.txt"); !os.IsPermission(err) { + t.Fatalf("expected RemoveAll through escaping symlink to be rejected, got %v", err) + } + if _, statErr := os.Stat(victim); statErr != nil { + t.Fatalf("VULNERABLE: out-of-scope victim file was deleted: %v", statErr) + } + }) + + // The guard added for the delete escape must not break legitimate deletes of + // in-scope files. + t.Run("RemoveAll of an in-scope file is allowed", func(t *testing.T) { + scope := t.TempDir() + target := filepath.Join(scope, "deleteme.txt") + if err := os.WriteFile(target, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + fs := NewScopedFs(afero.NewOsFs(), scope) + + if err := fs.RemoveAll("/deleteme.txt"); err != nil { + t.Fatalf("expected in-scope RemoveAll to succeed, got %v", err) + } + if _, statErr := os.Stat(target); statErr == nil { + t.Fatal("expected in-scope file to be deleted") + } + }) } // stat must reject a regular file reached through a symlinked ancestor that diff --git a/files/fs_test.go b/files/fs_test.go new file mode 100644 index 00000000..5974b20a --- /dev/null +++ b/files/fs_test.go @@ -0,0 +1,121 @@ +package files + +import ( + "os" + "path/filepath" + "testing" + + "github.com/spf13/afero" +) + +// TestNewFs verifies that NewFs picks the right implementation and that the +// follow-external-symlinks toggle flips whether a symlink pointing outside the +// scope is honored. +func TestNewFs(t *testing.T) { + base := t.TempDir() + scope := filepath.Join(base, "srv") + outside := filepath.Join(base, "outside") + for _, d := range []string{scope, outside} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatal(err) + } + } + if err := os.WriteFile(filepath.Join(outside, "secret.txt"), []byte("secret"), 0o644); err != nil { + t.Fatal(err) + } + // A symlink lexically inside the scope whose target resolves outside it. + if err := os.Symlink(outside, filepath.Join(scope, "escape")); err != nil { + t.Skipf("cannot create symlink: %v", err) + } + + t.Run("disabled returns a ScopedFs that rejects the escaping symlink", func(t *testing.T) { + fs := NewFs(afero.NewOsFs(), scope, false) + if _, ok := fs.(*ScopedFs); !ok { + t.Fatalf("expected *ScopedFs, got %T", fs) + } + if _, err := fs.Stat("/escape"); !os.IsPermission(err) { + t.Fatalf("expected stat of escaping symlink to be rejected, got %v", err) + } + }) + + t.Run("enabled returns a BasePathFs that follows the escaping symlink", func(t *testing.T) { + fs := NewFs(afero.NewOsFs(), scope, true) + if _, ok := fs.(*afero.BasePathFs); !ok { + t.Fatalf("expected *afero.BasePathFs, got %T", fs) + } + if _, err := fs.Stat("/escape"); err != nil { + t.Fatalf("expected escaping symlink to be followed, got %v", err) + } + b, err := afero.ReadFile(fs, "/escape/secret.txt") + if err != nil { + t.Fatalf("expected to read through escaping symlink, got %v", err) + } + if string(b) != "secret" { + t.Fatalf("got %q, want %q", b, "secret") + } + + // The link must also appear in a directory listing (the symptom in #5998). + entries, err := afero.ReadDir(fs, "/") + if err != nil { + t.Fatal(err) + } + var found bool + for _, e := range entries { + if e.Name() == "escape" { + found = true + } + } + if !found { + t.Fatal("expected escaping symlink to appear in the listing") + } + }) +} + +// TestBasePath verifies BasePath extracts the underlying *afero.BasePathFs from +// either filesystem NewFs may return, so User.FullPath keeps working. +func TestBasePath(t *testing.T) { + root := t.TempDir() + osFs := afero.NewOsFs() + + for _, tc := range []struct { + name string + followExternal bool + }{ + {"ScopedFs", false}, + {"BasePathFs", true}, + } { + t.Run(tc.name, func(t *testing.T) { + fs := NewFs(osFs, root, tc.followExternal) + base := BasePath(fs) + if base == nil { + t.Fatalf("expected non-nil base for %T", fs) + } + got := afero.FullBaseFsPath(base, "/x") + want := filepath.Join(root, "x") + if got != want { + t.Fatalf("FullBaseFsPath: got %q, want %q", got, want) + } + }) + } + + if got := BasePath(osFs); got != nil { + t.Fatalf("expected nil base for a plain OsFs, got %v", got) + } +} + +// TestFileInfoRealPathUsesBasePathFsRealPath mirrors +// TestFileInfoRealPathUsesScopedFsRealPath for the follow-external-symlinks case, +// where the user filesystem is a bare BasePathFs. +func TestFileInfoRealPathUsesBasePathFsRealPath(t *testing.T) { + root := t.TempDir() + file := &FileInfo{ + Fs: NewFs(afero.NewOsFs(), root, true), + Path: "/root/downloads", + } + + got := file.RealPath() + want := filepath.Join(root, "root", "downloads") + if got != want { + t.Fatalf("got %q, want %q", got, want) + } +} diff --git a/files/scoped.go b/files/scoped.go index eef6378b..97ce32d0 100644 --- a/files/scoped.go +++ b/files/scoped.go @@ -25,6 +25,11 @@ var ( _ afero.Lstater = (*ScopedFs)(nil) ) +// maxSymlinkHops bounds how many dangling symlinks within() will follow before +// giving up, so a pathological chain cannot loop forever. It mirrors the kernel +// MAXSYMLINKS limit; the operation is rejected once the bound is exceeded. +const maxSymlinkHops = 255 + func NewScopedFs(source afero.Fs, path string) *ScopedFs { if s, ok := source.(*ScopedFs); ok { source = s.base @@ -32,8 +37,31 @@ func NewScopedFs(source afero.Fs, path string) *ScopedFs { return &ScopedFs{base: afero.NewBasePathFs(source, path).(*afero.BasePathFs)} } -// Base returns the underlying *afero.BasePathFs. -func (s *ScopedFs) Base() *afero.BasePathFs { return s.base } +// NewFs builds a user filesystem rooted at path. When followExternal is true it +// returns a bare BasePathFs, so symlinks whose target resolves outside the scope +// are followed; otherwise it returns a ScopedFs that refuses to follow them. +func NewFs(source afero.Fs, path string, followExternal bool) afero.Fs { + if followExternal { + return afero.NewBasePathFs(source, path) + } + return NewScopedFs(source, path) +} + +// BasePath returns the underlying *afero.BasePathFs of a user filesystem built +// by NewFs, whether it is a *ScopedFs or a bare *afero.BasePathFs, or nil if it +// is neither. +func BasePath(fs afero.Fs) *afero.BasePathFs { + switch f := fs.(type) { + case *ScopedFs: + return f.BasePathFs() + case *afero.BasePathFs: + return f + } + return nil +} + +// BasePathFs returns the underlying *afero.BasePathFs. +func (s *ScopedFs) BasePathFs() *afero.BasePathFs { return s.base } // RealPath resolves a scoped path to the real on-disk path by delegating to // the underlying BasePathFs. This is needed by callers that need the actual @@ -61,13 +89,10 @@ func (s *ScopedFs) guard(name string) error { // // Paths that do not exist yet (e.g. a brand-new file being created) are // validated against their nearest existing ancestor, so legitimate new files -// are always allowed. -// -// Note: a dangling symlink whose target does not yet exist resolves to its -// containing directory and is therefore allowed; writing through such a link -// could still create a file outside the scope. This is treated as best-effort -// and relies on rejecting existing escaping symlinks, which covers the -// disclosure and overwrite vectors. +// are always allowed. A dangling symlink — a link whose target does not exist +// yet — is the exception: it is followed to where it points and validated +// there, so a write cannot dereference the link to create a file outside the +// scope. func (s *ScopedFs) within(p string) (bool, error) { root, err := filepath.EvalSymlinks(afero.FullBaseFsPath(s.base, "/")) if err != nil { @@ -76,12 +101,43 @@ func (s *ScopedFs) within(p string) (bool, error) { target := afero.FullBaseFsPath(s.base, p) resolved, err := filepath.EvalSymlinks(target) - for errors.Is(err, fs.ErrNotExist) { - parent := filepath.Dir(target) - if parent == target { - break + // When target does not resolve, work out where the operation would actually + // land. A non-existent regular path resolves to the file that would be + // created inside its containing directory, so walk up to the nearest + // existing ancestor and validate that. But when target itself is a dangling + // symlink, follow it one level instead: validating its lexical parent would + // wrongly accept a link pointing outside the scope, letting a write follow + // the link and create the file out of bounds. + for hops := 0; errors.Is(err, fs.ErrNotExist); { + if fi, lerr := os.Lstat(target); lerr == nil && fi.Mode()&os.ModeSymlink != 0 { + hops++ + if hops > maxSymlinkHops { + return false, os.ErrPermission + } + dest, rerr := os.Readlink(target) + if rerr != nil { + return false, rerr + } + if !filepath.IsAbs(dest) { + // Resolve the link relative to the directory that really contains + // it, not its lexical parent: a symlinked ancestor could otherwise + // shift the computed target back into scope while the real write + // lands outside it. The parent is guaranteed to resolve here + // because os.Lstat above already traversed it. + base, berr := filepath.EvalSymlinks(filepath.Dir(target)) + if berr != nil { + return false, berr + } + dest = filepath.Join(base, dest) + } + target = filepath.Clean(dest) + } else { + parent := filepath.Dir(target) + if parent == target { + break + } + target = parent } - target = parent resolved, err = filepath.EvalSymlinks(target) } if err != nil { @@ -136,10 +192,16 @@ func (s *ScopedFs) OpenFile(name string, flag int, perm os.FileMode) (afero.File } func (s *ScopedFs) Remove(name string) error { + if err := s.guard(name); err != nil { + return err + } return s.base.Remove(name) } func (s *ScopedFs) RemoveAll(path string) error { + if err := s.guard(path); err != nil { + return err + } return s.base.RemoveAll(path) } diff --git a/fileutils/copy_test.go b/fileutils/copy_test.go index ccdcc901..6899e032 100644 --- a/fileutils/copy_test.go +++ b/fileutils/copy_test.go @@ -2,6 +2,7 @@ package fileutils import ( "os" + "path" "path/filepath" "testing" @@ -9,6 +10,54 @@ import ( "github.com/spf13/afero" ) +// failingOpenFs wraps an afero.Fs and makes Open fail for one specific path, +// while every other operation (including Stat) is delegated unchanged. It +// simulates a directory that can be stat-ed but not opened/read — for example +// an unreadable sub-directory, or one whose permissions changed or that was +// removed after its parent was listed (a TOCTOU race) — encountered during a +// recursive copy. +type failingOpenFs struct { + afero.Fs + failOpen string +} + +func (f *failingOpenFs) Open(name string) (afero.File, error) { + if path.Clean(name) == path.Clean(f.failOpen) { + return nil, os.ErrPermission + } + return f.Fs.Open(name) +} + +// CopyDir is documented to keep going when it hits an error and to report the +// error afterwards. A sub-directory that cannot be opened must therefore yield +// an error (and leave the other, readable entries copied) rather than +// panicking on a nil directory handle. +func TestCopyDirUnreadableSubdirReturnsError(t *testing.T) { + mem := afero.NewMemMapFs() + if err := mem.MkdirAll("/srcdir/sub", 0o755); err != nil { + t.Fatal(err) + } + if err := afero.WriteFile(mem, "/srcdir/ok.txt", []byte("readable"), 0o644); err != nil { + t.Fatal(err) + } + + afs := &failingOpenFs{Fs: mem, failOpen: "/srcdir/sub"} + + err := Copy(afs, "/srcdir", "/dstdir", 0o644, 0o755) + if err == nil { + t.Fatal("expected an error when a sub-directory cannot be opened") + } + + // The readable sibling must still have been copied (continue-on-error). + data, readErr := afero.ReadFile(afs, "/dstdir/ok.txt") + if readErr != nil { + t.Fatalf("readable sibling was not copied: %v", readErr) + } + if string(data) != "readable" { + t.Fatalf("unexpected copied content: %q", string(data)) + } +} + // Copying an in-scope directory that contains a symlink whose target escapes // the user's scope must not dereference that symlink into the destination. // Otherwise a scoped user could exfiltrate out-of-scope file content via the diff --git a/fileutils/dir.go b/fileutils/dir.go index e0b049db..4bd7c925 100644 --- a/fileutils/dir.go +++ b/fileutils/dir.go @@ -23,7 +23,12 @@ func CopyDir(afs afero.Fs, source, dest string, fileMode, dirMode fs.FileMode) e return err } - dir, _ := afs.Open(source) + dir, err := afs.Open(source) + if err != nil { + return err + } + defer dir.Close() + obs, err := dir.Readdir(-1) if err != nil { return err diff --git a/frontend/public/index.html b/frontend/public/index.html index 15ff375e..dc59f230 100644 --- a/frontend/public/index.html +++ b/frontend/public/index.html @@ -5,7 +5,7 @@ [{[ if .ReCaptcha -]}] diff --git a/frontend/src/App.vue b/frontend/src/App.vue index 25917f7d..543b50b8 100644 --- a/frontend/src/App.vue +++ b/frontend/src/App.vue @@ -1,5 +1,6 @@ @@ -9,6 +10,7 @@ import { ref, onMounted, watch } from "vue"; import { useI18n } from "vue-i18n"; import { setHtmlLocale } from "./i18n"; import { getMediaPreference, getTheme, setTheme } from "./utils/theme"; +import { name } from "./utils/constants"; const { locale } = useI18n(); @@ -31,3 +33,17 @@ watch(locale, (newValue) => { newValue && setHtmlLocale(newValue); }); + + diff --git a/frontend/src/components/files/ExtendedImage.vue b/frontend/src/components/files/ExtendedImage.vue index 88b78304..62b8d566 100644 --- a/frontend/src/components/files/ExtendedImage.vue +++ b/frontend/src/components/files/ExtendedImage.vue @@ -57,6 +57,9 @@ const container = ref(null); onMounted(() => { if (!decodeUTIF() && imgex.value !== null) { imgex.value.src = props.src; + imgex.value.alt = decodeURIComponent( + props.src.split("/").pop() || "preview" + ); } props.classList.forEach((className) => @@ -88,6 +91,9 @@ watch( () => { if (!decodeUTIF() && imgex.value !== null) { imgex.value.src = props.src; + imgex.value.alt = decodeURIComponent( + props.src.split("/").pop() || "preview" + ); } scale.value = 1; diff --git a/frontend/src/components/files/ListingItem.vue b/frontend/src/components/files/ListingItem.vue index 33b5a993..143adda8 100644 --- a/frontend/src/components/files/ListingItem.vue +++ b/frontend/src/components/files/ListingItem.vue @@ -26,6 +26,7 @@ diff --git a/frontend/src/components/header/HeaderBar.vue b/frontend/src/components/header/HeaderBar.vue index d15ec060..bf281bcb 100644 --- a/frontend/src/components/header/HeaderBar.vue +++ b/frontend/src/components/header/HeaderBar.vue @@ -1,6 +1,6 @@