# EVALUATION_DOWNLOAD.md

**The evaluation-results Word download.** Traced 2026-08-25 while converting
`views-view-table--evaluation-results.tpl.php`. **D7 read-only throughout.**

---

## 1. What D7 actually does

```
GET /evaluation-results
 └─ views-view-table--evaluation-results.tpl.php RENDERS
      └─ if (count($rows) < 30) webform_document_download_results($rows);
           ├─ unlink(sites/default/files/webform_document/filled/merged.docx)
           ├─ $node = 27            <- HARD-CODED evaluation webform node
           ├─ file_load($node->webform['document_template']['fid'])
           └─ foreach ($rows as $row)
                ├─ webform_get_submission(27, $row['sid'])
                ├─ $comment = $submission->data[27]['value'][0]
                ├─ truncate to MAX_TEXT_LENGTH, append "..."
                ├─ str_replace('&', '&amp;', $comment)
                └─ webform_document_apply_replacements(...)   -> appends to merged.docx

 template also emits:
   <a href="/evaluation-results/download-link"><img src="{theme_default}/images/word.png"></a>

GET /evaluation-results/download-link
 └─ webform_document_download_link()
      └─ drupal_goto("sites/default/files/webform_document/filled/merged.docx")
```

**Rendering a page writes a file.** The document is a by-product of viewing the results table, not
of clicking download.

---

## 2. 🛑 Another hard-coded node ID: **27**

`webform_document_download_results()` hard-codes `$node = 27` — the evaluation webform node. This is
the **same node 27** that carries webform component **`cid = 30`** (the course nid,
`RISK_REGISTER.md` R15) which `course_actions.module` reads back as `data[30]`.

So node 27 now has **three** independent bindings:

| Binding | Reads |
|---|---|
| `course_actions.module` | `data[30]` → the course nid |
| `webform_document.module:238` | `data[27]` → the evaluation comment |
| the evaluation-results view | submissions of node 27 |

> **Node 27 must survive the migration with its ID intact**, exactly like 22–29 and 36.
> Added to `ce_migrate`'s node-ID requirement.

⚠️ Note the collision of numbers: **component `27`** holds the comment and **component `30`** holds
the course — both on **node `27`**. Easy to conflate; they are three different things.

---

## 3. Preserved defects

### E-1 · The 30-row threshold — stale downloads

```php
if (count($rows) < 30) { webform_document_download_results($rows); }
```

With **30 or more rows the document is not regenerated**, but the download link is **still
rendered**. Users then download whatever `merged.docx` was last written — someone else's earlier,
smaller result set — or hit a missing file.

There is no message, no disabled state, no indication. **Preserved**: the threshold and the
always-visible link are both reproduced.

### E-2 · Generation timing moves — the one observable change

| | D7 | D10 |
|---|---|---|
| Document built when | the results **page renders** | the **download is requested** |

**This is not optional.** Twig has no side effects by design, and Drupal caches rendered output —
so a render-time generation would fire only on a cache miss, making the document *less* current
than D7's, unpredictably. Moving it to the download request produces the same document from the
same rows, at a defined moment.

Classified **TECHNICAL REWRITE** (`CLAUDE.md` §5). Recorded here because it is a real, if subtle,
difference in *when* the file changes.

### E-3 · ✅ RESOLVED 2026-08-25 — security fix approved and implemented

**Approved by the business owner.** The D7 public shared-file vulnerability is **not reproduced**.

#### The precise flaw — D7's permission existed but did not work

```php
$items['evaluation-results/download-link'] = array(
  'page callback'    => 'webform_document_download_link',
  'access arguments' => array('access webform document template'),   // <- present!
);
function webform_document_download_link() {
  drupal_goto("sites/default/files/webform_document/filled/merged.docx");  // <- bypasses it
}
```

The **route** was protected. The callback then redirected the browser to a **public file path**,
served by the web server with no Drupal involvement. So anyone who knew the URL could download every
evaluation submission **without the permission, without logging in, and without a trace**. The file
was also **shared site-wide** — one `merged.docx`, rewritten by whichever admin last viewed the
results page, so concurrent renders interleaved.

#### What changed

| | D7 | D10 |
|---|---|---|
| File | one shared `public://…/merged.docx` | **per-request**, unpredictable name (`random_bytes(16)`) |
| Location | public files dir | **`private://`**, falling back to the OS temp dir |
| Delivery | `drupal_goto()` **redirect** | **streamed** via `BinaryFileResponse` |
| Permission | declared but bypassable | **the same permission now governs the bytes** |
| Lifetime | persisted indefinitely | **deleted after sending** (`deleteFileAfterSend`) |
| Caching | web-server cacheable | `private, no-store, max-age=0` |

#### What did **not** change

Route path `/evaluation-results/download-link` · permission name
`access webform document template` (preserved verbatim so the role/permission migration maps 1:1) ·
the link in the results table · the downloaded filename `merged.docx` · **document content, row
order, truncation (550 bytes, literal `...`), `&` escaping order, template source, node 27,
component 27**.

**No business rule or evaluation calculation was altered.**

#### One consequence of the fix, stated plainly

E-1's threshold is preserved: at **30+ rows no document is generated**. In D7 the user then received
the **stale shared file** — someone else's earlier result set. That *is* the cross-user leak being
removed, so there is no stale file to serve. D10 returns **404 "no document available"** instead.

> Below 30 rows: identical to D7. At or above 30: D7 served another admin's data; D10 serves
> nothing. This is a direct consequence of the approved fix, recorded rather than hidden.

#### Implementation

```
web/modules/custom/ce_reports/
├── ce_reports.permissions.yml   'access webform document template'  (D7 name)
├── ce_reports.routing.yml       /evaluation-results/download-link + _permission
├── ce_reports.services.yml
├── src/EvaluationDocumentBuilder.php     rules, truncation, escaping, temp file
└── src/Controller/EvaluationDocumentController.php   access + stream + delete
```

**RUNTIME-UNVERIFIED.** Two internals remain `TODO` and deliberately return empty rather than guess:
`loadResultRows()` needs the migrated **`evaluation-results` view** (a hand-written query could
silently cover the wrong submissions), and the template/submission readers need the **webform
migration**. Both return "no document", which is safe; neither fabricates one.

---

### E-3 (original finding) · one shared file, publicly readable

`merged.docx` is a **single file** at `sites/default/files/webform_document/filled/merged.docx`:

- **Shared by every administrator.** Two concurrent renders interleave writes; whoever downloads
  gets an unpredictable mixture.
- **In the public files directory.** `drupal_goto()` redirects the browser straight to it, so the
  file is served by the web server with **no access check**. Anyone who knows or guesses the URL
  can download evaluation submissions.
- `unlink()` runs with no `file_exists()` guard, emitting a PHP warning when absent.

`CLAUDE.md` §23 says security vulnerabilities *"must NOT be reproduced where doing so would expose
sensitive data"* — while §24 says not to silently fix defects. **These pull in opposite directions
here**, so I have not decided it unilaterally.

**Recommendation (not applied):** keep the identical document and the identical link, but serve it
through a controller with an access check and a per-user temporary file. That preserves the
observable flow for a legitimate admin while removing the public-URL exposure and the cross-user
race. It changes no business rule.

**Flagged for a decision.** Until then the D10 implementation reproduces D7's behaviour, and this
section is the record of why that is not a safe end state.

---

## 4. Other details preserved

- **Word icon path** resolves from `variable_get('theme_default')`, i.e. the **default** theme — not
  the active one. Reproduced via `word_icon_url`.
- **`&` escaping**: `str_replace('&', '&amp;', $comment)` runs *after* truncation, so a comment cut
  mid-entity can produce a broken entity. Preserved.
- **Truncation** appends a literal `"..."` (three periods, not an ellipsis character).
- `views-view--evaluation-results.tpl.php` is **stock** D7 markup; it exists only to override the
  bootstrap base theme.

---

## 5. Status

| Item | State |
|---|---|
| `views-view--evaluation-results.html.twig` | ✅ written, runtime-unverified |
| `views-view-table--evaluation-results.html.twig` | ✅ written, side effect removed from Twig |
| Document generation in a controller | ✅ `ce_reports` — written, runtime-unverified |
| Node 27 added to the node-ID requirement | ✅ |
| **E-3 security fix** | ✅ **RESOLVED 2026-08-25 — implemented, see §3** |
| `loadResultRows()` | 🛑 blocked — `evaluation-results` view migration |
| template + submission readers | 🛑 blocked — webform migration |