fix: preserve directory subjects in factor recovery

Assistant: codex
Assistant-Model: gpt-6-astra
Assistant-Session: 01a12d13-27f2-7163-b8c0-f682af02772c

scripts/factor_recovery.py

@@ -2,7 +2,18 @@
 import hashlib,json,re
 
 ACTION='keycape.factor.recovery'
-class RecoveryError(Exception):pass
+class RecoveryError(Exception):
+ def __init__(self, code, field=None):
+  super().__init__(code)
+  self.field=field
+
+def validate_actor(value):
+ # The issuer's subject is an opaque identity (LLDAP uses a DN), not a login.
+ # Preserve its exact bytes for ticket/receipt binding; never normalize it.
+ if (not isinstance(value,str) or not value or len(value)>150
+     or not value.isascii() or not value.isprintable() or not value.strip()):
+  raise RecoveryError('invalid_actor',field='actor')
+ return value
 
 def fingerprint(snapshot):
  return hashlib.sha256(json.dumps(snapshot,sort_keys=True,separators=(',',':')).encode()).hexdigest()
@@ -13,9 +24,10 @@ def recover(store, request):
   return _recover(store,request)
 
 def _recover(store, request):
- for key in ('user','serial','realm','actor','reference'):
+ validate_actor(request.get('actor'))
+ for key in ('user','serial','realm','reference'):
   value=request.get(key)
-  if not isinstance(value,str) or not value or len(value)>200 or not re.fullmatch(r'[A-Za-z0-9_.@:/-]+',value):raise RecoveryError('invalid_request')
+  if not isinstance(value,str) or not value or len(value)>200 or not re.fullmatch(r'[A-Za-z0-9_.@:/-]+',value):raise RecoveryError('invalid_request',field=key)
  if request['realm']!=store.realm:raise RecoveryError('wrong_realm')
  snapshot=store.snapshot(request['user'],request['serial'])
  if snapshot is None:raise RecoveryError('factor_not_owned_by_target')

scripts/recovery_service.py

@@ -4,7 +4,21 @@ import hashlib
 import hmac
 import json
 import time
-from factor_recovery import ProviderStore, RecoveryError, recover, fingerprint
+from factor_recovery import ProviderStore, RecoveryError, recover, fingerprint, validate_actor
+
+
+def report_validation_failure(error):
+    # Keep provider logging disabled. This separate diagnostic contains only
+    # closed field names and codes, never claims, bodies, tokens or exceptions.
+    import sys
+    if str(error) not in {'invalid_request', 'invalid_actor'}:
+        return
+    field = error.field if error.field in {
+        'actor', 'user', 'reference', 'serial', 'realm', 'action',
+        'request', 'content_length', 'body',
+    } else 'request'
+    sys.stderr.write(json.dumps(dict(event='factor_recovery_validation_failed',
+                                    code=str(error), field=field))+'\n')
 
 
 def authorized_actor(claims, now=None):
@@ -17,10 +31,7 @@ def authorized_actor(claims, now=None):
             or not isinstance(assurance.get('at'), (int, float))
             or not 0 <= now - assurance['at'] <= 300):
         raise RecoveryError('fresh_platform_mfa_required')
-    subject = claims.get('sub')
-    if not isinstance(subject, str) or not subject or len(subject) > 150:
-        raise RecoveryError('invalid_actor')
-    return subject
+    return validate_actor(claims.get('sub'))
 
 
 class RecoveryService:
@@ -52,9 +63,9 @@ class RecoveryService:
             user, reference = body.get('user', ''), body.get('reference', '')
             # Validate the target and support reference before any directory query.
             import re
-            if any(not isinstance(v, str) or not re.fullmatch(r'[A-Za-z0-9_.@:/-]{1,150}', v)
-                   for v in (user, reference)):
-                raise RecoveryError('invalid_request')
+            for field, value in (('user', user), ('reference', reference)):
+                if not isinstance(value, str) or not re.fullmatch(r'[A-Za-z0-9_.@:/-]{1,150}', value):
+                    raise RecoveryError('invalid_request', field=field)
             factors = []
             for serial in self.store.serials(user):
                 request = dict(user=user, reference=reference, serial=serial,
@@ -68,7 +79,7 @@ class RecoveryService:
             import re
             reference = body.get('reference', '')
             if not isinstance(reference,str) or not re.fullmatch(r'[A-Za-z0-9_.@:/-]{1,150}',reference):
-                raise RecoveryError('invalid_request')
+                raise RecoveryError('invalid_request', field='reference')
             row = self.store.receipt(reference)
             if row is None:
                 return dict(success=True,status='not_found',reference=reference)
@@ -90,7 +101,7 @@ class RecoveryService:
                 raise RecoveryError('identity_verification_required')
             request = self.unticket(body.get('confirmation', ''), actor)
             return dict(recover(self.store, dict(request, apply=True, identity_verified=True)), success=True)
-        raise RecoveryError('invalid_request')
+        raise RecoveryError('invalid_request', field='action')
 
     def __call__(self, environ, start_response):
         if environ.get('PATH_INFO') == '/healthz' and environ.get('REQUEST_METHOD') == 'GET':
@@ -99,18 +110,25 @@ class RecoveryService:
         status = '200 OK'
         try:
             if environ.get('PATH_INFO') != '/recover' or environ.get('REQUEST_METHOD') != 'POST':
-                raise RecoveryError('invalid_request')
-            length = int(environ.get('CONTENT_LENGTH') or 0)
+                raise RecoveryError('invalid_request', field='request')
+            try:
+                length = int(environ.get('CONTENT_LENGTH') or 0)
+            except (ValueError, TypeError):
+                raise RecoveryError('invalid_request', field='content_length') from None
             if not 0 < length <= 16384:
-                raise RecoveryError('invalid_request')
+                raise RecoveryError('invalid_request', field='content_length')
             auth = environ.get('HTTP_AUTHORIZATION', '')
             if not auth.startswith('Bearer ') or len(auth) > 16384:
                 raise RecoveryError('fresh_platform_mfa_required')
-            body = json.loads(environ['wsgi.input'].read(length))
+            try:
+                body = json.loads(environ['wsgi.input'].read(length))
+            except (ValueError, UnicodeError):
+                raise RecoveryError('invalid_request', field='body') from None
             if not isinstance(body, dict):
-                raise RecoveryError('invalid_request')
+                raise RecoveryError('invalid_request', field='body')
             result = self.operation(auth[7:], body)
         except RecoveryError as exc:
+            report_validation_failure(exc)
             status = '403 Forbidden' if str(exc) == 'fresh_platform_mfa_required' else '409 Conflict'
             result = dict(success=False, failure=str(exc))
         except Exception:

scripts/test_recovery_service.py

@@ -1,6 +1,7 @@
 import io
 import json
 import unittest
+from contextlib import redirect_stderr
 from recovery_service import RecoveryService, authorized_actor
 from factor_recovery import RecoveryError
 from test_factor_recovery import Store
@@ -59,3 +60,66 @@ class RecoveryServiceTests(unittest.TestCase):
         self.service.operation('signed',dict(action='apply',confirmation=status['confirmation'],identity_verified=True))
         self.assertEqual('recovered',self.service.operation('signed',dict(action='status',reference='case-1'))['status'])
         self.assertEqual(1,self.store.writes)
+
+    def test_directory_subject_preview_apply_and_replay(self):
+        # LLDAP adapter returns entry.DN; OIDC emits that exact user.ID as sub.
+        subject='uid=platform-root,ou=people,dc=netkingdom,dc=local'
+        self.claims['sub']=subject
+        preview=self.preview()
+        self.assertEqual(subject,preview['actor'])
+        self.assertEqual(0,self.store.writes)
+        body=dict(action='apply',confirmation=preview['confirmation'],identity_verified=True)
+        self.assertTrue(self.service.operation('signed',body)['changes_applied'])
+        self.assertTrue(self.service.operation('signed',body)['replayed'])
+        self.assertEqual(subject,self.store.rows['case-1']['actor'])
+        self.assertEqual(1,self.store.writes)
+
+    def test_directory_subject_ticket_is_bound_to_exact_actor(self):
+        self.claims['sub']='uid=admin,ou=people,dc=example'
+        ticket=self.preview()['confirmation']
+        self.claims['sub']='uid=other,ou=people,dc=example'
+        with self.assertRaisesRegex(RecoveryError,'preview_expired_or_changed'):
+            self.service.operation('signed',dict(action='apply',confirmation=ticket,identity_verified=True))
+        self.assertEqual(0,self.store.writes)
+
+    def test_actor_validation_preserves_identity_and_rejects_controls(self):
+        from factor_recovery import validate_actor
+        subject='cn=Platform Root,ou=people,dc=example'
+        self.assertEqual(subject,validate_actor(subject))
+        for value in [None,{},'', ' ', 'a'*151, 'uid=admin\nsecret', 'admin\x00', 'admin\x7f', 'ü']:
+            with self.subTest(value=repr(value)):
+                with self.assertRaisesRegex(RecoveryError,'invalid_actor'):
+                    authorized_actor(dict(self.claims,sub=value),1000)
+        self.assertEqual(0,self.store.writes)
+
+    def test_wsgi_validation_diagnostics_never_contain_values(self):
+        secret='private-value-must-not-appear'
+        cases=[
+            (dict(action='preview',user=secret+'!',reference='case-1'),None,'user'),
+            (dict(action='preview',user='alice',reference=secret+'!'),None,'reference'),
+            (dict(action=secret),None,'action'),
+            ([secret],None,'body'),
+            (dict(action='preview',user='alice',reference='case-1'),secret,'content_length'),
+        ]
+        for body,length,field in cases:
+            data=json.dumps(body).encode();diagnostic=io.StringIO()
+            with redirect_stderr(diagnostic):
+                result=b''.join(self.service(dict(PATH_INFO='/recover',REQUEST_METHOD='POST',
+                    HTTP_AUTHORIZATION='Bearer '+secret,CONTENT_LENGTH=length or str(len(data)),
+                    **{'wsgi.input':io.BytesIO(data)}),lambda s,h:None))
+            record=json.loads(diagnostic.getvalue())
+            self.assertEqual(dict(event='factor_recovery_validation_failed',code='invalid_request',field=field),record)
+            self.assertNotIn(secret,diagnostic.getvalue()+result.decode())
+        self.assertEqual(0,self.store.writes)
+
+    def test_provider_field_failure_is_identifiable_without_serial_value(self):
+        self.store.serials=lambda user:['private-serial!']
+        data=json.dumps(dict(action='preview',user='alice',reference='case-1')).encode()
+        diagnostic=io.StringIO()
+        with redirect_stderr(diagnostic):
+            result=b''.join(self.service(dict(PATH_INFO='/recover',REQUEST_METHOD='POST',
+                HTTP_AUTHORIZATION='Bearer signed',CONTENT_LENGTH=str(len(data)),
+                **{'wsgi.input':io.BytesIO(data)}),lambda s,h:None))
+        self.assertEqual('serial',json.loads(diagnostic.getvalue())['field'])
+        self.assertNotIn('private-serial',diagnostic.getvalue()+result.decode())
+        self.assertEqual(0,self.store.writes)