fix: review comments

steveiliop56 · steveiliop56 · commit e451b3d62f44 · 2026-04-07T18:27:45.000+03:00
diff --git a/internal/assets/migrations/000007_oidc_pkce.down.sql b/internal/assets/migrations/000007_oidc_pkce.down.sql
@@ -1,2 +1 @@
 ALTER TABLE "oidc_codes" DROP COLUMN "code_challenge";
-ALTER TABLE "oidc_codes" DROP COLUMN "code_challenge_method";
diff --git a/internal/assets/migrations/000007_oidc_pkce.up.sql b/internal/assets/migrations/000007_oidc_pkce.up.sql
@@ -1,2 +1 @@
 ALTER TABLE "oidc_codes" ADD COLUMN "code_challenge" TEXT DEFAULT "";
-ALTER TABLE "oidc_codes" ADD COLUMN "code_challenge_method" TEXT DEFAULT "";
diff --git a/internal/controller/context_controller_test.go b/internal/controller/context_controller_test.go
@@ -10,10 +10,12 @@ import (
 	"github.com/steveiliop56/tinyauth/internal/config"
 	"github.com/steveiliop56/tinyauth/internal/controller"
 	"github.com/steveiliop56/tinyauth/internal/utils"
+	"github.com/steveiliop56/tinyauth/internal/utils/tlog"
 	"github.com/stretchr/testify/assert"
 )
 
 func TestContextController(t *testing.T) {
+	tlog.NewTestLogger().Init()
 	controllerConfig := controller.ContextControllerConfig{
 		Providers: []controller.Provider{
 			{
diff --git a/internal/controller/health_controller_test.go b/internal/controller/health_controller_test.go
@@ -8,10 +8,12 @@ import (
 
 	"github.com/gin-gonic/gin"
 	"github.com/steveiliop56/tinyauth/internal/controller"
+	"github.com/steveiliop56/tinyauth/internal/utils/tlog"
 	"github.com/stretchr/testify/assert"
 )
 
 func TestHealthController(t *testing.T) {
+	tlog.NewTestLogger().Init()
 	tests := []struct {
 		description string
 		path        string
diff --git a/internal/controller/oidc_controller.go b/internal/controller/oidc_controller.go
@@ -309,7 +309,8 @@ func (controller *OIDCController) Token(c *gin.Context) {
 			return
 		}
 
-		ok := controller.oidc.ValidatePKCE(entry.CodeChallenge, entry.CodeChallengeMethod, req.CodeVerifier)
+		tlog.App.Debug().Str("challenge", entry.CodeChallenge).Str("verifier", req.CodeVerifier).Msg("Validating PKCE")
+		ok := controller.oidc.ValidatePKCE(entry.CodeChallenge, req.CodeVerifier)
 
 		if !ok {
 			tlog.App.Warn().Msg("PKCE validation failed")
diff --git a/internal/controller/oidc_controller_test.go b/internal/controller/oidc_controller_test.go
@@ -17,11 +17,13 @@ import (
 	"github.com/steveiliop56/tinyauth/internal/controller"
 	"github.com/steveiliop56/tinyauth/internal/repository"
 	"github.com/steveiliop56/tinyauth/internal/service"
+	"github.com/steveiliop56/tinyauth/internal/utils/tlog"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
 func TestOIDCController(t *testing.T) {
+	tlog.NewTestLogger().Init()
 	tempDir := t.TempDir()
 
 	oidcServiceCfg := service.OIDCServiceConfig{
@@ -473,6 +475,7 @@ func TestOIDCController(t *testing.T) {
 				assert.NotEmpty(t, code)
 
 				// Now exchange the code for a token
+				recorder = httptest.NewRecorder()
 				tokenReqBody := controller.TokenRequest{
 					GrantType:    "authorization_code",
 					Code:         code,
@@ -499,7 +502,7 @@ func TestOIDCController(t *testing.T) {
 				hasher := sha256.New()
 				hasher.Write([]byte("some-challenge"))
 				codeChallenge := hasher.Sum(nil)
-				codeChallengeEncoded := base64.URLEncoding.EncodeToString(codeChallenge)
+				codeChallengeEncoded := base64.RawURLEncoding.EncodeToString(codeChallenge)
 				reqBody := service.AuthorizeRequest{
 					Scope:               "openid",
 					ResponseType:        "code",
@@ -533,6 +536,7 @@ func TestOIDCController(t *testing.T) {
 				assert.NotEmpty(t, code)
 
 				// Now exchange the code for a token
+				recorder = httptest.NewRecorder()
 				tokenReqBody := controller.TokenRequest{
 					GrantType:    "authorization_code",
 					Code:         code,
@@ -559,7 +563,7 @@ func TestOIDCController(t *testing.T) {
 				hasher := sha256.New()
 				hasher.Write([]byte("some-challenge"))
 				codeChallenge := hasher.Sum(nil)
-				codeChallengeEncoded := base64.URLEncoding.EncodeToString(codeChallenge)
+				codeChallengeEncoded := base64.RawURLEncoding.EncodeToString(codeChallenge)
 				reqBody := service.AuthorizeRequest{
 					Scope:               "openid",
 					ResponseType:        "code",
@@ -593,6 +597,7 @@ func TestOIDCController(t *testing.T) {
 				assert.NotEmpty(t, code)
 
 				// Now exchange the code for a token
+				recorder = httptest.NewRecorder()
 				tokenReqBody := controller.TokenRequest{
 					GrantType:    "authorization_code",
 					Code:         code,
@@ -607,7 +612,7 @@ func TestOIDCController(t *testing.T) {
 				req.SetBasicAuth("some-client-id", "some-client-secret")
 				router.ServeHTTP(recorder, req)
 
-				assert.Equal(t, 200, recorder.Code)
+				assert.Equal(t, 400, recorder.Code)
 			},
 		},
 	}
diff --git a/internal/controller/proxy_controller_test.go b/internal/controller/proxy_controller_test.go
@@ -17,6 +17,7 @@ import (
 )
 
 func TestProxyController(t *testing.T) {
+	tlog.NewTestLogger().Init()
 	tempDir := t.TempDir()
 
 	authServiceCfg := service.AuthServiceConfig{
diff --git a/internal/controller/resources_controller_test.go b/internal/controller/resources_controller_test.go
@@ -8,11 +8,13 @@ import (
 
 	"github.com/gin-gonic/gin"
 	"github.com/steveiliop56/tinyauth/internal/controller"
+	"github.com/steveiliop56/tinyauth/internal/utils/tlog"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
 func TestResourcesController(t *testing.T) {
+	tlog.NewTestLogger().Init()
 	tempDir := t.TempDir()
 
 	resourcesControllerCfg := controller.ResourcesControllerConfig{
diff --git a/internal/controller/user_controller_test.go b/internal/controller/user_controller_test.go
@@ -22,6 +22,7 @@ import (
 )
 
 func TestUserController(t *testing.T) {
+	tlog.NewTestLogger().Init()
 	tempDir := t.TempDir()
 
 	authServiceCfg := service.AuthServiceConfig{
diff --git a/internal/controller/well_known_controller_test.go b/internal/controller/well_known_controller_test.go
@@ -13,11 +13,13 @@ import (
 	"github.com/steveiliop56/tinyauth/internal/controller"
 	"github.com/steveiliop56/tinyauth/internal/repository"
 	"github.com/steveiliop56/tinyauth/internal/service"
+	"github.com/steveiliop56/tinyauth/internal/utils/tlog"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
 func TestWellKnownController(t *testing.T) {
+	tlog.NewTestLogger().Init()
 	tempDir := t.TempDir()
 
 	oidcServiceCfg := service.OIDCServiceConfig{
diff --git a/internal/repository/models.go b/internal/repository/models.go
diff --git a/internal/repository/oidc_queries.sql.go b/internal/repository/oidc_queries.sql.go
diff --git a/internal/service/oidc_service.go b/internal/service/oidc_service.go
@@ -297,7 +297,7 @@ func (service *OIDCService) ValidateAuthorizeParams(req AuthorizeRequest) error
 
 	// PKCE code challenge method if set
 	if req.CodeChallenge != "" && req.CodeChallengeMethod != "" {
-		if req.CodeChallengeMethod != "S256" || req.CodeChallenge == "plain" {
+		if req.CodeChallengeMethod != "S256" && req.CodeChallengeMethod != "plain" {
 			return errors.New("invalid_request")
 		}
 	}
@@ -329,10 +329,8 @@ func (service *OIDCService) StoreCode(c *gin.Context, sub string, code string, r
 	if req.CodeChallenge != "" {
 		if req.CodeChallengeMethod == "S256" {
 			entry.CodeChallenge = req.CodeChallenge
-			entry.CodeChallengeMethod = "S256"
 		} else {
 			entry.CodeChallenge = service.hashAndEncodePKCE(req.CodeChallenge)
-			entry.CodeChallengeMethod = "plain"
 			tlog.App.Warn().Msg("Received plain PKCE code challenge, it's recommended to use S256 for better security")
 		}
 	}
@@ -751,19 +749,15 @@ func (service *OIDCService) GetJWK() ([]byte, error) {
 	return jwk.Public().MarshalJSON()
 }
 
-func (service *OIDCService) ValidatePKCE(codeChallenge string, codeChallengeMethod string, codeVerifier string) bool {
+func (service *OIDCService) ValidatePKCE(codeChallenge string, codeVerifier string) bool {
 	if codeChallenge == "" {
 		return true
 	}
-	if codeChallengeMethod == "plain" {
-		// Code challenge is hashed and encoded in the database for security reasons
-		return codeChallenge == service.hashAndEncodePKCE(codeVerifier)
-	}
-	return codeChallenge == codeVerifier
+	return codeChallenge == service.hashAndEncodePKCE(codeVerifier)
 }
 
 func (service *OIDCService) hashAndEncodePKCE(codeVerifier string) string {
 	hasher := sha256.New()
 	hasher.Write([]byte(codeVerifier))
-	return base64.URLEncoding.EncodeToString(hasher.Sum(nil))
+	return base64.RawURLEncoding.EncodeToString(hasher.Sum(nil))
 }
diff --git a/internal/utils/tlog/log_wrapper.go b/internal/utils/tlog/log_wrapper.go
@@ -55,6 +55,17 @@ func NewSimpleLogger() *Logger {
 	})
 }
 
+func NewTestLogger() *Logger {
+	return NewLogger(config.LogConfig{
+		Level: "trace",
+		Streams: config.LogStreams{
+			HTTP:  config.LogStreamConfig{Enabled: true},
+			App:   config.LogStreamConfig{Enabled: true},
+			Audit: config.LogStreamConfig{Enabled: true},
+		},
+	})
+}
+
 func (l *Logger) Init() {
 	Audit = l.Audit
 	HTTP = l.HTTP
diff --git a/sql/oidc_queries.sql b/sql/oidc_queries.sql
@@ -7,10 +7,9 @@ INSERT INTO "oidc_codes" (
     "client_id",
     "expires_at",
     "nonce",
-    "code_challenge",
-    "code_challenge_method"
+    "code_challenge"
 ) VALUES (
-    ?, ?, ?, ?, ?, ?, ?, ?, ?
+    ?, ?, ?, ?, ?, ?, ?, ?
 )
 RETURNING *;
 
diff --git a/sql/oidc_schemas.sql b/sql/oidc_schemas.sql
@@ -6,8 +6,7 @@ CREATE TABLE IF NOT EXISTS "oidc_codes" (
     "client_id" TEXT NOT NULL,
     "expires_at" INTEGER NOT NULL,
     "nonce" TEXT DEFAULT "",
-    "code_challenge" TEXT DEFAULT "",
-    "code_challenge_method" TEXT DEFAULT ""
+    "code_challenge" TEXT DEFAULT ""
 );
 
 CREATE TABLE IF NOT EXISTS "oidc_tokens" (
diff --git a/sqlc.yml b/sqlc.yml
@@ -28,5 +28,3 @@ sql:
             go_type: "string"
           - column: "oidc_codes.code_challenge"
             go_type: "string"
-          - column: "oidc_codes.code_challenge_method"
-            go_type: "string"

Original file line number	Diff line number	Diff line change
`@@ -1,2 +1 @@`
`1`	`1`	`ALTER TABLE "oidc_codes" DROP COLUMN "code_challenge";`
`2`		`-ALTER TABLE "oidc_codes" DROP COLUMN "code_challenge_method";`
Original file line number	Diff line number	Diff line change
`@@ -1,2 +1 @@`
`1`	`1`	`ALTER TABLE "oidc_codes" ADD COLUMN "code_challenge" TEXT DEFAULT "";`
`2`		`-ALTER TABLE "oidc_codes" ADD COLUMN "code_challenge_method" TEXT DEFAULT "";`
Original file line number	Diff line number	Diff line change
`@@ -10,10 +10,12 @@ import (`
`10`	`10`	`"github.com/steveiliop56/tinyauth/internal/config"`
`11`	`11`	`"github.com/steveiliop56/tinyauth/internal/controller"`
`12`	`12`	`"github.com/steveiliop56/tinyauth/internal/utils"`
	`13`	`+ "github.com/steveiliop56/tinyauth/internal/utils/tlog"`
`13`	`14`	`"github.com/stretchr/testify/assert"`
`14`	`15`	`)`
`15`	`16`
`16`	`17`	`func TestContextController(t *testing.T) {`
	`18`	`+ tlog.NewTestLogger().Init()`
`17`	`19`	`controllerConfig := controller.ContextControllerConfig{`
`18`	`20`	`Providers: []controller.Provider{`
`19`	`21`	`{`
Original file line number	Diff line number	Diff line change
`@@ -309,7 +309,8 @@ func (controller OIDCController) Token(c gin.Context) {`
`309`	`309`	`return`
`310`	`310`	`}`
`311`	`311`
`312`		`- ok := controller.oidc.ValidatePKCE(entry.CodeChallenge, entry.CodeChallengeMethod, req.CodeVerifier)`
	`312`	`+ tlog.App.Debug().Str("challenge", entry.CodeChallenge).Str("verifier", req.CodeVerifier).Msg("Validating PKCE")`
	`313`	`+ ok := controller.oidc.ValidatePKCE(entry.CodeChallenge, req.CodeVerifier)`
`313`	`314`
`314`	`315`	`if !ok {`
`315`	`316`	`tlog.App.Warn().Msg("PKCE validation failed")`
Original file line number	Diff line number	Diff line change
`@@ -17,6 +17,7 @@ import (`
`17`	`17`	`)`
`18`	`18`
`19`	`19`	`func TestProxyController(t *testing.T) {`
	`20`	`+ tlog.NewTestLogger().Init()`
`20`	`21`	`tempDir := t.TempDir()`
`21`	`22`
`22`	`23`	`authServiceCfg := service.AuthServiceConfig{`
Original file line number	Diff line number	Diff line change
`@@ -22,6 +22,7 @@ import (`
`22`	`22`	`)`
`23`	`23`
`24`	`24`	`func TestUserController(t *testing.T) {`
	`25`	`+ tlog.NewTestLogger().Init()`
`25`	`26`	`tempDir := t.TempDir()`
`26`	`27`
`27`	`28`	`authServiceCfg := service.AuthServiceConfig{`