fix: strict usage of shiori prefix for environment variables in configuration (#807)

* fix: disable direct os lookuper

* config.setdefaults call config.http.setdefaults

* tests

* log level default in local run server

* store log level in configuration

Makefile

@@ -44,7 +44,7 @@ serve:
 ## Runs server for local development
 .PHONY: run-server
 run-server:
-	GIN_MODE=$(GIN_MODE) SHIORI_DEVELOPMENT=$(SHIORI_DEVELOPMENT) SHIORI_DIR=$(SHIORI_DIR) SHIORI_HTTP_SECRET_KEY=shiori go run main.go server
+	GIN_MODE=$(GIN_MODE) SHIORI_DEVELOPMENT=$(SHIORI_DEVELOPMENT) SHIORI_DIR=$(SHIORI_DIR) SHIORI_HTTP_SECRET_KEY=shiori go run main.go server --log-level debug
 
 ## Generate swagger docs
 .PHONY: swagger

internal/cmd/root.go

@@ -71,6 +71,7 @@ func initShiori(ctx context.Context, cmd *cobra.Command) (*config.Config, *depen
 	}
 
 	cfg := config.ParseServerConfiguration(ctx, logger)
+	cfg.LogLevel = logger.Level.String()
 
 	if storageDirectory != "" && cfg.Storage.DataDir != "" {
 		logger.Warn("--storage-directory is set, overriding SHIORI_DIR.")
@@ -125,6 +126,8 @@ func initShiori(ctx context.Context, cmd *cobra.Command) (*config.Config, *depen
 		}
 	}
 
+	cfg.DebugConfiguration(logger)
+
 	return cfg, dependencies
 }
 

internal/cmd/server.go

@@ -41,8 +41,6 @@ func newServerCommandHandler() func(cmd *cobra.Command, args []string) {
 
 		cfg, dependencies := initShiori(ctx, cmd)
 
-		cfg.Http.SetDefaults(dependencies.Log)
-
 		// Validate root path
 		if rootPath == "" {
 			rootPath = "/"

internal/config/config.go

@@ -16,14 +16,14 @@ import (
 
 // readDotEnv reads the configuration from variables in a .env file (only for contributing)
 func readDotEnv(logger *logrus.Logger) map[string]string {
+	result := make(map[string]string)
+
 	file, err := os.Open(".env")
 	if err != nil {
-		return nil
+		return result
 	}
 	defer file.Close()
 
-	result := make(map[string]string)
-
 	scanner := bufio.NewScanner(file)
 
 	for scanner.Scan() {
@@ -33,6 +33,11 @@ func readDotEnv(logger *logrus.Logger) map[string]string {
 		}
 
 		keyval := strings.SplitN(line, "=", 2)
+		if len(keyval) != 2 {
+			logger.WithField("line", line).Warn("invalid line in .env file")
+			continue
+		}
+
 		result[keyval[0]] = keyval[1]
 	}
 
@@ -60,6 +65,19 @@ type HttpConfig struct {
 	DisablePreParseMultipartForm bool          `env:"HTTP_DISABLE_PARSE_MULTIPART_FORM,default=true"`
 }
 
+// SetDefaults sets the default values for the configuration
+func (c *HttpConfig) SetDefaults(logger *logrus.Logger) {
+	// Set a random secret key if not set
+	if len(c.SecretKey) == 0 {
+		logger.Warn("SHIORI_HTTP_SECRET_KEY is not set, using random value. This means that all sessions will be invalidated on server restart.")
+		randomUUID, err := uuid.NewV4()
+		if err != nil {
+			logger.WithError(err).Fatal("couldn't generate a random UUID")
+		}
+		c.SecretKey = []byte(randomUUID.String())
+	}
+}
+
 type DatabaseConfig struct {
 	DBMS string `env:"DBMS"` // Deprecated
 	// DBMS requires more environment variables. Check the database package for more information.
@@ -73,23 +91,10 @@ type StorageConfig struct {
 type Config struct {
 	Hostname    string `env:"HOSTNAME,required"`
 	Development bool   `env:"DEVELOPMENT,default=False"`
+	LogLevel    string // Set only from the CLI flag
 	Database    *DatabaseConfig
 	Storage     *StorageConfig
-	// LogLevel string `env:"LOG_LEVEL,default=info"`
+	Http        *HttpConfig
-	Http *HttpConfig
-}
-
-// SetDefaults sets the default values for the configuration
-func (c *HttpConfig) SetDefaults(logger *logrus.Logger) {
-	// Set a random secret key if not set
-	if len(c.SecretKey) == 0 {
-		logger.Warn("SHIORI_HTTP_SECRET_KEY is not set, using random value. This means that all sessions will be invalidated on server restart.")
-		randomUUID, err := uuid.NewV4()
-		if err != nil {
-			logger.WithError(err).Fatal("couldn't generate a random UUID")
-		}
-		c.SecretKey = []byte(randomUUID.String())
-	}
 }
 
 // SetDefaults sets the default values for the configuration
@@ -108,8 +113,33 @@ func (c Config) SetDefaults(logger *logrus.Logger, portableMode bool) {
 	if c.Database.DBMS == "" && c.Database.URL == "" {
 		c.Database.URL = fmt.Sprintf("sqlite:///%s", filepath.Join(c.Storage.DataDir, "shiori.db"))
 	}
+
+	c.Http.SetDefaults(logger)
 }
 
+func (c *Config) DebugConfiguration(logger *logrus.Logger) {
+	logger.Debug("Configuration:")
+	logger.Debugf(" SHIORI_HOSTNAME: %s", c.Hostname)
+	logger.Debugf(" SHIORI_DEVELOPMENT: %t", c.Development)
+	logger.Debugf(" SHIORI_DATABASE_URL: %s", c.Database.URL)
+	logger.Debugf(" SHIORI_DBMS: %s", c.Database.DBMS)
+	logger.Debugf(" SHIORI_DIR: %s", c.Storage.DataDir)
+	logger.Debugf(" SHIORI_HTTP_ENABLED: %t", c.Http.Enabled)
+	logger.Debugf(" SHIORI_HTTP_PORT: %d", c.Http.Port)
+	logger.Debugf(" SHIORI_HTTP_ADDRESS: %s", c.Http.Address)
+	logger.Debugf(" SHIORI_HTTP_ROOT_PATH: %s", c.Http.RootPath)
+	logger.Debugf(" SHIORI_HTTP_ACCESS_LOG: %t", c.Http.AccessLog)
+	logger.Debugf(" SHIORI_HTTP_SERVE_WEB_UI: %t", c.Http.ServeWebUI)
+	logger.Debugf(" SHIORI_HTTP_SECRET_KEY: %d characters", len(c.Http.SecretKey))
+	logger.Debugf(" SHIORI_HTTP_BODY_LIMIT: %d", c.Http.BodyLimit)
+	logger.Debugf(" SHIORI_HTTP_READ_TIMEOUT: %s", c.Http.ReadTimeout)
+	logger.Debugf(" SHIORI_HTTP_WRITE_TIMEOUT: %s", c.Http.WriteTimeout)
+	logger.Debugf(" SHIORI_HTTP_IDLE_TIMEOUT: %s", c.Http.IDLETimeout)
+	logger.Debugf(" SHIORI_HTTP_DISABLE_KEEP_ALIVE: %t", c.Http.DisableKeepAlive)
+	logger.Debugf(" SHIORI_HTTP_DISABLE_PARSE_MULTIPART_FORM: %t", c.Http.DisablePreParseMultipartForm)
+}
+
+// ParseServerConfiguration parses the configuration from the enabled lookupers
 func ParseServerConfiguration(ctx context.Context, logger *logrus.Logger) *Config {
 	var cfg Config
 
@@ -117,7 +147,6 @@ func ParseServerConfiguration(ctx context.Context, logger *logrus.Logger) *Confi
 		envconfig.MapLookuper(map[string]string{"HOSTNAME": os.Getenv("HOSTNAME")}),
 		envconfig.MapLookuper(readDotEnv(logger)),
 		envconfig.PrefixLookuper("SHIORI_", envconfig.OsLookuper()),
-		envconfig.OsLookuper(),
 	)
 	if err := envconfig.ProcessWith(ctx, &cfg, lookuper); err != nil {
 		logger.WithError(err).Fatal("Error parsing configuration")

internal/config/config_test.go

@@ -0,0 +1,113 @@
+package config
+
+import (
+	"context"
+	"os"
+	"testing"
+
+	"github.com/sirupsen/logrus"
+	"github.com/stretchr/testify/require"
+)
+
+func TestHostnameVariable(t *testing.T) {
+	os.Setenv("HOSTNAME", "test_hostname")
+	defer os.Unsetenv("HOSTNAME")
+
+	log := logrus.New()
+	cfg := ParseServerConfiguration(context.TODO(), log)
+
+	require.Equal(t, "test_hostname", cfg.Hostname)
+}
+
+// TestBackwardsCompatibility tests that the old environment variables changed from 1.5.5 onwards
+// are still supported and working with the new configuration system.
+func TestBackwardsCompatibility(t *testing.T) {
+	for _, env := range []struct {
+		env  string
+		want string
+		eval func(t *testing.T, cfg *Config)
+	}{
+		{"HOSTNAME", "test_hostname", func(t *testing.T, cfg *Config) {
+			require.Equal(t, "test_hostname", cfg.Hostname)
+		}},
+		{"SHIORI_DIR", "test", func(t *testing.T, cfg *Config) {
+			require.Equal(t, "test", cfg.Storage.DataDir)
+		}},
+		{"SHIORI_DBMS", "test", func(t *testing.T, cfg *Config) {
+			require.Equal(t, "test", cfg.Database.DBMS)
+		}},
+	} {
+		t.Run(env.env, func(t *testing.T) {
+			os.Setenv(env.env, env.want)
+			t.Cleanup(func() {
+				os.Unsetenv(env.env)
+			})
+
+			log := logrus.New()
+			cfg := ParseServerConfiguration(context.Background(), log)
+			env.eval(t, cfg)
+		})
+	}
+}
+
+func TestReadDotEnv(t *testing.T) {
+	log := logrus.New()
+
+	for _, testCase := range []struct {
+		name string
+		line string
+		env  map[string]string
+	}{
+		{"empty", "", map[string]string{}},
+		{"comment", "# comment", map[string]string{}},
+		{"ignore invalid lines", "invalid line", map[string]string{}},
+		{"single variable", "SHIORI_HTTP_PORT=9999", map[string]string{"SHIORI_HTTP_PORT": "9999"}},
+		{"multiple variable", "SHIORI_HTTP_PORT=9999\nSHIORI_HTTP_SECRET_KEY=123123", map[string]string{"SHIORI_HTTP_PORT": "9999", "SHIORI_HTTP_SECRET_KEY": "123123"}},
+	} {
+		t.Run(testCase.name, func(t *testing.T) {
+			tmpDir, err := os.MkdirTemp("", "")
+			require.NoError(t, err)
+
+			os.Chdir(tmpDir)
+
+			t.Cleanup(func() {
+				require.NoError(t, os.RemoveAll(tmpDir))
+			})
+
+			// Write the .env file in the temporary directory
+			handler, err := os.OpenFile(".env", os.O_CREATE|os.O_WRONLY, 0655)
+			require.NoError(t, err)
+			handler.Write([]byte(testCase.line + "\n"))
+			handler.Close()
+
+			e := readDotEnv(log)
+
+			require.Equal(t, testCase.env, e)
+		})
+	}
+
+	t.Run("no file", func(t *testing.T) {
+		tmpDir, err := os.MkdirTemp("", "")
+		require.NoError(t, err)
+
+		os.Chdir(tmpDir)
+
+		t.Cleanup(func() {
+			require.NoError(t, os.RemoveAll(tmpDir))
+		})
+
+		e := readDotEnv(log)
+
+		require.Equal(t, map[string]string{}, e)
+	})
+}
+
+func TestConfigSetDefaults(t *testing.T) {
+	log := logrus.New()
+	cfg := ParseServerConfiguration(context.TODO(), log)
+	cfg.SetDefaults(log, false)
+
+	require.NotEmpty(t, cfg.Http.SecretKey)
+	require.NotEmpty(t, cfg.Storage.DataDir)
+	require.NotEmpty(t, cfg.Database.URL)
+}