# Digital Signage Recommendations & Roadmap

> **Last Updated**: December 2025
> **Related**: [Overview](./digital-signage-overview.md)
> **Analysis Date**: December 2025
> **Implementation Date**: December 2025

## Executive Summary

This document captures identified technical debt, security concerns, and improvement opportunities discovered during the December 2025 system analysis. Recommendations are prioritized by impact and effort.

## ✅ Implementation Status (December 2025)

The following items have been **completed**:

| Priority | Task | Status | Files Modified |
|----------|------|--------|----------------|
| 🔴 Critical | Input sanitization in SlideController | ✅ Complete | `Controllers/SlideController.php` |
| 🔴 Critical | Permission checks on routes | ✅ Complete | `routes/groups/digitalsign.php` |
| 🔴 Critical | CSRF protection | ✅ Complete | Already handled by CsrfGuard middleware |
| 🟠 High | Fix updateLoop() bug | ✅ Complete | `Controllers/LoopController.php` |
| 🟠 High | Transaction management | ✅ Complete | `Controllers/LoopController.php` |
| 🟡 Medium | Constants for magic numbers | ✅ Complete | `Constants.php` (new file) |
| 🟡 Medium | Consolidated logging | ✅ Complete | All controllers now use KLogger |
| 🟡 Medium | Sanitized error output | ✅ Complete | Removed raw errorInfo() from logs |
| 🟡 Medium | Remove dead code | ✅ Complete | Deleted `GlobalSlideController.php` |

---

## Priority Matrix

| Priority | Impact | Effort | Timeline |
|----------|--------|--------|----------|
| **Critical** | Security/Data | Low-Medium | Immediate |
| **High** | Reliability | Medium | 1-2 weeks |
| **Medium** | Maintainability | Medium | 2-4 weeks |
| **Low** | Nice-to-have | Variable | Backlog |

---

## Critical Priority (Security)

### 1. Add Input Sanitization to SlideController

**Issue**: `SlideController.php` uses raw `$_POST` superglobals without sanitization.

**Location**: `userfrosting/src/BuyerKiosk/DigitalSign/Controllers/SlideController.php:16-20`

**Current Code**:
```php
public function addSlide() {
    $slideName = $_POST['slideName'];  // UNSAFE
    $fileName = $_POST['fileName'];     // UNSAFE
    $type = $_POST['type'];             // UNSAFE
}
```

**Recommended Fix**:
```php
public function addSlide() {
    $slideName = filter_var(
        $this->_app->request->post('slideName'),
        FILTER_SANITIZE_STRING
    );
    $fileName = filter_var(
        $this->_app->request->post('fileName'),
        FILTER_SANITIZE_STRING
    );
    $type = filter_var(
        $this->_app->request->post('type'),
        FILTER_SANITIZE_NUMBER_INT
    );
}
```

**Risk**: SQL injection, XSS attacks
**Effort**: Low (1-2 hours)

---

### 2. Add Permission Checks to Route Handlers

**Issue**: Digital signage routes lack explicit permission checks.

**Location**: `userfrosting/routes/groups/digitalsign.php`

**Current Code**:
```php
$app->post('/', function($typeNum) use ($app) {
    $storeController = new BuyerKiosk\StoreController($typeNum);
    $store = $storeController->getStore();
    $controller = new LoopController($app, $store);
    // No permission check!
    if($controller->addSlideToLoop()) {
        $app->response->setStatus(200);
    }
});
```

**Recommended Fix**:
```php
$app->post('/', function($typeNum) use ($app) {
    // Add permission checks
    if (!$app->user->checkAccess('uri_digital_signage')) {
        $app->notAuthorized();
        return;
    }
    if (!$app->user->checkStoreGroup($typeNum)) {
        $app->notAuthorized();
        return;
    }

    $storeController = new BuyerKiosk\StoreController($typeNum);
    // ... rest of handler
});
```

**Risk**: Unauthorized access to signage management
**Effort**: Low (2-3 hours for all routes)

---

### 3. Add CSRF Token Protection

**Issue**: State-changing routes lack CSRF token validation.

**Affected Routes**:
- `POST /:typeNum/upload`
- `POST /:typeNum/loop/`
- `DELETE /:typeNum/loop/:slideID`
- `POST /api/upload-media/:typeNum`
- `DELETE /upload-media`

**Recommended Implementation**:
```php
// In route handler
$csrfToken = $app->request->post('csrf_token');
if (!$app->csrf->validateToken($csrfToken)) {
    $app->response->setStatus(403);
    return;
}
```

**Risk**: Cross-site request forgery attacks
**Effort**: Medium (4-6 hours, requires form updates)

---

## High Priority (Reliability)

### 4. Fix updateLoop() Bug

**Issue**: Variable `$i` is referenced but never properly incremented.

**Location**: `userfrosting/src/BuyerKiosk/DigitalSign/Controllers/LoopController.php:60-80`

**Current Code**:
```php
public function updateLoop() {
    $data = $this->_app->request->post("data");
    foreach($data as $item) {
        $id = filter_var($item, FILTER_SANITIZE_NUMBER_INT);
        // $item is being used as both the array key and value - bug!
        $updateQuery->bindParam(":position", $item['position']);
    }
}
```

**Recommended Fix**:
```php
public function updateLoop() {
    $data = $this->_app->request->post("data");
    foreach($data as $index => $item) {
        $id = filter_var($item['id'], FILTER_SANITIZE_NUMBER_INT);
        $position = filter_var($item['position'], FILTER_SANITIZE_NUMBER_INT);

        $updateQuery = $this->storeDB->prepare(
            "UPDATE dsLoop SET position = :position WHERE id = :id"
        );
        $updateQuery->bindParam(":id", $id, \PDO::PARAM_INT);
        $updateQuery->bindParam(":position", $position, \PDO::PARAM_INT);
        // ...
    }
}
```

**Risk**: Loop reordering may not work correctly
**Effort**: Low (1-2 hours)

---

### 5. Add Transaction Management

**Issue**: Multi-table operations lack transaction wrapping.

**Affected Operations**:
- Adding scheduled slides (dsLoop + digitalSignSchedule)
- Schedule activation/deactivation
- Slide deletion with position reorder

**Current Pattern**:
```php
// Two separate inserts, no transaction
$loopItem->addLoopItem();  // Insert to dsLoop
$this->addToGlobalSchedule($loopItem);  // Insert to digitalSignSchedule
```

**Recommended Pattern**:
```php
try {
    $this->storeDB->beginTransaction();
    $this->globalDB->beginTransaction();

    $loopItem->addLoopItem();
    $this->addToGlobalSchedule($loopItem);

    $this->storeDB->commit();
    $this->globalDB->commit();
    return true;
} catch (\Exception $e) {
    $this->storeDB->rollBack();
    $this->globalDB->rollBack();
    $this->log->LogError("Transaction failed: " . $e->getMessage());
    return false;
}
```

**Risk**: Data inconsistency on partial failures
**Effort**: Medium (4-6 hours)

---

## Medium Priority (Maintainability)

### 6. Define Constants for Magic Numbers

**Issue**: Type codes (0, 1, 2, 3) are hardcoded throughout codebase.

**Locations**:
- `UploadController.php:30-40`
- `StoreLoop.php:56, 93-96`
- `loop.html:12-20`
- `AvailableSlides.php:107-115`

**Recommended Solution**:

Create constants file:
```php
// src/BuyerKiosk/DigitalSign/Constants.php
namespace BuyerKiosk\DigitalSign;

class Constants
{
    // Slide Types
    public const SLIDE_TYPE_IMAGE = 0;
    public const SLIDE_TYPE_VIDEO = 1;
    public const SLIDE_TYPE_QUEUE = 3;

    // Slide Sources (slideUploader)
    public const SOURCE_STORE = 0;
    public const SOURCE_CORPORATE = 1;
    public const SOURCE_HIPBONE = 2;

    // Schedule States
    public const SCHEDULE_ACTIVE = 0;
    public const SCHEDULE_PENDING = 1;
}
```

**Usage**:
```php
use BuyerKiosk\DigitalSign\Constants;

if ($slide->type === Constants::SLIDE_TYPE_VIDEO) {
    $slide->videoDuration = $handler->response['files'][0]->duration;
}

if ($slideUploader === Constants::SOURCE_CORPORATE) {
    // Use corpSlides table
}
```

**Risk**: Code clarity and maintenance burden
**Effort**: Medium (3-4 hours to define and replace)

---

### 7. Consolidate Logging Strategy

**Issue**: Mixed use of KLogger and error_log throughout codebase.

**Current State**:
- `LoopController`: KLogger
- `SlideController`: error_log
- `UploadHandler`: error_log
- `StoreLoop`: error_log

**Recommended Approach**:

Standardize on KLogger:
```php
// In all controllers/models
private $log;

public function __construct($app, \Store $store) {
    $this->log = new \KLogger(
        $_ENV['LOG_DIR'] . "digital_sign.log",
        \KLogger::DEBUG
    );
}

// Replace error_log calls
$this->log->LogDebug("Added slide to loop: " . $slideID);
$this->log->LogError("Failed to add slide: " . $e->getMessage());
$this->log->LogWarning("Scheduled slide has no start date");
```

**Risk**: Debugging difficulty with scattered logs
**Effort**: Medium (2-3 hours)

---

### 8. Sanitize Error Log Output

**Issue**: Raw database errors logged without sanitization.

**Current Pattern**:
```php
error_log("Error: " . print_r($query->errorInfo(), true));
```

**Risk**: Sensitive information (table names, column names) in logs

**Recommended Pattern**:
```php
// Log for debugging
$this->log->LogError("Database operation failed", [
    'operation' => 'addSlide',
    'error_code' => $query->errorInfo()[0],
    // Don't log full SQL or data
]);

// In development only
if ($_ENV['APP_ENV'] === 'development') {
    $this->log->LogDebug("SQL Error Details: " . print_r($query->errorInfo(), true));
}
```

**Effort**: Low (1-2 hours)

---

### 9. Remove Dead Code

**Issue**: `GlobalSlideController.php` is an empty stub file.

**Location**: `userfrosting/src/BuyerKiosk/DigitalSign/Controllers/GlobalSlideController.php`

**Current Content**:
```php
<?php
namespace BuyerKiosk\DigitalSign\Controllers;

class GlobalSlideController extends \BuyerKiosk\Core\Controllers\BaseController
{
    // Empty class
}
```

**Recommendation**: Delete file if not planned for use, or add TODO comment with planned functionality.

**Effort**: Trivial (5 minutes)

---

## Low Priority (Enhancements)

### 10. WebSocket Upgrade for Display Updates

**Current**: Displays may use polling or page reload for updates.

**Enhancement**: Full Ably subscription on client for real-time updates without reload.

```javascript
// In loop.html or separate JS module
const ably = new Ably.Realtime(ablyApiKey);
const channel = ably.channels.get(typeNum);

channel.subscribe('refresh', (message) => {
    // Fetch new loop data via AJAX instead of full reload
    fetchAndUpdateLoop();
});

function fetchAndUpdateLoop() {
    fetch(`/${typeNum}/api/loop`)
        .then(response => response.json())
        .then(data => updateSlideshow(data));
}
```

**Effort**: Medium (8-12 hours)

---

### 11. API Versioning for Sync App

**Current**: Single API version, breaking changes could affect deployed sync apps.

**Enhancement**: Version prefix for sync app endpoints.

```php
// Current
GET /:typeNum/DigitalSignSyncApp/

// With versioning
GET /api/v1/:typeNum/DigitalSignSyncApp/
GET /api/v2/:typeNum/DigitalSignSyncApp/
```

**Effort**: Medium (4-6 hours)

---

### 12. Add API Documentation

**Enhancement**: Document all digital signage APIs with OpenAPI/Swagger.

**Deliverable**: OpenAPI 3.0 spec file for:
- Upload endpoints
- Loop management endpoints
- Sync app endpoints

**Effort**: Medium (6-8 hours)

---

## Implementation Roadmap

### Week 1: Critical Security

| Task | Effort | Owner |
|------|--------|-------|
| Add input sanitization to SlideController | 2h | Backend |
| Add permission checks to routes | 3h | Backend |
| Test security fixes | 2h | QA |

### Week 2: High Priority Fixes

| Task | Effort | Owner |
|------|--------|-------|
| Fix updateLoop() bug | 2h | Backend |
| Add transaction management | 6h | Backend |
| Test reliability fixes | 3h | QA |

### Week 3-4: Maintainability

| Task | Effort | Owner |
|------|--------|-------|
| Define constants for magic numbers | 4h | Backend |
| Consolidate logging strategy | 3h | Backend |
| Sanitize error logging | 2h | Backend |
| Remove dead code | 1h | Backend |
| Update documentation | 2h | Docs |

### Backlog: Enhancements

| Task | Effort | Priority |
|------|--------|----------|
| CSRF token protection | 6h | When resources allow |
| WebSocket upgrade | 12h | Future sprint |
| API versioning | 6h | Future sprint |
| API documentation | 8h | Future sprint |

---

## Success Metrics

### Security

- [ ] No direct `$_POST` access in controllers
- [ ] All routes have permission checks
- [ ] CSRF protection on state-changing routes

### Reliability

- [ ] updateLoop() functions correctly
- [ ] Multi-table operations are transactional
- [ ] Zero data inconsistency incidents

### Maintainability

- [ ] All type codes use constants
- [ ] Single logging strategy (KLogger)
- [ ] No sensitive data in logs
- [ ] No dead code files

---

## Related Documentation

- [Overview](./digital-signage-overview.md)
- [Business Rules](./digital-signage-business-rules.md)
- [Technical Patterns](./digital-signage-technical-patterns.md)
- [Integration Map](./digital-signage-integrations.md)