Handle scope field as string or array in DynamicClientRegistrationResponse

[amirejaz]

Some authorization servers return the scope field as a space-delimited
string (per RFC 7591), while others return it as a JSON array of strings.
This caused decoding errors when the format didn’t match expectations.

This change introduces a custom ScopeList type with a flexible
UnmarshalJSON implementation that correctly handles:

• space-delimited strings (e.g. "openid profile email")
• JSON arrays (e.g. ["openid","profile","email"])
• explicit null values
• empty string values

This ensures consistent behavior and provides a unified []string
representation in Go, regardless of how the server encodes scopes.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 48.37%. Comparing base (4cfd83d) to head (84af70f).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2083      +/-   ##
==========================================
+ Coverage   48.27%   48.37%   +0.10%     
==========================================
  Files         238      238              
  Lines       30006    30034      +28     
==========================================
+ Hits        14484    14529      +45     
+ Misses      14423    14402      -21     
- Partials     1099     1103       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[JAORMX]

@claude please review this with a focus on security

[claude]

Claude finished @JAORMX's task —— View job

---

Security Review

• Read and analyze the changed file
• Review for security vulnerabilities
• Assess error handling and edge cases
• Provide comprehensive feedback

---

Summary

The implementation is generally secure with good defensive practices, but there are several areas that need attention for security hardening and robustness.

🔴 Critical Issues

1. Potential Denial of Service via Memory Exhaustion (pkg/auth/oauth/dynamic_registration.go:77)

The UnmarshalJSON implementation uses strings.Fields() which can cause excessive memory allocation with maliciously crafted input:

*s = strings.Fields(str) // split by spaces

Problem: A malicious authorization server could send a very long string with many spaces, causing strings.Fields() to allocate a large slice. While the HTTP response is limited to 1MB (line 226), a string filled with alternating characters and spaces could still generate thousands of allocations.

Recommendation: Add a maximum scope count check:

fields := strings.Fields(str)
const maxScopes = 100 // reasonable limit
if len(fields) > maxScopes {
    return fmt.Errorf("too many scopes: %d (max %d)", len(fields), maxScopes)
}
*s = fields

2. Information Disclosure in Error Messages (pkg/auth/oauth/dynamic_registration.go:93)

return fmt.Errorf("invalid scope format: %s", string(data))

Problem: This error message could leak the raw JSON data received from the authorization server, which might contain sensitive information or be excessively long. This could be logged or exposed to users.

Recommendation: Limit the error message length:

maxLen := 100
dataStr := string(data)
if len(dataStr) > maxLen {
    dataStr = dataStr[:maxLen] + "... (truncated)"
}
return fmt.Errorf("invalid scope format: %s", dataStr)

🟡 Medium Severity Issues

3. Missing Scope Validation (pkg/auth/oauth/dynamic_registration.go:62-93)

The UnmarshalJSON method doesn't validate that scopes follow the expected format per RFC 6749 Section 3.3:

scope-token = 1*( %x21 / %x23-5B / %x5D-7E )

Problem: Invalid characters in scopes (spaces, control characters, quotes, etc.) are not detected, which could cause issues downstream or potentially be exploited for injection attacks if scopes are used in logging, SQL queries, or shell commands elsewhere in the codebase.

Recommendation: Add scope validation:

func isValidScope(s string) bool {
    if s == "" {
        return false
    }
    for _, r := range s {
        // RFC 6749: %x21 / %x23-5B / %x5D-7E
        if r < 0x21 || r == 0x22 || r == 0x5C || r > 0x7E {
            return false
        }
    }
    return true
}

// In UnmarshalJSON:
for _, v := range arr {
    v = strings.TrimSpace(v)
    if v == "" {
        continue
    }
    if !isValidScope(v) {
        return fmt.Errorf("invalid scope value")
    }
    *s = append(*s, v)
}

4. Array Unmarshaling Can Bypass Trimming (pkg/auth/oauth/dynamic_registration.go:84-89)

When unmarshaling from an array, the code trims each element but doesn't validate for empty strings before trimming:

for _, v := range arr {
    if v = strings.TrimSpace(v); v != "" {
        *s = append(*s, v)
    }
}

Issue: This allows scopes like " " (only spaces) in the array to be silently dropped, which might mask data quality issues or inconsistent server behavior.

Recommendation: Consider logging a warning when empty/whitespace-only scopes are encountered, or return an error for strict validation.

🟢 Low Severity / Best Practices

5. Missing Tests for Security-Critical Edge Cases

The codecov report shows 0% coverage for the new ScopeList.UnmarshalJSON method. For security-critical parsing code, comprehensive testing is essential.

Missing test cases:

• Very long scope strings (near 1MB limit)
• Strings with unusual whitespace (tabs, newlines, multiple spaces)
• Strings with special characters that should be rejected
• Arrays with empty strings, whitespace-only strings
• Arrays with many elements (stress testing)
• Mixed valid/invalid scopes
• Malformed JSON that should fail gracefully
• Unicode characters in scopes
• Boundary conditions (exactly at limits)

6. Type Conversion Without Explicit Length Check (pkg/auth/oauth/dynamic_registration.go:64)

if string(data) == "null" {

Minor Issue: Converting data to string without checking length first. While this is safe in Go, it's wasteful for large payloads and the check could be done on the byte slice directly:

if len(data) == 4 && string(data) == "null" {

7. Inconsistent Null Handling (pkg/auth/oauth/dynamic_registration.go:64-66, 73-76)

The code treats both null and empty string ("") as nil, which is semantically different:

• null = field not provided
• "" = field provided but empty

Consideration: Depending on OAuth server semantics, these might need different handling. RFC 7591 doesn't specify this clearly, so current behavior might be acceptable, but document this decision.

✅ Good Security Practices Observed

1. HTTPS Enforcement: Registration endpoint validation requires HTTPS (line 134)
2. Response Size Limiting: 1MB limit on response body (line 226)
3. Timeout Configuration: HTTP client has proper timeouts (lines 200-204)
4. Content-Type Validation: Checks response content type (lines 220-223)
5. Required Field Validation: Validates client_id is present (lines 237-239)

Recommendations Summary

Must Fix:

1. Add maximum scope count limit to prevent DoS
2. Limit error message length to prevent information disclosure
3. Add comprehensive unit tests for ScopeList.UnmarshalJSON

Should Fix:
4. Add RFC 6749-compliant scope validation
5. Consider stricter handling of whitespace-only scopes

Nice to Have:
6. Optimize null check with length pre-check
7. Document null vs empty string handling decision

Test Coverage Action Items

Add tests in pkg/auth/oauth/dynamic_registration_test.go for:

func TestScopeList_UnmarshalJSON(t *testing.T) {
    // Test cases for: string format, array format, null, empty string,
    // whitespace handling, invalid formats, DoS attempts, etc.
}

---

Job Run | Branch: fix-scopes-type

[Copilot]

Pull Request Overview

Adds a custom ScopeList type with a flexible JSON unmarshaler so DynamicClientRegistrationResponse.scope can accept either a space-delimited string or a JSON string array (plus null/empty handling).

• Introduces ScopeList with custom UnmarshalJSON supporting string, array, null, and empty input forms
• Replaces Scopes []string with Scopes ScopeList in DynamicClientRegistrationResponse
• Adds normalization logic (trimming, filtering empties)

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[blkt]

Can be improved later if necessary, but I suggest using assert and require from testify to write tests.