Merge pull request #89 from MicroPyramid/dev

Dev

Author: ashwin31

.github/workflows/ci.yml

@@ -24,10 +24,10 @@ jobs:
         django: ["4.2", "5.2"]
 
     steps:
-      - uses: actions/checkout@v4
+      - uses: actions/checkout@v7
 
       - name: Install uv
-        uses: astral-sh/setup-uv@v5
+        uses: astral-sh/setup-uv@v10.0.0
         with:
           enable-cache: true
 
@@ -54,8 +54,8 @@ jobs:
     name: Coverage
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v4
-      - uses: astral-sh/setup-uv@v5
+      - uses: actions/checkout@v7
+      - uses: astral-sh/setup-uv@v10.0.0
         with:
           enable-cache: true
       - run: uv run coverage run --source=django_mfa test_runner.py
@@ -65,8 +65,8 @@ jobs:
     name: Docs
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v4
-      - uses: astral-sh/setup-uv@v5
+      - uses: actions/checkout@v7
+      - uses: astral-sh/setup-uv@v10.0.0
         with:
           enable-cache: true
 
@@ -149,8 +149,8 @@ jobs:
     name: Build and install-smoke
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v4
-      - uses: astral-sh/setup-uv@v5
+      - uses: actions/checkout@v7
+      - uses: astral-sh/setup-uv@v10.0.0
         with:
           enable-cache: true
 
@@ -180,7 +180,7 @@ jobs:
         working-directory: /tmp
         run: /tmp/smoke/bin/python "$GITHUB_WORKSPACE/.github/scripts/smoke_installed_wheel.py"
 
-      - uses: actions/upload-artifact@v4
+      - uses: actions/upload-artifact@v7
         with:
           name: distributions
           path: dist/

.github/workflows/publish.yml

@@ -67,12 +67,13 @@ jobs:
           - python-version: "3.13"
             django: "5.2"
     steps:
-      - uses: actions/checkout@v4
+      - uses: actions/checkout@v7
 
       - name: Install uv
-        uses: astral-sh/setup-uv@v5
+        uses: astral-sh/setup-uv@v10.0.0
         with:
-          enable-cache: true
+          # No cache anywhere in this workflow -- see the build job below.
+          enable-cache: false
 
       - name: Run the test suite
         run: >
@@ -86,12 +87,28 @@ jobs:
     needs: test
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v4
+      - uses: actions/checkout@v7
 
       - name: Install uv
-        uses: astral-sh/setup-uv@v5
+        uses: astral-sh/setup-uv@v10.0.0
         with:
-          enable-cache: true
+          # Deliberately off, and not just for speed reasons.
+          #
+          # The GitHub Actions cache is writable from less-trusted contexts
+          # (a pull_request run can poison an entry a later run restores), so
+          # a cache is an input to whatever this job builds -- and what this
+          # job builds is what gets published to PyPI under the project's
+          # name. setup-uv v10 made exactly this its headline change,
+          # disabling caching for release, tag-push, pull_request_target and
+          # workflow_run events when `enable-cache: auto`.
+          #
+          # `auto` is not enough here: this workflow's normal entry point is
+          # workflow_dispatch on a tag, which is not in that list. And an
+          # explicit `true` skips the check entirely -- setup-uv reads it as
+          # `return enableCacheInput === "true"`, before any event test. Only
+          # an explicit `false` is actually off. ci.yml still caches; it
+          # publishes nothing.
+          enable-cache: false
 
       - name: Refuse to publish if the tag and the packaged version disagree
         # Nothing else connects the two. hatchling reads the version from
@@ -141,7 +158,7 @@ jobs:
         run: /tmp/smoke/bin/python "$GITHUB_WORKSPACE/.github/scripts/smoke_installed_wheel.py"
 
       - name: Upload the distributions for the publish job
-        uses: actions/upload-artifact@v4
+        uses: actions/upload-artifact@v7
         with:
           name: distributions
           path: dist/
@@ -161,7 +178,7 @@ jobs:
       - name: Download the distributions built above
         # Deliberately not a rebuild: what gets uploaded is byte-for-byte the
         # artifact the install-smoke actually exercised.
-        uses: actions/download-artifact@v4
+        uses: actions/download-artifact@v8
         with:
           name: distributions
           path: dist/

.github/workflows/tag-release.yml

@@ -41,15 +41,15 @@ jobs:
     name: Tag and release if the version changed
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v4
+      - uses: actions/checkout@v7
         with:
           # Existing tags are how this decides whether the version was already
           # released, and a shallow clone fetches none of them -- it would
           # re-release every version, every time.
           fetch-depth: 0
 
       - name: Install uv
-        uses: astral-sh/setup-uv@v5
+        uses: astral-sh/setup-uv@v10.0.0
         with:
           enable-cache: true
 

django_mfa/adapters/recovery_codes.py

@@ -4,6 +4,7 @@
 
 from django.contrib.auth.hashers import check_password, make_password
 
+from django_mfa.atomic import update_data
 from django_mfa.models import Authenticator
 from django_mfa.registry import Adapter
 from django_mfa.utils import strings_equal
@@ -52,17 +53,29 @@ def complete_verify(self, request, user, data):
         if auth is None:
             return False
         submitted = data.get("code", "")
-        used = set(auth.data.get("used", []))
-        plaintext = auth.data.get("migrated_plaintext", False)
 
-        for index, stored in enumerate(auth.data.get("codes", [])):
-            if index in used:
-                continue
-            matched = (strings_equal(stored, submitted) if plaintext
-                       else check_password(submitted, stored))
-            if matched:
-                auth.data["used"] = sorted(used | {index})
-                auth.save(update_fields=["data"])
-                auth.record_usage()
-                return True
-        return False
+        def spend(current):
+            """Match and mark one code, against the blob as committed.
+
+            Deliberately does the matching *inside* the compare-and-set
+            rather than before it: the `used` set this consults has to be the
+            one the write is conditioned on, or a code another request spent
+            microseconds ago still reads as unspent here. Returning None
+            declines the write, which update_data() reports as a failed
+            verification.
+            """
+            used = set(current.get("used", []))
+            plaintext = current.get("migrated_plaintext", False)
+            for index, stored in enumerate(current.get("codes", [])):
+                if index in used:
+                    continue
+                matched = (strings_equal(stored, submitted) if plaintext
+                           else check_password(submitted, stored))
+                if matched:
+                    return {**current, "used": sorted(used | {index})}
+            return None
+
+        if not update_data(auth, spend):
+            return False
+        auth.record_usage()
+        return True

django_mfa/adapters/totp.py

@@ -1,9 +1,9 @@
 import base64
-import codecs
-import random
 import re
+import secrets
 
 from django_mfa import totp as totp_mod
+from django_mfa.atomic import update_data
 from django_mfa.conf import settings as mfa_settings
 from django_mfa.crypto import decrypt, encrypt
 from django_mfa.models import Authenticator
@@ -18,10 +18,26 @@
 TOTP_VALID_WINDOW = 1
 
 
+#: RFC 4226 R6: "The algorithm MUST use a strong shared secret. The length of
+#: the shared secret MUST be at least 128 bits. This document RECOMMENDs a
+#: shared secret length of 160 bits." 20 bytes also base32-encodes to exactly
+#: 32 characters with no '=' padding, which matters: OTP.byte_secret() re-pads
+#: self.secret in place and provisioning_uri() embeds the secret verbatim in
+#: the otpauth:// query string, so a padded secret would put '=' mid-URI.
+SECRET_BYTES = 20
+
+
 def generate_secret():
-    raw = codecs.decode(codecs.encode(f"{random.getrandbits(80):020x}"),
-                        "hex_codec")
-    return base64.b32encode(raw).decode("utf-8")
+    """Return a fresh base32 TOTP shared secret.
+
+    Uses `secrets` (os.urandom) rather than the `random` module. This
+    previously drew from random.getrandbits(80): the Mersenne Twister is not
+    a CSPRNG -- its 19937-bit state is recoverable from 624 observed 32-bit
+    outputs, after which every subsequently issued secret is predictable --
+    and it is the process-global instance shared with all other application
+    code. 80 bits was also below the RFC 4226 floor.
+    """
+    return base64.b32encode(secrets.token_bytes(SECRET_BYTES)).decode("utf-8")
 
 
 class TOTPAdapter(Adapter):
@@ -51,8 +67,27 @@ def complete_verify(self, request, user, data):
         auth = self.get_instances(user).first()
         if auth is None:
             return False
-        if totp_mod.TOTP(decrypt(auth.data["secret"])).verify(
-                data.get("code", ""), valid_window=TOTP_VALID_WINDOW):
-            auth.record_usage()
-            return True
-        return False
+        counter = totp_mod.TOTP(decrypt(auth.data["secret"])).match(
+            data.get("code", ""), valid_window=TOTP_VALID_WINDOW)
+        if counter is None:
+            return False
+        # RFC 6238 section 5.2: an OTP must not be accepted twice. Rejecting
+        # any counter at or below the last accepted one closes both the
+        # straight replay of the same code and the replay of an *earlier*
+        # still-in-window code after a later one has been used -- with
+        # TOTP_VALID_WINDOW = 1 a code would otherwise stay replayable for
+        # about 90 seconds, which is ample for a shoulder-surfed or
+        # phished-then-forwarded code.
+        #
+        # The compare-and-set is what makes this hold under concurrency: two
+        # simultaneous POSTs of the same code both match here, and without it
+        # both would go on to succeed.
+        def spend(data):
+            if counter <= data.get("last_verified_counter", -1):
+                return None
+            return {**data, "last_verified_counter": counter}
+
+        if not update_data(auth, spend):
+            return False
+        auth.record_usage()
+        return True

django_mfa/adapters/webauthn.py

@@ -19,6 +19,7 @@
     PublicKeyCredentialUserEntity,
 )
 
+from django_mfa.atomic import update_data
 from django_mfa.conf import settings as mfa_settings
 from django_mfa.handles import user_handle_for
 from django_mfa.models import Authenticator
@@ -251,30 +252,48 @@ def complete_verify(self, request, user, data):
             # still propagate unchanged, since that is a distinct,
             # deliberate "reject this" signal, not a missing-row race.
             return False
-        stored = auth.data.get("sign_count", 0)
         # The return value of authenticate_complete() carries no counter at
         # all in fido2 2.2.1, so the new counter has to be recovered
         # independently by re-parsing the same response the client sent.
         # Confirmed empirically (task-14-report.md) that this equals the
         # authenticator's own idea of its counter.
         new_count = AuthenticationResponse.from_dict(
             credential).response.authenticator_data.counter
-        # Clone detection: a signature counter that fails to advance
-        # suggests the authenticator (or its key material) has been cloned
-        # and a second device is replaying/racing assertions. But counters
-        # are OPTIONAL in the WebAuthn spec -- many authenticators
-        # (notably Apple/iCloud passkeys) never implement one and always
-        # report 0. Treat "both stored and new are 0" as the legitimate
-        # counter-less case and accept it unconditionally; for every other
-        # case, the new counter must be strictly greater than the stored
-        # one or this assertion is rejected as a possible clone. Do NOT
-        # simplify this to a plain "new > stored" check -- that would lock
-        # out every user of a counter-less authenticator, which today is a
-        # very large share of them.
-        if not (new_count == 0 and stored == 0) and new_count <= stored:
-            raise ValueError("Authenticator sign count did not increase.")
 
-        auth.data["sign_count"] = new_count
-        auth.save(update_fields=["data"])
+        def advance(current):
+            """Clone-check against the committed counter, then advance it.
+
+            Runs inside the compare-and-set rather than before it, and for
+            the same reason clone detection exists at all: two racing
+            assertions from a cloned credential would otherwise both read the
+            pre-advance counter, both clear the check below, and the loser's
+            write would drag the stored counter *backwards* -- leaving the
+            clone undetected and every subsequent replay looking fresh.
+            """
+            stored = current.get("sign_count", 0)
+            # Clone detection: a signature counter that fails to advance
+            # suggests the authenticator (or its key material) has been
+            # cloned and a second device is replaying/racing assertions. But
+            # counters are OPTIONAL in the WebAuthn spec -- many
+            # authenticators (notably Apple/iCloud passkeys) never implement
+            # one and always report 0. Treat "both stored and new are 0" as
+            # the legitimate counter-less case and accept it
+            # unconditionally; for every other case, the new counter must be
+            # strictly greater than the stored one or this assertion is
+            # rejected as a possible clone. Do NOT simplify this to a plain
+            # "new > stored" check -- that would lock out every user of a
+            # counter-less authenticator, which today is a very large share
+            # of them.
+            #
+            # Raising (rather than returning None to decline) is deliberate:
+            # it propagates straight out of update_data() without retrying,
+            # keeping "this is a clone" a distinct, loud signal from "this
+            # assertion simply could not be honoured".
+            if not (new_count == 0 and stored == 0) and new_count <= stored:
+                raise ValueError("Authenticator sign count did not increase.")
+            return {**current, "sign_count": new_count}
+
+        if not update_data(auth, advance):
+            return False
         auth.record_usage()
         return True

django_mfa/admin.py

@@ -1,5 +1,56 @@
 from django.contrib import admin
+from django.contrib.auth import get_user_model
 
 from .models import Authenticator
 
-admin.site.register(Authenticator)
+
+@admin.register(Authenticator)
+class AuthenticatorAdmin(admin.ModelAdmin):
+    """Inspect-and-revoke only. Never exposes ``Authenticator.data``.
+
+    That blob holds the TOTP shared secret, the recovery-code hashes and the
+    WebAuthn credential. Under the default ModelAdmin it was rendered as an
+    editable form field, which made the admin a privilege-escalation surface:
+    any staff account with ``view_authenticator`` could read another user's
+    TOTP secret -- including a superuser's -- and generate valid codes for
+    them, and one with ``change_authenticator`` could overwrite it with a
+    secret of its own choosing. ``MFA_SECRET_ENCRYPTION_KEYS`` is not a
+    mitigation: django_mfa.crypto *signs*, it does not encrypt, and the
+    payload is plain base64 that recovers without any key (see
+    docs/security.md).
+
+    ``data`` is therefore absent from ``fields``, ``list_display`` and
+    ``get_search_fields()`` alike -- a searchable ``data`` would leak the
+    secret a character at a time even while never rendering it -- and adding
+    and editing are switched off outright, since nothing in this model is
+    meaningfully hand-editable and every write to it is a way to weaken
+    somebody's second factor.
+
+    Deleting is deliberately still allowed. Revoking a lost or stolen
+    authenticator on behalf of a locked-out user is the one legitimate
+    support operation here, and removing it would push operators towards
+    editing the database by hand.
+    """
+
+    list_display = ("user", "type", "name", "created_at", "last_used_at")
+    list_filter = ("type", "created_at", "last_used_at")
+    date_hierarchy = "created_at"
+    ordering = ("user", "type", "created_at")
+
+    #: Everything except `data`. Also the complete readonly set -- with
+    #: has_change_permission() off these render as an inspectable, read-only
+    #: detail page rather than a form.
+    fields = ("user", "type", "name", "created_at", "last_used_at")
+    readonly_fields = fields
+
+    def get_search_fields(self, request):
+        # Derived from USERNAME_FIELD rather than hardcoding "username":
+        # AUTH_USER_MODEL is swappable and a host project's user model need
+        # not have that field at all.
+        return ("name", f"user__{get_user_model().USERNAME_FIELD}")
+
+    def has_add_permission(self, request):
+        return False
+
+    def has_change_permission(self, request, obj=None):
+        return False