Skip to content
This repository was archived by the owner on Feb 8, 2026. It is now read-only.

Harmonize configs: refactor database config nesting #88

Merged
neavra merged 3 commits into from
Jun 11, 2025
Merged

Harmonize configs: refactor database config nesting #88

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f66c93b
refactor database config nesting
neavra Jun 11, 2025
7cce445
fix migration-test
neavra Jun 11, 2025
0be5cfc
remove outdated comments
neavra Jun 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions .github/workflows/migration-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,10 @@ jobs:
"server": {
"host": "localhost",
"port": 8080,
"jwt_secret": "test-secret",
"database": {
"dsn": "postgres://myuser:mypassword@localhost:5432/vultisig-plugin?sslmode=disable"
}
"jwt_secret": "test-secret"
},
"database": {
"dsn": "postgres://myuser:mypassword@localhost:5432/vultisig-plugin?sslmode=disable"
},
Comment on lines +55 to 59

@coderabbitai coderabbitai Bot Jun 11, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Config key mismatch: jwt_secret vs encryption_secret

api.ServerConfig exposes EncryptionSecret, but the JSON fixture still uses jwt_secret.
This field will be silently dropped by viper.Unmarshal, so authentication may break at runtime.

Change the snippet to:

-          "jwt_secret": "test-secret"
+          "encryption_secret": "test-secret"

(or add a mapstructure tag alias if you must retain the old name).

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
"jwt_secret": "test-secret"
},
"database": {
"dsn": "postgres://myuser:mypassword@localhost:5432/vultisig-plugin?sslmode=disable"
},
"encryption_secret": "test-secret"
},
"database": {
"dsn": "postgres://myuser:mypassword@localhost:5432/vultisig-plugin?sslmode=disable"
},
🧰 Tools
🪛 Checkov (3.2.334)

[MEDIUM] 58-59: Basic Auth Credentials

(CKV_SECRET_4)

🤖 Prompt for AI Agents 
In .github/workflows/migration-test.yml around lines 55 to 59, the configuration
key "jwt_secret" should be renamed to "encryption_secret" to match the expected
field name in api.ServerConfig. Update the JSON fixture to use
"encryption_secret" instead of "jwt_secret" to ensure viper.Unmarshal correctly
maps the value and prevents runtime authentication issues.

"redis": {
"host": "localhost",
Expand Down
7 changes: 1 addition & 6 deletions api/server_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,13 @@
package api

// ServerConfig holds the configuration parameters for the server.
//
//
// Fields:
// - Host: The hostname or IP address where the server will run.
// - Port: The port number on which the server will listen for incoming requests.
// - EncryptionSecret: A secret key used for encrypting sensitive data.
// - Database: Configuration for the database connection, including:
// - DSN: The Data Source Name (DSN) for connecting to the database.
type ServerConfig struct {
Host string `mapstructure:"host" json:"host,omitempty"`
Port int64 `mapstructure:"port" json:"port,omitempty"`
EncryptionSecret string `mapstructure:"encryption_secret" json:"encryption_secret,omitempty"`
Database struct {
DSN string `mapstructure:"dsn" json:"dsn,omitempty"`
} `mapstructure:"database" json:"database,omitempty"`
}
5 changes: 4 additions & 1 deletion cmd/dca/server/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ import (
)

type DCAServerConfig struct {
Server api.ServerConfig `mapstructure:"server" json:"server"`
Server api.ServerConfig `mapstructure:"server" json:"server"`
Database struct {
DSN string `mapstructure:"dsn" json:"dsn,omitempty"`
} `mapstructure:"database" json:"database,omitempty"`
Comment on lines +15 to +18

@coderabbitai coderabbitai Bot Jun 11, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Factor Database into a shared type to avoid copy-paste divergence

Database is now declared anonymously in both DCA and Payroll configs (and likely elsewhere).
Anonymous duplication makes future additions (e.g., max-open-conns) error-prone because every struct has to be edited.

+type DatabaseConfig struct {
+    DSN string `mapstructure:"dsn" json:"dsn,omitempty"`
+}
 
 type DCAServerConfig struct {
     Server api.ServerConfig `mapstructure:"server" json:"server"`
-    Database struct {
-        DSN string `mapstructure:"dsn" json:"dsn,omitempty"`
-    } `mapstructure:"database" json:"database,omitempty"`
+    Database DatabaseConfig `mapstructure:"database" json:"database,omitempty"`
     …
 }

Placing DatabaseConfig in plugin/api (or a small config package) keeps the definition single-sourced and import-friendly.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Server api.ServerConfig `mapstructure:"server" json:"server"`
Database struct {
DSN string `mapstructure:"dsn" json:"dsn,omitempty"`
} `mapstructure:"database" json:"database,omitempty"`
// Extract a shared DatabaseConfig type (e.g. in plugin/api or a config package)
type DatabaseConfig struct {
DSN string `mapstructure:"dsn" json:"dsn,omitempty"`
}
type DCAServerConfig struct {
Server api.ServerConfig `mapstructure:"server" json:"server"`
Database DatabaseConfig `mapstructure:"database" json:"database,omitempty"`
…
}
🤖 Prompt for AI Agents 
In cmd/dca/server/config.go around lines 15 to 18, the Database struct is
declared anonymously, causing duplication across DCA and Payroll configs. To fix
this, define a shared DatabaseConfig struct in a common package like plugin/api
or a dedicated config package, then replace the anonymous Database struct with
this shared type in all relevant configs. This centralizes the definition and
prevents divergence when adding new fields.

BaseConfigPath string `mapstructure:"base_config_path" json:"base_config_path,omitempty"`
Redis storage.RedisConfig `mapstructure:"redis" json:"redis,omitempty"`
BlockStorage vault_config.BlockStorageConfig `mapstructure:"block_storage" json:"block_storage,omitempty"`
Expand Down
2 changes: 1 addition & 1 deletion cmd/dca/server/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func main() {
panic(err)
}

db, err := postgres.NewPostgresBackend(cfg.Server.Database.DSN, nil)
db, err := postgres.NewPostgresBackend(cfg.Database.DSN, nil)
if err != nil {
logger.Fatalf("Failed to connect to database: %v", err)
Comment on lines +53 to 55

@coderabbitai coderabbitai Bot Jun 11, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid logging raw DSNs – credentials will leak to logs

postgres.NewPostgresBackend logs the full DSN (see storage/postgres/db.go:28). Because the DSN contains the DB password, every server start currently prints credentials to your log aggregator.

-db, err := postgres.NewPostgresBackend(cfg.Database.DSN, nil)
+// TODO: strip password before passing to NewPostgresBackend or change the
+// backend constructor to accept a masked string for logging purposes.
+db, err := postgres.NewPostgresBackend(cfg.Database.DSN, nil)

Consider:

  1. Changing NewPostgresBackend to log a redacted DSN (e.g., postgres://****@host/db).
  2. Passing the password via PGPASSWORD/SSLKEY env vars instead of embedding it in the DSN.

This is a high-risk credential-exposure vector.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
db, err := postgres.NewPostgresBackend(cfg.Database.DSN, nil)
if err != nil {
logger.Fatalf("Failed to connect to database: %v", err)
// TODO: strip password before passing to NewPostgresBackend or change the
// backend constructor to accept a masked string for logging purposes.
db, err := postgres.NewPostgresBackend(cfg.Database.DSN, nil)
if err != nil {
logger.Fatalf("Failed to connect to database: %v", err)
}
🤖 Prompt for AI Agents 
In cmd/dca/server/main.go around lines 53 to 55, the current code passes the
full DSN containing credentials to NewPostgresBackend, which logs the raw DSN
and exposes sensitive information. To fix this, modify NewPostgresBackend in
storage/postgres/db.go to redact sensitive parts of the DSN (like the password)
before logging, replacing them with placeholders such as "****". Additionally,
refactor the code to avoid embedding the password in the DSN string by passing
it through environment variables like PGPASSWORD or SSLKEY instead, ensuring
credentials are not exposed in logs.

}
Expand Down
5 changes: 4 additions & 1 deletion cmd/payroll/server/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ import (
)

type PayrollServerConfig struct {
Server api.ServerConfig `mapstructure:"server" json:"server"`
Server api.ServerConfig `mapstructure:"server" json:"server"`
Database struct {
DSN string `mapstructure:"dsn" json:"dsn,omitempty"`
} `mapstructure:"database" json:"database,omitempty"`
Comment thread
neavra marked this conversation as resolved.
BaseConfigPath string `mapstructure:"base_config_path" json:"base_config_path,omitempty"`
Redis storage.RedisConfig `mapstructure:"redis" json:"redis,omitempty"`
BlockStorage vault_config.BlockStorageConfig `mapstructure:"block_storage" json:"block_storage,omitempty"`
Expand Down
2 changes: 1 addition & 1 deletion cmd/payroll/server/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func main() {
panic(err)
}

db, err := postgres.NewPostgresBackend(cfg.Server.Database.DSN, nil)
db, err := postgres.NewPostgresBackend(cfg.Database.DSN, nil)
if err != nil {
logger.Fatalf("Failed to connect to database: %v", err)
}
Expand Down
5 changes: 3 additions & 2 deletions config-plugin.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,14 @@ server:
port: 8081
verifier_url: "http://localhost:8081"
base_config_path: /etc/vultisig
database:
dsn: postgres://myuser:mypassword@localhost:5432/vultisig-plugin?sslmode=disable
vaults_file_path: /tmp/plugin/vaults
mode: plugin
plugin:
type: dca

database:
dsn: postgres://myuser:mypassword@localhost:5432/vultisig-plugin?sslmode=disable

Comment on lines +13 to +15

@coderabbitai coderabbitai Bot Jun 11, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Plain-text DB credentials in committed config

The dsn embeds myuser:mypassword directly in a repo-tracked YAML file. Even for local/dev use, this normalises storing secrets in VCS.

Recommended:

-dsn: postgres://myuser:mypassword@localhost:5432/vultisig-plugin?sslmode=disable
+dsn: ${PLUGIN_DB_DSN:?set PLUGIN_DB_DSN in env/secret manager}

Pair this with a .env, GitHub Actions secret, or HashiCorp Vault entry.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
database:
dsn: postgres://myuser:mypassword@localhost:5432/vultisig-plugin?sslmode=disable
database:
- dsn: postgres://myuser:mypassword@localhost:5432/vultisig-plugin?sslmode=disable
+ dsn: ${PLUGIN_DB_DSN:?set PLUGIN_DB_DSN in env/secret manager}
🧰 Tools
🪛 Checkov (3.2.334)

[MEDIUM] 14-15: Basic Auth Credentials

(CKV_SECRET_4)

🤖 Prompt for AI Agents 
In config-plugin.yaml around lines 13 to 15, the database DSN contains
plain-text credentials directly in the version-controlled file, which is
insecure. Remove the hardcoded username and password from the DSN and instead
reference environment variables or external secret management solutions like a
.env file or Vault. Update the configuration to load these sensitive values
dynamically at runtime rather than storing them in the YAML file.

relay:
server: https://api.vultisig.com/router

Expand Down
5 changes: 3 additions & 2 deletions config-verifier.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,14 @@
server:
host: localhost
port: 8080
database:
dsn: postgres://myuser:mypassword@localhost:5432/vultisig-verifier?sslmode=disable
vaults_file_path: /tmp/verifier/vaults
mode: verifier
plugin:
type: dca

database:
dsn: postgres://myuser:mypassword@localhost:5432/vultisig-verifier?sslmode=disable

plugin:
plugin_configs:
dca:
Expand Down