Address demo feedback items

[mlissner]

Fixes

Fixes: #72, which contained about ten things identified during the first week with real users.

Summary

Addresses the demo feedback items from #72:

• Auto-subscribe on edit — Users are now auto-subscribed to pages they edit, not just pages they create (@rachlllg)
• Propose Change for logged-in users — Removed editor block on feedback page; added "Propose Change" to the actions dropdown (@rachlllg, @mitch-mitchel, implied :) )
• Proposal notification privacy — Confirmed that reviewer info (display name / email prefix) is not viewed by user making an edit proposal (@JessicaFrank)
• Syntax highlighting in preview — Load highlight.js on the form page and apply highlighting after preview HTML is injected (@ERosendo)
• Inherited visibility icon — Subdirectories with inherited visibility now show the correct visibility badge by resolving effective values
• Location field autocomplete:
  • Fixed bug where next-level dropdown didn't appear after selecting a directory via Tab/slash
  • Location picker now has keyboard nav — Arrow up/down, Enter, and mouse hover highlighting in the directory dropdown
  • Permissions now updated as directories are selected — New /api/dir-inherit/ endpoint returns resolved inheritance metadata; plain <select> elements upgrade to inherit-select Alpine components when a directory is picked; existing inherit-selects update dynamically
• Scrollable TOC sidebar — Added max-h-[calc(100vh-2rem)] overflow-y-auto to the TOC nav
• Default change message — Pre-fill "Add new page" for new page creation

This also pushed it past what it could do with backend-testing only, so this adds Playwright browser testing infrastructure:

• Switched dev deps from [project.optional-dependencies] to [dependency-groups] (prod builds use --no-dev)
• Dockerfile installs Chromium only in dev builds
• 8 browser tests covering inherit-select behavior and keyboard navigation

Deployment

This PR should:

• skip-deploy (skips everything below)
  • skip-web-deploy
  • skip-daemon-deploy

🤖 Generated with Claude Code

[claude]

⚠️ Code review skipped — your organization's overage spend limit has been reached.

Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.

Once credits are available, reopen this pull request to trigger a review.

[claude]

🔴 In _upgradeSelectsToInherit (page-form.js:105-107), inheritOpt.innerHTML is built via string concatenation using fieldMeta.source (the directory title from /api/dir-inherit/), which is user-controlled and not HTML-escaped. A malicious directory title injects arbitrary HTML into every page-creation form that picks that directory. The fix is to build the inherit option with createElement/textContent the same way _rebuildOptions in alpine-components.js already does.

Extended reasoning...

What the bug is

In _upgradeSelectsToInherit (page-form.js lines 105-107), the inherit option DOM node is populated using innerHTML with raw string concatenation:

inheritOpt.innerHTML =
  '<div class="text-sm font-medium">' + fieldMeta.display + '</div>' +
  '<div ...>Provided by ' + fieldMeta.source + '</div>';

fieldMeta.source is source.title — the raw title string of the directory that provides the inherited value, returned verbatim by /api/dir-inherit/ (see directory_inherit_meta in directories/views.py: result[field_name] = {'source': source.title, ...}). Directory titles are plain CharField values with no sanitization on save or retrieval.

The specific code path that triggers it

1. Attacker creates a directory whose title contains HTML, e.g. <img src=x onerror=alert(document.cookie)> or <style>body{display:none}</style>.
2. A victim navigates to /c/new/, then picks that directory in the location picker.
3. fetchInheritMeta fires, fetches /api/dir-inherit/?path=..., and the API returns the malicious title as source.
4. _upgradeSelectsToInherit replaces the plain <select> with a custom dropdown by setting inheritOpt.innerHTML — injecting the attacker-controlled HTML directly into the DOM.

Why existing code doesn't prevent it

fieldMeta.display is safe (it comes from Django TextChoices static string values), but no escaping is applied to fieldMeta.source. The contrast with _rebuildOptions in alpine-components.js (which correctly uses textContent for both inheritDisplay and inheritSource) shows that the risk was understood for the Alpine update path but overlooked for the initial upgrade path in page-form.js.

Impact

The Content-Security-Policy's script-src: 'self' (no unsafe-inline) blocks inline JS event handlers like onerror, which reduces the impact below full JavaScript execution. However: (1) style-src includes 'unsafe-inline', so <style> injection executes and can be used for CSS-based data exfiltration, UI defacement, or phishing overlays; (2) arbitrary HTML injection still enables form injection, redirect via <meta http-equiv=refresh>, and visual deception attacks. Any user with directory creation/edit permission can trigger this for every user who subsequently creates a page in that directory.

Step-by-step proof

1. Attacker with directory-edit permission calls PUT /c/evil-dir/edit-dir/ (or creates a new directory) with title: <style>#page-form{display:none}</style>Legit Title.
2. Victim logs in, navigates to /c/new/, and types evil in the location picker.
3. The evil-dir suggestion appears; victim selects it.
4. fetchInheritMeta('/api/dir-inherit/?path=evil-dir') fires.
5. Backend returns {"visibility": {"source": "<style>#page-form{display:none}</style>Legit Title", ...}}.
6. _upgradeSelectsToInherit sets inheritOpt.innerHTML = '...' + '<style>#page-form{display:none}</style>Legit Title' + '...'.
7. The <style> block executes, hiding the entire page form. Any attacker-controlled CSS runs.

How to fix

Replace the innerHTML assignment with element construction using textContent, mirroring the pattern already used in _rebuildOptions in alpine-components.js:

var titleEl = document.createElement('div');
titleEl.className = 'text-sm font-medium';
titleEl.textContent = fieldMeta.display;
inheritOpt.appendChild(titleEl);

var sourceEl = document.createElement('div');
sourceEl.className = 'text-xs text-gray-400 dark:text-gray-500';
sourceEl.setAttribute('aria-hidden', 'true');
sourceEl.textContent = 'Provided by ' + fieldMeta.source;
inheritOpt.appendChild(sourceEl);

[claude]

🟡 The init() function in the inheritSelect Alpine component declares var fieldName twice on lines 425 and 429, both assigning the same expression (this.$el.getAttribute('data-field')). This is a copy-paste artifact — the first declaration at line 425 is dead code, immediately overwritten by the identical assignment four lines later. No runtime bug occurs due to JavaScript's var hoisting, but removing the first declaration would clarify the code.

Extended reasoning...

What the bug is

In the inheritSelect component's init() function, var fieldName is declared and assigned twice in the same function scope. The pattern is:

var fieldName = this.$el.getAttribute('data-field') // line 425 (dead)

// Update inherit metadata when the directory changes
var self = this
var fieldName = this.$el.getAttribute('data-field') // line 429 (used)
if (fieldName) {
document.addEventListener('dir-inherit-update', ...)
}

The specific code path

The block at line 425 was inserted first (likely when wiring up the label-map logic), then the entire block including its own var fieldName was added immediately after, with var self = this sandwiched in between. The result is two declarations of the same variable with the same value.

Why existing code does not prevent it

JavaScript's var declarations are function-scoped and hoisted to the top of the containing function. A second var fieldName in the same function is perfectly legal and simply re-assigns the already-hoisted variable. No linter warning fires in non-strict mode. Since both assignments set identical values, runtime behavior is unaffected.

Impact

There is no runtime bug. The first assignment (line 425) is immediately overwritten by the second (line 429) before any code that uses fieldName runs. A reader of the code is likely to be confused about whether the first fieldName was ever used for something.

How to fix

Remove the first dead declaration and its assignment at line 425. Only the declaration at line 429 should remain.

Step-by-step proof

1. JavaScript hoists var fieldName to the top of init().
2. Line 425 executes: fieldName = this.$el.getAttribute('data-field'). Value is say visibility.
3. Line 428 executes: self = this.
4. Line 429 executes: fieldName = this.$el.getAttribute('data-field'). Same value overwrites.
5. The if (fieldName) check proceeds. The first assignment at line 425 had zero observable effect.

Addressing the refutation

The refutation correctly notes there is no runtime bug. Filing as nit is appropriate — this is a genuine copy-paste error introduced by this PR that adds confusion without serving any purpose.

[claude]

🟡 fetchInheritMeta has no .catch() handler, so two failure paths produce unhandled promise rejections: a network error causes fetch() to reject directly, and a session expiry causes Django's @login_required to redirect to the login page — fetch follows the 302 to a 200 HTML response where r.ok is true but r.json() throws a SyntaxError. The form continues working with plain select fallbacks in both cases, but a one-line .catch(function(){}) would silence the console errors.

Extended reasoning...

What the bug is and how it manifests

The fetchInheritMeta function in page-form.js (lines 29-38) makes a fetch() call but ends its .then() chain without any .catch(). When the Promise rejects for any reason, the rejection propagates without a handler, producing an unhandled promise rejection logged as a console error in modern browsers.

The two real failure paths

1. Network failure: If the user is on a flaky connection or the server is temporarily unreachable, fetch() itself rejects with a TypeError. Since there is no .catch(), this becomes an unhandled rejection.

2. Session expiry: The endpoint is protected by Django's @login_required decorator. If the session expires while the page is open and the user then selects a directory, Django issues a 302 redirect to the login page. fetch() follows the redirect automatically and receives a 200 OK HTML response. Because the status is 200, r.ok is true, so the code calls r.json() - but the body is HTML. r.json() returns a Promise that rejects with a SyntaxError, which propagates as an unhandled rejection.

Why existing code does not prevent it

The r.ok ? r.json() : null guard only handles non-2xx HTTP status codes (e.g. 404, 500). It does not handle network errors (which reject fetch() before any response is received) or the login-redirect scenario (which returns 200 HTML). Neither failure path is caught.

Impact

Functional impact is low: both failures are silent, _upgradeSelectsToInherit is never called, and the existing plain select elements remain so the form is still submittable. The refutation correctly notes that "can crash the page" is overstated for modern browsers - this is a code-quality issue, not a crash. The UI degrades silently rather than catastrophically.

How to fix it

Append .catch(function() {}) to the chain at the end of fetchInheritMeta.

Step-by-step proof of the session-expiry path

1. User opens /c/new/ with a valid session.
2. Session expires.
3. User selects a directory in the location picker, triggering fetchInheritMeta('staff').
4. Browser sends GET /api/dir-inherit/?path=staff with the expired session cookie.
5. Django @login_required issues a 302 to /u/login/?next=....
6. fetch() follows the redirect automatically; receives 200 OK with Content-Type: text/html.
7. r.ok is true (status 200), so the code calls r.json().
8. r.json() tries to parse HTML as JSON and the parser throws a SyntaxError.
9. The returned Promise rejects with no .catch(), so the rejection is unhandled and logged to the browser console.

[claude]

🟣 In fetchSuggestions (page-form.js), directory titles from the API (d.title) and the typed query are concatenated unescaped into innerHTML attribute values, allowing attribute boundary breakout with a double-quote character. A directory title like foo" onclick="alert(1) would inject an event handler; with style-src: unsafe-inline in the CSP, a title like foo"><style>body{display:none}</style> would inject a working style element. This vulnerability pre-dates the PR — the diff shows only hover CSS classes were removed from these lines — but the PR touches the code, making this a good time to fix it by HTML-escaping d.title and query before interpolation.

Extended reasoning...

What the bug is and how it manifests

In fetchSuggestions in page-form.js (lines ~195-210), directory dropdown items are built by concatenating server-supplied and user-supplied strings directly into an HTML string that is assigned to dirDropdown.innerHTML:

html += '<div ... data-title="' + d.title + '">' + d.title + '</div>';
html += '<div ... data-title="' + query + '">Create "' + query + '"</div>';

Neither d.title (from /api/dir-search/ JSON) nor query (from locationInput.value) is HTML-escaped before interpolation.

The specific code path that triggers it

A directory title containing a double-quote character (e.g. foo" data-evil="injected) breaks out of the data-title attribute boundary when assigned to innerHTML. A payload like foo" onclick="alert(1) injects an onclick attribute. The d.path field is safe because it is produced by Django slugify(), emitting only [a-z0-9-] characters.

Why existing code does not prevent it

The CSP blocks inline event handler execution (script-src: self, no unsafe-inline), so onclick= injection cannot execute JavaScript directly. However, style-src includes unsafe-inline, so a title like foo"><style>body{display:none}</style> would inject a fully functional style element when the dropdown renders. HTML structure injection (premature tag closing, anchor injection, etc.) is also possible regardless of CSP.

What the impact would be

This is a stored XSS / HTML injection vector: any user with directory-edit permissions can set a malicious title. When any authenticated user opens the page-creation form and the dropdown renders that directory, the injected markup executes in their browser session. The query self-XSS (user attacking their own session) is lower severity and not a security concern.

How to fix it

HTML-escape special characters before interpolation. A small helper suffices:

function escapeHtml(s) {
  return String(s)
    .replace(/&/g, "&")
    .replace(/</g, "&lt;")
    .replace(/>/g, "&gt;")
    .replace(/"/g, "&quot;");
}

Replace d.title and query in the html += lines with escapeHtml(d.title) and escapeHtml(query). The data-title attribute values read back via el.dataset.title automatically decode HTML entities, so downstream use in selectDirItem requires no changes.

Step-by-step proof

1. An admin creates a directory with title: Staff"><style>* { display: none }</style>
2. Any user visits /c/new/ and focuses the location input.
3. fetchSuggestions("") fires, calls /api/dir-search/, and receives {"path": "staff", "title": "Staff"><style>..."}.
4. The code builds: html += <div ... data-title="Staff"><style>* { display: none }</style>"...
5. dirDropdown.innerHTML = html is called — the browser parses the injected style tag.
6. Because style-src includes unsafe-inline, the injected style is applied and the page content is hidden.

Pre-existing note: The git diff confirms the PR only changed CSS class names on these exact lines (removing hover:bg-gray-100 dark:hover:bg-gray-700). The injection pattern was identical before and after the PR. Severity is pre_existing.