The admin API key routes now require any pending password change or
two-factor enrollment to be completed first, like the other admin routes.
Reported-by: Yves Soete, Blacksight LLC <yves.soete@blacksight.io>
Signed-off-by: Nicola Murino <nicola.murino@gmail.com>
A user or admin changing its own password, profile or two-factor
settings wrote the whole account back and could overwrite a concurrent
administrator change. Self-service updates are now conditional on the
stored updated_at matching the value read, and every update stores a
strictly increasing updated_at.
Thanks to Thomas Toth for the report.
Signed-off-by: Nicola Murino <nicola.murino@gmail.com>
HTTP builds a connection per request, so checkHTTPClientUser is where a
request is authorized, the way the credential check is for SFTP and FTP:
it evaluates the login conditions of the account together with the
denied protocols, the login method and the source address.
An account that is disabled, that reaches its expiration date or that
falls outside its access time restrictions is refused on the next
request. The check is redundant on the paths that already run it before
the call, and costs nothing: the account is in memory.
Signed-off-by: Nicola Murino <nicola.murino@gmail.com>
Downloading a public share limited to a single directory as a zip archive
rewrote share.Paths[0] to "/" so the entry names came out relative to that
directory. The value it overwrote is the policy every later public request is
authorized against: the browsable listing resolves the requested path under
Paths[0] and the upload handler derives its destination prefix from it.
The rewrite stays confined to the copy the handler holds only if the data
provider returns a share that owns its paths. The memory provider did not:
getACopy assigned the Paths slice by header, so the copy kept pointing at the
array stored in the provider map and the rewrite reached the stored share.
From that moment the link served the whole filesystem of the share owner. Any
recipient, with no account and nothing beyond the share URL, could list and
download every file the owner can reach, and a read-write share also accepted
uploads at the owner's root. The archive returned to the recipient who
triggered the change was still limited to the shared directory, so the
response revealed nothing. The SQL and bolt providers rebuild the slice while
decoding the stored row and were not affected.
The list of files to archive is now local to the handler, so the share is no
longer written to, and getACopy clones the paths, so a copy never shares state
with the provider.
The regression test downloads a share limited to one directory twice and
checks the archive, the stored paths, the browsable listing and a file outside
the share in between, with whichever data provider the test suite runs with.
Thanks to Yutaka Sasaki (@SAYUTIM) for the report.
Signed-off-by: Nicola Murino <nicola.murino@gmail.com>
The file pattern filter matches names, and the operations that read an item
evaluate it on the item itself. Three did not agree with that: a copy
evaluated it on the target only, a rename mapped every refusal to a
permission error and so discarded the deny policy configured on the filter,
and symlink creation honored the hide policy alone, letting the default one
through.
They now all evaluate the source and report the configured policy through
GetErrorForDeniedFile, as stat, download and delete already do, so the same
filter gives the same answer whichever operation reaches a name. The target
keeps reporting a permission error: it is a path being written, not an item
being read. isRenamePermitted returns an error and is renamed accordingly;
the recursive walk still reports a permission error, because the entries it
walks are the contents of the directory being renamed and a hidden one among
them must not make that directory look missing.
Signed-off-by: Nicola Murino <nicola.murino@gmail.com>
With query_userinfo enabled, the UserInfo endpoint is queried after
authentication and the user claims are read from both sources. Claims
from the verified ID token take precedence on conflicts, empty values
do not override, and the UserInfo subject must match the ID token
subject. The option is validated at startup against the provider
metadata.
Reading the claims from the UserInfo endpoint was requested in #2265
and an implementation, enabled by default and with the opposite claim
precedence, was proposed in #2266.
Fixes#2265Closes#2266
Suggested-by: TEC <git@tecosaur.net>
Signed-off-by: Nicola Murino <nicola.murino@gmail.com>
The token manager Add now reports whether the entry was persisted and the
provider-backed manager logs the failure, so an invalidation that does not
happen leaves a trace instead of being dropped in silence.
The REST admin and user logout propagate the error and answer 500: the
response states that the token has been invalidated, so it must not be
sent while the token is still usable. invalidateToken also reports an
error for a request without a verified token, there is nothing to
invalidate in that case.
Invalidation that is a side effect of another operation stays best effort
and does not gate that operation's result: the password is already
changed, the request is already rejected, the pre-login token goes away
with the cookie. Those call sites rely on the token manager log.
The web logout paths keep clearing the login cookie and redirecting
instead of rendering an error page: the client is logged out locally
either way, and a failed store write only matters to someone who already
holds a copy of the token.
Signed-off-by: Nicola Murino <nicola.murino@gmail.com>
Invalidation was keyed on the exact token text, so an equivalent
Base64URL spelling of the same signed token did not match the stored
entry and kept working after a logout. The store is now keyed on the
"jti" claim, which is invariant across equivalent encodings of one token.
This is a correctness fix and not a privilege boundary: producing an
equivalent spelling requires already holding a valid signed token, it
grants no additional access, and the token remains usable only until its
own expiry, a bound it never exceeds.
Reported-by: Corban Villa (@corbanvilla)
Reported-by: Sohee Kim (@soh3e)
Reported-by: Austin Chu (@dderpym)
Signed-off-by: Nicola Murino <nicola.murino@gmail.com>
A virtual folder mapping can now re-root the mount onto a sub-path of
the folder: the mount at the virtual path serves the folder starting
from the configured subpath, which stays invisible in the virtual tree.
The same folder can be mapped multiple times with distinct subpaths;
quota remains a property of the whole folder, so every mapping must use
the same limits.
Supported for local, encrypted, S3, GCS, Azure Blob and SFTP folders.
Fixes#2037
Signed-off-by: Nicola Murino <nicola.murino@gmail.com>
Apply the fixes suggested by the modernize analyzers enabled by
default in gopls v0.23: range over int, errors.AsType, min/max,
slices.Backward/Contains, maps.Copy, strings.Cut/CutPrefix/CutSuffix/
SplitSeq, fmt.Appendf, WaitGroup.Go and b.Loop in tests, new(expr)
instead of to.Ptr, removal of ineffective omitempty tags on nested
struct fields.
Signed-off-by: Nicola Murino <nicola.murino@gmail.com>
The previous commit fixed the test cases failing on Windows, but which
cleanup loses the race is not deterministic: a client disconnection is
processed after the client returns, so removing the home dir before
deleting the user fails if the connection still holds the fs root when
os.RemoveAll runs. TestResume passed in one CI run and failed in the
next one on the same unchanged code.
Swap the order in all the remaining cleanups: deleting the user first
disconnects it and httpdtest.RemoveUser waits for its connections to be
removed, so the fs roots are released before the home dir removal.
Signed-off-by: Nicola Murino <nicola.murino@gmail.com>
Confine all local filesystem access to an os.Root, with path resoultion
enforced by the underlying OS.
Behavior changes an upgrading deployment may notice:
- temp_path is removed and ignored if still configured. Its purpose
was to stage files (atomic uploads, scratch and streaming data)
outside the user's storage and rename them into place, and that is
fundamentally incompatible with the confinement: an os.Root cannot
rename a file in from outside the root, so every staged upload would
degrade to a full copy-then-delete even when temp_path is on the
same filesystem, defeating the very atomicity the option existed to
provide. It was not a great idea before this change either: a
temp_path on a different device already turned the final rename into
a silent full copy. Staged and scratch files (the atomic upload
temporary, the CryptFs download pipe backing file) now live in the
storage root itself, on the same filesystem as the data, so the
final rename stays atomic.
- Cross-folder / cross-root renames are no longer atomic. A rename
spanning the home and a local virtual folder, or two local folders,
falls back to copy-then-delete: slower for large data even on the
same device, and a midway failure can leave a partial tree at the
destination. Symlinks and other non-regular entries inside a moved
directory are skipped, not copied, and the rename still reports
success. File mode bits are not preserved; regular files keep their
modification time, directories do not.
- Absolute symlinks created outside SFTPGo, or by versions predating
this change, are no longer traversable and Readlink rejects them with
permission denied, even when the target is inside the root: os.Root
refuses an absolute link target regardless of where it points.
Symlinks created through SFTPGo are stored relative to the link
directory and keep working.
- Execute-only home directories (e.g. mode 0111) are unusable:
os.OpenRoot requires read permission, so every operation fails with
permission denied until the directory is made readable, where
previously only the listing failed. Since the root open is retried on
demand, fixing the permissions (or creating a missing home
externally) makes the same connection work again without
reconnecting.
Signed-off-by: Nicola Murino <nicola.murino@gmail.com>
The `inline` query parameter on the browsable-share file download
and on the authenticated user file download suppressed the
`Content-Disposition: attachment` header. A file placed in a share
or user home (for example an HTML document) could then be served
inline as `text/html` and execute in SFTPGo's web origin,
allowing stored XSS through a shared link.
These endpoints now always respond with `Content-Disposition:
attachment`. The `inline` parameter is removed from the API and from
the OpenAPI schema: it dates back to a time when the WebClient had no
built-in viewers, and the current web client previews images, video,
PDF and other files directly within shares through its own viewer
endpoints, so the flag is obsolete.
Signed-off-by: Nicola Murino <nicola.murino@gmail.com>
The browsable share partial download validated ZIP entries with a raw
byte-prefix check, letting prefix-overlapping siblings (/share vs
/share2) escape the share root. Use a directory-boundary aware check;
harden the event manager twin and add a regression test.
Signed-off-by: Nicola Murino <nicola.murino@gmail.com>
Nicola Murino