feat: Add the OIDC authentication plugin.

[fengyily]

Summary

Brief description of changes.

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update
• Refactoring
• CI/CD improvement

Related Issues

Fixes #

Testing

• Unit tests added/updated
• Manual testing performed
• All existing tests pass

Checklist

• Code follows project style guidelines
• Self-review completed
• Documentation updated (if needed)
• No new warnings introduced

[claude]

🚨 Changes Requested - Found 5 critical security issues that must be addressed

Critical Security Issues

1. SECURITY: Hardcoded Secrets Committed to Repository

Severity: CRITICAL

examples/server_plugin/oidc/etc/config.toml:1-4

The config file contains hardcoded OAuth2 credentials:

• AUTH0_CLIENT_SECRET: JO2EM1fXTkaQB0bnXs_OPcRmjdpmTVoSuwAjJrH0LDwG4FRFT-xJ_ZCvmipgbELH
• AUTH0_CLIENT_ID: CF75aEpFxZ1AU71Z2R6KLrLcOU3JOZYS

Impact: These are real credentials for an Auth0 application that are now exposed publicly. Anyone can use these credentials to impersonate your application, potentially compromising user authentication.

Required Actions:

1. Immediately revoke these credentials in your Auth0 dashboard
2. Remove this file from the repository (including git history if already merged)
3. Use .env files (excluded via .gitignore) or environment variables for secrets
4. Provide a config.toml.example template file instead with placeholder values
5. Update documentation to explain how to configure secrets securely

2. SECURITY: Mixed Content Vulnerability (HTTP Resource on HTTPS Page)

Severity: HIGH

examples/server_plugin/oidc/templates/auth0home.html:3

The HTML template loads jQuery over HTTP instead of HTTPS:

<script src="http://code.jquery.com/jquery-3.1.0.min.js" type="text/javascript"></script>

Impact:

• On HTTPS pages, this will be blocked by browsers (mixed content policy)
• Allows man-in-the-middle attacks to inject malicious JavaScript
• In authentication context, this could compromise user credentials

Fix: Change to HTTPS:

<script src="https://code.jquery.com/jquery-3.1.0.min.js" type="text/javascript"></script>

3. SECURITY: Unrestricted CORS Policy

Severity: HIGH

examples/server_plugin/oidc/main.go:425-434

The CORS middleware reflects any origin from the request header without validation:

func corsMiddleware(ctx *gin.Context) {
    originResource := ctx.Request.Header.Get("Origin")
    if originResource != "" {
        ctx.Writer.Header.Set("Access-Control-Allow-Origin", originResource)
    }
    ctx.Next()
}

Impact: This allows any website to make authenticated requests to your authentication endpoints, potentially enabling:

• Cross-site request forgery (CSRF) attacks
• Credential theft
• Session hijacking

Fix: Implement an allowlist of trusted origins or use a more restrictive CORS policy:

allowedOrigins := []string{"https://nhp.opennhp.org", "https://acdemo.opennhp.org"}
// Validate origin before setting header

4. SECURITY: Missing State Validation in OAuth Flow

Severity: HIGH

examples/server_plugin/oidc/main.go:1437-1442

The OAuth state parameter validation is insufficient:

stateVal := sessionGet(ctx, "state")
stateStr, _ := stateVal.(string)
if ctx.Query("state") != stateStr {
    ctx.String(http.StatusOK, "{\"errMsg\": \"invalid authentication session\"}")
    return nil, fmt.Errorf("invalid authentication session")
}

Issues:

• Empty state values would pass validation (both empty == equal)
• No check that state was actually set in the session
• State is not cleared after use (allows replay attacks)

Fix: Add proper validation:

stateVal := sessionGet(ctx, "state")
if stateVal == nil {
    return nil, fmt.Errorf("no state in session")
}
stateStr, ok := stateVal.(string)
if !ok || stateStr == "" || ctx.Query("state") != stateStr {
    return nil, fmt.Errorf("invalid state")
}
// Clear state after validation to prevent replay
sessionSet(ctx, "state", "")

5. BUG: Race Condition in Session Helper Access

Severity: MEDIUM

examples/server_plugin/oidc/main.go:23-24

The global currentHelper variable is not thread-safe:

var currentHelper *plugins.HttpServerPluginHelper

func AuthWithHttp(...) {
    currentHelper = helper  // Race condition in concurrent requests

Impact: In concurrent HTTP requests, the currentHelper can be overwritten mid-request, causing one request to use another request's helper/session context. This could lead to:

• Session data leaking between users
• Authentication bypass
• Unpredictable behavior

Fix: Pass helper directly to session functions instead of using a global variable, or use request-scoped storage.

Minor Issues

6. Security: Outdated jQuery Version

examples/server_plugin/oidc/templates/auth0home.html:3

jQuery 3.1.0 (from 2016) has known XSS vulnerabilities. Update to jQuery 3.7+ or use a modern framework.

7. Code Quality: Error Handling

examples/server_plugin/oidc/main.go:262-264

Silent error ignoring with _ = err makes debugging difficult. Consider logging errors even if not failing.

Summary

This PR introduces a functional OIDC authentication plugin, but contains critical security vulnerabilities that must be fixed before merging:

1. ✅ Good: Session helper architecture properly addresses Go plugin type limitations
2. ✅ Good: OAuth2/OIDC flow implementation follows correct pattern
3. 🚨 Critical: Hardcoded secrets must be removed
4. 🚨 High: Security vulnerabilities (CORS, mixed content, state validation) must be fixed
5. ⚠️ Medium: Race condition in session helper needs resolution

Please address issues #1-5 before this PR can be approved.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 12 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
endpoints/server/httpserver.go	0.00%	12 Missing ⚠️

@@           Coverage Diff            @@
##            main   #1418      +/-   ##
========================================
- Coverage   0.57%   0.57%   -0.01%     
========================================
  Files         87      87              
  Lines      12523   12535      +12     
========================================
  Hits          72      72              
- Misses     12445   12457      +12     
  Partials       6       6              

Flag	Coverage Δ
unittests	0.57% <0.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
nhp/plugins/serverpluginhandler.go	0.00% <ø> (ø)
endpoints/server/httpserver.go	0.00% <0.00%> (ø)

🚀 New features to boost your workflow:

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

[claude]

🚨 Changes Requested - Found 4 critical security issues that must be addressed

Critical Security Issues

1. CSRF Token Reuse Vulnerability (main.go:272, 321-322)

Severity: HIGH - Authentication bypass risk

In authAndShowLogin(), the state token is reused in the redirect URL without proper validation:

ctx.Redirect(http.StatusSeeOther, "/plugins/oidc?resid=demo&action=valid"+"&state="+state)

The state is then cleared in authRegular() at lines 321-322:

sessionSet(ctx, "state", "")
sessionSave(ctx)

Problem: If an attacker captures this redirect URL (via referer header, logs, or browser history), they can replay the state parameter even after it's been cleared from the session. This defeats CSRF protection.

Fix Required: After validation, immediately perform a redirect without the state in the URL, or generate a new single-use token for the redirect.

---

2. Global Race Condition on oidcAuth (main.go:86, 282, 299, 462)

Severity: HIGH - Race condition, potential auth bypass

The oidcAuth variable (line 86) is:

• Written in authOidc() at line 282
• Read in authRegular() at line 299
• Read in authAndShowLogin() at line 270

Problem: Without synchronization, concurrent requests can cause:

1. Auth checks passing with nil authenticator
2. One request's authenticator being used by another
3. Configuration mixups between different OAuth providers

Fix Required: Add mutex protection around oidcAuth access, or use sync.Once for initialization, or make it request-scoped.

---

3. Workflow Secret Injection Uses sed (deploy-demo.yml:705-706)

Severity: MEDIUM - Potential secret exposure

sed -i "s/__AUTH0_CLIENT_ID__//g" ""
sed -i "s/__AUTH0_CLIENT_SECRET__//g" ""

Problems:

• Secrets may appear in process listings (ps aux)
• Shell expansion occurs before sed execution
• No validation that placeholders exist before injection

Fix Required: Use a proper templating tool or heredoc approach that doesn't expose secrets in process args.

---

4. Missing Content-Type Validation (main.go:241)

Severity: MEDIUM - XSS risk

Error responses return JSON strings without proper Content-Type:

ctx.String(http.StatusOK, "{\"errMsg\": \"resource error: %v\"}", err)

Problem: Using ctx.String() with JSON format but without Content-Type: application/json header could allow browsers to misinterpret the response. Combined with error message reflection, this could lead to XSS.

Fix Required: Use ctx.JSON() consistently for JSON responses, or explicitly set Content-Type header.

---

Minor Issues

5. Typo in Error Message (main.go:357)

Line 357: "invalid session paramete" - missing 'r'

6. OAuth Token Not Cleared on Error (main.go:332-335)

When token exchange fails, the session should be cleared to prevent inconsistent state.

7. baseConf Lacks Mutex Protection (main.go:96, 166, 282)

baseConf is written in updateConfig() and read in authOidc() without synchronization.

---

Code Quality Observations (non-blocking)

1. Error handling inconsistency: Some functions ignore errors (updateConfig lines 129-131), while others propagate them
2. Hardcoded redirect URLs: Line 272 and 366 have hardcoded paths that should come from config
3. Import formatting: The imports in main.go need proper grouping per CLAUDE.md requirements

---

Recommendation

REQUEST CHANGES - The race conditions and CSRF token reuse are critical security vulnerabilities that must be fixed before merging. The workflow secret handling should also be improved to prevent accidental exposure.

[claude]

🚨 Changes Requested - Found 5 critical security issues that must be addressed

Critical Security Issues (Category 1)

1. OAuth State Not Stored in Session Before Redirect (examples/server_plugin/oidc/main.go:321)

Severity: CRITICAL - CSRF Vulnerability

After validating the state parameter, the code clears it but never stores the new state value:

// Line 321: State is cleared
sessionSet(ctx, "state", "")
sessionSave(ctx)

However, when authOidc() is called (line 255), it generates a NEW state and stores it (auth.go:66). But in authRegular() (line 298-323), the code validates against the OLD state from the session, then clears it. This creates a race condition where:

• User initiates OAuth flow → state1 stored
• Callback arrives → validates state1, clears it
• If user refreshes or retries → validation fails because state was cleared

Fix: The state should be cleared AFTER successful authentication, not during validation. The clearing on line 321 should be removed or moved to after the OAuth exchange is complete (after line 368).

2. Secrets Hardcoded in Checked-in Config File (examples/server_plugin/oidc/etc/config.toml:2-3)

Severity: HIGH - Secret Exposure

The config file contains placeholder values that are replaced at deployment:

AUTH0_CLIENT_ID = "__AUTH0_CLIENT_ID__"
AUTH0_CLIENT_SECRET = "__AUTH0_CLIENT_SECRET__"

Issues:

• If the workflow fails to inject secrets (e.g., secrets not configured), the plugin will run with placeholder values
• No validation that secrets were actually replaced
• Config file should have example values, not placeholders that look like valid config

Fix: Add validation in updateConfig() to check that secrets don't contain "__" prefix/suffix, and fail startup with a clear error message if secrets weren't properly configured.

3. Missing Origin Validation in Main Server CORS (endpoints/server/httpserver.go:301)

Severity: MEDIUM - CORS Misconfiguration

The main server sets unrestricted CORS:

c.Writer.Header().Set("Access-Control-Allow-Origin", "*")
c.Writer.Header().Set("Access-Control-Allow-Credentials", "true")

This is a dangerous combination - browsers will reject credentials when origin is "*". While the OIDC plugin implements proper origin allowlisting (main.go:458-478), the main server doesn't.

Fix: Implement the same origin allowlist validation in corsMiddleware() as used in the OIDC plugin, or remove the credentials header from the wildcard CORS policy.

4. Error Messages Leak Implementation Details (examples/server_plugin/oidc/main.go:307-318)

Severity: LOW - Information Disclosure

Error messages expose internal state:

• "no state in session" (line 307)
• "invalid state in session" (line 312)
• "invalid authentication session" (line 317)

These help attackers understand the session mechanism and could aid in crafting attacks.

Fix: Return generic "Authentication failed" message to client, log detailed errors server-side only.

5. Session Helper Not Checked for Nil (examples/server_plugin/oidc/main.go:42-70)

Severity: MEDIUM - Potential Null Pointer Dereference

The session helper functions check if helper exists, but don't handle errors when helper functions are nil:

func sessionGet(ctx *gin.Context, key string) interface{} {
    helper := getHelper(ctx)
    if helper != nil && helper.SessionGet != nil {
        return helper.SessionGet(ctx, key)
    }
    return nil  // Silent failure
}

If SessionGet is nil, this silently returns nil, which could cause authentication to succeed when it should fail (e.g., line 306 checks if state is nil).

Fix: Log an error or panic when helper is missing, as this indicates a serious initialization problem.

Other Issues

Code Quality Issue (examples/server_plugin/oidc/main.go:357)

Line 357 has a typo: "invalid session paramete" should be "invalid session parameter"

Positive Changes

✅ Good use of session helper callbacks to work around Go plugin type limitations
✅ Proper CORS origin allowlisting in OIDC plugin
✅ State parameter validation to prevent CSRF (though implementation has the clearing issue noted above)
✅ Secret injection via GitHub Actions (good security practice)

Files Reviewed

• examples/server_plugin/oidc/main.go
• examples/server_plugin/oidc/auth.go
• endpoints/server/httpserver.go
• nhp/plugins/serverpluginhandler.go
• .github/workflows/build-binaries.yml
• .github/workflows/deploy-demo.yml

Recommendation

Please address the critical security issues before merging, particularly #1 (state handling) and #3 (CORS credentials).

[claude]

🚨 Changes Requested - Found 2 critical security issues that must be addressed

Critical Issues

1. CRITICAL: Insecure CORS Configuration (endpoints/server/httpserver.go:301-305)

The HTTP server implements a dangerous wildcard CORS policy that allows credential theft:

c.Writer.Header().Set("Access-Control-Allow-Origin", "*")
c.Writer.Header().Set("Access-Control-Allow-Credentials", "true")

Security Impact:

• Combining Access-Control-Allow-Origin: * with Access-Control-Allow-Credentials: true violates CORS specification
• Any malicious website can make authenticated requests to the OpenNHP server
• Enables cookie/token theft and CSRF attacks
• The OIDC plugin's allowlist (main.go:501-505) partially mitigates this, but the underlying HTTP server still permits wildcard CORS globally

Required Fix:

• Replace "*" with specific allowed origins from configuration
• Or remove Access-Control-Allow-Credentials: true if wildcard CORS is required
• Implement per-route CORS policies for sensitive endpoints

---

2. HIGH: Missing SameSite Cookie Attribute (examples/server_plugin/oidc/main.go:437-454)

Authentication cookies lack CSRF protection:

ctx.SetCookie(
    "nhp-token",
    url.QueryEscape(token),
    int(res.OpenTime),
    "/",
    res.CookieDomain,
    true,  // Secure
    true)  // HttpOnly
// Missing: SameSite attribute

Security Impact:

• Cookies are sent on all cross-site requests (no SameSite protection)
• Combined with the wildcard CORS above, this creates a critical CSRF vulnerability
• Attackers can trick users into performing unintended authenticated actions

Required Fix:
Add SameSite=Strict or SameSite=Lax to all cookie calls. The Gin framework requires using SetSameSite() before SetCookie():

ctx.SetSameSite(http.SameSiteStrictMode)  // or SameSiteLaxMode
ctx.SetCookie(...)

---

Additional Issues to Address

3. MEDIUM: HTTP Status Code Misuse (endpoints/server/httpserver.go:301, httpauth.go:27)

Authentication failures incorrectly return HTTP 200 OK:

c.String(http.StatusOK, "{\"errMsg\": \"auth error: %v\"}", err)

This should return HTTP 401 or 403 for proper error handling.

---

4. MEDIUM: No Redirect URI Validation (examples/server_plugin/oidc/main.go:425-429)

RedirectUrl from configuration is used without validation:

ackMsg.RedirectUrl = res.RedirectUrl

Should validate redirect URLs against an allowlist and ensure HTTPS protocol.

---

5. LOW: Potential Race Condition (examples/server_plugin/oidc/main.go:108, 316, 333)

Global oidcAuth variable is accessed without synchronization. Consider using local instances or proper synchronization.

---

What's Good

• ✅ OAuth state parameter validation (lines 340-357)
• ✅ OIDC token verification (lines 404-407)
• ✅ Secure and HttpOnly cookie flags
• ✅ Placeholder secret validation (lines 191-198)
• ✅ CORS allowlist in OIDC plugin (lines 501-505)

---

Required Actions

Before this PR can be merged:

1. Fix the wildcard CORS configuration in endpoints/server/httpserver.go
2. Add SameSite attribute to all cookies in examples/server_plugin/oidc/main.go

These two issues combined create a critical security vulnerability that allows cross-site credential theft and must be addressed before deployment.

---

Testing Recommendations

• Test CORS behavior with cross-origin requests
• Verify cookies include SameSite attribute in browser dev tools
• Test authentication failure returns proper HTTP status codes
• Validate redirect URI sanitization

[claude]

✅ Approved - Code looks good

I've completed a comprehensive security review of this OIDC authentication plugin. The implementation demonstrates excellent security practices throughout.

Security Strengths

Authentication & Authorization:

• ✅ Proper CSRF protection with cryptographically random state (crypto/rand, 256-bit entropy)
• ✅ Correct ID token verification with OIDC provider
• ✅ Secure session management with thread-safe helper functions
• ✅ Configuration validation prevents deployment with placeholder secrets

Cookie & Session Security:

• ✅ Secure and HttpOnly flags properly set
• ✅ Appropriate cookie domain scoping
• ✅ Session state cleared on auth failure (prevents session fixation)

GitHub Actions Security:

• ✅ Excellent input validation with whitelist checking (prevents command injection)
• ✅ Path traversal prevention for external plugin repos
• ✅ Token sanitization in error messages
• ✅ SSH strict host key checking with pre-configured known_hosts
• ✅ Hostname validation (RFC 1123 compliance)
• ✅ Minimal OAuth2 scopes (principle of least privilege)

Code Quality:

• ✅ Plugin helper pattern provides clean capability model
• ✅ Short access windows (15 seconds) limit exposure
• ✅ Proper error handling throughout
• ✅ HTTPS enforcement for OIDC provider

Minor Suggestions (Non-blocking)

1. CORS Configuration (examples/server_plugin/oidc/main.go:501-505): Consider moving hardcoded CORS allowlist to config.toml for easier customization without code changes.

2. Rate Limiting: Consider adding rate limiting on OAuth callback endpoints to prevent potential abuse scenarios.

3. Documentation: The PR description template is not filled out. Consider adding:

   • Brief summary of the OIDC plugin functionality
   • Link to Auth0 setup documentation
   • Note about SkipAuth = true design choice (consistent with other example plugins)

Design Notes

The SkipAuth = true configuration is intentional and consistent with other example plugins (basic, authenticator). The plugin correctly enforces this requirement at main.go:482-486. This is appropriate for an HTTP-based authentication flow where the plugin handles auth directly.

Conclusion

This PR adds valuable OIDC/Auth0 integration with strong security practices. The implementation follows the project's plugin architecture correctly and maintains the zero-trust security model appropriately for HTTP-based authentication flows.

Recommendation: Approve and merge. ✅