From f2fbfca27379fcaf179e7a82832535ca255af168 Mon Sep 17 00:00:00 2001 From: zzstoatzz Date: Sun, 23 Aug 2026 00:27:30 -0500 Subject: [PATCH] oauth: make oauthError terminate the request; PAR scope regression in smoke MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit oauthError wrote the error body and returned success, so every helper that did `return oauthError(...)` — validateScope, requireClientAuth — returned normally to its `try` caller and the handler went on to overwrite the 400 with a 201/200. Observed live: /oauth/par answered 201 for an empty scope, `garbage:thing`, an unresolvable include, mixed transition+granular scopes, and scopes not registered in the client's metadata. requireClientAuth has the same shape on the token and refresh paths, so a failed private_key_jwt assertion was also overwritten. oauthError now returns error.HandledResponse after writing; the server loop treats that as "response already written" instead of a 500. The route switch moves into App.dispatch so the telemetry status stays honest. tools/smoke.sh hosts a public client's metadata on the PLC stub and asserts PAR answers 400 invalid_scope for unregistered, unresolvable and mixed scopes, and reaches the DPoP check for a valid one. Co-Authored-By: Claude Fable 5 --- src/atproto/oauth.zig | 5 +++++ src/http/server.zig | 11 +++++++++- tools/smoke.sh | 50 ++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 64 insertions(+), 2 deletions(-) diff --git a/src/atproto/oauth.zig b/src/atproto/oauth.zig index 0d5bf28..2f001f9 100644 --- a/src/atproto/oauth.zig +++ b/src/atproto/oauth.zig @@ -1259,10 +1259,15 @@ fn now() i64 { return clock.nowSeconds(); } +// writes the error response and then fails with HandledResponse, so a helper +// that does `return oauthError(...)` stops its caller too: before this, `try +// validateScope(...)` and `try requireClientAuth(...)` returned success after +// writing a 400 and the handler went on to overwrite it with a 201/200. fn oauthError(request: *http_api.Request, status: http.Status, err: []const u8, description: []const u8) !void { var buf: [512]u8 = undefined; const body = try std.fmt.bufPrint(&buf, "{{\"error\":{f},\"error_description\":{f}}}", .{ std.json.fmt(err, .{}), std.json.fmt(description, .{}) }); try http_api.json(request, status, body); + return error.HandledResponse; } fn handleAuthorizationDpopError(request: *http_api.Request, allocator: std.mem.Allocator, err: anyerror) !void { diff --git a/src/http/server.zig b/src/http/server.zig index 3f453f0..9b925cf 100644 --- a/src/http/server.zig +++ b/src/http/server.zig @@ -75,6 +75,16 @@ const App = struct { return; } + app.dispatch(route, request) catch |err| switch (err) { + // the handler already wrote its response (an oauth/xrpc error body); + // anything else is a genuine failure and becomes a 500 upstream. + error.HandledResponse => {}, + else => return err, + }; + handler_failed = false; + } + + fn dispatch(app: *App, route: router.Route, request: *httpz.Request) !void { switch (route) { .cors_preflight => try corsPreflight(request), .root => try landing.serve(request), @@ -162,7 +172,6 @@ const App = struct { .proxy_xrpc => unreachable, .not_found => try xrpcError(request, .not_found, "UnknownMethod", "Unknown XRPC method"), } - handler_failed = false; } }; diff --git a/tools/smoke.sh b/tools/smoke.sh index ff231ba..787e846 100755 --- a/tools/smoke.sh +++ b/tools/smoke.sh @@ -27,11 +27,35 @@ mkdir -p "$blob_root" zig build -# stand-in PLC directory so createAccount genesis never reaches plc.directory +# stand-in PLC directory so createAccount genesis never reaches plc.directory; +# it also hosts a public OAuth client's metadata so PAR can be exercised. plc_port="${ZDS_SMOKE_PLC_PORT:-2586}" +client_id="http://127.0.0.1:${plc_port}/oauth-client-metadata.json" python3 -c " +import json from http.server import BaseHTTPRequestHandler, HTTPServer +metadata = json.dumps({ + 'client_id': '$client_id', + 'client_name': 'smoke', + 'redirect_uris': ['http://127.0.0.1:$plc_port/callback'], + 'scope': 'atproto repo:*', + 'grant_types': ['authorization_code', 'refresh_token'], + 'response_types': ['code'], + 'token_endpoint_auth_method': 'none', + 'application_type': 'web', + 'dpop_bound_access_tokens': True, +}).encode() class Stub(BaseHTTPRequestHandler): + def do_GET(self): + if self.path != '/oauth-client-metadata.json': + self.send_response(404) + self.end_headers() + return + self.send_response(200) + self.send_header('content-type', 'application/json') + self.send_header('content-length', str(len(metadata))) + self.end_headers() + self.wfile.write(metadata) def do_POST(self): self.rfile.read(int(self.headers.get('content-length', 0))) self.send_response(200) @@ -109,6 +133,30 @@ test "$invalid_dpop_status" = "401" grep -qi '^www-authenticate: DPoP error="invalid_token", error_description="Token is invalid"' "$invalid_auth_headers" grep -qi '^dpop-nonce:' "$invalid_auth_headers" +# PAR scope validation must terminate the request: an unregistered scope is a +# 400 invalid_scope, never a 201 (a helper once wrote the error and returned +# success, and the handler overwrote it with a request_uri). +par_body="${TMPDIR:-/tmp}/zds-smoke-par.json" +par() { + curl -sS -o "$par_body" -w '%{http_code}' -X POST "$base/oauth/par" \ + --data-urlencode "client_id=$client_id" \ + --data-urlencode "redirect_uri=http://127.0.0.1:${plc_port}/callback" \ + --data-urlencode 'response_type=code' \ + --data-urlencode 'code_challenge=E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM' \ + --data-urlencode 'code_challenge_method=S256' \ + --data-urlencode 'state=x' \ + --data-urlencode "scope=$1" +} +test "$(par 'atproto garbage:thing')" = "400" +grep -q '"error":"invalid_scope"' "$par_body" +test "$(par 'atproto include:com.example.nope')" = "400" +grep -q '"error":"invalid_scope"' "$par_body" +test "$(par 'atproto transition:generic repo:*')" = "400" +grep -q '"error":"invalid_scope"' "$par_body" +# a valid scope gets past validation to the DPoP check (no proof was sent) +test "$(par 'atproto repo:*')" = "400" +grep -q '"error":"use_dpop_nonce"' "$par_body" + describe=$(curl -fsS "$base/xrpc/com.atproto.server.describeServer") printf '%s' "$describe" | grep -q '"inviteCodeRequired":true' missing_invite_status=$(curl -sS -o /tmp/zds-missing-invite.json -w '%{http_code}' -X POST "$base/xrpc/com.atproto.server.createAccount" \ -- 2.51.2