Conversation
The exact `overrides.postcss: "8.5.16"` pin was the sole constraint holding postcss at a vulnerable version -- vite@8.1.4 (the only dependent) already declares `postcss: ^8.5.16`, so the override was redundant. It also made Dependabot fail with `security_update_not_possible`, reporting the pin back as though vite required it. Removing it resolves postcss to 8.5.23, above the 8.5.18 patched floor for GHSA-r28c-9q8g-f849 (path traversal in previous-source-map auto-loading). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…all (GHSA-mc93-rc3x-fpgq) The hostname parameter was concatenated into a bash -c string executed as root via run_privileged(). Validate the hostname against a strict charset and pipe debconf selections via stdin so no user input ever reaches a shell command line. Co-authored-by: tonghuaroot <tonghuaroot@gmail.com>
…ns (GHSA-4wqh-7f4f-5qmx) Mutating File Manager endpoints (write/create/mkdir/delete/rename/copy/ move/chmod/upload and the S3 equivalents) only required a valid JWT, so a viewer account (files.write=false) could write and delete files. They now require the files.write permission via permission_required(). Read endpoints are unchanged. Adds regression tests covering both advisories. Co-authored-by: AruvasgaChithan <AruvasgaChithan@users.noreply.github.com>
Follow-ups from the GHSA-4wqh-7f4f-5qmx / GHSA-mc93-rc3x-fpgq review: - File Manager read endpoints (browse/info/read/search/disk-*/analyze/ type-breakdown/download + s3/browse, s3/read, s3/download-url) only required a valid JWT, so a user whose files.read permission is revoked via custom permissions could still read and download files. They now require permission_required('files', 'read'), matching the write side. - LogService._read_syslog() interpolated the unit name into a bash -c string using subprocess.list2cmdline(), which is cmd.exe-style quoting and does not stop $(...)/backtick expansion under bash. grep now runs as a plain argv list (with --) and the tail happens in Python. Reachable only on non-systemd hosts by admins, but the same bug class as the Postfix advisory. Regression tests: revoked files.read 403s every read endpoint; the syslog filter never invokes a shell and passes payloads verbatim.
Now that every /api/v1/files endpoint enforces the files.read permission, a user with that permission revoked would only hit 403 toasts. Filter the Files item out of the sidebar and the command palette (usePaletteAuthz) instead, using the resolved permissions already returned by /api/v1/auth/me.
There was a problem hiding this comment.
Pull request overview
This PR tightens ServerKit’s File Manager authorization by enforcing per-user feature permissions on the /api/v1/files API surface (including S3 equivalents), updates the UI to hide File Manager entry points when files.read is revoked, and removes shell-based privileged command construction in sensitive backend paths.
Changes:
- Backend: Apply
permission_required('files', 'read'|'write')across File Manager (local + S3) endpoints and add regression tests for the RBAC/security advisories. - Backend: Remove shell-based execution in Postfix install debconf seeding and syslog filtering to prevent command injection vectors.
- Frontend + deps: Hide File Manager nav/palette entries when
files.readis missing; remove thepostcssoverride so patched versions can resolve.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
VERSION |
Bumps ServerKit version to 1.7.66. |
backend/app/api/files.py |
Enforces files.read/files.write via permission_required() on all File Manager endpoints (local + S3). |
backend/app/services/postfix_service.py |
Validates hostname and pipes debconf selections via stdin (no bash -c). |
backend/app/services/log_service.py |
Removes shell-based syslog filtering; runs grep via argv and tails in Python. |
backend/tests/test_files_rbac.py |
Adds regression coverage for File Manager RBAC and the subprocess hardening changes. |
frontend/src/components/Sidebar.jsx |
Hides the File Manager sidebar item when files.read is not granted. |
frontend/src/hooks/usePaletteAuthz.js |
Prevents File Manager entries in the command palette when files.read is not granted. |
frontend/package.json |
Removes the postcss override pin. |
frontend/package-lock.json |
Updates lock resolution (notably postcss and transitive deps) after override removal. |
Files not reviewed (1)
- frontend/package-lock.json: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+332
to
+336
| log_lines = result.stdout.split('\n') if result.stdout else [] | ||
| if log_lines and log_lines[-1] == '': | ||
| log_lines.pop() # trailing newline artifact | ||
| if service: | ||
| log_lines = log_lines[-int(lines):] |
Comment on lines
+100
to
+109
| READ_CASES = [ | ||
| '/api/v1/files/browse?path=/tmp', | ||
| '/api/v1/files/info?path=/tmp/x', | ||
| '/api/v1/files/read?path=/tmp/x', | ||
| '/api/v1/files/search?path=/tmp&query=x', | ||
| '/api/v1/files/download?path=/tmp/x', | ||
| '/api/v1/files/s3/browse?path=/', | ||
| '/api/v1/files/s3/read?path=/x', | ||
| '/api/v1/files/s3/download-url?path=/x', | ||
| ] |
… venv mismatch - create_app(): skip background daemons (queue consumers, metrics, job system, analytics flush, linked-panel client) when loaded by a Flask CLI one-shot such as 'flask db upgrade', or when SERVERKIT_SKIP_BACKGROUND=1. They query the DB before migrations run, so any error there (corrupt DB, pre-migration schema) aborted the migration and sank the whole update. - update.sh: set SERVERKIT_SKIP_BACKGROUND=1 for both flask db upgrade invocations as an explicit contract. - update.sh: fail fast with an actionable message when the SQLite slot copy fails PRAGMA integrity_check, instead of dying mid-boot with 'database disk image is malformed'. - update.sh: rebuild the release tarball's prebuilt venv when its baked VIRTUAL_ENV path resolves to the old slot (it is baked as /opt/serverkit/venv), which previously ran new code on the old slot's dependencies during migration. Resolved-path comparison keeps the fast prebuilt path for fresh installs.
…d RBAC coverage - log_service: grep the service/unit filter with -F so names like 'nginx.service' are matched literally instead of as a regex. - log_service: clamp the in-Python tail like 'tail -n' — lines=0 sliced as [-0:] == all lines, negatives kept nearly everything. - test_files_rbac: add files/upload and files/s3/upload to WRITE_CASES, and disk-usage/disk-mounts/analyze/type-breakdown to READ_CASES, so the permission gates on the full /api/v1/files surface are covered.
The SQLite pre-check in migrate_database treated an EMPTY probe result
(unusable venv python) the same as real corruption and halted the update —
a false positive that broke the T10/T24 contract tests. Now:
- probe output != 'ok' -> genuine corruption -> halt with repair guidance
- empty probe output -> probe couldn't run -> warn and let flask surface
any real error
test_update.sh: T10/T24 fixtures stub the venv python (healthy-DB path);
new T10b asserts the corrupt-DB halt fires before flask runs; new T10c
asserts an unrunnable probe warns and proceeds.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The File Manager had a permission system it never actually consulted — every endpoint under
/api/v1/fileschecked only that you held a valid JWT, so a viewer account could happily write, chmod, and delete anything the panel process could reach, and a user withfiles.readexplicitly revoked could still browse and download. This PR wires those endpoints topermission_required()on both the write and read side, including the S3 object-browser equivalents, and then follows through on the frontend so a user withoutfiles.readsimply doesn't see the File Manager rather than collecting 403 toasts. Alongside that, two places were building shell command lines out of user input and running them as root:PostfixService.install()concatenated the hostname into abash -cstring, andLogService._read_syslog()filtered by unit name usingsubprocess.list2cmdline()— which is cmd.exe-style quoting and does nothing about$(...)or backticks under bash. Both now pass argv lists with no shell involved, with the hostname additionally charset-validated and debconf values piped over stdin. The postcss override pin is also gone: it was the sole constraint holding the package below the patched floor for GHSA-r28c-9q8g-f849, and it made Dependabot report the pin back as though vite required it.Contributors
Highlights
Technical changes
backend/app/api/files.py: replaced@jwt_required()with@permission_required('files', 'write')onwrite,create,mkdir,delete,rename,copy,move,chmod,upload, pluss3/write,s3/delete, ands3/upload(GHSA-4wqh-7f4f-5qmx)backend/app/api/files.py: replaced@jwt_required()with@permission_required('files', 'read')onbrowse,info,read,search,disk-usage,disk-mounts,analyze,type-breakdown,download, pluss3/browse,s3/read, ands3/download-url— previously any authenticated identity passed regardless of revoked custom permissionsbackend/app/services/postfix_service.py:install()validateshostnameagainst^[a-zA-Z0-9.-]+$and returns{'success': False, 'error': 'Invalid hostname format'}on mismatch; the tworun_privileged(['bash', '-c', 'echo ... | debconf-set-selections'])calls collapse into a singlerun_privileged(['debconf-set-selections'], input=debconf_lines)so no user input reaches a command line (GHSA-mc93-rc3x-fpgq)backend/app/services/log_service.py:_read_syslog()drops thebash -c 'grep ... | tail -n N'construction forrun_privileged(['grep', '-i', '--', service, filepath]);--terminates option parsing so a leading-dash service name can't become a grep flag, and the line limit is applied by slicinglog_linesin Python, with the trailing-newline artifact popped so the slice returns the intended countfrontend/src/components/Sidebar.jsx: pullshasPermissionfromuseAuth()and filters thefilesnav item out of the computed item list, withhasPermissionadded to the memo dependency arrayfrontend/src/hooks/usePaletteAuthz.js:allowNavreturnsfalsefornavId === 'files'whenhasPermission('files', 'read')is false, ahead of the existing workspace nav-map logic, so palette entries follow the same rule as the sidebar. Both read the resolved permissions already returned by/api/v1/auth/me— no extra requestfrontend/package.json: removed the exactoverrides.postcss: "8.5.16"entry.vite@8.1.4is the only dependent and already declarespostcss: ^8.5.16, so the override was redundant while pinning below the 8.5.18 floor patched for GHSA-r28c-9q8g-f849 (path traversal in previous-source-map auto-loading); the lockfile resolves to 8.5.23 withnanoidmoving to^3.3.16