Skip to content

Fix silent form failure when creating pages from root with location picker#78

Merged
mlissner merged 4 commits intomainfrom
fix-root-page-create-silent-fail-20260327
Mar 28, 2026
Merged

Fix silent form failure when creating pages from root with location picker#78
mlissner merged 4 commits intomainfrom
fix-root-page-create-silent-fail-20260327

Conversation

@mlissner
Copy link
Copy Markdown
Member

@mlissner mlissner commented Mar 28, 2026

Fixes

This fixes a bug where creating a page from /c/new/ (root level) and selecting a directory via the location picker silently failed — the form reloaded with all values intact except the location field (blank), and the page was not created.

Summary

Root cause: page_create initialized PageForm with directory=None (from the URL), which removed "inherit" from valid field choices. But the JS location picker upgraded selects to inherit-select components that defaulted hidden fields (in_sitemap, in_llms_txt) to "inherit". On POST, form validation rejected "inherit" as invalid. The errors were invisible (in a hidden fieldset) and the location picker state was lost on re-render.

Fix: On POST, resolve the directory from the location picker's directory_path POST data (not just the URL) when initializing the form. This makes "inherit" a valid choice. Also preserve the location picker state from POST data on validation error re-renders.

Two new helpers in views.py:

  • _resolve_directory_from_post() — finds closest existing directory or ancestor from POST data
  • _build_dir_segments_from_post() — reconstructs location picker segments for re-renders

Applied same fix to page_edit for consistency.

Tests: 3 unit tests + 3 Playwright browser tests covering: existing dir selection, new subdir creation, and validation error preservation.

Deployment

This PR should:

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

🤖 Generated with Claude Code

mlissner and others added 2 commits March 27, 2026 22:44
…icker

When creating a page from /c/new/ and selecting a directory via the
location picker, the form silently failed because PageForm was initialized
with directory=None (from the URL), which removed "inherit" from valid
field choices. The JS location picker upgraded selects to inherit-select
components that defaulted in_sitemap and in_llms_txt to "inherit", causing
form validation to reject them.

Adds two helpers:
- _resolve_directory_from_post(): finds the closest existing directory
  from the POST directory_path, walking up ancestors for new directories
- _build_dir_segments_from_post(): reconstructs location picker segments
  from POST data so the picker preserves state on validation error re-renders

Updates page_create and page_edit to use the location picker's directory
(not just the URL directory) when initializing the form on POST.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Unit tests (3):
- Create from root with existing directory and inherit values
- Create from root with new subdirectory via directory_titles
- Validation error preserves location picker state

Playwright browser tests (3):
- Create page in new subdirectory (full user flow)
- Create page selecting existing directory with inherit defaults
- Validation error preserves location picker selection

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
values and place the page in the chosen directory."""
client.force_login(user)
r = client.post(
"/c/new/",
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always, always use reverse for URLs!

@mlissner mlissner marked this pull request as ready for review March 28, 2026 05:48
Comment on lines +141 to +158

Used when re-rendering the form after validation failure so the
location picker preserves the user's directory selection.
"""
dir_path = post_data.get("directory_path", "").strip()
if not dir_path:
return []
title_overrides = _parse_directory_titles(post_data)
slugs = dir_path.strip("/").split("/")
segments = []
current_path = ""
for slug in slugs:
current_path = f"{current_path}/{slug}" if current_path else slug
directory = Directory.objects.filter(path=current_path).first()
if directory:
title = directory.title
elif current_path in title_overrides:
title = title_overrides[current_path]
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 The new _build_dir_segments_from_post() function includes user-controlled POST values (from directory_path and directory_titles) in a dict that is passed to json.dumps(), then rendered with |safe inside a <script> block in the template. Python's json.dumps() does not HTML-escape </script>, so a payload like </script><script>alert(1)</script> in directory_path breaks out of the script element, causing reflected XSS on POST validation failure re-renders. Fix by using Django's json_script template tag or django.utils.html.escapejs() instead of json.dumps() + |safe.

Extended reasoning...

What the bug is and how it manifests

The new _build_dir_segments_from_post() helper (views.py:141–158) reads user-controlled directory_path and directory_titles POST fields, constructs a list of dicts containing titles derived from that input, and returns it. The caller in page_create (and page_edit) wraps it in json.dumps() and passes it as the template context variable dir_segments_json. The template renders it as {{ dir_segments_json|safe }} directly inside a <script type="application/json"> block. Because |safe disables Django's auto-escaping, the raw JSON bytes land verbatim in the HTML response.

The specific code path that triggers it

Python's json.dumps() intentionally does not escape HTML-special characters by default (ensure_ascii=True only escapes non-ASCII). A string like </script><img src=x onerror=alert(1)> passes through intact. When the HTML parser sees </script> inside a <script> element it immediately closes the script block—regardless of the element's type attribute—and the remainder of the injected string becomes raw HTML in the document. Attack path: POST to /c/new/ with directory_path=</script><img src=x onerror=alert(1)> and a blank title field. The blank title triggers a form validation error, causing the view to re-render the form and embed _build_dir_segments_from_post(request.POST) directly into the response.

Why existing code doesn't prevent it

The original _build_dir_segments(directory) function only used titles fetched from the database (trusted data). The new _build_dir_segments_from_post() function is the first place that passes unsanitized user input through this code path. Django's template auto-escaping is intentionally bypassed with |safe (needed because the value is JSON, not plain text), but no alternative HTML-safe serialisation is applied.

What the impact would be

This is reflected XSS: an attacker can craft a form that, when submitted by a victim, injects and executes arbitrary JavaScript in the victim's browser session. The attack requires the victim to be authenticated (@login_required) and to have a valid CSRF token, limiting cross-user exploitation to social-engineering scenarios. The project's CSP (script-src: 'self', no unsafe-inline) would block inline script execution in modern browsers, but the </script> injection still corrupts the page's JSON block causing JS errors, and injected HTML attributes like onerror can still fire in some contexts. This is a real introduced vulnerability absent from the pre-existing code.

How to fix it

Replace the json.dumps() + |safe pattern with Django's json_script template filter, which internally uses django.utils.html._json_script_escapes to replace <, >, &, and ' with safe Unicode escape sequences. Alternatively, serialize with json.dumps(data) then call .replace("</", "<\/") before passing to the template, or use django.utils.html.escapejs() on each user-supplied string value before serialization.

Step-by-step proof

  1. Attacker (authenticated) submits POST to /c/new/ with: directory_path=foo</script><img src=x onerror=alert(document.cookie)>, directory_titles={}, title= (empty — triggers validation failure), plus a valid CSRF token from their own session.
  2. _build_dir_segments_from_post() builds [{"path": "foo</script>...", "title": "Foo</Script>..."}].
  3. json.dumps() produces the literal string [{"path": "foo</script><img src=x onerror=alert(document.cookie)>", ...}] with no escaping.
  4. The template embeds this inside <script type="application/json" id="page-config">...dirSegments: [{"path": "foo</script>.... The HTML parser closes the script element at the first </script> it encounters.
  5. The remaining <img src=x onerror=alert(document.cookie)> is parsed as raw HTML and the onerror handler fires, exfiltrating session data.
  6. Note: slug.replace('-', ' ').title() transforms </script> to </Script> in the title field only — HTML tag name matching is case-insensitive so this does not mitigate the path field injection.

- Replace hardcoded "/c/new/" with reverse("page_create") in all new
  unit tests per project URL rules.
- HTML-escape user-supplied path and title values in
  _build_dir_segments_from_post() to prevent XSS via </script>
  injection when the result is rendered with |safe in a <script> block.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@mlissner mlissner enabled auto-merge March 28, 2026 06:11
The test_page_form_no_duplicate_after_dir_change test was flaky in CI
because it used a direct .click() on a dropdown item that could become
hidden due to a race condition with dropdown rebuilds. Switch to the
same wait_for_function + evaluate/dispatchEvent pattern used by the
adjacent test.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@mlissner mlissner merged commit 69aebe1 into main Mar 28, 2026
8 checks passed
@mlissner mlissner deleted the fix-root-page-create-silent-fail-20260327 branch March 28, 2026 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant