Aug 26 - Enhance security 3

tests/js/test_psl.js

@@ -0,0 +1,127 @@
+/**
+ * tests/js/test_psl.js — run with:  node tests/js/test_psl.js
+ *
+ * Covers extension/shared/psl.js, which decides whether a stored credential
+ * belongs to the page being viewed (review finding #5).
+ *
+ * The old matcher in content.js compared hostnames by plain suffix:
+ *
+ *     h === host || h.endsWith("." + host) || host.endsWith("." + h)
+ *
+ * which treated evil.github.io and victim.github.io as the same site, and let
+ * an item saved for a bare TLD match every site under it. The attack cases
+ * below pin that shut; the Mozilla vectors verify the PSL algorithm itself.
+ */
+'use strict';
+
+const fs = require('fs');
+const path = require('path');
+const vm = require('vm');
+
+const PSL_PATH = path.join(__dirname, '..', '..', 'extension', 'shared', 'psl.js');
+
+// Load as a classic script in a bare context, the way a content script sees it.
+const ctx = vm.createContext({});
+vm.runInContext(fs.readFileSync(PSL_PATH, 'utf8'), ctx, { filename: 'psl.js' });
+const PkPsl = vm.runInContext('PkPsl', ctx);
+
+let failures = 0;
+
+function check(label, got, want) {
+  if (got !== want) {
+    failures++;
+    console.log(`FAIL  ${label}\n      got=${JSON.stringify(got)} want=${JSON.stringify(want)}`);
+  }
+}
+
+// ── Canonical vectors from Mozilla's PSL test suite ─────────────────────────
+// https://github.com/publicsuffix/list/blob/master/tests/test_psl.txt
+const MOZILLA_VECTORS = [
+  ['example.COM', 'example.com'], ['WwW.example.COM', 'example.com'],
+  ['example', null], ['b.example', 'b.example'], ['a.b.example', 'b.example'],
+  ['biz', null], ['domain.biz', 'domain.biz'], ['b.domain.biz', 'domain.biz'],
+  ['a.b.domain.biz', 'domain.biz'],
+  ['com', null], ['example.com', 'example.com'], ['b.example.com', 'example.com'],
+  ['a.b.example.com', 'example.com'], ['uk.com', null],
+  ['example.uk.com', 'example.uk.com'], ['b.example.uk.com', 'example.uk.com'],
+  ['a.b.example.uk.com', 'example.uk.com'], ['test.ac', 'test.ac'],
+  // TLD with only a wildcard rule
+  ['mm', null], ['c.mm', null], ['b.c.mm', 'b.c.mm'], ['a.b.c.mm', 'b.c.mm'],
+  // More complex TLD
+  ['jp', null], ['test.jp', 'test.jp'], ['www.test.jp', 'test.jp'],
+  ['ac.jp', null], ['test.ac.jp', 'test.ac.jp'], ['www.test.ac.jp', 'test.ac.jp'],
+  ['kyoto.jp', null], ['test.kyoto.jp', 'test.kyoto.jp'],
+  ['ide.kyoto.jp', null], ['b.ide.kyoto.jp', 'b.ide.kyoto.jp'],
+  ['a.b.ide.kyoto.jp', 'b.ide.kyoto.jp'],
+  ['c.kobe.jp', null], ['b.c.kobe.jp', 'b.c.kobe.jp'], ['a.b.c.kobe.jp', 'b.c.kobe.jp'],
+  ['city.kobe.jp', 'city.kobe.jp'], ['www.city.kobe.jp', 'city.kobe.jp'],
+  // Wildcard rule plus exceptions
+  ['ck', null], ['test.ck', null], ['b.test.ck', 'b.test.ck'],
+  ['a.b.test.ck', 'b.test.ck'], ['www.ck', 'www.ck'], ['www.www.ck', 'www.ck'],
+  ['us', null], ['test.us', 'test.us'], ['www.test.us', 'test.us'],
+  ['ak.us', null], ['test.ak.us', 'test.ak.us'], ['www.test.ak.us', 'test.ak.us'],
+  ['k12.ak.us', null], ['test.k12.ak.us', 'test.k12.ak.us'],
+  ['www.test.k12.ak.us', 'test.k12.ak.us'],
+];
+
+for (const [input, want] of MOZILLA_VECTORS) {
+  check(`getRegistrableDomain(${input})`, PkPsl.getRegistrableDomain(input), want);
+}
+
+// ── The PRIVATE section must be present ─────────────────────────────────────
+// These are the suffixes where an attacker can actually register a neighbouring
+// subdomain, so dropping the PRIVATE section would silently reopen the hole.
+check('github.io is a public suffix', PkPsl.getRegistrableDomain('github.io'), null);
+check('vercel.app is a public suffix', PkPsl.getRegistrableDomain('vercel.app'), null);
+check('user.github.io is registrable',
+      PkPsl.getRegistrableDomain('victim.github.io'), 'victim.github.io');
+
+// ── Attack cases: these must NOT be treated as the same site ────────────────
+const MUST_NOT_MATCH = [
+  ['evil.github.io', 'victim.github.io', 'siblings on github.io'],
+  ['attacker.vercel.app', 'realapp.vercel.app', 'siblings on vercel.app'],
+  ['evil.herokuapp.com', 'real.herokuapp.com', 'siblings on herokuapp.com'],
+  ['evil.co.uk', 'bank.co.uk', 'siblings under co.uk'],
+  ['anything.com', 'com', 'item saved for a bare TLD'],
+  ['login.evil.com', 'evil.com.attacker.net', 'suffix confusion'],
+  ['example.com.evil.net', 'example.com', 'apex embedded in an attacker host'],
+  ['192.168.1.10', '192.168.1.11', 'different IPs'],
+  ['github.io', 'victim.github.io', 'bare suffix vs a site under it'],
+];
+for (const [a, b, label] of MUST_NOT_MATCH) {
+  check(`isSameSite(${a}, ${b}) [${label}]`, PkPsl.isSameSite(a, b), false);
+}
+
+// ── Legitimate matches must keep working ────────────────────────────────────
+const MUST_MATCH = [
+  ['login.example.com', 'example.com', 'subdomain to apex'],
+  ['www.example.com', 'accounts.example.com', 'sibling subdomains'],
+  ['a.b.c.example.co.uk', 'example.co.uk', 'deep subdomain under an ICANN suffix'],
+  ['example.com', 'example.com', 'identical'],
+  ['WWW.Example.COM', 'example.com', 'case-insensitive'],
+  ['example.com.', 'example.com', 'trailing root dot'],
+  ['localhost', 'localhost', 'localhost falls back to exact equality'],
+  ['192.168.1.10', '192.168.1.10', 'IP falls back to exact equality'],
+  ['github.io', 'github.io', 'bare suffix matches itself exactly'],
+];
+for (const [a, b, label] of MUST_MATCH) {
+  check(`isSameSite(${a}, ${b}) [${label}]`, PkPsl.isSameSite(a, b), true);
+}
+
+// ── Malformed input must not throw ──────────────────────────────────────────
+for (const bad of [null, undefined, '', '.', '..', ' ', 'a..b']) {
+  try {
+    PkPsl.getRegistrableDomain(bad);
+    PkPsl.isSameSite(bad, 'example.com');
+  } catch (e) {
+    failures++;
+    console.log(`FAIL  threw on input ${JSON.stringify(bad)}: ${e.message}`);
+  }
+}
+
+const total = MOZILLA_VECTORS.length + MUST_NOT_MATCH.length + MUST_MATCH.length + 3;
+if (failures) {
+  console.log(`\n${failures} failure(s)`);
+  process.exit(1);
+}
+console.log(`OK: ${total} PSL assertions passed`);

tests/test_deploy_config.py

@@ -107,3 +107,100 @@ def test_unit_loads_the_gunicorn_config_file():
     assert 'gunicorn.conf.py' in UNIT, (
         'the unit no longer references gunicorn.conf.py, so its tuning is dead code'
     )
+
+
+def test_api_rate_limit_is_not_absurdly_tight():
+    """
+    api_limit was 60r/m — 1 req/s for the entire API. A vault page load fires
+    several /api/* calls, so normal use produced spurious 429s. The units are
+    easy to misread, which is exactly why this is pinned.
+    """
+    m = re.search(r'zone=api_limit:\S+\s+rate=(\d+)r/([sm]);', NGINX)
+    assert m, 'api_limit zone not found'
+    per_second = int(m.group(1)) / (1 if m.group(2) == 's' else 60)
+    assert per_second >= 5, (
+        f'api_limit is {per_second:.2f} req/s — too tight for normal vault use'
+    )
+
+
+def test_auth_rate_limit_stays_tight():
+    """The brute-force surface must NOT be widened along with api_limit."""
+    m = re.search(r'zone=auth_limit:\S+\s+rate=(\d+)r/([sm]);', NGINX)
+    assert m, 'auth_limit zone not found'
+    per_minute = int(m.group(1)) * (60 if m.group(2) == 's' else 1)
+    assert per_minute <= 60, f'auth_limit is {per_minute} req/min — too permissive'
+
+
+# -- Extension packaging -----------------------------------------------------
+
+EXT = ROOT / 'extension'
+
+
+@pytest.mark.parametrize('manifest_name',
+                         ['manifest.json', 'manifest.firefox.json'])
+def test_manifest_loads_psl_before_content_script(manifest_name):
+    """
+    content.js calls PkPsl at match time. If psl.js is missing from the manifest
+    the matcher silently falls back to exact-hostname equality, quietly losing
+    every subdomain match.
+    """
+    import json
+    manifest = json.loads((EXT / manifest_name).read_text(encoding='utf-8'))
+    blocks = [cs for cs in manifest.get('content_scripts', [])
+              if any('content/content.js' in f for f in cs['js'])]
+    assert blocks, f'{manifest_name} has no content.js block'
+    for cs in blocks:
+        assert 'shared/psl.js' in cs['js'], f'{manifest_name}: psl.js not loaded'
+        assert cs['js'].index('shared/psl.js') < cs['js'].index('content/content.js'), \
+            f'{manifest_name}: psl.js must load BEFORE content.js'
+
+
+def test_popup_html_loads_psl():
+    # Compare the parsed <script src> order, not raw string positions — the
+    # surrounding comments mention these filenames too.
+    html = (EXT / 'popup' / 'popup.html').read_text(encoding='utf-8')
+    srcs = re.findall(r'<script src="([^"]+)"', html)
+    assert '../shared/psl.js' in srcs, 'popup.html does not load psl.js'
+    assert srcs.index('../shared/psl.js') < srcs.index('popup.js'), \
+        f'psl.js must load before popup.js, got {srcs}'
+
+
+def test_psl_includes_the_private_section():
+    """
+    The PRIVATE section (github.io, vercel.app, herokuapp.com) is where an
+    attacker can actually register a neighbouring subdomain. Regenerating the
+    list with only the ICANN section would silently reopen finding #5.
+    """
+    psl = (EXT / 'shared' / 'psl.js').read_text(encoding='utf-8')
+    for suffix in ('github.io', 'vercel.app', 'herokuapp.com'):
+        assert f'\n{suffix}\n' in psl, f'PSL is missing the private suffix {suffix}'
+
+
+def test_no_suffix_matching_remains_in_the_extension():
+    """
+    The endsWith("." + host) pattern is the finding-#5 bug. If it reappears,
+    credentials are being offered across public-suffix boundaries again.
+    """
+    for rel in ('content/content.js', 'popup/popup.js',
+                'background.js', 'background.firefox.js'):
+        src = (EXT / rel).read_text(encoding='utf-8')
+        code = '\n'.join(ln for ln in src.splitlines()
+                         if not ln.strip().startswith(('*', '//', '/*')))
+        for pattern in ('endsWith("." + host', 'endsWith(`.${host',
+                        'endsWith(`.${h}`)', "endsWith('.' + host"):
+            assert pattern not in code, f'{rel}: suffix matching is back ({pattern})'
+
+
+def test_background_scripts_load_psl():
+    """
+    The badge counts matching items and must use the same same-site rule.
+    Chrome pulls psl.js in via importScripts; Firefox via background.scripts.
+    """
+    import json
+    mv3 = (EXT / 'background.js').read_text(encoding='utf-8')
+    assert 'importScripts("shared/psl.js")' in mv3,         'background.js does not importScripts psl.js'
+
+    ff = json.loads((EXT / 'manifest.firefox.json').read_text(encoding='utf-8'))
+    scripts = ff['background']['scripts']
+    assert 'shared/psl.js' in scripts, 'firefox background does not load psl.js'
+    assert scripts.index('shared/psl.js') < scripts.index('background.firefox.js'),         'psl.js must load before background.firefox.js'

tests/test_sharing_expiry.py

@@ -0,0 +1,87 @@
+"""
+Regression tests for share expiry failing open (review finding #10).
+
+create_share parsed expires_days inside `try: ... except: pass`, so any value it
+could not parse silently became "never expires" — the opposite of what the user
+asked for, with no error to notice.
+"""
+from datetime import datetime, timedelta, timezone
+
+from tests.conftest import add_item, auth_headers, make_user
+
+
+def _share(client, token, item_id, recipient='friend@example.com', **extra):
+    body = {
+        'item_id': item_id,
+        'recipient_email': recipient,
+        'enc_data': 'ECDH-CT',
+        'iv': 'ECDH-IV',
+        'item_name': 'password',
+        'item_type': 'password',
+    }
+    body.update(extra)
+    return client.post('/api/sharing', headers=auth_headers(token), json=body)
+
+
+def test_valid_expiry_is_applied(client, app):
+    token, _ = make_user(client)
+    item_id = add_item(client, token)
+
+    res = _share(client, token, item_id, expires_days=7)
+    assert res.status_code == 201, res.get_json()
+
+    expires_at = res.get_json()['expires_at']
+    assert expires_at is not None
+    parsed = datetime.fromisoformat(expires_at)
+    expected = datetime.now(timezone.utc).replace(tzinfo=None) + timedelta(days=7)
+    assert abs((parsed - expected).total_seconds()) < 60
+
+
+def test_null_expiry_means_never(client, app):
+    token, _ = make_user(client)
+    item_id = add_item(client, token)
+
+    res = _share(client, token, item_id, expires_days=None)
+    assert res.status_code == 201
+    assert res.get_json()['expires_at'] is None
+
+
+def test_omitted_expiry_means_never(client, app):
+    token, _ = make_user(client)
+    item_id = add_item(client, token)
+
+    res = _share(client, token, item_id)
+    assert res.status_code == 201
+    assert res.get_json()['expires_at'] is None
+
+
+def test_unparseable_expiry_is_rejected_not_silently_dropped(client, app):
+    """The bug: 'seven' used to yield a share that never expires."""
+    token, _ = make_user(client)
+
+    for bad in ('seven', '7 days', {}, [], 'NaN', ''):
+        item_id = add_item(client, token)
+        res = _share(client, token, item_id, expires_days=bad)
+        assert res.status_code == 400, (
+            f'expires_days={bad!r} accepted; share would never expire'
+        )
+        assert 'expires_days' in res.get_json()['error']
+
+
+def test_out_of_range_expiry_is_rejected(client, app):
+    token, _ = make_user(client)
+
+    for bad in (-1, -30, 4000):
+        item_id = add_item(client, token)
+        res = _share(client, token, item_id, expires_days=bad)
+        assert res.status_code == 400, f'expires_days={bad!r} accepted'
+
+
+def test_zero_expiry_means_never(client, app):
+    """0 is 'no expiry', consistent with null — not 'expires immediately'."""
+    token, _ = make_user(client)
+    item_id = add_item(client, token)
+
+    res = _share(client, token, item_id, expires_days=0)
+    assert res.status_code == 201
+    assert res.get_json()['expires_at'] is None