Fable - #214
Open
misterunknown wants to merge 7 commits into
Open
Conversation
Contributor
|
Hi, @misterunknown. The only potential issue is that we cache the MIME type of the first file in the directory with the new extension, and if the "first" file has the "wrong" extension, the file types and action buttons will be incorrectly associated in UI. Alternatively, we could create a static array with MIME type = extension, for example, from here. P.S. I used this file to update MIME types in Nginx. Everything works fine. |
Owner
Author
|
@novashdima Hm, this was mostly for correctness, because the AI mentioned it, but yes, it's quite costly, so I reinstated the mimeCache in this branch. The shipped mime types are limited though, I don't think the complete list from apache2 is necessary. |
- send X-IFM-CSRF token on every POST request from the frontend - require POST + valid CSRF token for state-changing APIs - exempt stateless header-auth (X-IFM-AUTH/Authorization) requests from the CSRF check, since they are not subject to CSRF - harden session cookies (httponly, samesite=Lax, secure on HTTPS), regenerate id on login, fully destroy session on logout - pin resolved IP in remoteUpload to mitigate DNS-rebinding SSRF - guard recursive walks against symlink loops - avoid leaking internal exception details to clients - assorted validation, error-handling and code cleanup
Testing
- Add PHPUnit suites for the IFM HTTP/JSON API (frontend out of scope):
- Unit (white-box): Htpasswd/APR1_MD5, IFMArchive round-trips,
filename/path-jail validation, SSRF guard (via reflection).
- Integration (black-box): boots `php -S` per test against a
configurable front controller, per-test sandbox root_dir jail, and a
cookie-aware client reproducing the real session/CSRF/Basic-auth flows.
Covers auth, CSRF, file listing/editing, copy/move, upload/download,
archives, search, remote upload, and per-feature-flag gating.
- Security: jail-escape, traversal, self-overwrite and symlink-loop cases
using a two-part convention (assert error AND no side effect on disk).
- Tests target the compiled dist/libifm.php; bootstrap rebuilds it on demand.
- 128 tests / 351 assertions, all green.
- Add phpunit.xml, tests/README.md, Makefile test targets, composer scripts.
Static analysis
- Add PHPStan (level 5) with a baseline for pre-existing findings;
`make analyse` + composer `analyse` script + phpstan.yml workflow.
- Remove PHPMD: redundant with PHPStan (unused-code overlap) and previously
non-blocking; drop workflow, dev dep, Makefile target and docs.
CI
- New phpunit.yml (matrix PHP 8.1-8.5) and phpstan.yml.
- Hygiene: least-privilege permissions, concurrency cancel-in-progress,
composer caching; align triggers to master + v4.0.
- CodeQL: remove the no-op JavaScript autobuild step.
- Add Dependabot (composer ecosystem only) and SECURITY.md.
- Declare "php": ">=8.1" in composer.json.
Stop hand-editing the version. compiler.php now resolves IFM_VERSION at build time from (in order): the IFM_VERSION env var, a VERSION file, then `git describe --tags --always --dirty`, falling back to v0.0.0-dev. Git tags become the single source of truth; dev builds embed a descriptive version (e.g. v4.1.1-9-gb86f1eb-dirty). Add .github/workflows/release.yml: on push of a `v*` tag it builds the bundled and CDN artifacts with IFM_VERSION pinned to the tag, verifies the embedded version matches, and publishes a GitHub Release (via the gh CLI, no third-party action) with auto-generated notes and ifm.php, ifm.min.php, cdn.ifm.php and cdn.ifm.min.php attached. Document the SemVer (vX.Y.Z) scheme and the tag-to-release flow in README.
PHPUnit 13 requires PHP >=8.4.1, which broke composer resolution on the 8.1-8.3 matrix runners (and the 8.3 PHPStan/Release jobs). No single PHPUnit major covers 8.1-8.5, so allow ^10.5 || ^11.5 || ^12.2 || ^13.2 and let composer pick a compatible release per runner (PU10 on 8.1, 11 on 8.2, 12 on 8.3, 13 on 8.4/8.5). Switch CI install steps to `composer update` (no lock file is committed, so resolution must happen per-runner). phpunit.xml is already limited to PHPUnit 10+ features, so it works across all four majors.
getDummyHash() called password_hash(random_bytes(16), PASSWORD_BCRYPT). random_bytes can contain a 0x00 byte (~6% for 16 bytes), and bcrypt rejects NUL bytes in the password (ValueError "Bcrypt password must not contain null character") and truncates at the first NUL / 72 bytes. This made the unknown-user timing-mitigation path crash intermittently - surfaced as a flaky failure on the PHP 8.2 CI job (version-independent; just lost the dice roll). Hash bin2hex(random_bytes(16)) instead: hex output is NUL-free and within bcrypt's length limit, so the dummy hash is always generated safely while preserving the timing-attack mitigation.
The LDAP auth branch passed the user-supplied password straight to ldap_bind(). Most directory servers treat a bind with an empty password as an anonymous/unauthenticated bind that succeeds, so a valid username with an empty password could bypass authentication. Reject empty (or non-string) username/password before connecting.
mime_content_type() opens and reads every file, which dominates the cost of building a directory listing (~780ms for 2000 files here). The former extension-keyed cache removed in 41301fa avoided that, but keyed a content-derived value by extension, so the first file of a given extension decided the MIME type for all its siblings. Resolve well-known extensions from a static table instead: - MIME_MAP covers the extension families getTypeIcon() already knows (images, audio/video, office documents, archives, text/code) and is answered without touching the file at all - unknown extensions still fall back to content sniffing, memoized per request in mimeCache - extension-less files are always sniffed and never cached, since the extension is not a usable cache key for them - a failing mime_content_type() now yields application/octet-stream instead of an empty Content-Type - inline downloads use the same helper, so a file is served as the type the listing announced Listing 2000 files: 778.9ms -> 3.1ms. Note that empty text files now report text/plain rather than application/x-empty; both keep the frontend's edit button enabled.
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
3 participants
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.
No description provided.