Fix a syntax error I introduced, which put a password in a URL

The commit immediately before this one - the password-toggle fix - deleted a closing
brace and did not replace it. app.js did not parse. The browser reported it in a console
nobody was watching, and every script-driven behaviour on the sign-in page stopped.

The consequence was not a cosmetic one. The login form's submit handler, which is the
only thing calling preventDefault(), never ran, so the browser performed a native form
submission. Neither form declared a method, and a form with no method attribute submits
as GET. Both password fields went into the query string:

    GET /?email=umbra%40bdmurphy.me&password=...

The server logged it in plain text, three times, before it was noticed. It would also
have sat in browser history and in any proxy access log between here and the public
internet.

Two fixes, the second of which is the one that matters:

- The toggle fix is reapplied correctly to the last known-good version of app.js, with
  nothing else in the file touched - the diff is four lines against thirteen and two
  against one.
- **Every form on every page now declares method=post.** The script still intercepts
  and submits via fetch; this decides what happens when the script does not run, which is
  precisely the case that failed. Five more forms carried the same latent defect,
  including password reset and email verification.

test_form_safety.py checks both, and was confirmed to fail when app.js is broken the
same way. It uses esprima rather than a hand-written brace counter, which gave two false
readings during this work - one of them 'balanced' for a file that did not parse at all.
A check that can be wrong about a syntax error is worse than no check.

The account password should be treated as exposed and changed. So should any password
entered into that form while the script was broken.

umbratrans_web/tests/test_form_safety.py

@@ -0,0 +1,120 @@
+"""A broken script must not be able to submit a password in a URL.
+
+This exists because it happened. A syntax error I introduced into `app.js` meant the
+submit handler which calls `preventDefault()` never ran, and both password fields were
+submitted as a native form GET:
+
+    GET /?email=umbra%40bdmurphy.me&password=...&
+
+which the server logged in plain text, three times, and which would also have sat in
+browser history and any proxy's access log.
+
+Two defences, and this file checks both:
+
+1. **Every form declares `method="post"`.** A form with no method attribute defaults to
+   GET. The script intercepts and submits via fetch; `method` only decides what happens
+   when the script does not run, which is exactly the case that failed.
+
+2. **Every script parses.** A syntax error is silent in a browser - the console shows it,
+   the page just does not work - so it is checked here against a real parser rather than
+   by counting braces. A hand-written brace counter was used first and gave two false
+   readings, one of them "balanced" for a file that did not parse at all.
+
+The parser is imported lazily and the file skips when it is absent, so this does not
+become a hard dependency on a package that may not be installed.
+"""
+
+from __future__ import annotations
+
+from pathlib import Path
+
+import pytest
+
+WEB = Path(__file__).resolve().parents[2] / "web"
+
+PAGES = sorted(WEB.glob("*.html"))
+
+
+def _forms(html: str) -> list[str]:
+    import re
+
+    return re.findall(r"<form[^>]*>", html)
+
+
+def _parser():
+    try:
+        import esprima
+    except ImportError:  # pragma: no cover - optional dependency
+        return None
+    return esprima
+
+
+def test_there_are_pages_to_check() -> None:
+    """A glob that matches nothing would make every test below pass vacuously."""
+
+    assert PAGES, f"no pages found under {WEB}"
+
+
+@pytest.mark.parametrize("page", PAGES, ids=lambda p: p.name)
+def test_every_form_declares_post(page: Path) -> None:
+    """The fallback must be POST, or a broken script puts secrets in the query string."""
+
+    offenders = [form for form in _forms(page.read_text()) if 'method="post"' not in form]
+    assert not offenders, (
+        f"{page.name} has a form without method=\"post\": {offenders}. "
+        "A form with no method attribute submits as GET, so every named field - "
+        "including passwords - goes into the URL and the server's request log."
+    )
+
+
+@pytest.mark.parametrize("page", PAGES, ids=lambda p: p.name)
+def test_a_form_holding_a_password_is_post(page: Path) -> None:
+    """Stated as its own check so the security property is not implied by the one above.
+
+    Most of these forms do not hold a password; the two that do were the ones that
+    leaked. If a password field is ever added to a page, this is what catches it.
+    """
+
+    html = page.read_text()
+    if 'type="password"' not in html:
+        pytest.skip("this page has no password field")
+    for form in _forms(html):
+        assert 'method="post"' in form, (
+            f"{page.name} has a password field inside a form that could submit as GET"
+        )
+
+
+def _scripts() -> list[Path]:
+    scripts = sorted(WEB.glob("assets/*.js"))
+    assert scripts, f"no scripts found under {WEB}/assets"
+    return scripts
+
+
+@pytest.mark.parametrize("script", _scripts(), ids=lambda p: p.name)
+def test_every_script_parses(script: Path) -> None:
+    """A syntax error is silent in a browser and fatal to everything the script does.
+
+    This is the check whose absence let the incident happen: a broken `app.js` left the
+    login form with no submit handler at all.
+    """
+
+    esprima = _parser()
+    if esprima is None:
+        pytest.skip("esprima is not installed; cannot parse JavaScript here")
+    try:
+        esprima.parseScript(script.read_text())
+    except Exception as error:  # noqa: BLE001 - the parser raises its own error type
+        pytest.fail(f"{script.name} does not parse: {error}")
+
+
+def test_the_login_form_has_a_method_and_the_other_forms_do_too() -> None:
+    """Named explicitly, because this is the one that leaked.
+
+    A count of "forms with method=post" would pass with the login form missing and
+    another form double-counted.
+    """
+
+    html = (WEB / "index.html").read_text()
+    login = [form for form in _forms(html) if 'id="login-form"' in form]
+    assert len(login) == 1, f"expected exactly one login form, found {len(login)}"
+    assert 'method="post"' in login[0], login[0]

web/assets/app.js

@@ -65,12 +65,20 @@
     if (!button || !input) {
       return;
     }
-    // `visible`, not `reveal`: the old name held the *hidden* test and was then
-    // used to pick the label, so a hidden password offered "Hide". See the note above.
+    /*
+     * The button names the action a click will take. This was inverted: the value
+     * below was called `reveal` but held `input.type === 'password'` - true when the
+     * password is HIDDEN - and then chose the label, so a masked field offered
+     * "Hide" and clicking it revealed the password and relabelled itself "Show".
+     *
+     * Renamed to `visible`, which is what the value means. A correct-looking name is
+     * not what prevents this; the name agreeing with the value is.
+     */
     var visible = input.type === 'text';
     button.setAttribute('aria-pressed', visible ? 'true' : 'false');
     button.textContent = visible ? 'Hide' : 'Show';
-    button.setAttribute('aria-label', (visible ? 'Hide' : 'Show') + ' ' + (input.getAttribute('data-toggle-label') || 'password'));
+    button.setAttribute('aria-label', (visible ? 'Hide' : 'Show') + ' ' + (input.getAttribute('data-field-label') || 'password'));
+  }
 
   function wirePasswordToggle(buttonId, inputId) {
     var button = query('#' + buttonId);

web/index.html

@@ -35,7 +35,19 @@
           <h2 id="signin-title">Sign in to your workspace</h2>
           <p>Use the email address your account was issued with, and your password. Access is determined by your organization membership and role.</p>
         </div>
-        <form id="login-form" class="stack-form" novalidate>
+        <!-- method="post" is load-bearing, not decoration.
+
+     A form with no method attribute defaults to GET, and a GET submission puts every
+     named field into the query string. If the script fails to load for any reason -
+     a syntax error, a truncated deploy, a blocked asset - the submit handler that
+     calls preventDefault() never runs, and the browser performs a native GET. That
+     happened: a syntax error I introduced left both password fields going into
+     `/?email=...&password=...`, which the server logged in plain text.
+
+     With method="post" the fallback is a POST body, which does not appear in the
+     URL, in the request line, or in referrer headers. The JavaScript still intercepts
+     and submits via fetch; this is what happens when the JavaScript does not. -->
+<form id="login-form" class="stack-form" method="post" action="/api/auth/login" novalidate>
           <label for="login-email">Email address</label>
           <input id="login-email" name="email" type="email" inputmode="email" autocomplete="username" autocapitalize="none" autocorrect="off" spellcheck="false" placeholder="you@example.com" required>
           <label for="login-password">Password</label>
@@ -74,7 +86,10 @@
             <li>Must differ from the temporary password you are replacing.</li>
           </ul>
         </div>
-        <form id="password-form" class="stack-form" novalidate>
+        <!-- method="post" for the same reason as the login form: a native fallback
+     submission of a password change must never reach the query string. The action is
+     the real endpoint so that even the fallback is the right request. -->
+<form id="password-form" class="stack-form" method="post" action="/api/auth/change-password" novalidate>
           <label for="current-password">Current temporary password</label>
           <div class="password-field">
             <input id="current-password" name="current_password" type="password" autocomplete="current-password" required>

web/reset-password.html

@@ -49,7 +49,11 @@
           <p id="reset-intro">Pick something you have not used here before. Every other signed-in device will be signed out when you do.</p>
         </div>
 
-        <form id="reset-form" class="stack-form" novalidate>
+        <!-- method="post": a form with no method defaults to GET, and a native
+         fallback submission would put password reset into the query string and into
+         the server log. No action: the script submits via fetch; this only
+         decides what happens when the script does not run. -->
+<form id="reset-form" class="stack-form" method="post" novalidate>
           <label for="new-password">New password</label>
           <div class="password-field">
             <input id="new-password" name="new_password" type="password" autocomplete="new-password" required>

web/settings.html

@@ -40,7 +40,11 @@
 
       <div class="auth-divider"><span>Password</span></div>
 
-      <form id="password-form" class="stack-form" novalidate>
+      <!-- method="post": a form with no method defaults to GET, and a native
+         fallback submission would put the password-change form into the query string and into
+         the server log. No action: the script submits via fetch; this only
+         decides what happens when the script does not run. -->
+<form id="password-form" class="stack-form" method="post" novalidate>
         <p class="field-hint">You need your current password to change it, so a signed-in session on its own is not enough. Every other device is signed out when you do.</p>
         <label for="current-password">Current password</label>
         <div class="password-field">

web/setup.html

@@ -40,7 +40,11 @@
           <p id="setup-intro">Paste the single-use setup code from the service log, then choose the administrator's email address and password.</p>
         </div>
 
-        <form id="setup-form" class="stack-form" novalidate>
+        <!-- method="post": a form with no method defaults to GET, and a native
+         fallback submission would put first-administrator setup into the query string and into
+         the server log. No action: the script submits via fetch; this only
+         decides what happens when the script does not run. -->
+<form id="setup-form" class="stack-form" method="post" novalidate>
           <label for="setup-token">Setup code</label>
           <input id="setup-token" name="token" type="text" autocomplete="off" autocapitalize="none" autocorrect="off" spellcheck="false" required>
           <p class="field-hint">Written to the service log when the service started. Shown once, and not stored.</p>

web/verify-email.html

@@ -34,7 +34,11 @@
         <div class="form-message" role="status" aria-live="polite">Confirming your address…</div>
       </div>
 
-      <form id="verify-form" class="stack-form" novalidate>
+      <!-- method="post": a form with no method defaults to GET, and a native
+         fallback submission would put email verification code into the query string and into
+         the server log. No action: the script submits via fetch; this only
+         decides what happens when the script does not run. -->
+<form id="verify-form" class="stack-form" method="post" novalidate>
         <label for="verify-token">Confirmation link or token</label>
         <input id="verify-token" name="token" type="text" autocomplete="off" autocapitalize="none" autocorrect="off" spellcheck="false" placeholder="Paste the link, or just the token" required>
         <p class="field-hint">The link from your email opens this page automatically. You can also paste just the token.</p>