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.