From 3419485aa75231201807be3fa119057db6605a2a Mon Sep 17 00:00:00 2001 From: umbra audio Date: Sat, 3 Oct 2026 20:06:05 -0700 Subject: [PATCH] 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 | 120 +++++++++++++++++++++++ web/assets/app.js | 14 ++- web/index.html | 19 +++- web/reset-password.html | 6 +- web/settings.html | 6 +- web/setup.html | 6 +- web/verify-email.html | 6 +- 7 files changed, 168 insertions(+), 9 deletions(-) create mode 100644 umbratrans_web/tests/test_form_safety.py diff --git a/umbratrans_web/tests/test_form_safety.py b/umbratrans_web/tests/test_form_safety.py new file mode 100644 index 0000000..4fcccf8 --- /dev/null +++ b/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"]*>", 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] \ No newline at end of file diff --git a/web/assets/app.js b/web/assets/app.js index 3d6db64..1238f86 100644 --- a/web/assets/app.js +++ b/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); diff --git a/web/index.html b/web/index.html index a0b203b..7872555 100644 --- a/web/index.html +++ b/web/index.html @@ -35,7 +35,19 @@

Sign in to your workspace

Use the email address your account was issued with, and your password. Access is determined by your organization membership and role.

-
+ + @@ -74,7 +86,10 @@
  • Must differ from the temporary password you are replacing.
    • - + +
    diff --git a/web/reset-password.html b/web/reset-password.html index 19f32d0..9d719d6 100644 --- a/web/reset-password.html +++ b/web/reset-password.html @@ -49,7 +49,11 @@

    Pick something you have not used here before. Every other signed-in device will be signed out when you do.

    - + +
    diff --git a/web/settings.html b/web/settings.html index f975160..723266b 100644 --- a/web/settings.html +++ b/web/settings.html @@ -40,7 +40,11 @@
    Password
    - + +

    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.

    diff --git a/web/setup.html b/web/setup.html index 30594ac..2b7596f 100644 --- a/web/setup.html +++ b/web/setup.html @@ -40,7 +40,11 @@

    Paste the single-use setup code from the service log, then choose the administrator's email address and password.

    - + +

    Written to the service log when the service started. Shown once, and not stored.

    diff --git a/web/verify-email.html b/web/verify-email.html index 60cc97c..58194cb 100644 --- a/web/verify-email.html +++ b/web/verify-email.html @@ -34,7 +34,11 @@
    Confirming your address…
    - + +

    The link from your email opens this page automatically. You can also paste just the token.