# Patch Analysis: Horilla `protected_media()` Containment Fix and Authorization Gap

## Scope and source identities

Repository: `https://github.com/horilla/horilla-hr.git`

- Submitted vulnerable tag 1.5.0: `61bd5173220d19925ad8220db9152a75c881ea73`
- Containment-fix commit: `67ac2056813ee95d4c4a0bfe7c0124a361cb6c48`
- Fixed tag 1.6.0: `b3bd29d15819cbece45c58e6268ddd0614e387d6`
- Current 1.x/master tested: `11c4e3a2596c58f2381bda4c6bbc319a4430b097`
- Newer release sibling tested (`release/v2.1.0`): `b8cbf0af11d105e6e8c3708c3156322248f93e77`
- Current v2 tested (`origin/dev/v2.0`): `a971ff98662904f798819415203ccb26bc4b0db3`

## What the fix changes

Commit `67ac2056813ee95d4c4a0bfe7c0124a361cb6c48` modifies only `base/views.py` for the containment issue:

1. Imports `safe_join` from `django.utils._os`.
2. Replaces:

   ```python
   media_path = os.path.join(settings.MEDIA_ROOT, path)
   ```

   with:

   ```python
   try:
       media_path = safe_join(settings.MEDIA_ROOT, path)
   except Exception:
       raise Http404("Invalid file path")
   ```

3. Changes the existence check to require a regular file:

   ```python
   if not os.path.exists(media_path) or not os.path.isfile(media_path):
       raise Http404("File not found")
   ```

4. Leaves the real entrypoint and sink unchanged:
   - `base/urls.py`: `^media/(?P<path>.*)$`
   - `base.views.protected_media()`
   - `FileResponse(open(media_path, "rb"))`

The patch correctly blocks normalized targets outside `MEDIA_ROOT` and directory opens. Runtime controls returned 404 for `/media/../outside_<canary>.txt` on tag 1.6.0 and both current branches while a valid public in-root file remained readable.

## Fix assumptions

The patch relies on the valid invariant that `safe_join(MEDIA_ROOT, path)` returns a normalized target lexically contained under `MEDIA_ROOT` or raises. However, the surrounding authorization logic assumes that the raw `path` string and the normalized target identify the same authorization namespace.

In fixed/current code, authorization uses one of these equivalent forms:

```python
any(path.startswith(folder) for folder in exempted_folders)
```

or:

```python
is_public_asset = any(path.startswith(prefix) for prefix in public_media_prefixes)
```

The assumption is that a raw path beginning with `base/icon/` resolves to a target beneath `MEDIA_ROOT/base/icon/`. Dot-segment normalization disproves that assumption.

## Code paths and inputs not covered

The fix does not cover authorization-boundary traversal that stays within `MEDIA_ROOT`:

```text
raw path:        base/icon/../../private.txt
public decision: startswith("base/icon/") == true
normalized open: MEDIA_ROOT/private.txt
```

`safe_join()` accepts the target because it remains inside `MEDIA_ROOT`. The later raw-prefix check skips authentication. The file-open sink then reads a file outside the intended public subtree.

The same gap applies to every configured public prefix unless the target is checked against the canonical prefix directory. Current 1.x/v2 prefixes include:

- `base/icon/`
- `base/company/icon/`
- `recruitment/candidate/profile/`

Additional hardening not provided by the patch:

- canonical public-subtree containment;
- component-aware prefix comparison;
- rejection of dot segments before authorization;
- realpath/symlink containment where links can be influenced; and
- separation of public and private storage/serving routes.

The source scan found only one `FileResponse(open(media_path, "rb"))` protected-media sink and one route to it, so this is not a fabricated alternate API. It is a materially different data path through the same endpoint and sink.

## Threat-model and security-policy scope

`SECURITY.md` was read from submitted, fixed, release-sibling, and current branch source. It explicitly identifies strong authentication/authorization and validation/sanitization of user input as security best practices. It does not classify remote unauthorized media reads as expected behavior or an out-of-scope limitation.

The confirmed request crosses the same trust boundary as the parent claim: an unauthenticated remote HTTP caller controls the `/media/<path>` route argument. The target—not a local user—opens and returns the selected file. The evidence uses only fresh harmless canaries.

## Behavior before and after the fix

| Candidate | 1.5.0 | 1.6.0 | release/v2.1.0 | current 1.x | current v2 |
|---|---:|---:|---:|---:|---:|
| Valid public in-root canary | 200 | 200 | 200 | 200 | 200 |
| Plain outside-root traversal | 200 | 404 | 200 | 404 | 404 |
| Double-percent traversal | 404 | 404 | 404 | 404 | 404 |
| Public-prefix normalization to private in-root canary | **200** | **200** | **200** | **200** | **200** |

For every public-prefix-normalization response, the body was exactly the private canary and the request carried neither Cookie nor Authorization. The confirmed fixed/latest bypass is therefore not an outside-root containment bypass; it is an authentication/authorization bypass caused by inconsistent path representations.

The old `release/v2.1.0` line also retained the original outside-root bug because its duplicate 2.x fix commit postdates that release. Current v2 includes `safe_join()` and blocks outside-root traversal, but still reproduces the public-prefix bypass.

## Completeness assessment

The original patch is **complete for lexical containment beneath `MEDIA_ROOT`** in the tested direct filesystem path. It is **incomplete for the endpoint's overall file-authorization contract** because it validates containment against the broad media root but makes the public/private decision against an unnormalized string.

Later removal of Referer authorization improves the parent chain but does not close the normalization mismatch. The gap persists at the exact current 1.x and v2 revisions tested.

## Recommended complete fix

1. Normalize once and authorize the normalized target.
2. For each public directory, derive `public_root = safe_join(MEDIA_ROOT, prefix)` and require `os.path.commonpath([media_path, public_root]) == public_root` (with platform-appropriate normalization), rather than `raw_path.startswith(prefix)`.
3. Alternatively, compute a canonical relative path from `media_path` to `MEDIA_ROOT` and compare complete path components, rejecting `.` and `..` transitions.
4. Prefer distinct public and protected storage roots/endpoints.
5. If symlinks can be influenced, enforce realpath containment or use descriptor-relative/openat-style access to prevent lexical-containment bypasses.
6. Add live HTTP regressions for `base/icon/../private`, `base/icon/../../private`, each other public prefix, duplicate separators, percent encodings, and symlink cases, with positive public and authenticated-private controls.