diff --git a/CLAUDE.md b/CLAUDE.md index f7ae03b..9dd4011 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -53,6 +53,7 @@ passkeeper/ │ │ ├── emergency_access.py # State machine │ │ ├── recovery_challenge.py # Server-side recovery challenge (multi-worker safe) │ │ ├── webauthn_credential.py # Passkey / WebAuthn credentials (one row per key) +│ │ ├── login_attempt.py # Failed-login lockout scoped to (user, IP) │ │ └── audit_log.py │ ├── routes/ │ │ ├── auth.py # Register, login, MFA, logout, refresh, change-password, recovery @@ -120,6 +121,7 @@ passkeeper/ │ ├── test_sharing_expiry.py # expires_days fails closed │ ├── test_registration_privacy.py # register does not disclose account existence │ ├── test_emergency_visibility.py # grantor sees requests + retrievals +│ ├── test_login_lockout.py # per-IP lockout; no disclosure, no DoS │ ├── test_deploy_config.py # nginx/gunicorn/systemd/extension packaging guards │ └── js/ │ ├── test_psl.js # PSL same-site matching (node, run in CI) @@ -249,6 +251,7 @@ CREATE TABLE webauthn_credentials ( | `j0k1l2m3n4o5` | Add recovery_verifier (decouple recovery proof) | | `k1l2m3n4o5p6` | Add token_epoch (revoke sessions on pw change) | | `l2m3n4o5p6q7` | Add emergency vault retrieval tracking | +| `m3n4o5p6q7r8` | Add login_attempts (per-IP lockout) | --- @@ -261,6 +264,13 @@ CREATE TABLE webauthn_credentials ( - **Item name:** `enc_name`/`iv_name` in `vault_items`; server `name` column = item type only - **Shared item name:** `enc_name`/`iv_name` encrypted with ECDH shared key; server `item_name` = item type only - **Tags:** `plain.tags: string[]` inside `enc_data`; server never sees them +- **Login lockout:** scoped to (account, source IP) in `login_attempts`, NOT + global. A global counter made it a DoS primitive — anyone knowing an address + could lock the real owner out for 15 minutes, repeatedly. Every failure mode + (unknown account / wrong password / locked out) returns one identical 401 with + matching timing, so it discloses nothing. `users.failed_login_count` and + `locked_until` remain as an aggregate audit signal only; they no longer gate + authentication. - **Argon2id:** double-hashes `authHash` server-side; transparently rehashes on login if parameters are upgraded - **JWT:** HS256, 15 min access / 7 day refresh, JTI blacklisted on logout. Every token carries an `epoch` claim checked against `users.token_epoch`; @@ -647,6 +657,35 @@ is ever dispatched and the save-credentials banner never appeared. `_captureCooldown` (2 s) prevents two triggers double-prompting for one login. +### Insecure-page warning + +The extension keeps `http://*/*` permission deliberately: routers, NAS boxes, +printers and self-hosted panels are often reachable only over plain HTTP on the +LAN, and those are exactly the devices whose passwords get reused. + +`_isTrustworthyOrigin()` classifies the page. HTTPS, `file:`, `localhost`, +reserved TLDs (`.local` / `.lan` / `.home` / `.internal`) and RFC1918 / +loopback / link-local / RFC4193 addresses are accepted silently. Any other +`http://` origin gets a red warning row prepended to the suggestion dropdown. + +**IPv4 checks must match in FULL** (`$`-anchored). A prefix test like +`hostname.startsWith("127.")` also accepts the registrable +`127.0.0.1.evil.com`, which would silently suppress the warning on a hostile +site. Guarded by `tests/js/test_field_heuristics.js`. + +Filling is never automatic, so this warns rather than blocks — silently +offering nothing would just look like a broken extension. + +### Autologin without a `
` + +`_findSubmitControl(pwField)` locates the control that submits the login, +walking up to 5 ancestors when there is no ``. It skips invisible +elements, wrappers containing other inputs, labels over 40 chars, and anything +matching `_NEGATIVE_CONTROL` (cancel / reset / back / forgot / register / sign +up), then clicks it — `form.submit()` is only the last resort because it +bypasses site handlers entirely. Returns null when nothing is convincing; +leaving a filled form for the user beats clicking the wrong thing. + ### MutationObserver guard Inspects added/removed nodes — if all carry `__pk` prefix, returns early. Prevents re-decoration loops when the extension injects/removes its own UI. @@ -682,7 +721,7 @@ Audit log details **never** contain plaintext item names, shared item names, or - `#vault-list` ID must not be renamed — `vault.js` renders into it directly - `_validate_folder_id` must be called for any user-supplied `folder_id` before DB write - `verify_auth_token` must receive `user=user` at both `login` and `change_password` to enable Argon2 rehash -- APScheduler cleanup job handles `TokenBlacklist`, `RecoveryChallenge`, AND `TotpUsedCode`; guard with `os.environ.get('WERKZEUG_RUN_MAIN') == 'true'` in Flask debug mode to prevent double-start +- APScheduler cleanup job handles `TokenBlacklist`, `RecoveryChallenge`, `TotpUsedCode`, expired shares AND `LoginAttempt`; guard with `os.environ.get('WERKZEUG_RUN_MAIN') == 'true'` in Flask debug mode to prevent double-start - `password_changed_at` lives inside `plain` (encrypted) — never in the server schema - WebAuthn `attachment`: `"cross-platform"` for security keys; `"platform"` for device biometrics (default) - `enc_vault_is_legacy` check in `EmergencyAccess.to_dict()` is pure JSON inspection — no decryption @@ -690,6 +729,9 @@ Audit log details **never** contain plaintext item names, shared item names, or - `/register` must return the SAME body and status for new and existing addresses, and hash on both paths — returning early on duplicate reinstates a timing oracle - Never return `str(e)` from exception handlers — log with `_log.exception(...)` and return a generic user-facing message to avoid leaking DB schema details or query fragments - `extension/shared/psl.js` is GENERATED — never hand-edit; run `python scripts/update_psl.py`. It must load BEFORE content.js / popup.js / background.js in every manifest +- Host/IP allowlists must be `$`-anchored — `startsWith("127.")` also matches the attacker-registrable `127.0.0.1.evil.com` +- `escHtml` must escape `&`, `<`, `>`, `"` AND `'` in all three copies (vault.js, popup.js, content.js) — templates mix quote styles +- Login lockout lives in `login_attempts` keyed by (user, IP); never move it back to a global per-account counter - Never add `off` back to `NON_CRED_AC` in `content.js` — `tests/js/test_field_heuristics.js` fails the build if you do - Login detection must not assume a `` exists; route new capture triggers through `maybeCaptureCredentials()` - Never reintroduce `endsWith("." + host)` host matching anywhere in the extension — `tests/test_deploy_config.py` fails the build if it reappears @@ -711,7 +753,7 @@ Audit log details **never** contain plaintext item names, shared item names, or | `auth.py` | `auth.register` | New account | | `auth.py` | `auth.register_duplicate` | Register attempt on an existing address (no email in detail) | | `auth.py` | `auth.login` / `auth.login_failed` | Login success/fail | -| `auth.py` | `auth.account_locked` | Failed login lockout | +| `auth.py` | `auth.account_locked` / `auth.login_blocked` | Per-IP lockout triggered / hit | | `auth.py` | `auth.mfa_enable/disable/verify` | TOTP actions | | `auth.py` | `auth.mfa_backup_code_used` | Backup code login | | `auth.py` | `auth.mfa_backup_codes_regenerated` | Backup code regen | @@ -908,8 +950,12 @@ Features planned for future implementation. Ordered by priority within each cate - Share `expires_days` fails closed instead of silently meaning "never" - nginx `api_limit` corrected from 60r/m to 10r/s - Registration no longer discloses account existence (status, body and timing) +- Login lockout scoped per-IP: no account disclosure, no lock-out-the-owner DoS +- `escHtml` escapes single quotes; dead `User.check_password` removed +- Extension warns before filling on plaintext public pages; autologin works + without a `` - Emergency access: requests and vault retrievals are now visible to the grantor -- pytest suite (66 tests) + PSL node test + CI jobs; `gunicorn.conf.py`; +- pytest suite (77 tests) + 2 node test files + CI jobs; `gunicorn.conf.py`; systemd watchdog removed ### High priority — user-facing diff --git a/app/__init__.py b/app/__init__.py index 41323b0..790247f 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -114,6 +114,7 @@ def create_app(config_name: str = 'development') -> Flask: from .models.recovery_challenge import RecoveryChallenge from .models.totp_used_code import TotpUsedCode from .models.webauthn_credential import WebAuthnCredential + from .models.login_attempt import LoginAttempt @login_manager.user_loader def load_user(user_id): @@ -204,9 +205,11 @@ def create_app(config_name: str = 'development') -> Flask: from app.models.recovery_challenge import RecoveryChallenge from app.models.totp_used_code import TotpUsedCode from app.models.shared_item import SharedItem + from app.models.login_attempt import LoginAttempt TokenBlacklist.cleanup_expired() RecoveryChallenge.cleanup_expired() TotpUsedCode.cleanup_expired() + LoginAttempt.cleanup_expired() # Delete expired unaccepted shares. from datetime import datetime, timezone SharedItem.query.filter( diff --git a/app/models/login_attempt.py b/app/models/login_attempt.py new file mode 100644 index 0000000..889e645 --- /dev/null +++ b/app/models/login_attempt.py @@ -0,0 +1,112 @@ +from datetime import datetime, timedelta, timezone + +from sqlalchemy.dialects.mysql import INTEGER + +from app import db + + +class LoginAttempt(db.Model): + """ + Failed-login tracking scoped to (account, source IP). + + The lockout used to live on the users table as a single global counter, which + made it a denial-of-service primitive: anyone who knew an address could send + five wrong passwords and lock the real owner out for 15 minutes, repeatedly + and indefinitely. Locking someone out of their password manager is a serious + harm on its own — it can mean losing access to everything at the worst + possible moment — and it cost an attacker almost nothing. + + Scoping by IP means an attacker locks out only themselves. The victim signing + in from their own address is unaffected. A distributed attacker has to rotate + IPs, and each one is independently capped by Flask-Limiter (10/min on + /login) plus the Nginx auth_limit zone. + + users.failed_login_count / users.locked_until still exist and are still + maintained, but ONLY as an aggregate signal for the audit log and security + dashboard. They no longer gate authentication — enforcement is here. + """ + __tablename__ = 'login_attempts' + + id = db.Column(INTEGER(unsigned=True), autoincrement=True, primary_key=True) + user_id = db.Column( + INTEGER(unsigned=True), + db.ForeignKey('users.id', ondelete='CASCADE'), + nullable=False, + ) + # 45 chars covers IPv6; may be empty when the proxy supplies no address. + ip_address = db.Column(db.String(45), nullable=False, default='') + failed_count = db.Column(db.Integer, nullable=False, default=0, server_default='0') + locked_until = db.Column(db.DateTime, nullable=True) + updated_at = db.Column( + db.DateTime, + nullable=False, + default=lambda: datetime.now(timezone.utc).replace(tzinfo=None), + ) + + __table_args__ = ( + db.UniqueConstraint('user_id', 'ip_address', name='uq_login_attempt_user_ip'), + ) + + MAX_FAILED = 5 + LOCKOUT_MINUTES = 15 + # Rows older than this carry no information and are pruned by the scheduler. + RETENTION_HOURS = 24 + + @staticmethod + def _now(): + return datetime.now(timezone.utc).replace(tzinfo=None) + + @classmethod + def get(cls, user_id: int, ip_address: str): + return cls.query.filter_by( + user_id=user_id, ip_address=ip_address or '' + ).first() + + @classmethod + def is_locked(cls, user_id: int, ip_address: str) -> bool: + """True if this IP is currently locked out of this account.""" + row = cls.get(user_id, ip_address) + if not row or not row.locked_until: + return False + if row.locked_until > cls._now(): + return True + # Expired — reset so the next failure starts a fresh count. + row.failed_count = 0 + row.locked_until = None + row.updated_at = cls._now() + return False + + @classmethod + def record_failure(cls, user_id: int, ip_address: str) -> bool: + """ + Count a failed attempt. Returns True if this attempt triggered a lockout. + Caller commits. + """ + now = cls._now() + row = cls.get(user_id, ip_address) + if row is None: + row = cls(user_id=user_id, ip_address=ip_address or '', failed_count=0) + db.session.add(row) + + row.failed_count = (row.failed_count or 0) + 1 + row.updated_at = now + if row.failed_count >= cls.MAX_FAILED: + row.locked_until = now + timedelta(minutes=cls.LOCKOUT_MINUTES) + return True + return False + + @classmethod + def clear(cls, user_id: int, ip_address: str) -> None: + """Successful authentication — drop this IP's failure history.""" + row = cls.get(user_id, ip_address) + if row is not None: + db.session.delete(row) + + @classmethod + def cleanup_expired(cls) -> int: + """Delete rows untouched for RETENTION_HOURS. Called by the scheduler.""" + cutoff = cls._now() - timedelta(hours=cls.RETENTION_HOURS) + return cls.query.filter(cls.updated_at <= cutoff).delete() + + def __repr__(self): + return f'' diff --git a/app/models/user.py b/app/models/user.py index 167b54f..794c0a9 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -1,7 +1,5 @@ from datetime import datetime, timezone from flask_login import UserMixin -from argon2 import PasswordHasher -from argon2.exceptions import VerifyMismatchError, VerificationError, InvalidHashError from sqlalchemy.dialects.mysql import INTEGER from app import db @@ -70,12 +68,14 @@ class User(db.Model, UserMixin): folders = db.relationship('Folder', backref='owner', lazy='dynamic', cascade='all, delete-orphan') vault_items = db.relationship('VaultItem', backref='owner', lazy='dynamic', cascade='all, delete-orphan') - def check_password(self, auth_hash: str) -> bool: - ph = PasswordHasher() - try: - return ph.verify(self.master_hash, auth_hash) - except (VerifyMismatchError, VerificationError, InvalidHashError): - return False + # NOTE: there is intentionally no check_password() here. + # + # It existed, was called from nowhere, and used default Argon2 parameters + # with no rehash-on-login handling — so any caller that found it would have + # silently bypassed the transparent parameter upgrade in + # auth_service.verify_auth_token(). Verification goes through + # verify_auth_token(auth_hash, user.master_hash, user=user) so the stored + # hash is upgraded when ARGON2_* settings change. def __repr__(self): return f'' diff --git a/app/models/vault_item.py b/app/models/vault_item.py index af6ceb2..b306e14 100644 --- a/app/models/vault_item.py +++ b/app/models/vault_item.py @@ -23,9 +23,11 @@ class VaultItem(db.Model): user_id = db.Column(INTEGER(unsigned=True), db.ForeignKey('users.id', ondelete='CASCADE'), nullable=False) folder_id = db.Column(INTEGER(unsigned=True), db.ForeignKey('folders.id', ondelete='SET NULL'), nullable=True) item_type = db.Column(db.String(20), nullable=False, default=ItemType.PASSWORD.value) - # name is stored in plaintext for display in the vault list. - # All other sensitive fields (username, password, URL, notes, etc.) - # are inside enc_data and are encrypted client-side with AES-256-GCM. + # NOT the user-visible name — that lives encrypted in enc_name/iv_name below. + # This column holds the item TYPE string only (the same value as item_type), + # kept because the column is NOT NULL and predates enc_name. Writing a real + # item name here would hand the server plaintext the zero-knowledge model + # promises it never sees. name = db.Column(db.String(255), nullable=False) enc_data = db.Column(db.Text, nullable=False) # base64-encoded AES-256-GCM ciphertext iv = db.Column(db.String(64), nullable=False) # base64-encoded 12-byte GCM nonce diff --git a/app/routes/auth.py b/app/routes/auth.py index 7b6079a..0dbfd41 100644 --- a/app/routes/auth.py +++ b/app/routes/auth.py @@ -209,12 +209,9 @@ def register(): @auth_bp.route('/login', methods=['POST']) @limiter.limit('10 per minute') def login(): - from datetime import datetime, timezone, timedelta - from sqlalchemy.exc import OperationalError - - # Number of consecutive failures before a temporary lockout is applied. - MAX_FAILED_LOGINS = 5 - LOCKOUT_MINUTES = 15 + from datetime import datetime, timezone + from sqlalchemy.exc import OperationalError, ProgrammingError + from app.models.login_attempt import LoginAttempt data = request.get_json(silent=True) or {} email = (data.get('email') or '').strip().lower() @@ -225,73 +222,91 @@ def login(): if not email or not auth_hash: return jsonify({'error': 'Email and auth_hash are required'}), 400 + # ── One response for every failure mode ───────────────────────────────── + # + # Unknown account, wrong password, and locked-out must be indistinguishable. + # The lockout branch used to answer 429 "Account temporarily locked. Try + # again in N minute(s)", which confirmed the address had an account — the + # same disclosure /register was just fixed for. + # + # The trailing hint is shown for ALL of these, so it explains a lockout to + # the legitimate owner without revealing anything to someone probing. + def _reject(): + return jsonify({ + 'error': ( + 'Invalid email or password. If you have made several failed ' + 'attempts, wait a few minutes and try again.' + ) + }), 401 + + ip = client_ip() user = User.query.filter_by(email=email).first() - # Per-account lockout check. - # Guarded with try/except so that a deployment where the migration has not - # yet been run (columns missing) degrades gracefully instead of returning - # an HTML 500 page that breaks JSON parsing in the extension. - try: - if user and user.locked_until: - now = datetime.now(timezone.utc).replace(tzinfo=None) - if user.locked_until > now: - remaining = int((user.locked_until - now).total_seconds() // 60) + 1 - AuditLog.log( - user_id=user.id, - action='auth.login_blocked', - resource_type='user', - resource_id=user.id, - detail=f'Login blocked — account locked for {remaining} more minute(s)', - ip_address=client_ip(), - ) - db.session.commit() - return jsonify({ - 'error': f'Account temporarily locked. Try again in {remaining} minute(s).' - }), 429 - else: - # Lockout has expired — reset the counter. - user.failed_login_count = 0 - user.locked_until = None - except OperationalError: - # Columns do not exist yet — migration pending. Skip lockout check. - db.session.rollback() + # Lockout is scoped to (account, IP) — see app/models/login_attempt.py. + # Guarded so a deployment where the migration has not yet run degrades to + # "no lockout" rather than returning an HTML 500 that breaks JSON parsing + # in the extension. + locked = False + if user: + try: + locked = LoginAttempt.is_locked(user.id, ip) + db.session.commit() + except (OperationalError, ProgrammingError): + db.session.rollback() # table missing — migration pending + + if locked: + # Do the Argon2 work anyway. Returning early would make the locked + # branch measurably faster than a wrong password and reinstate the + # existence oracle through timing. + verify_auth_token(auth_hash, user.master_hash) + AuditLog.log( + user_id=user.id, + action='auth.login_blocked', + resource_type='user', + resource_id=user.id, + detail='Login blocked — this IP is temporarily locked out', + ip_address=ip, + ) + db.session.commit() + return _reject() if not user or not verify_auth_token(auth_hash, user.master_hash, user=user): if user: try: - user.failed_login_count = (user.failed_login_count or 0) + 1 - if user.failed_login_count >= MAX_FAILED_LOGINS: - user.locked_until = datetime.now(timezone.utc).replace(tzinfo=None) + timedelta(minutes=LOCKOUT_MINUTES) - AuditLog.log( - user_id=user.id, - action='auth.account_locked', - resource_type='user', - resource_id=user.id, - detail=f'Account locked for {LOCKOUT_MINUTES} minutes after {user.failed_login_count} failed attempts', - ip_address=client_ip(), - ) - else: - AuditLog.log( - user_id=user.id, - action='auth.login_failed', - resource_type='user', - resource_id=user.id, - detail=f'Failed login attempt — invalid password ({user.failed_login_count}/{MAX_FAILED_LOGINS})', - ip_address=client_ip(), - ) - db.session.commit() - except OperationalError: + triggered = LoginAttempt.record_failure(user.id, ip) + except (OperationalError, ProgrammingError): db.session.rollback() - AuditLog.log( - user_id=user.id, - action='auth.login_failed', - resource_type='user', - resource_id=user.id, - detail='Failed login attempt — invalid password', - ip_address=client_ip(), - ) - db.session.commit() - return jsonify({'error': 'Invalid email or password'}), 401 + triggered = False + + # users.failed_login_count / locked_until are kept up to date purely + # as an aggregate signal for the audit log and security dashboard. + # They no longer gate authentication. + try: + user.failed_login_count = (user.failed_login_count or 0) + 1 + except (OperationalError, ProgrammingError): + db.session.rollback() + + AuditLog.log( + user_id=user.id, + action='auth.account_locked' if triggered else 'auth.login_failed', + resource_type='user', + resource_id=user.id, + detail=( + f'This IP locked out for {LoginAttempt.LOCKOUT_MINUTES} minutes ' + f'after {LoginAttempt.MAX_FAILED} failed attempts' + if triggered else + 'Failed login attempt — invalid password' + ), + ip_address=ip, + ) + db.session.commit() + return _reject() + + # Successful authentication — clear this IP's failure history. + try: + LoginAttempt.clear(user.id, ip) + except (OperationalError, ProgrammingError): + db.session.rollback() # Successful authentication — reset lockout state. try: diff --git a/app/static/js/vault.js b/app/static/js/vault.js index 457311c..f0709e8 100644 --- a/app/static/js/vault.js +++ b/app/static/js/vault.js @@ -4640,11 +4640,15 @@ const Vault = (() => { } function escHtml(str) { - return String(str) + // " and ' are both required: templates in this file use a mix + // of double- and single-quoted attributes, and an unescaped quote of + // either kind lets injected text break out of an attribute. + return String(str ?? "") .replace(/&/g, "&") .replace(//g, ">") - .replace(/"/g, """); + .replace(/"/g, """) + .replace(/'/g, "'"); } function itemIcon(type) { diff --git a/extension/content/content.js b/extension/content/content.js index b5e18ae..eeffa9b 100644 --- a/extension/content/content.js +++ b/extension/content/content.js @@ -32,10 +32,17 @@ // ── Helpers ────────────────────────────────────────────────────────────────── function escHtml(str) { + // " and ' are both required: templates in this file use a mix + // of double- and single-quoted attributes, and an unescaped quote of + // either kind lets injected text break out of an attribute. + // This copy previously escaped neither, while injecting attacker-influenced + // values (site hostname, stored item names) into attributes. return String(str ?? "") .replace(/&/g, "&") .replace(//g, ">"); + .replace(/>/g, ">") + .replace(/"/g, """) + .replace(/'/g, "'"); } /** @@ -74,6 +81,69 @@ }; } + /** + * True when this page is safe enough to put a stored credential into. + * + * The extension holds http://*\/* permission on purpose: a great many devices + * that genuinely need a password manager — routers, NAS boxes, printers, + * self-hosted admin panels — are only reachable over plain HTTP on the local + * network, and dropping the permission would make PassKeeper useless exactly + * where people reuse weak passwords most. + * + * What is NOT acceptable is filling a credential into a plaintext page on the + * public internet, where anyone on the path can read it. Loopback and RFC1918 + * / RFC4193 / link-local addresses and .local names are treated as acceptable; + * every other http:// origin gets a warning in the dropdown before the user + * chooses an item. + */ + function _isTrustworthyOrigin() { + if (location.protocol === "https:" || location.protocol === "file:") return true; + var h = (location.hostname || "").toLowerCase().replace(/^\[|\]$/g, ""); + + if (h === "localhost" || h.endsWith(".localhost")) return true; + // Reserved TLDs that cannot be registered publicly. + if (/\.(local|lan|home|internal)$/.test(h)) return true; + // RFC4193 unique-local / RFC4291 link-local IPv6. + if (h === "::1") return true; + if (/^f[cd][0-9a-f]{2}:/i.test(h) || /^fe80:/i.test(h)) return true; + + // IPv4 must match in FULL. Prefix checks like h.startsWith("127.") also + // accept attacker-registrable names such as "127.0.0.1.evil.com", which + // would silently suppress the insecure-page warning on a hostile site. + var m = h.match(/^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/); + if (!m) return false; + var o = m.slice(1).map(Number); + if (o.some(function (n) { return n > 255; })) return false; + if (o[0] === 127) return true; // loopback + if (o[0] === 10) return true; // RFC1918 + if (o[0] === 192 && o[1] === 168) return true; // RFC1918 + if (o[0] === 172 && o[1] >= 16 && o[1] <= 31) return true; // RFC1918 + if (o[0] === 169 && o[1] === 254) return true; // link-local + return false; + } + + /** + * Prepend an unmissable warning to the dropdown on plaintext public pages. + * Deliberately a warning and not a block: filling is always user-initiated, + * and silently offering nothing would look like a broken extension. + */ + function _insecureWarningRow() { + if (_isTrustworthyOrigin()) return null; + var row = document.createElement("div"); + Object.assign(row.style, { + padding: "8px 12px", + background: "#fdecea", + color: "#b71c1c", + borderBottom: "1px solid #f5c6cb", + fontSize: "12px", + lineHeight: "1.35", + }); + row.textContent = + "⚠ This page is not encrypted (http://). A credential filled here " + + "can be read by anyone on the network."; + return row; + } + function visiblePasswordFields() { return Array.from( document.querySelectorAll('input[type="password"]'), @@ -231,24 +301,65 @@ el.style.outline = ""; }, 1500); }); - // Autologin: submit the form automatically after filling. + // Autologin: submit automatically after filling. if (autologin) { - const form = pwField.closest("form"); - if (form) { - setTimeout(function () { - // Prefer clicking a visible submit button so site-specific submit - // handlers (React, Vue, etc.) fire correctly. - var submitBtn = form.querySelector( - '[type="submit"]:not([disabled])', - ); - if (submitBtn) { - submitBtn.click(); - } else { - form.submit(); - } - }, 400); + setTimeout(function () { + var form = pwField.closest("form"); + // Prefer clicking a real control so site-specific handlers (React, Vue, + // inline onclick) fire — form.submit() bypasses them entirely. + var control = _findSubmitControl(pwField); + if (control) { + control.click(); + } else if (form) { + form.submit(); + } + }, 400); + } + } + + // Controls that look like submits but would discard the login instead. + var _NEGATIVE_CONTROL = /cancel|reset|back|close|forgot|register|sign\s*up|create/i; + + /** + * Find the control that submits the login containing `pwField`. + * + * Autologin previously required a and did nothing without one, so it + * silently never worked on the many login UIs built from plain divs (the ASUS + * router admin page submits with + *
Sign In
). + * + * Returns null when nothing convincing is found — better to leave the filled + * form for the user than to click the wrong thing. + */ + function _findSubmitControl(pwField) { + var scope = pwField.closest("form") || pwField.closest('[role="form"]'); + if (scope) { + var explicit = scope.querySelector('[type="submit"]:not([disabled])'); + if (explicit && isVisible(explicit)) return explicit; + } + + // No form (or no explicit submit in it): search progressively wider + // ancestors so the nearest plausible control wins. + var node = scope || pwField.parentElement; + for (var depth = 0; depth < 5 && node; depth++, node = node.parentElement) { + var candidates = node.querySelectorAll( + 'button, [role="button"], [onclick], input[type="submit"], ' + + 'input[type="button"], div, a', + ); + for (var i = 0; i < candidates.length; i++) { + var el = candidates[i]; + if (el === pwField || el.disabled) continue; + if (!_looksLikeSubmitControl(el)) continue; + if (!isVisible(el)) continue; + // Only leaf-ish controls — a wrapping div can carry a button class. + if (el.querySelector("input, button")) continue; + var label = (el.textContent || el.value || "").trim(); + if (label.length > 40) continue; + if (_NEGATIVE_CONTROL.test(label)) continue; + return el; } } + return null; } // ── Icon button (fixed-position, outside the DOM tree of the field) ─────────── @@ -318,6 +429,9 @@ overflow: "hidden", }); + var warning = _insecureWarningRow(); + if (warning) dropdown.appendChild(warning); + if (panel === "more") { buildMorePanel(dropdown, anchorField, pwField, freshItems, filterText); } else { diff --git a/extension/popup/popup.js b/extension/popup/popup.js index 3321384..12d3e1e 100644 --- a/extension/popup/popup.js +++ b/extension/popup/popup.js @@ -60,11 +60,15 @@ function hideError(elId) { } function escHtml(str) { + // " and ' are both required: templates in this file use a mix of + // double- and single-quoted attributes, and an unescaped quote of either + // kind lets injected text break out of an attribute. return String(str ?? "") .replace(/&/g, "&") .replace(//g, ">") - .replace(/"/g, """); + .replace(/"/g, """) + .replace(/'/g, "'"); } // Auto-clear clipboard 30 s after a sensitive copy. diff --git a/migrations/versions/m3n4o5p6q7r8_add_login_attempts.py b/migrations/versions/m3n4o5p6q7r8_add_login_attempts.py new file mode 100644 index 0000000..843ae8f --- /dev/null +++ b/migrations/versions/m3n4o5p6q7r8_add_login_attempts.py @@ -0,0 +1,52 @@ +"""add login_attempts table (per-IP lockout) + +Revision ID: m3n4o5p6q7r8 +Revises: l2m3n4o5p6q7 +Create Date: 2026-08-26 00:00:00.000000 + +Moves failed-login lockout from a global per-account counter to per (account, IP). + +The old design was a denial-of-service primitive: anyone who knew an email +address could send five wrong passwords and lock the real owner out for 15 +minutes, repeatedly and indefinitely, at near-zero cost. Locking someone out of +their password manager is a serious harm in itself. + +Scoping by IP means an attacker locks out only their own address. The legitimate +owner signing in from their own IP is unaffected, and a distributed attacker +still faces Flask-Limiter (10/min per IP on /login) plus the Nginx auth_limit +zone on every address they rotate through. + +users.failed_login_count / users.locked_until are left in place and still +maintained as an aggregate signal for the audit log, but no longer gate +authentication. +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects.mysql import INTEGER + + +revision = 'm3n4o5p6q7r8' +down_revision = 'l2m3n4o5p6q7' +branch_labels = None +depends_on = None + + +def upgrade(): + op.create_table( + 'login_attempts', + sa.Column('id', INTEGER(unsigned=True), autoincrement=True, primary_key=True), + sa.Column('user_id', INTEGER(unsigned=True), nullable=False), + sa.Column('ip_address', sa.String(45), nullable=False, server_default=''), + sa.Column('failed_count', sa.Integer, nullable=False, server_default='0'), + sa.Column('locked_until', sa.DateTime, nullable=True), + sa.Column('updated_at', sa.DateTime, nullable=False), + sa.ForeignKeyConstraint(['user_id'], ['users.id'], ondelete='CASCADE'), + sa.UniqueConstraint('user_id', 'ip_address', name='uq_login_attempt_user_ip'), + ) + # The cleanup job sweeps by updated_at. + op.create_index('ix_login_attempts_updated_at', 'login_attempts', ['updated_at']) + + +def downgrade(): + op.drop_index('ix_login_attempts_updated_at', table_name='login_attempts') + op.drop_table('login_attempts') diff --git a/tests/js/test_field_heuristics.js b/tests/js/test_field_heuristics.js index 633a5a9..cda7844 100644 --- a/tests/js/test_field_heuristics.js +++ b/tests/js/test_field_heuristics.js @@ -112,6 +112,64 @@ check('click fallback registered', /addEventListener\(\s*"click"/.test(SRC), tru check('keydown fallback registered', /addEventListener\(\s*"keydown"/.test(SRC), true); check('submit listener retained', /addEventListener\(\s*"submit"/.test(SRC), true); +// ── _isTrustworthyOrigin: where credentials may be filled ────────────────── +// Extracted from the real source and evaluated against a stubbed `location`, +// so this exercises the shipped function rather than a copy of its rules. +{ + const fnSrc = SRC.match( + /function _isTrustworthyOrigin\(\) \{[\s\S]*?\n \}/, + ); + if (!fnSrc) { + failures++; + console.log('FAIL could not extract _isTrustworthyOrigin from content.js'); + } else { + const make = new Function( + 'location', + `${fnSrc[0]}; return _isTrustworthyOrigin();`, + ); + const at = (protocol, hostname) => make({ protocol, hostname }); + + // HTTPS is always fine. + check('https is trustworthy', at('https:', 'example.com'), true); + + // Local devices over plain HTTP — routers, NAS, printers. Dropping + // http://*/* entirely would break exactly these. + for (const host of ['localhost', '127.0.0.1', '::1', 'router.local', + '10.0.0.1', '192.168.1.1', '172.16.5.4', '172.31.0.1', + '169.254.1.1', 'nas.lan', 'box.home']) { + check(`http://${host} is treated as local`, at('http:', host), true); + } + + // Plaintext on the public internet must warn. + for (const host of ['example.com', 'bank.co.uk', '8.8.8.8', + '172.15.0.1', '172.32.0.1', '11.0.0.1', + '192.169.1.1', 'evil-localhost.com', + 'localhost.evil.com', '127.0.0.1.evil.com']) { + check(`http://${host} is NOT trusted`, at('http:', host), false); + } + } +} + +// ── Autologin must not click a control that discards the login ──────────── +{ + const m = SRC.match(/var _NEGATIVE_CONTROL = (\/[^\n]+\/[a-z]*);/); + if (!m) { + failures++; + console.log('FAIL could not find _NEGATIVE_CONTROL in content.js'); + } else { + // eslint-disable-next-line no-eval + const NEG = eval(m[1]); + for (const label of ['Cancel', 'Reset', 'Go back', 'Forgot password?', + 'Register', 'Sign up', 'Create account']) { + check(`autologin skips "${label}"`, NEG.test(label), true); + } + for (const label of ['Sign In', 'Log in', 'Submit', 'Continue', 'OK']) { + check(`autologin allows "${label}"`, NEG.test(label), false); + } + } +} + +// ── Summary (must stay last so every block above is counted) ─────────────── if (failures) { console.log(`\n${failures} failure(s)`); process.exit(1); diff --git a/tests/test_login_lockout.py b/tests/test_login_lockout.py new file mode 100644 index 0000000..3f3a58b --- /dev/null +++ b/tests/test_login_lockout.py @@ -0,0 +1,168 @@ +""" +Regression tests for the account lockout (two related problems). + +1. Disclosure — a locked account answered 429 "Account temporarily locked. Try + again in N minute(s)", confirming the address had an account. That is the same + leak /register was fixed for, reachable by anyone willing to send five wrong + passwords. + +2. Denial of service — the lockout was global to the account, so anyone who knew + an address could lock the real owner out for 15 minutes, repeatedly and + indefinitely. Locking someone out of their password manager is a serious harm + on its own. + +Lockout is now scoped to (account, source IP): an attacker locks out only +themselves, and every failure mode returns one identical response. +""" +from app import db +from app.models.audit_log import AuditLog +from app.models.login_attempt import LoginAttempt +from app.models.user import User +from tests.conftest import make_user, register + +ATTACKER = '203.0.113.9' +OWNER = '198.51.100.4' + + +def _login(client, ip, email='user@example.com', auth_hash='AUTH-HASH-V1'): + # ProxyFix(x_for=1) makes the rightmost XFF hop the client address, which is + # what client_ip() and the lockout key off. + return client.post('/api/auth/login', + json={'email': email, 'auth_hash': auth_hash}, + headers={'X-Forwarded-For': ip}) + + +def _lock_out(client, ip, email='user@example.com'): + for _ in range(LoginAttempt.MAX_FAILED): + _login(client, ip, email, 'WRONG-HASH') + + +# ── Disclosure ────────────────────────────────────────────────────────────── + +def test_locked_response_matches_wrong_password(client, app): + make_user(client) + _lock_out(client, ATTACKER) + + locked = _login(client, ATTACKER, auth_hash='WRONG-HASH') + wrong_on_fresh_ip = _login(client, '203.0.113.77', auth_hash='WRONG-HASH') + + assert locked.status_code == wrong_on_fresh_ip.status_code == 401 + assert locked.get_json() == wrong_on_fresh_ip.get_json(), ( + 'the locked response is distinguishable, so it discloses the account' + ) + + +def test_locked_response_matches_unknown_account(client, app): + make_user(client) + _lock_out(client, ATTACKER) + + locked = _login(client, ATTACKER, auth_hash='WRONG-HASH') + unknown = _login(client, ATTACKER, 'nobody@example.com', 'WRONG-HASH') + + assert locked.status_code == unknown.status_code == 401 + assert locked.get_json() == unknown.get_json() + + +def test_response_never_names_the_lockout(client, app): + make_user(client) + _lock_out(client, ATTACKER) + body = _login(client, ATTACKER, auth_hash='WRONG-HASH').get_json() + text = ' '.join(str(v) for v in body.values()).lower() + + for leak in ('locked', 'lockout', 'minute(s) remaining', 'too many'): + assert leak not in text, f'response leaks lockout state via {leak!r}: {body}' + + +# ── Denial of service ─────────────────────────────────────────────────────── + +def test_attacker_cannot_lock_the_owner_out(client, app): + """The core DoS fix: the victim's own IP is untouched.""" + make_user(client) + _lock_out(client, ATTACKER) + + assert _login(client, ATTACKER, auth_hash='WRONG-HASH').status_code == 401 + res = _login(client, OWNER) + assert res.status_code == 200, 'the owner was locked out by someone else' + assert 'access_token' in res.get_json() + + +def test_lockout_actually_applies_to_the_offending_ip(client, app): + """The DoS fix must not have removed brute-force protection.""" + make_user(client) + _lock_out(client, ATTACKER) + + # Even the CORRECT password is refused from a locked-out IP. + assert _login(client, ATTACKER).status_code == 401 + + row = LoginAttempt.query.filter_by(ip_address=ATTACKER).first() + assert row is not None and row.locked_until is not None + + +def test_each_ip_gets_its_own_budget(client, app): + make_user(client) + _lock_out(client, ATTACKER) + + # A second IP is still four failures away from its own lockout. + for _ in range(LoginAttempt.MAX_FAILED - 1): + _login(client, '203.0.113.55', auth_hash='WRONG-HASH') + assert _login(client, '203.0.113.55').status_code == 200 + + assert LoginAttempt.query.filter_by(ip_address=ATTACKER).first().locked_until + + +def test_successful_login_clears_that_ips_history(client, app): + make_user(client) + for _ in range(LoginAttempt.MAX_FAILED - 1): + _login(client, OWNER, auth_hash='WRONG-HASH') + + assert _login(client, OWNER).status_code == 200 + assert LoginAttempt.query.filter_by(ip_address=OWNER).first() is None, ( + 'failure history survived a successful login, so the next typo locks out' + ) + + +# ── Bookkeeping ───────────────────────────────────────────────────────────── + +def test_lockout_is_audited(client, app): + make_user(client) + _lock_out(client, ATTACKER) + + entry = (AuditLog.query.filter_by(action='auth.account_locked') + .order_by(AuditLog.id.desc()).first()) + assert entry is not None + assert entry.ip_address == ATTACKER + + _login(client, ATTACKER, auth_hash='WRONG-HASH') + blocked = (AuditLog.query.filter_by(action='auth.login_blocked') + .order_by(AuditLog.id.desc()).first()) + assert blocked is not None, 'blocked attempts are not recorded' + + +def test_aggregate_counter_still_tracks_all_ips(client, app): + """users.failed_login_count no longer gates login but stays informative.""" + make_user(client) + _login(client, ATTACKER, auth_hash='WRONG-HASH') + _login(client, '203.0.113.55', auth_hash='WRONG-HASH') + + assert User.query.filter_by(email='user@example.com').first().failed_login_count == 2 + + +def test_cleanup_prunes_stale_rows(client, app): + from datetime import datetime, timedelta, timezone + + make_user(client) + _login(client, ATTACKER, auth_hash='WRONG-HASH') + row = LoginAttempt.query.filter_by(ip_address=ATTACKER).first() + row.updated_at = (datetime.now(timezone.utc).replace(tzinfo=None) + - timedelta(hours=LoginAttempt.RETENTION_HOURS + 1)) + db.session.commit() + + assert LoginAttempt.cleanup_expired() == 1 + db.session.commit() + assert LoginAttempt.query.filter_by(ip_address=ATTACKER).first() is None + + +def test_unknown_account_creates_no_rows(client, app): + """No user, nothing to track — must not be a way to grow the table.""" + _login(client, ATTACKER, 'nobody@example.com', 'WRONG-HASH') + assert LoginAttempt.query.count() == 0