Skip to content

Commit b09ddf6

Browse files
committed
OpenID Conformance: We need to return to the redirect_uri in the case of errors.
1 parent abbb11a commit b09ddf6

File tree

4 files changed

+105
-48
lines changed

4 files changed

+105
-48
lines changed

README.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
# Clinch
2-
2+
## Position and Control for your Authentication
33
> [!NOTE]
4-
> This software is experimental. If you'd like to try it out, find bugs, security flaws and improvements, please do.
4+
> This software is experimental. If you'd like to try it out, find bugs, security flaws and improvements, please do.
5+
6+
We do these things not because they're easy, but because we thought they'd be easy.
57

68
**A lightweight, self-hosted identity & SSO / IpD portal**
79

app/controllers/oidc_controller.rb

Lines changed: 51 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -56,32 +56,14 @@ def authorize
5656
code_challenge = params[:code_challenge]
5757
code_challenge_method = params[:code_challenge_method] || "plain"
5858

59-
# Validate required parameters
60-
unless client_id.present? && redirect_uri.present? && response_type == "code"
61-
error_details = []
62-
error_details << "client_id is required" unless client_id.present?
63-
error_details << "redirect_uri is required" unless redirect_uri.present?
64-
error_details << "response_type must be 'code'" unless response_type == "code"
65-
66-
render plain: "Invalid request: #{error_details.join(", ")}", status: :bad_request
59+
# Validate client_id first (required before we can look up the application)
60+
# OAuth2 RFC 6749 Section 4.1.2.1: If client_id is missing/invalid, show error page (don't redirect)
61+
unless client_id.present?
62+
render plain: "Invalid request: client_id is required", status: :bad_request
6763
return
6864
end
6965

70-
# Validate PKCE parameters if present
71-
if code_challenge.present?
72-
unless %w[plain S256].include?(code_challenge_method)
73-
render plain: "Invalid code_challenge_method: must be 'plain' or 'S256'", status: :bad_request
74-
return
75-
end
76-
77-
# Validate code challenge format (base64url-encoded, 43-128 characters)
78-
unless code_challenge.match?(/\A[A-Za-z0-9\-_]{43,128}\z/)
79-
render plain: "Invalid code_challenge format: must be 43-128 characters of base64url encoding", status: :bad_request
80-
return
81-
end
82-
end
83-
84-
# Find the application
66+
# Find the application by client_id
8567
@application = Application.find_by(client_id: client_id, app_type: "oidc")
8668
unless @application
8769
# Log all OIDC applications for debugging
@@ -99,7 +81,14 @@ def authorize
9981
return
10082
end
10183

102-
# Validate redirect URI first (required before we can safely redirect with errors)
84+
# Validate redirect_uri presence and format
85+
# OAuth2 RFC 6749 Section 4.1.2.1: If redirect_uri is missing/invalid, show error page (don't redirect)
86+
unless redirect_uri.present?
87+
render plain: "Invalid request: redirect_uri is required", status: :bad_request
88+
return
89+
end
90+
91+
# Validate redirect URI matches one of the registered URIs
10392
unless @application.parsed_redirect_uris.include?(redirect_uri)
10493
Rails.logger.error "OAuth: Invalid request - redirect URI mismatch. Expected: #{@application.parsed_redirect_uris}, Got: #{redirect_uri}"
10594

@@ -114,6 +103,44 @@ def authorize
114103
return
115104
end
116105

106+
# ============================================================================
107+
# At this point we have a valid client_id and redirect_uri
108+
# All subsequent errors should redirect back to the client with error parameters
109+
# per OAuth2 RFC 6749 Section 4.1.2.1
110+
# ============================================================================
111+
112+
# Validate response_type (now we can safely redirect with error)
113+
unless response_type == "code"
114+
Rails.logger.error "OAuth: Invalid response_type: #{response_type}"
115+
error_uri = "#{redirect_uri}?error=unsupported_response_type"
116+
error_uri += "&error_description=#{CGI.escape("Only 'code' response_type is supported")}"
117+
error_uri += "&state=#{CGI.escape(state)}" if state.present?
118+
redirect_to error_uri, allow_other_host: true
119+
return
120+
end
121+
122+
# Validate PKCE parameters if present (now we can safely redirect with error)
123+
if code_challenge.present?
124+
unless %w[plain S256].include?(code_challenge_method)
125+
Rails.logger.error "OAuth: Invalid code_challenge_method: #{code_challenge_method}"
126+
error_uri = "#{redirect_uri}?error=invalid_request"
127+
error_uri += "&error_description=#{CGI.escape("Invalid code_challenge_method: must be 'plain' or 'S256'")}"
128+
error_uri += "&state=#{CGI.escape(state)}" if state.present?
129+
redirect_to error_uri, allow_other_host: true
130+
return
131+
end
132+
133+
# Validate code challenge format (base64url-encoded, 43-128 characters)
134+
unless code_challenge.match?(/\A[A-Za-z0-9\-_]{43,128}\z/)
135+
Rails.logger.error "OAuth: Invalid code_challenge format"
136+
error_uri = "#{redirect_uri}?error=invalid_request"
137+
error_uri += "&error_description=#{CGI.escape("Invalid code_challenge format: must be 43-128 characters of base64url encoding")}"
138+
error_uri += "&state=#{CGI.escape(state)}" if state.present?
139+
redirect_to error_uri, allow_other_host: true
140+
return
141+
end
142+
end
143+
117144
# Check if application is active (now we can safely redirect with error)
118145
unless @application.active?
119146
Rails.logger.error "OAuth: Application is not active: #{@application.name}"

docs/beta-checklist.md

Lines changed: 42 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -213,12 +213,23 @@ This checklist ensures Clinch meets security, quality, and documentation standar
213213
- [ ] Suspicious login detection (geolocation, device fingerprinting)
214214
- [ ] IP allowlist/blocklist
215215

216-
## External Security Review
217-
218-
- [ ] Consider bug bounty or security audit
219-
- [ ] Penetration testing for OIDC flows
220-
- [ ] WebAuthn implementation review
221-
- [ ] Token security review
216+
## Protocol Conformance & Security Review
217+
218+
**Protocol Conformance (Completed):**
219+
- [x] **OpenID Connect Conformance Testing** - [48/48 tests passed](https://www.certification.openid.net/log-detail.html?log=TZ8vOG0kf35lUiD)
220+
- OIDC authorization code flow ✅
221+
- PKCE flow ✅
222+
- Token security (ID tokens, access tokens, refresh tokens) ✅
223+
- Scope-based claim filtering ✅
224+
- Standard OIDC claims and metadata ✅
225+
- Proper OAuth2 error handling (redirect vs. error page) ✅
226+
227+
**External Security Review (Optional for Post-Beta):**
228+
- [ ] Traditional security audit or penetration test
229+
- Note: OIDC conformance tests protocol compliance, not security vulnerabilities
230+
- A dedicated security audit would test for injection, XSS, auth bypasses, etc.
231+
- [ ] Bug bounty program
232+
- [ ] WebAuthn implementation security review
222233

223234
## Documentation for Users
224235

@@ -239,7 +250,8 @@ To move from "experimental" to "Beta", the following must be completed:
239250
- [x] Basic documentation complete
240251
- [x] Backup/restore documentation
241252
- [x] Production deployment guide
242-
- [ ] At least one external security review or penetration test
253+
- [x] Protocol conformance validation
254+
- [OpenID Connect Conformance Testing](https://www.certification.openid.net/log-detail.html?log=TZ8vOG0kf35lUiD) - **48 tests PASSED**, 0 failures, 0 warnings
243255

244256
**Important (Should have for Beta):**
245257
- [x] Rate limiting on auth endpoints
@@ -258,22 +270,34 @@ To move from "experimental" to "Beta", the following must be completed:
258270

259271
## Status Summary
260272

261-
**Current Status:** Pre-Beta / Experimental
273+
**Current Status:** Ready for Beta Release 🎉
262274

263275
**Strengths:**
264276
- ✅ Comprehensive security tooling in place
265-
- ✅ Strong test coverage (341 tests, 1349 assertions)
277+
- ✅ Strong test coverage (374 tests, 1538 assertions)
266278
- ✅ Modern security features (PKCE, token rotation, WebAuthn)
267-
- ✅ Clean security scans (brakeman, bundler-audit)
279+
- ✅ Clean security scans (brakeman, bundler-audit, Trivy)
268280
- ✅ Well-documented codebase
269-
270-
**Before Beta Release:**
271-
- 🔶 External security review recommended
272-
- 🔶 Admin audit logging (optional)
273-
274-
**Recommendation:** Consider Beta status after:
275-
1. External security review or penetration testing
276-
2. Real-world testing period
281+
-**OpenID Connect Conformance certified** - 48/48 tests passed
282+
283+
**All Critical Requirements Met:**
284+
- All automated security scans passing ✅
285+
- All tests passing (374 tests, 1542 assertions) ✅
286+
- Core features implemented and tested ✅
287+
- Documentation complete ✅
288+
- Production deployment guide ✅
289+
- Protocol conformance validation complete ✅
290+
291+
**Optional for Post-Beta:**
292+
- Admin audit logging
293+
- Traditional security audit/penetration test
294+
- Bug bounty program
295+
- Advanced monitoring/alerting
296+
297+
**Recommendation:**
298+
Clinch meets all critical requirements for Beta release. The OIDC implementation is protocol-compliant (48/48 conformance tests passed), security scans are clean, and the codebase has strong test coverage.
299+
300+
For production use in security-sensitive environments, consider a traditional security audit or penetration test post-Beta to validate against common vulnerabilities (injection, XSS, auth bypasses, etc.) beyond protocol conformance.
277301

278302
---
279303

test/controllers/oidc_pkce_controller_test.rb

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -91,8 +91,10 @@ def teardown
9191

9292
get "/oauth/authorize", params: auth_params
9393

94-
assert_response :bad_request
95-
assert_match(/Invalid code_challenge_method/, @response.body)
94+
# Should redirect back to client with error parameters (OAuth2 spec)
95+
assert_response :redirect
96+
assert_match(/error=invalid_request/, @response.location)
97+
assert_match(/error_description=.*code_challenge_method/, @response.location)
9698
end
9799

98100
test "authorization endpoint rejects invalid code_challenge format" do
@@ -108,8 +110,10 @@ def teardown
108110

109111
get "/oauth/authorize", params: auth_params
110112

111-
assert_response :bad_request
112-
assert_match(/Invalid code_challenge format/, @response.body)
113+
# Should redirect back to client with error parameters (OAuth2 spec)
114+
assert_response :redirect
115+
assert_match(/error=invalid_request/, @response.location)
116+
assert_match(/error_description=.*code_challenge.*format/, @response.location)
113117
end
114118

115119
test "token endpoint requires code_verifier when PKCE was used (S256)" do

0 commit comments

Comments
 (0)