fix: address CodeRabbit PR review — error wrapping, redirect, validation, dedup
- commit
- c8a82f022190dbe11a49b51e24039aa0bbcf6573
- author
- brice · · 2026-03-22 18:13 UTC
- parent
- 3094000
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
7 files · +45 -45
M docs/upgrading.md +1 -1
| @@ -14,7 +14,7 @@ Gordon v2.30.0 removes password-based authentication entirely. Only token-based | |||
| 14 | 14 | - `auth.password` and `auth.password_hash` config fields are removed | |
| 15 | 15 | - The `gordon auth password hash` CLI command is removed | |
| 16 | 16 | - The `gordon auth login` command no longer accepts `--username` or `--password` flags | |
| 17 | - | - The `/auth/password` endpoint is removed | |
| 17 | + | - The `/auth/password` endpoint now returns `410 Gone` | |
| 18 | 18 | ||
| 19 | 19 | **New features:** | |
| 20 | 20 | ||
| @@ -14,7 +14,7 @@ Gordon v2.30.0 removes password-based authentication entirely. Only token-based | |||||
| 14 | - `auth.password` and `auth.password_hash` config fields are removed | 14 | - `auth.password` and `auth.password_hash` config fields are removed | ||
| 15 | - The `gordon auth password hash` CLI command is removed | 15 | - The `gordon auth password hash` CLI command is removed | ||
| 16 | - The `gordon auth login` command no longer accepts `--username` or `--password` flags | 16 | - The `gordon auth login` command no longer accepts `--username` or `--password` flags | ||
| 17 | - | - The `/auth/password` endpoint is removed | 17 | + | - The `/auth/password` endpoint now returns `410 Gone` |
| 18 | 18 | ||||
| 19 | **New features:** | 19 | **New features:** | ||
| 20 | 20 | ||||
M internal/adapters/in/http/auth/handler.go +28 -33
| @@ -189,42 +189,18 @@ func (h *Handler) intersectRequestedScopes(requestedScopes, grantedScopes []stri | |||
| 189 | 189 | continue | |
| 190 | 190 | } | |
| 191 | 191 | ||
| 192 | + | var checkAccess func([]string, string, string) bool | |
| 192 | 193 | switch reqScope.Type { | |
| 193 | 194 | case domain.ScopeTypeRepository: | |
| 194 | - | allowedActions := make([]string, 0, len(reqScope.Actions)) | |
| 195 | - | for _, action := range reqScope.Actions { | |
| 196 | - | if domain.ScopesGrantRegistryAccess(grantedScopes, reqScope.Name, action) { | |
| 197 | - | allowedActions = append(allowedActions, action) | |
| 198 | - | } | |
| 199 | - | } | |
| 200 | - | ||
| 201 | - | if len(allowedActions) == 0 { | |
| 202 | - | continue | |
| 203 | - | } | |
| 204 | - | ||
| 205 | - | effective = append(effective, (&domain.Scope{ | |
| 206 | - | Type: reqScope.Type, | |
| 207 | - | Name: reqScope.Name, | |
| 208 | - | Actions: allowedActions, | |
| 209 | - | }).String()) | |
| 210 | - | ||
| 195 | + | checkAccess = domain.ScopesGrantRegistryAccess | |
| 211 | 196 | case domain.ScopeTypeAdmin: | |
| 212 | - | allowedActions := make([]string, 0, len(reqScope.Actions)) | |
| 213 | - | for _, action := range reqScope.Actions { | |
| 214 | - | if domain.ScopesGrantAdminAccess(grantedScopes, reqScope.Name, action) { | |
| 215 | - | allowedActions = append(allowedActions, action) | |
| 216 | - | } | |
| 217 | - | } | |
| 218 | - | ||
| 219 | - | if len(allowedActions) == 0 { | |
| 220 | - | continue | |
| 221 | - | } | |
| 222 | - | ||
| 223 | - | effective = append(effective, (&domain.Scope{ | |
| 224 | - | Type: reqScope.Type, | |
| 225 | - | Name: reqScope.Name, | |
| 226 | - | Actions: allowedActions, | |
| 227 | - | }).String()) | |
| 197 | + | checkAccess = domain.ScopesGrantAdminAccess | |
| 198 | + | default: | |
| 199 | + | continue | |
| 200 | + | } | |
| 201 | + | ||
| 202 | + | if s := buildEffectiveScope(reqScope, grantedScopes, checkAccess); s != "" { | |
| 203 | + | effective = append(effective, s) | |
| 228 | 204 | } | |
| 229 | 205 | } | |
| 230 | 206 | ||
| @@ -237,6 +213,25 @@ func (h *Handler) intersectRequestedScopes(requestedScopes, grantedScopes []stri | |||
| 237 | 213 | return effective | |
| 238 | 214 | } | |
| 239 | 215 | ||
| 216 | + | // buildEffectiveScope filters a requested scope's actions through checkAccess and returns | |
| 217 | + | // the resulting scope string, or "" if no actions were granted. | |
| 218 | + | func buildEffectiveScope(reqScope *domain.Scope, grantedScopes []string, checkAccess func([]string, string, string) bool) string { | |
| 219 | + | allowedActions := make([]string, 0, len(reqScope.Actions)) | |
| 220 | + | for _, action := range reqScope.Actions { | |
| 221 | + | if checkAccess(grantedScopes, reqScope.Name, action) { | |
| 222 | + | allowedActions = append(allowedActions, action) | |
| 223 | + | } | |
| 224 | + | } | |
| 225 | + | if len(allowedActions) == 0 { | |
| 226 | + | return "" | |
| 227 | + | } | |
| 228 | + | return (&domain.Scope{ | |
| 229 | + | Type: reqScope.Type, | |
| 230 | + | Name: reqScope.Name, | |
| 231 | + | Actions: allowedActions, | |
| 232 | + | }).String() | |
| 233 | + | } | |
| 234 | + | ||
| 240 | 235 | // parseRequestedScopes extracts and validates scope parameters from the request. | |
| 241 | 236 | // Per Docker Registry v2 auth spec, scope format is: repository:name:actions | |
| 242 | 237 | // Example: GET /auth/token?scope=repository:myrepo:push,pull&scope=repository:other:pull | |
| @@ -189,42 +189,18 @@ func (h *Handler) intersectRequestedScopes(requestedScopes, grantedScopes []stri | |||||
| 189 | continue | 189 | continue | ||
| 190 | } | 190 | } | ||
| 191 | 191 | ||||
| 192 | + | var checkAccess func([]string, string, string) bool | |||
| 192 | switch reqScope.Type { | 193 | switch reqScope.Type { | ||
| 193 | case domain.ScopeTypeRepository: | 194 | case domain.ScopeTypeRepository: | ||
| 194 | - | allowedActions := make([]string, 0, len(reqScope.Actions)) | 195 | + | checkAccess = domain.ScopesGrantRegistryAccess |
| 195 | - | for _, action := range reqScope.Actions { | |||
| 196 | - | if domain.ScopesGrantRegistryAccess(grantedScopes, reqScope.Name, action) { | |||
| 197 | - | allowedActions = append(allowedActions, action) | |||
| 198 | - | } | |||
| 199 | - | } | |||
| 200 | - | ||||
| 201 | - | if len(allowedActions) == 0 { | |||
| 202 | - | continue | |||
| 203 | - | } | |||
| 204 | - | ||||
| 205 | - | effective = append(effective, (&domain.Scope{ | |||
| 206 | - | Type: reqScope.Type, | |||
| 207 | - | Name: reqScope.Name, | |||
| 208 | - | Actions: allowedActions, | |||
| 209 | - | }).String()) | |||
| 210 | - | ||||
| 211 | case domain.ScopeTypeAdmin: | 196 | case domain.ScopeTypeAdmin: | ||
| 212 | - | allowedActions := make([]string, 0, len(reqScope.Actions)) | 197 | + | checkAccess = domain.ScopesGrantAdminAccess |
| 213 | - | for _, action := range reqScope.Actions { | 198 | + | default: |
| 214 | - | if domain.ScopesGrantAdminAccess(grantedScopes, reqScope.Name, action) { | 199 | + | continue |
| 215 | - | allowedActions = append(allowedActions, action) | 200 | + | } |
| 216 | - | } | 201 | + | |
| 217 | - | } | 202 | + | if s := buildEffectiveScope(reqScope, grantedScopes, checkAccess); s != "" { |
| 218 | - | 203 | + | effective = append(effective, s) | |
| 219 | - | if len(allowedActions) == 0 { | |||
| 220 | - | continue | |||
| 221 | - | } | |||
| 222 | - | ||||
| 223 | - | effective = append(effective, (&domain.Scope{ | |||
| 224 | - | Type: reqScope.Type, | |||
| 225 | - | Name: reqScope.Name, | |||
| 226 | - | Actions: allowedActions, | |||
| 227 | - | }).String()) | |||
| 228 | } | 204 | } | ||
| 229 | } | 205 | } | ||
| 230 | 206 | ||||
| @@ -237,6 +213,25 @@ func (h *Handler) intersectRequestedScopes(requestedScopes, grantedScopes []stri | |||||
| 237 | return effective | 213 | return effective | ||
| 238 | } | 214 | } | ||
| 239 | 215 | ||||
| 216 | + | // buildEffectiveScope filters a requested scope's actions through checkAccess and returns | |||
| 217 | + | // the resulting scope string, or "" if no actions were granted. | |||
| 218 | + | func buildEffectiveScope(reqScope *domain.Scope, grantedScopes []string, checkAccess func([]string, string, string) bool) string { | |||
| 219 | + | allowedActions := make([]string, 0, len(reqScope.Actions)) | |||
| 220 | + | for _, action := range reqScope.Actions { | |||
| 221 | + | if checkAccess(grantedScopes, reqScope.Name, action) { | |||
| 222 | + | allowedActions = append(allowedActions, action) | |||
| 223 | + | } | |||
| 224 | + | } | |||
| 225 | + | if len(allowedActions) == 0 { | |||
| 226 | + | return "" | |||
| 227 | + | } | |||
| 228 | + | return (&domain.Scope{ | |||
| 229 | + | Type: reqScope.Type, | |||
| 230 | + | Name: reqScope.Name, | |||
| 231 | + | Actions: allowedActions, | |||
| 232 | + | }).String() | |||
| 233 | + | } | |||
| 234 | + | ||||
| 240 | // parseRequestedScopes extracts and validates scope parameters from the request. | 235 | // parseRequestedScopes extracts and validates scope parameters from the request. | ||
| 241 | // Per Docker Registry v2 auth spec, scope format is: repository:name:actions | 236 | // Per Docker Registry v2 auth spec, scope format is: repository:name:actions | ||
| 242 | // Example: GET /auth/token?scope=repository:myrepo:push,pull&scope=repository:other:pull | 237 | // Example: GET /auth/token?scope=repository:myrepo:push,pull&scope=repository:other:pull | ||
M internal/adapters/out/domainsecrets/store.go +2 -2
| @@ -233,7 +233,7 @@ func validateSecretValues(secrets map[string]string) error { | |||
| 233 | 233 | // It writes to a temporary file first, syncs it, then renames to the final path. | |
| 234 | 234 | func (s *FileStore) writeSecretsAtomic(domainName string, secrets map[string]string) error { | |
| 235 | 235 | if err := validateSecretValues(secrets); err != nil { | |
| 236 | - | return err | |
| 236 | + | return fmt.Errorf("invalid domain secret values for %q: %w", domainName, err) | |
| 237 | 237 | } | |
| 238 | 238 | ||
| 239 | 239 | envFile, err := s.validateEnvFilePath(domainName) | |
| @@ -378,7 +378,7 @@ func (s *FileStore) validateAttachmentEnvFilePath(containerName string) (string, | |||
| 378 | 378 | // It writes to a temporary file first, syncs it, then renames to the final path. | |
| 379 | 379 | func (s *FileStore) writeAttachmentSecretsAtomic(containerName string, secrets map[string]string) error { | |
| 380 | 380 | if err := validateSecretValues(secrets); err != nil { | |
| 381 | - | return err | |
| 381 | + | return fmt.Errorf("invalid attachment secret values for %q: %w", containerName, err) | |
| 382 | 382 | } | |
| 383 | 383 | ||
| 384 | 384 | envFile, err := s.validateAttachmentEnvFilePath(containerName) | |
| @@ -233,7 +233,7 @@ func validateSecretValues(secrets map[string]string) error { | |||||
| 233 | // It writes to a temporary file first, syncs it, then renames to the final path. | 233 | // It writes to a temporary file first, syncs it, then renames to the final path. | ||
| 234 | func (s *FileStore) writeSecretsAtomic(domainName string, secrets map[string]string) error { | 234 | func (s *FileStore) writeSecretsAtomic(domainName string, secrets map[string]string) error { | ||
| 235 | if err := validateSecretValues(secrets); err != nil { | 235 | if err := validateSecretValues(secrets); err != nil { | ||
| 236 | - | return err | 236 | + | return fmt.Errorf("invalid domain secret values for %q: %w", domainName, err) |
| 237 | } | 237 | } | ||
| 238 | 238 | ||||
| 239 | envFile, err := s.validateEnvFilePath(domainName) | 239 | envFile, err := s.validateEnvFilePath(domainName) | ||
| @@ -378,7 +378,7 @@ func (s *FileStore) validateAttachmentEnvFilePath(containerName string) (string, | |||||
| 378 | // It writes to a temporary file first, syncs it, then renames to the final path. | 378 | // It writes to a temporary file first, syncs it, then renames to the final path. | ||
| 379 | func (s *FileStore) writeAttachmentSecretsAtomic(containerName string, secrets map[string]string) error { | 379 | func (s *FileStore) writeAttachmentSecretsAtomic(containerName string, secrets map[string]string) error { | ||
| 380 | if err := validateSecretValues(secrets); err != nil { | 380 | if err := validateSecretValues(secrets); err != nil { | ||
| 381 | - | return err | 381 | + | return fmt.Errorf("invalid attachment secret values for %q: %w", containerName, err) |
| 382 | } | 382 | } | ||
| 383 | 383 | ||||
| 384 | envFile, err := s.validateAttachmentEnvFilePath(containerName) | 384 | envFile, err := s.validateAttachmentEnvFilePath(containerName) | ||
M internal/app/run.go +4 -2
| @@ -997,7 +997,7 @@ func createAuthService(ctx context.Context, cfg Config, log zerowrap.Logger) (ou | |||
| 997 | 997 | ||
| 998 | 998 | authType, err := resolveAuthType(cfg) | |
| 999 | 999 | if err != nil { | |
| 1000 | - | return nil, nil, err | |
| 1000 | + | return nil, nil, fmt.Errorf("resolve auth type: %w", err) | |
| 1001 | 1001 | } | |
| 1002 | 1002 | backend, err := resolveSecretsBackend(cfg.Auth.SecretsBackend) | |
| 1003 | 1003 | if err != nil { | |
| @@ -1807,12 +1807,14 @@ func runServers(ctx context.Context, v *viper.Viper, cfg Config, registryHandler | |||
| 1807 | 1807 | host := r.Host | |
| 1808 | 1808 | if h, _, err := net.SplitHostPort(host); err == nil { | |
| 1809 | 1809 | host = h | |
| 1810 | + | } else if strings.HasPrefix(host, "[") && strings.HasSuffix(host, "]") { | |
| 1811 | + | host = strings.TrimSuffix(strings.TrimPrefix(host, "["), "]") | |
| 1810 | 1812 | } | |
| 1811 | 1813 | if tlsPort != 443 { | |
| 1812 | 1814 | host = net.JoinHostPort(host, strconv.Itoa(tlsPort)) | |
| 1813 | 1815 | } | |
| 1814 | 1816 | target := "https://" + host + r.RequestURI | |
| 1815 | - | http.Redirect(w, r, target, http.StatusMovedPermanently) | |
| 1817 | + | http.Redirect(w, r, target, http.StatusPermanentRedirect) | |
| 1816 | 1818 | }) | |
| 1817 | 1819 | proxySrv, proxyReady = startServer(fmt.Sprintf(":%d", cfg.Server.Port), redirectHandler, "proxy-redirect", nil, errChan, log) | |
| 1818 | 1820 | tlsSrv, tlsReady = startTLSServer( | |
| @@ -997,7 +997,7 @@ func createAuthService(ctx context.Context, cfg Config, log zerowrap.Logger) (ou | |||||
| 997 | 997 | ||||
| 998 | authType, err := resolveAuthType(cfg) | 998 | authType, err := resolveAuthType(cfg) | ||
| 999 | if err != nil { | 999 | if err != nil { | ||
| 1000 | - | return nil, nil, err | 1000 | + | return nil, nil, fmt.Errorf("resolve auth type: %w", err) |
| 1001 | } | 1001 | } | ||
| 1002 | backend, err := resolveSecretsBackend(cfg.Auth.SecretsBackend) | 1002 | backend, err := resolveSecretsBackend(cfg.Auth.SecretsBackend) | ||
| 1003 | if err != nil { | 1003 | if err != nil { | ||
| @@ -1807,12 +1807,14 @@ func runServers(ctx context.Context, v *viper.Viper, cfg Config, registryHandler | |||||
| 1807 | host := r.Host | 1807 | host := r.Host | ||
| 1808 | if h, _, err := net.SplitHostPort(host); err == nil { | 1808 | if h, _, err := net.SplitHostPort(host); err == nil { | ||
| 1809 | host = h | 1809 | host = h | ||
| 1810 | + | } else if strings.HasPrefix(host, "[") && strings.HasSuffix(host, "]") { | |||
| 1811 | + | host = strings.TrimSuffix(strings.TrimPrefix(host, "["), "]") | |||
| 1810 | } | 1812 | } | ||
| 1811 | if tlsPort != 443 { | 1813 | if tlsPort != 443 { | ||
| 1812 | host = net.JoinHostPort(host, strconv.Itoa(tlsPort)) | 1814 | host = net.JoinHostPort(host, strconv.Itoa(tlsPort)) | ||
| 1813 | } | 1815 | } | ||
| 1814 | target := "https://" + host + r.RequestURI | 1816 | target := "https://" + host + r.RequestURI | ||
| 1815 | - | http.Redirect(w, r, target, http.StatusMovedPermanently) | 1817 | + | http.Redirect(w, r, target, http.StatusPermanentRedirect) |
| 1816 | }) | 1818 | }) | ||
| 1817 | proxySrv, proxyReady = startServer(fmt.Sprintf(":%d", cfg.Server.Port), redirectHandler, "proxy-redirect", nil, errChan, log) | 1819 | proxySrv, proxyReady = startServer(fmt.Sprintf(":%d", cfg.Server.Port), redirectHandler, "proxy-redirect", nil, errChan, log) | ||
| 1818 | tlsSrv, tlsReady = startTLSServer( | 1820 | tlsSrv, tlsReady = startTLSServer( | ||
M internal/usecase/auth/service_test.go +6 -6
| @@ -180,7 +180,7 @@ func TestService_GenerateAccessToken_RejectsInvalidExpiry(t *testing.T) { | |||
| 180 | 180 | assert.Contains(t, err.Error(), "expiry must be positive") | |
| 181 | 181 | ||
| 182 | 182 | // Expiry exceeding max should be rejected | |
| 183 | - | _, err = svc.GenerateAccessToken(ctx, "testuser", []string{"repository:myrepo:pull"}, 2*time.Hour) | |
| 183 | + | _, err = svc.GenerateAccessToken(ctx, "testuser", []string{"repository:myrepo:pull"}, MaxAccessTokenLifetime+time.Minute) | |
| 184 | 184 | assert.Error(t, err) | |
| 185 | 185 | assert.Contains(t, err.Error(), "exceeds maximum") | |
| 186 | 186 | } | |
| @@ -206,7 +206,7 @@ func TestService_ValidateToken_LongLivedRequiresStore(t *testing.T) { | |||
| 206 | 206 | }). | |
| 207 | 207 | Return(nil) | |
| 208 | 208 | ||
| 209 | - | token, err := svc.GenerateToken(ctx, "testuser", []string{"push", "pull"}, 2*time.Hour) | |
| 209 | + | token, err := svc.GenerateToken(ctx, "testuser", []string{"push", "pull"}, MaxAccessTokenLifetime+time.Minute) | |
| 210 | 210 | assert.NoError(t, err) | |
| 211 | 211 | ||
| 212 | 212 | // Validation should call GetToken and IsRevoked | |
| @@ -283,7 +283,7 @@ func TestService_ValidateToken_Success(t *testing.T) { | |||
| 283 | 283 | Return(nil) | |
| 284 | 284 | ||
| 285 | 285 | ctx := testContext() | |
| 286 | - | tokenStr, err := svc.GenerateToken(ctx, "testsubject", []string{"push", "pull"}, 2*time.Hour) | |
| 286 | + | tokenStr, err := svc.GenerateToken(ctx, "testsubject", []string{"push", "pull"}, MaxAccessTokenLifetime+time.Minute) | |
| 287 | 287 | require.NoError(t, err) | |
| 288 | 288 | ||
| 289 | 289 | // Expect GetToken to be called to verify token exists | |
| @@ -341,7 +341,7 @@ func TestService_ValidateToken_Revoked(t *testing.T) { | |||
| 341 | 341 | Return(nil) | |
| 342 | 342 | ||
| 343 | 343 | ctx := testContext() | |
| 344 | - | tokenStr, err := svc.GenerateToken(ctx, "testsubject", []string{"push", "pull"}, 2*time.Hour) | |
| 344 | + | tokenStr, err := svc.GenerateToken(ctx, "testsubject", []string{"push", "pull"}, MaxAccessTokenLifetime+time.Minute) | |
| 345 | 345 | require.NoError(t, err) | |
| 346 | 346 | ||
| 347 | 347 | // Expect GetToken to be called to verify token exists | |
| @@ -752,7 +752,7 @@ func TestExtendTokenSlidesExpiry(t *testing.T) { | |||
| 752 | 752 | }). | |
| 753 | 753 | Return(nil) | |
| 754 | 754 | ||
| 755 | - | tokenStr, err := svc.GenerateToken(ctx, "testuser", []string{"admin:*:*"}, 2*time.Hour) | |
| 755 | + | tokenStr, err := svc.GenerateToken(ctx, "testuser", []string{"admin:*:*"}, MaxAccessTokenLifetime+time.Minute) | |
| 756 | 756 | require.NoError(t, err) | |
| 757 | 757 | ||
| 758 | 758 | // ExtendToken flow: | |
| @@ -824,7 +824,7 @@ func TestExtendTokenDebounce(t *testing.T) { | |||
| 824 | 824 | }). | |
| 825 | 825 | Return(nil) | |
| 826 | 826 | ||
| 827 | - | tokenStr, err := svc.GenerateToken(ctx, "testuser", []string{"admin:*:*"}, 2*time.Hour) | |
| 827 | + | tokenStr, err := svc.GenerateToken(ctx, "testuser", []string{"admin:*:*"}, MaxAccessTokenLifetime+time.Minute) | |
| 828 | 828 | require.NoError(t, err) | |
| 829 | 829 | ||
| 830 | 830 | // First extend: flow is ValidateToken(GetToken+IsRevoked) + GetToken(debounce) + UpdateTokenExpiry | |
| @@ -180,7 +180,7 @@ func TestService_GenerateAccessToken_RejectsInvalidExpiry(t *testing.T) { | |||||
| 180 | assert.Contains(t, err.Error(), "expiry must be positive") | 180 | assert.Contains(t, err.Error(), "expiry must be positive") | ||
| 181 | 181 | ||||
| 182 | // Expiry exceeding max should be rejected | 182 | // Expiry exceeding max should be rejected | ||
| 183 | - | _, err = svc.GenerateAccessToken(ctx, "testuser", []string{"repository:myrepo:pull"}, 2*time.Hour) | 183 | + | _, err = svc.GenerateAccessToken(ctx, "testuser", []string{"repository:myrepo:pull"}, MaxAccessTokenLifetime+time.Minute) |
| 184 | assert.Error(t, err) | 184 | assert.Error(t, err) | ||
| 185 | assert.Contains(t, err.Error(), "exceeds maximum") | 185 | assert.Contains(t, err.Error(), "exceeds maximum") | ||
| 186 | } | 186 | } | ||
| @@ -206,7 +206,7 @@ func TestService_ValidateToken_LongLivedRequiresStore(t *testing.T) { | |||||
| 206 | }). | 206 | }). | ||
| 207 | Return(nil) | 207 | Return(nil) | ||
| 208 | 208 | ||||
| 209 | - | token, err := svc.GenerateToken(ctx, "testuser", []string{"push", "pull"}, 2*time.Hour) | 209 | + | token, err := svc.GenerateToken(ctx, "testuser", []string{"push", "pull"}, MaxAccessTokenLifetime+time.Minute) |
| 210 | assert.NoError(t, err) | 210 | assert.NoError(t, err) | ||
| 211 | 211 | ||||
| 212 | // Validation should call GetToken and IsRevoked | 212 | // Validation should call GetToken and IsRevoked | ||
| @@ -283,7 +283,7 @@ func TestService_ValidateToken_Success(t *testing.T) { | |||||
| 283 | Return(nil) | 283 | Return(nil) | ||
| 284 | 284 | ||||
| 285 | ctx := testContext() | 285 | ctx := testContext() | ||
| 286 | - | tokenStr, err := svc.GenerateToken(ctx, "testsubject", []string{"push", "pull"}, 2*time.Hour) | 286 | + | tokenStr, err := svc.GenerateToken(ctx, "testsubject", []string{"push", "pull"}, MaxAccessTokenLifetime+time.Minute) |
| 287 | require.NoError(t, err) | 287 | require.NoError(t, err) | ||
| 288 | 288 | ||||
| 289 | // Expect GetToken to be called to verify token exists | 289 | // Expect GetToken to be called to verify token exists | ||
| @@ -341,7 +341,7 @@ func TestService_ValidateToken_Revoked(t *testing.T) { | |||||
| 341 | Return(nil) | 341 | Return(nil) | ||
| 342 | 342 | ||||
| 343 | ctx := testContext() | 343 | ctx := testContext() | ||
| 344 | - | tokenStr, err := svc.GenerateToken(ctx, "testsubject", []string{"push", "pull"}, 2*time.Hour) | 344 | + | tokenStr, err := svc.GenerateToken(ctx, "testsubject", []string{"push", "pull"}, MaxAccessTokenLifetime+time.Minute) |
| 345 | require.NoError(t, err) | 345 | require.NoError(t, err) | ||
| 346 | 346 | ||||
| 347 | // Expect GetToken to be called to verify token exists | 347 | // Expect GetToken to be called to verify token exists | ||
| @@ -752,7 +752,7 @@ func TestExtendTokenSlidesExpiry(t *testing.T) { | |||||
| 752 | }). | 752 | }). | ||
| 753 | Return(nil) | 753 | Return(nil) | ||
| 754 | 754 | ||||
| 755 | - | tokenStr, err := svc.GenerateToken(ctx, "testuser", []string{"admin:*:*"}, 2*time.Hour) | 755 | + | tokenStr, err := svc.GenerateToken(ctx, "testuser", []string{"admin:*:*"}, MaxAccessTokenLifetime+time.Minute) |
| 756 | require.NoError(t, err) | 756 | require.NoError(t, err) | ||
| 757 | 757 | ||||
| 758 | // ExtendToken flow: | 758 | // ExtendToken flow: | ||
| @@ -824,7 +824,7 @@ func TestExtendTokenDebounce(t *testing.T) { | |||||
| 824 | }). | 824 | }). | ||
| 825 | Return(nil) | 825 | Return(nil) | ||
| 826 | 826 | ||||
| 827 | - | tokenStr, err := svc.GenerateToken(ctx, "testuser", []string{"admin:*:*"}, 2*time.Hour) | 827 | + | tokenStr, err := svc.GenerateToken(ctx, "testuser", []string{"admin:*:*"}, MaxAccessTokenLifetime+time.Minute) |
| 828 | require.NoError(t, err) | 828 | require.NoError(t, err) | ||
| 829 | 829 | ||||
| 830 | // First extend: flow is ValidateToken(GetToken+IsRevoked) + GetToken(debounce) + UpdateTokenExpiry | 830 | // First extend: flow is ValidateToken(GetToken+IsRevoked) + GetToken(debounce) + UpdateTokenExpiry | ||
M pkg/validation/path.go +3 -1
| @@ -136,7 +136,9 @@ func ValidateDomainParam(domain string) error { | |||
| 136 | 136 | if strings.Contains(domain, "..") { | |
| 137 | 137 | return fmt.Errorf("domain contains path traversal sequence") | |
| 138 | 138 | } | |
| 139 | - | if strings.ContainsAny(domain, "\x00\n\r/") { | |
| 139 | + | if strings.ContainsRune(domain, '/') || strings.IndexFunc(domain, func(r rune) bool { | |
| 140 | + | return r < 0x20 || r == 0x7f | |
| 141 | + | }) != -1 { | |
| 140 | 142 | return fmt.Errorf("domain contains invalid characters") | |
| 141 | 143 | } | |
| 142 | 144 | return nil | |
| @@ -136,7 +136,9 @@ func ValidateDomainParam(domain string) error { | |||||
| 136 | if strings.Contains(domain, "..") { | 136 | if strings.Contains(domain, "..") { | ||
| 137 | return fmt.Errorf("domain contains path traversal sequence") | 137 | return fmt.Errorf("domain contains path traversal sequence") | ||
| 138 | } | 138 | } | ||
| 139 | - | if strings.ContainsAny(domain, "\x00\n\r/") { | 139 | + | if strings.ContainsRune(domain, '/') || strings.IndexFunc(domain, func(r rune) bool { |
| 140 | + | return r < 0x20 || r == 0x7f | |||
| 141 | + | }) != -1 { | |||
| 140 | return fmt.Errorf("domain contains invalid characters") | 142 | return fmt.Errorf("domain contains invalid characters") | ||
| 141 | } | 143 | } | ||
| 142 | return nil | 144 | return nil | ||
M pkg/validation/path_test.go +1 -0
| @@ -218,6 +218,7 @@ func TestValidateDomainParam(t *testing.T) { | |||
| 218 | 218 | {"empty", "", false}, | |
| 219 | 219 | {"double dot in middle", "foo..bar", false}, | |
| 220 | 220 | {"forward slash", "example/com", false}, | |
| 221 | + | {"tab character", "example\t.com", false}, | |
| 221 | 222 | } | |
| 222 | 223 | for _, tt := range tests { | |
| 223 | 224 | t.Run(tt.name, func(t *testing.T) { | |
| @@ -218,6 +218,7 @@ func TestValidateDomainParam(t *testing.T) { | |||||
| 218 | {"empty", "", false}, | 218 | {"empty", "", false}, | ||
| 219 | {"double dot in middle", "foo..bar", false}, | 219 | {"double dot in middle", "foo..bar", false}, | ||
| 220 | {"forward slash", "example/com", false}, | 220 | {"forward slash", "example/com", false}, | ||
| 221 | + | {"tab character", "example\t.com", false}, | |||
| 221 | } | 222 | } | ||
| 222 | for _, tt := range tests { | 223 | for _, tt := range tests { | ||
| 223 | t.Run(tt.name, func(t *testing.T) { | 224 | t.Run(tt.name, func(t *testing.T) { | ||