From 17191cdc5abc08abb9b754261ce789ec7ec6d01f Mon Sep 17 00:00:00 2001 From: zzstoatzz Date: Fri, 9 Oct 2026 02:07:11 -0500 Subject: [PATCH] space: log managing-app denials and honor ZDS_PLC_DIRECTORY A managing-app space answered UserNotAuthorized from getSpaceCredential without recording why. Resolution failure, a failed checkUserAccess request, and a non-authorizing answer all returned false silently, so a report of a denied requester whose managing app never saw the check could not be diagnosed from logs. Each of those now logs the space, managing app, endpoint, and error or upstream status. Spaces DID resolution always used plc.directory regardless of ZDS_PLC_DIRECTORY. It now follows the configured directory, which also lets the permissioned smoke stand in a managing app: the suite covers a grant, the service-auth claims and query the app receives, and denial on authorized=false, a missing field, 401, and 500. That path had no end-to-end coverage. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 7 ++++ src/atproto/space.zig | 54 +++++++++++++++++++++++++----- tools/smoke-permissioned.sh | 5 ++- tools/smoke-space-access.py | 65 ++++++++++++++++++++++++++++++++++++- 4 files changed, 120 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ec7ace..3c549c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,13 @@ Reconstructed from git history for everything up to `v0.1.1`; kept by hand from matching the reference PDS. Clients such as Bridgy that call `app.bsky.feed.getAuthorFeed` on the PDS previously got `404 UnknownMethod`. The caller's session is still required. +- **fix**: a managing-app space that refuses a requester now logs why: the + managing app could not be resolved, the `checkUserAccess` request failed, or + the app answered with a status other than an authorized `200`. These denials + were silent, so `UserNotAuthorized` from `getSpaceCredential` could not be + told apart from an unreachable app. +- **fix**: DID resolution for Spaces honors `ZDS_PLC_DIRECTORY`; it previously + always used `https://plc.directory`. - **build**: the container build fetches dependencies with backoff before compiling, like CI. - **change**: the homepage and api reference headers drop the glass pill and diff --git a/src/atproto/space.zig b/src/atproto/space.zig index 0617422..e4149c1 100644 --- a/src/atproto/space.zig +++ b/src/atproto/space.zig @@ -10,6 +10,7 @@ const clock = @import("../core/clock.zig"); const auth = @import("../auth/tokens.zig"); const config = @import("../core/config.zig"); const http_api = @import("../http/api.zig"); +const log = @import("../core/log.zig"); const cbor_json = @import("../internal/cbor_json.zig"); const permissioned = @import("../internal/permissioned_data.zig"); const client_attestation = @import("../internal/client_attestation.zig"); @@ -1102,7 +1103,7 @@ fn signingKeyForDidKid(allocator: std.mem.Allocator, did: []const u8, kid: []con return allocator.dupe(u8, did_key["did:key:".len..]); } const parsed = zat.Did.parse(did) orelse return error.InvalidDid; - var resolver = zat.DidResolver.init(store.currentIo(), allocator); + var resolver = didResolver(allocator); defer resolver.deinit(); var doc = try resolver.resolve(parsed); defer doc.deinit(); @@ -1174,7 +1175,7 @@ fn serviceAudienceMatchesPds(aud: []const u8) bool { fn spaceHostEndpointForDid(allocator: std.mem.Allocator, did: []const u8) ![]const u8 { const parsed = zat.Did.parse(did) orelse return error.InvalidDid; - var resolver = zat.DidResolver.init(store.currentIo(), allocator); + var resolver = didResolver(allocator); defer resolver.deinit(); var doc = try resolver.resolve(parsed); defer doc.deinit(); @@ -1186,12 +1187,18 @@ fn spaceHostEndpointForDid(allocator: std.mem.Allocator, did: []const u8) ![]con return allocator.dupe(u8, doc.pdsEndpoint() orelse return error.MissingSpaceHostEndpoint); } +fn didResolver(allocator: std.mem.Allocator) zat.DidResolver { + var resolver = zat.DidResolver.init(store.currentIo(), allocator); + resolver.plc_url = config.plcDirectory(); + return resolver; +} + fn serviceEndpointForId(allocator: std.mem.Allocator, service_id: []const u8) ![]const u8 { const fragment_index = std.mem.indexOfScalar(u8, service_id, '#'); const did_text = if (fragment_index) |idx| service_id[0..idx] else service_id; const fragment = if (fragment_index) |idx| service_id[idx..] else null; const did = zat.Did.parse(did_text) orelse return error.InvalidServiceId; - var resolver = zat.DidResolver.init(store.currentIo(), allocator); + var resolver = didResolver(allocator); defer resolver.deinit(); var doc = try resolver.resolve(did); defer doc.deinit(); @@ -1237,10 +1244,19 @@ fn managingAppAllowsRequester( access: store.SpaceAccess, client_id: ?[]const u8, ) !bool { - const managing_app = (if (access == .read) space.read_managing_app else space.write_managing_app) orelse return false; - const endpoint = serviceEndpointForId(allocator, managing_app) catch return false; + const managing_app = (if (access == .read) space.read_managing_app else space.write_managing_app) orelse { + log.err("space managing app missing space={s} access={s}\n", .{ space.uri, @tagName(access) }); + return false; + }; + const endpoint = serviceEndpointForId(allocator, managing_app) catch |err| { + log.err("space managing app unresolved space={s} managing_app={s} err={s}\n", .{ space.uri, managing_app, @errorName(err) }); + return false; + }; const parsed = parseSpaceUri(space.uri) orelse return false; - const authority = (try store.findAccount(allocator, parsed.authority_did)) orelse return false; + const authority = (try store.findAccount(allocator, parsed.authority_did)) orelse { + log.err("space managing app check has no local authority space={s}\n", .{space.uri}); + return false; + }; var keypair = try store.signingKeypair(authority.did); const token = try auth.createServiceJwtWithKeypair( allocator, @@ -1260,10 +1276,20 @@ fn managingAppAllowsRequester( .accept = "application/json", .max_response_size = 16 * 1024, .on_redirect = .refuse, - }) catch return false; + }) catch |err| { + log.err("space managing app check failed space={s} managing_app={s} endpoint={s} user={s} err={s}\n", .{ space.uri, managing_app, endpoint, requester_did, @errorName(err) }); + return false; + }; defer result.deinit(allocator); - if (result.status != .ok) return false; - const response = std.json.parseFromSlice(std.json.Value, allocator, result.body, .{}) catch return false; + const authorized = result.status == .ok and managingAppAuthorized(allocator, result.body); + if (!authorized) { + log.info("space managing app denied space={s} managing_app={s} endpoint={s} user={s} access={s} status={d}\n", .{ space.uri, managing_app, endpoint, requester_did, @tagName(access), @backingInt(result.status) }); + } + return authorized; +} + +fn managingAppAuthorized(allocator: std.mem.Allocator, body: []const u8) bool { + const response = std.json.parseFromSlice(std.json.Value, allocator, body, .{}) catch return false; defer response.deinit(); return switch (response.value) { .object => |object| switch (object.get("authorized") orelse return false) { @@ -2035,6 +2061,16 @@ test "space access selects independent read and write policies" { try std.testing.expect(try spacePolicyAllowsRequester(a, space, owner.did, .write, null)); } +test "managing app grants only on an explicit authorized true" { + const a = std.testing.allocator; + try std.testing.expect(managingAppAuthorized(a, "{\"authorized\":true}")); + try std.testing.expect(!managingAppAuthorized(a, "{\"authorized\":false}")); + try std.testing.expect(!managingAppAuthorized(a, "{\"authorized\":\"true\"}")); + try std.testing.expect(!managingAppAuthorized(a, "{}")); + try std.testing.expect(!managingAppAuthorized(a, "[true]")); + try std.testing.expect(!managingAppAuthorized(a, "not json")); +} + test "managing app checks include access and omit client identity for writes" { var arena = std.heap.ArenaAllocator.init(std.testing.allocator); defer arena.deinit(); diff --git a/tools/smoke-permissioned.sh b/tools/smoke-permissioned.sh index 9a1f81e..ed70a88 100755 --- a/tools/smoke-permissioned.sh +++ b/tools/smoke-permissioned.sh @@ -7,6 +7,8 @@ port="${ZDS_PERMISSIONED_SMOKE_PORT:-2586}" db="${work}/zds-permissioned-smoke.sqlite3" blob_root="${work}/zds-permissioned-smoke-blobs" log="${work}/zds-permissioned-smoke.log" +# stand-in PLC directory and managing app, served by tools/smoke-space-access.py +stub_port="${ZDS_PERMISSIONED_SMOKE_STUB_PORT:-$((port + 1))}" base="http://127.0.0.1:${port}" jwt_secret="smoke-jwt-secret" space_uri="at://did:plc:permissionsmoke/space/fm.plyr.privateMedia/self" @@ -55,6 +57,7 @@ ZDS_PERMISSIONED_DATA=true ./zig-out/bin/zds \ --server-did did:web:localhost \ --handle-domains .test \ --admin-token permissioned-smoke-admin-token \ + --plc-directory "http://127.0.0.1:${stub_port}" \ >"$log" 2>&1 & server_pid=$! @@ -405,7 +408,7 @@ other_space_blob_status=$(curl -sS -o "$work/zds-space-other-blob.json" -w '%{ht test "$other_space_blob_status" = "404" grep -q '"error":"BlobNotFound"' "$work/zds-space-other-blob.json" -SMOKE_BASE="$base" SMOKE_TOKEN="$token" SMOKE_BOB_TOKEN="$bob_token" python3 tools/smoke-space-access.py +SMOKE_BASE="$base" SMOKE_TOKEN="$token" SMOKE_BOB_TOKEN="$bob_token" SMOKE_STUB_PORT="$stub_port" python3 tools/smoke-space-access.py if [ -n "${ZDS_SPACE_SDK_DIR:-}" ]; then SMOKE_BASE="$base" SMOKE_TOKEN="$token" SMOKE_BOB_TOKEN="$bob_token" node tools/smoke-space-interop.mjs diff --git a/tools/smoke-space-access.py b/tools/smoke-space-access.py index 43f6439..b5235d9 100644 --- a/tools/smoke-space-access.py +++ b/tools/smoke-space-access.py @@ -1,8 +1,11 @@ #!/usr/bin/env python3 """Exercise independent space access using the disposable smoke server/accounts.""" +import base64 +import http.server import json import os import subprocess +import threading import urllib.error import urllib.parse import urllib.request @@ -63,6 +66,67 @@ for index, (read, write) in enumerate(((True, False), (False, True), (False, Fal expect('com.atproto.space.getSpaceCredential', 200 if read else 403, token=delegation, body={'space': space}, headers=signature_headers) +# A managing-app space asks the app over service auth before granting anyone +# but the authority. The stub is both the PLC directory that resolves the app's +# DID and the app itself. +stub_port = int(os.environ['SMOKE_STUB_PORT']) +managing_app = 'did:plc:spacesmokeapp#smoke_app' +checks = [] +answer = {'status': 200, 'body': {'authorized': True}} + + +class ManagingApp(http.server.BaseHTTPRequestHandler): + def do_GET(self): + url = urllib.parse.urlsplit(self.path) + if url.path == '/did:plc:spacesmokeapp': + status, body = 200, {'id': 'did:plc:spacesmokeapp', 'service': [ + {'id': '#smoke_app', 'type': 'SmokeManagingApp', 'serviceEndpoint': f'http://127.0.0.1:{stub_port}'}]} + elif url.path == '/xrpc/com.atproto.simplespace.checkUserAccess': + token = self.headers.get('authorization', '').removeprefix('Bearer ') + claims = json.loads(base64.urlsafe_b64decode(token.split('.')[1] + '===')) + checks.append((dict(urllib.parse.parse_qsl(url.query)), claims)) + status, body = answer['status'], answer['body'] + else: + status, body = 404, {'message': 'not found'} + payload = json.dumps(body).encode() + self.send_response(status) + self.send_header('content-type', 'application/json') + self.send_header('content-length', str(len(payload))) + self.end_headers() + self.wfile.write(payload) + + def log_message(self, *args): + pass + + +stub = http.server.ThreadingHTTPServer(('127.0.0.1', stub_port), ManagingApp) +threading.Thread(target=stub.serve_forever, daemon=True).start() +app_policy = {'$type': 'com.atproto.simplespace.defs#managingAppPolicy', 'managingApp': managing_app} +managed = expect('com.atproto.simplespace.createSpace', 200, body={ + **create, 'spaceType': 'test.access.managed', 'readPolicy': app_policy, 'writePolicy': app_policy})['uri'] + + +def managed_credential(expected): + token = expect('com.atproto.space.getDelegationToken', 200, token=member, params={'space': managed})['token'] + proof = subprocess.check_output(['./zig-out/bin/zds-space-signature', 'Bearer ' + token], text=True, stderr=subprocess.STDOUT) + return expect('com.atproto.space.getSpaceCredential', expected, token=token, body={'space': managed}, + headers=dict(line.split(': ', 1) for line in proof.strip().splitlines())) + + +managed_credential(200) +params, claims = checks[-1] +assert params == {'space': managed, 'user': member_did, 'access': 'read'}, params +assert (claims['iss'], claims['aud'], claims['lxm']) == ( + 'did:plc:permissionsmoke', managing_app, 'com.atproto.simplespace.checkUserAccess'), claims +for denial in ({'status': 200, 'body': {'authorized': False}}, {'status': 200, 'body': {}}, + {'status': 401, 'body': {'error': 'AuthenticationRequired'}}, {'status': 500, 'body': {}}): + answer.update(denial) + asked = len(checks) + assert managed_credential(403)['error'] == 'UserNotAuthorized' + assert len(checks) == asked + 1 +stub.shutdown() + + expect('com.atproto.simplespace.updateSpace', 200, body={'space': space, 'readPolicy': public_policy}) view = expect('com.atproto.simplespace.getSpace', 200, params={'space': space}) assert view['readPolicy'] == public_policy and view['writePolicy'] == member_policy and 'policy' not in view @@ -83,7 +147,6 @@ credential = expect('com.atproto.space.getSpaceCredential', 200, token=delegatio body={'space': space}, headers=exchange)['credential'] expect('com.atproto.space.getSpaceCredential', 401, token=delegation, body={'space': space}, headers=exchange) -import base64 claims = json.loads(base64.urlsafe_b64decode(credential.split('.')[1] + '===')) assert claims['exp'] - claims['iat'] == 600 and claims['cnf']['kid'].startswith('did:key:') authority = 'did:plc:permissionsmoke' -- 2.51.2