Repository: bluesky-social/indigo

Issue:
OAuth client SDK: Improve callback URL error handling
If the user cancels the authorization (or something else goes wrong), the AS may pass an error message back to the client in the callback URL, in the `error` and (optionally) `error_description` paramaters, per https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1

Right now `ProcessCallback()` will generate a nondescript `fmt.Errorf("missing required query param")`

Discussion:

Solution patch:
diff --git a/atproto/auth/oauth/oauth.go b/atproto/auth/oauth/oauth.go
index 3e35c618..5c733e93 100644
--- a/atproto/auth/oauth/oauth.go
+++ b/atproto/auth/oauth/oauth.go
@@ -579,21 +579,51 @@ func (app *ClientApp) StartAuthFlow(ctx context.Context, identifier string) (str
 
 // High-level helper for completing auth flow: verifies callback query parameters against persisted auth request info, makes initial token request to the auth server, validates account identifier, and persists session data.
 func (app *ClientApp) ProcessCallback(ctx context.Context, params url.Values) (*ClientSessionData, error) {
+	// There are two callback response formats, for error and non-error conditions, each expecting different
+	// parameters.
+	//
+	// Error responses expect: state, error (and optionally: error_description, error_uri)
+	// Non-error responses expect: state, iss, code
 
 	state := params.Get("state")
-	authserverURL := params.Get("iss")
-	authCode := params.Get("code")
-	if state == "" || authserverURL == "" || authCode == "" {
-		return nil, fmt.Errorf("missing required query param")
+	if state == "" {
+		return nil, fmt.Errorf("missing state query param")
 	}
 
 	info, err := app.Store.GetAuthRequestInfo(ctx, state)
 	if err != nil {
 		return nil, fmt.Errorf("loading auth request info: %w", err)
 	}
+	// This check should never fail, but it guards against a faulty ClientAuthStore implementation
+	if info.State != state {
+		return nil, fmt.Errorf("callback state doesn't match request info")
+	}
+
+	// NOTE: A corresponding `state` is expected even under error conditions,
+	// hence we check error *after* checking state.
+	errorCode := params.Get("error")
+	if errorCode != "" {
+		var errorUri *syntax.URI
+		parsedUri, err := syntax.ParseURI(params.Get("error_uri"))
+		if err == nil {
+			errorUri = &parsedUri
+		}
+		return nil, &AuthRequestCallbackError{
+			ErrorCode:        errorCode,
+			ErrorDescription: params.Get("error_description"),
+			ErrorURI:         errorUri,
+		}
+	}
+
+	// If we reached here, there was no `error` and we can process the rest of the parameters
+	authserverURL := params.Get("iss")
+	authCode := params.Get("code")
+	if authserverURL == "" || authCode == "" {
+		return nil, fmt.Errorf("missing required query param")
+	}
 
-	if info.State != state || info.AuthServerURL != authserverURL {
-		return nil, fmt.Errorf("callback params don't match request info")
+	if info.AuthServerURL != authserverURL {
+		return nil, fmt.Errorf("callback iss doesn't match request info")
 	}
 
 	tokenResp, err := app.SendInitialTokenRequest(ctx, authCode, *info)
diff --git a/atproto/auth/oauth/types.go b/atproto/auth/oauth/types.go
index cea551c5..0d5f6cc7 100644
--- a/atproto/auth/oauth/types.go
+++ b/atproto/auth/oauth/types.go
@@ -407,3 +407,23 @@ type TokenResponse struct {
 	// Refresh token, for doing additional token requests to the auth server.
 	RefreshToken string `json:"refresh_token"`
 }
+
+// Returned by [ClientApp.ProcessCallback] if the AS signals an error in the redirect URL parameters, per rfc6749 section 4.1.2.1
+//
+// NOTE: This is untrusted data and should not be e.g. rendered to HTML without appropriate escaping
+type AuthRequestCallbackError struct {
+	ErrorCode        string
+	ErrorDescription string
+	ErrorURI         *syntax.URI
+}
+
+func (e *AuthRequestCallbackError) Error() string {
+	res := "OAuth request callback error: " + e.ErrorCode
+	if e.ErrorDescription != "" {
+		res += ": " + e.ErrorDescription
+	}
+	if e.ErrorURI != nil {
+		res += " (" + e.ErrorURI.String() + ")"
+	}
+	return res
+}
diff --git a/atproto/lexicon/language.go b/atproto/lexicon/language.go
index 0f547a27..4173b472 100644
--- a/atproto/lexicon/language.go
+++ b/atproto/lexicon/language.go
@@ -83,18 +83,22 @@ func (s *SchemaDef) SetBase(base string) {
 		}
 		s.Inner = v
 	case SchemaQuery:
-		for i, val := range v.Parameters.Properties {
-			val.SetBase(base)
-			v.Parameters.Properties[i] = val
+		if v.Parameters != nil {
+			for i, val := range v.Parameters.Properties {
+				val.SetBase(base)
+				v.Parameters.Properties[i] = val
+			}
 		}
 		if v.Output != nil && v.Output.Schema != nil {
 			v.Output.Schema.SetBase(base)
 		}
 		s.Inner = v
 	case SchemaProcedure:
-		for i, val := range v.Parameters.Properties {
-			val.SetBase(base)
-			v.Parameters.Properties[i] = val
+		if v.Parameters != nil {
+			for i, val := range v.Parameters.Properties {
+				val.SetBase(base)
+				v.Parameters.Properties[i] = val
+			}
 		}
 		if v.Input != nil && v.Input.Schema != nil {
 			v.Input.Schema.SetBase(base)
@@ -104,9 +108,11 @@ func (s *SchemaDef) SetBase(base string) {
 		}
 		s.Inner = v
 	case SchemaSubscription:
-		for i, val := range v.Parameters.Properties {
-			val.SetBase(base)
-			v.Parameters.Properties[i] = val
+		if v.Parameters != nil {
+			for i, val := range v.Parameters.Properties {
+				val.SetBase(base)
+				v.Parameters.Properties[i] = val
+			}
 		}
 		if v.Message != nil {
 			v.Message.Schema.SetBase(base)
@@ -326,8 +332,8 @@ func (s *SchemaRecord) CheckSchema() error {
 type SchemaQuery struct {
 	Type        string        `json:"type"` // "query"
 	Description *string       `json:"description,omitempty"`
-	Parameters  SchemaParams  `json:"parameters"`
-	Output      *SchemaBody   `json:"output"`
+	Parameters  *SchemaParams `json:"parameters"`       // optional
+	Output      *SchemaBody   `json:"output"`           // optional
 	Errors      []SchemaError `json:"errors,omitempty"` // optional
 }
 
@@ -337,18 +343,23 @@ func (s *SchemaQuery) CheckSchema() error {
 			return err
 		}
 	}
+	if s.Parameters != nil {
+		if err := s.Parameters.CheckSchema(); err != nil {
+			return err
+		}
+	}
 	for _, e := range s.Errors {
 		if err := e.CheckSchema(); err != nil {
 			return err
 		}
 	}
-	return s.Parameters.CheckSchema()
+	return nil
 }
 
 type SchemaProcedure struct {
 	Type        string        `json:"type"` // "procedure"
 	Description *string       `json:"description,omitempty"`
-	Parameters  SchemaParams  `json:"parameters"`
+	Parameters  *SchemaParams `json:"parameters"`       // optional
 	Output      *SchemaBody   `json:"output"`           // optional
 	Errors      []SchemaError `json:"errors,omitempty"` // optional
 	Input       *SchemaBody   `json:"input"`            // optional
@@ -365,18 +376,23 @@ func (s *SchemaProcedure) CheckSchema() error {
 			return err
 		}
 	}
+	if s.Parameters != nil {
+		if err := s.Parameters.CheckSchema(); err != nil {
+			return err
+		}
+	}
 	for _, e := range s.Errors {
 		if err := e.CheckSchema(); err != nil {
 			return err
 		}
 	}
-	return s.Parameters.CheckSchema()
+	return nil
 }
 
 type SchemaSubscription struct {
 	Type        string         `json:"type"` // "subscription"
 	Description *string        `json:"description,omitempty"`
-	Parameters  SchemaParams   `json:"parameters"`
+	Parameters  *SchemaParams  `json:"parameters"`        // optional
 	Message     *SchemaMessage `json:"message,omitempty"` // TODO(specs): is this really optional?
 }
 
@@ -386,7 +402,12 @@ func (s *SchemaSubscription) CheckSchema() error {
 			return err
 		}
 	}
-	return s.Parameters.CheckSchema()
+	if s.Parameters != nil {
+		if err := s.Parameters.CheckSchema(); err != nil {
+			return err
+		}
+	}
+	return nil
 }
 
 type SchemaPermissionSet struct {



Test patch:
diff --git a/atproto/lexicon/testdata/catalog/minimal-procedure.json b/atproto/lexicon/testdata/catalog/minimal-procedure.json
new file mode 100644
index 00000000..e94d627a
--- /dev/null
+++ b/atproto/lexicon/testdata/catalog/minimal-procedure.json
@@ -0,0 +1,16 @@
+{
+  "lexicon": 1,
+  "id": "example.lexicon.minimal.procedure",
+  "description": "demonstrates lexicon features for the procedure type",
+  "defs": {
+    "main": {
+      "type": "procedure",
+      "input": {
+        "encoding": "application/json",
+        "schema": {
+          "type": "object"
+        }
+      }
+    }
+  }
+}
diff --git a/atproto/lexicon/testdata/catalog/minimal-query.json b/atproto/lexicon/testdata/catalog/minimal-query.json
new file mode 100644
index 00000000..c15d8fd7
--- /dev/null
+++ b/atproto/lexicon/testdata/catalog/minimal-query.json
@@ -0,0 +1,12 @@
+{
+  "lexicon": 1,
+  "id": "example.lexicon.minimal.query",
+  "revision": 1,
+  "description": "exercizes many lexicon features for the query type",
+  "defs": {
+    "main": {
+      "type": "query",
+      "description": "a query type"
+    }
+  }
+}
diff --git a/atproto/lexicon/testdata/catalog/procedure.json b/atproto/lexicon/testdata/catalog/procedure.json
new file mode 100644
index 00000000..a64195b5
--- /dev/null
+++ b/atproto/lexicon/testdata/catalog/procedure.json
@@ -0,0 +1,78 @@
+{
+  "lexicon": 1,
+  "id": "example.lexicon.procedure",
+  "description": "demonstrates lexicon features for the procedure type",
+  "defs": {
+    "main": {
+      "type": "procedure",
+      "parameters": {
+        "type": "params",
+        "properties": {
+          "boolean": {
+            "type": "boolean",
+            "description": "field of type boolean"
+          },
+          "integer": {
+            "type": "integer",
+            "description": "field of type integer"
+          },
+          "string": {
+            "type": "string",
+            "description": "field of type string"
+          }
+        }
+      },
+      "input": {
+        "encoding": "application/json",
+        "schema": {
+          "type": "object",
+          "required": [
+            "preferences"
+          ],
+          "properties": {
+            "preferences": {
+              "type": "ref",
+              "ref": "app.bsky.actor.defs#preferences"
+            }
+          }
+        }
+      },
+      "output": {
+        "encoding": "application/json",
+        "schema": {
+          "type": "object",
+          "required": [],
+          "properties": {
+            "blob": {
+              "type": "blob",
+              "description": "field of type blob"
+            },
+            "unknown": {
+              "type": "unknown",
+              "description": "field of type unknown"
+            },
+            "array": {
+              "type": "array",
+              "description": "field of type array",
+              "items": {
+                "type": "integer"
+              }
+            },
+            "object": {
+              "type": "object",
+              "description": "field of type null",
+              "properties": {
+                "a": {
+                  "type": "integer"
+                },
+                "b": {
+                  "type": "integer"
+                }
+              }
+            }
+          }
+        }
+      }
+    }
+  }
+}
diff --git a/atproto/lexicon/testdata/catalog/record.json b/atproto/lexicon/testdata/catalog/record.json
index 164e7eed..33b9c4f3 100644
--- a/atproto/lexicon/testdata/catalog/record.json
+++ b/atproto/lexicon/testdata/catalog/record.json
@@ -2,7 +2,7 @@
   "lexicon": 1,
   "id": "example.lexicon.record",
   "revision": 1,
-  "description": "exercizes many lexicon features for the record type",
+  "description": "demonstrates lexicon features for the record type",
   "defs": {
     "main": {
       "type": "record",
diff --git a/atproto/lexicon/testdata/catalog/subscription.json b/atproto/lexicon/testdata/catalog/subscription.json
new file mode 100644
index 00000000..c0fd0738
--- /dev/null
+++ b/atproto/lexicon/testdata/catalog/subscription.json
@@ -0,0 +1,47 @@
+{
+  "lexicon": 1,
+  "id": "example.lexicon.subscription",
+  "description": "demonstrates lexicon features for the subscription type",
+  "defs": {
+    "main": {
+      "type": "subscription",
+      "parameters": {
+        "type": "params",
+        "properties": {
+          "cursor": {
+            "type": "integer",
+            "description": "start at the given sequence number"
+          }
+        }
+      },
+      "message": {
+        "schema": {
+          "type": "union",
+          "refs": ["#yo", "#info"]
+        }
+      },
+      "errors": [{ "name": "FutureCursor" }]
+    },
+    "yo": {
+      "type": "object",
+      "required": ["seq", "yo"],
+      "properties": {
+        "seq": { "type": "integer" },
+        "yo": { "type": "boolean" }
+      }
+    },
+    "info": {
+      "type": "object",
+      "required": ["name"],
+      "properties": {
+        "name": {
+          "type": "string",
+          "knownValues": ["OutdatedCursor"]
+        },
+        "message": {
+          "type": "string"
+        }
+      }
+    }
+  }
+}