# Simple File Manager - Code Review & Improvement Recommendations

## Current State
- **Fork**: `hoelee/simple-file-manager` (based on jcampbell1/simple-file-manager)
- **Original Inspiration**: prasathmani/tinyfilemanager (v2.6, Nov 2024)
- **Current File**: Single `index.php` (~20KB, 494 lines)
- **Last Update**: 2040331 - "default sort ordering is directory first, then name"

---

## 🔴 CRITICAL Security Issues

### 1. **No Authentication by Default** (`$PASSWORD = ''`)
- **Risk**: Anyone with URL access gets full file system access
- **Fix**: Enforce password or disable by default with clear warning
- **Reference**: TinyFM v2.6 has `$use_auth = true` with bcrypt password hashing

### 2. **Weak Path Traversal Protection**
```php
// Current: Only checks prefix, can be bypassed
if(substr($tmp, 0,strlen($tmp_dir)) !== $tmp_dir) err(403,"Forbidden");
```
- **Issues**: Symlinks, case-insensitive FS, Unicode normalization bypasses
- **Fix**: Use `realpath()` on both paths AFTER resolving symlinks

### 3. **XSRF Token in Cookie (Not HttpOnly)**
```php
setcookie('_sfm_xsrf', bin2hex(openssl_random_pseudo_bytes(16)));
```
- **Risk**: Accessible via JavaScript (XSS → CSRF bypass)
- **Fix**: Add `HttpOnly; Secure; SameSite=Strict` flags

### 4. **Timing Attack on Password Check**
```php
sha1($t.$_POST['p']) === sha1($t.$PASSWORD)  // String comparison leaks timing
```
- **Fix**: Use `hash_equals()` for constant-time comparison

### 5. **No File Type Validation on Upload (MIME Only)**
```php
foreach($disallowed_patterns as $pattern)
    if(fnmatch($pattern, $_FILES['file_data']['name'])) err(403,...);
```
- **Risk**: Extension-based only, easily bypassed (e.g., `shell.php.jpg`, polyglots)
- **Fix**: Validate MIME type with `finfo_file()`, check magic bytes

### 6. **No Upload Size Limit Enforcement (Server-Side)**
- Only client-side JS check (`MAX_UPLOAD_SIZE`)
- **Fix**: Server-side check before `move_uploaded_file()`

### 7. **Information Disclosure in Errors**
```php
echo json_encode(['error' => ['code'=>intval($code), 'msg' => $msg]]);
```
- **Risk**: Stack traces, paths leaked in production
- **Fix**: Generic error messages in production mode

---

## 🟠 HIGH Priority Improvements

### 8. **Outdated jQuery (1.8.2 from 2012!)**
```html
<script src="//ajax.googleapis.com/ajax/libs/jquery/1.8.2/jquery.min.js"></script>
```
- **Risks**: XSS vulnerabilities, no modern browser support, deprecated APIs
- **Fix**: Update to jQuery 3.7+ or use vanilla JS (tinyfilemanager uses 3.6.1)

### 9. **No Content Security Policy (CSP)**
- Inline scripts/styles, external CDN → XSS risk
- **Fix**: Add CSP header, move inline JS to separate file or nonce

### 10. **No Rate Limiting / Brute Force Protection**
- Password auth has no lockout, delay, or logging
- **Fix**: Add exponential backoff, IP-based rate limiting

### 11. **Insecure Default Permissions**
```php
$allow_delete = true;
$allow_upload = true;
$allow_create_folder = true;
$allow_direct_link = true;
```
- **Risk**: Dangerous defaults for public deployment
- **Fix**: Secure defaults (all `false`), opt-in via config

### 12. **No HTTPS Enforcement**
- Password sent in cleartext over HTTP
- **Fix**: Check `$_SERVER['HTTPS']`, redirect or reject

---

## 🟡 MEDIUM Priority Improvements

### 13. **Missing Features from TinyFM v2.6**
| Feature | TinyFM | Current |
|---------|--------|---------|
| File editing (ace.js) | ✅ | ❌ |
| Syntax highlighting | ✅ | ❌ |
| Multi-user with permissions | ✅ | ❌ |
| Read-only users | ✅ | ❌ |
| User-specific directories | ✅ | ❌ |
| IP whitelist/blacklist | ✅ | ❌ |
| Chunked uploads (large files) | ✅ | ❌ |
| Online doc viewer (Google/MS) | ✅ | ❌ |
| Dark/Light theme | ✅ | ❌ |
| Multiple languages | ✅ | ❌ |
| Config file override | ✅ | ❌ |
| Hidden files toggle | ✅ | ❌ |
| File permissions/owner display | ✅ | ❌ |
| Search/filter | ✅ | ❌ |
| Copy/move/rename | ✅ | ❌ |
| Zip download | ✅ | ❌ |
| Image preview | ✅ | ❌ |

### 14. **Code Quality Issues**
- Mixed PHP/HTML (hard to maintain)
- No PSR standards, inconsistent formatting
- Global variables instead of dependency injection
- No autoloading, no Composer support
- Functions defined after usage (works but confusing)

### 15. **Accessibility Issues**
- No ARIA labels, no keyboard navigation for upload area
- Color-only indicators (drag-over state)
- No focus management

### 16. **Mobile/Responsive Gaps**
- Fixed width: `width:1024` (not responsive)
- Touch targets too small
- No viewport meta tag

---

## 🟢 LOW Priority / Nice to Have

### 17. **Modern PHP Practices**
- Type hints, strict types, scalar/return types
- PSR-12 coding standard
- Dependency injection container
- Unit tests (PHPUnit)

### 18. **Performance**
- No caching headers for static assets
- No gzip/compression hints
- Scandir() on every request (no caching)

### 19. **Developer Experience**
- No `.editorconfig`, no `phpstan`/`psalm` config
- No GitHub Actions CI
- No Dockerfile for testing

---

## 📋 Recommended Action Plan (Keeping Single File Constraint)

### Phase 1: Security Hardening (Week 1)
1. ✅ Enforce password auth by default
2. ✅ Fix path traversal with `realpath()`
3. ✅ Add `HttpOnly; Secure; SameSite=Strict` to cookies
4. ✅ Use `hash_equals()` for password check
5. ✅ Server-side upload size validation
6. ✅ MIME type validation with magic bytes
7. ✅ Generic error messages (configurable debug mode)

### Phase 2: Modernization (Week 2)
8. ✅ Update jQuery to 3.7.1 (or vanilla JS)
9. ✅ Add CSP header
10. ✅ Add rate limiting for auth
11. ✅ Secure defaults (disable upload/delete by default)
12. ✅ HTTPS enforcement check

### Phase 3: Feature Parity (Selective, Week 3)
13. ✅ Add file editing (ace.js via CDN)
14. ✅ Add syntax highlighting (highlight.js)
15. ✅ Add dark/light theme toggle
16. ✅ Add search/filter
17. ✅ Add copy/move/rename
18. ✅ Make responsive (viewport, flexbox)

### Phase 4: Polish (Week 4)
19. ✅ Accessibility improvements
20. ✅ Mobile UX
21. ✅ Config file support (optional external `config.php`)
22. ✅ PSR-12 formatting within single file

---

## 🔍 Reference: TinyFM v2.6 Key Differences

| Aspect | TinyFM v2.6 | Current Fork |
|--------|-------------|--------------|
| Lines of code | ~5,700 | ~494 |
| Auth system | Multi-user, bcrypt, readonly | Single password, sha1 |
| File operations | Full CRUD + zip + edit | List, upload, delete, mkdir |
| Security | IP rules, CSP, rate limit | Basic XSRF only |
| UI | Bootstrap 5, DataTables, responsive | Custom CSS, jQuery 1.8 |
| Extensibility | Config file, plugins | Hardcoded config only |
| PHP requirement | 7.4+ | 5.6+ (implied) |

---

## 💡 Decision Point

**Option A**: Harden current minimal fork (keep ~500 lines)
- Pros: Simple, fast, familiar
- Cons: Missing many useful features, security debt

**Option B**: Adopt TinyFM v2.6 as base, customize
- Pros: Battle-tested, feature-rich, active maintenance
- Cons: Larger (~230KB), more complex

**Option C**: Hybrid - cherry-pick security + key features from TinyFM
- Pros: Best of both worlds
- Cons: More integration work

**Recommendation**: **Option C** - The current fork is too minimal for production use. Cherry-pick:
1. All security fixes from TinyFM
2. Auth system (bcrypt, multi-user)
3. File editing + syntax highlighting
4. Responsive UI (Bootstrap 5)
5. Config file support

This keeps it ~single-file (with optional config.php) while gaining production-ready security and features.

---

## 📦 Files to Reference from TinyFM v2.6
- `tinyfilemanager.php` (main) - security patterns, auth, file ops
- Config structure - for optional `config.php` support
- CSS/JS CDN references - for modern deps
- Chunked upload logic - for large file support

*Generated: 2026-08-19*