[self-hosted] support embedded IDP postgres db#5443
Merged
Conversation
Entire-Checkpoint: 9ace190c1067
Entire-Checkpoint: 73a896c79614
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ServerConfig.AuthStore and wiring to let the embedded IdP use sqlite3 (default) or postgres via DSN; implements Postgres DSN parsing and builds storage configs (sqlite file or postgres DSN) with error propagation during config conversion. Changes
Sequence Diagram(s)sequenceDiagram
participant Loader as rgba(52,152,219,0.5) Config Loader
participant Server as rgba(46,204,113,0.5) ServerConfig
participant Parser as rgba(241,196,15,0.5) Postgres DSN Parser
participant Builder as rgba(155,89,182,0.5) Storage Config Builder
participant IdP as rgba(231,76,60,0.5) Embedded IdP
Loader->>Server: Load config (includes authStore)
Server->>Builder: Build relay/storage config (may include AuthStore override)
alt AuthStore.engine == "postgres"
Builder->>Parser: Parse DSN
Parser-->>Builder: Return connection params / error
end
Builder->>IdP: Provide Dex storage config (sqlite3 file or postgres dsn)
IdP->>IdP: Initialize embedded IdP storage backend
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
combined/cmd/config.go (1)
74-74: Add early validation forserver.authStorevalues.Line 74 introduces a new user-facing config surface, but
Validate()does not enforce allowed engines (sqlite3,postgres) or require DSN whenengine=postgres. This currently fails much later during IdP init and is harder to diagnose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@combined/cmd/config.go` at line 74, Validate new server.authStore early in the Config.Validate() path: ensure the StoreConfig.Engine (the server.authStore field of type StoreConfig) is one of the allowed values "sqlite3" or "postgres", and if Engine == "postgres" require a non-empty DSN (and return a clear validation error mentioning server.authStore) so misconfiguration fails fast rather than during IdP init; implement these checks inside the existing Validate() method that runs on the top-level config (refer to Validate() and the StoreConfig type to locate where to add them).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@combined/cmd/config.go`:
- Around line 604-615: The code always overwrites the SQLite auth storage file
to idp.db, ignoring a configured mgmt.Auth.Storage.File; update the
authStorageFile assignment (variable authStorageFile used in this block) so that
when authStorageType != "postgres" you first ensure a default storage type
(authStorageType = "sqlite3" if empty) and then only set authStorageFile =
path.Join(mgmt.DataDir, "idp.db") when the explicit file config is empty—i.e.,
if mgmt.Auth.Storage.File (or c.Server.AuthStore.File if that config exists) is
non-empty, preserve that value instead of replacing it.
In `@idp/dex/config.go`:
- Around line 232-238: After parsing params["port"] with strconv.ParseUint and
assigning to v, add a check that v != 0 and return a clear config error if it is
zero; i.e., after the ParseUint call in the DSN parsing block, if v == 0 return
an error like "invalid port %q: must be non-zero" (propagate the original value
p) instead of accepting port = uint16(v), so the function rejects port=0 during
DSN parsing.
- Around line 215-259: parsePostgresDSN currently only splits by strings.Fields
and therefore drops URI-style DSNs and quoted values and also only applies
sslmode="disable" while ignoring other valid modes; update parsePostgresDSN to
accept both libpq URI and key=value forms by parsing a postgres:// URI via
net/url when the input has a scheme or by implementing proper quoted key=value
parsing (instead of strings.Fields), and normalize/validate sslmode: accept the
standard modes (disable, allow, prefer, require, verify-ca, verify-full) and set
pg.SSL.Mode accordingly or return a clear error for unsupported values; make the
changes in the parsePostgresDSN function and the sslMode handling block so all
valid sslmode inputs are applied or rejected rather than silently ignored.
---
Nitpick comments:
In `@combined/cmd/config.go`:
- Line 74: Validate new server.authStore early in the Config.Validate() path:
ensure the StoreConfig.Engine (the server.authStore field of type StoreConfig)
is one of the allowed values "sqlite3" or "postgres", and if Engine ==
"postgres" require a non-empty DSN (and return a clear validation error
mentioning server.authStore) so misconfiguration fails fast rather than during
IdP init; implement these checks inside the existing Validate() method that runs
on the top-level config (refer to Validate() and the StoreConfig type to locate
where to add them).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
combined/cmd/config.gocombined/config.yaml.exampleidp/dex/config.gomanagement/server/idp/embedded.go
Entire-Checkpoint: 6556783c0df3
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
idp/dex/config.go (1)
238-244:⚠️ Potential issue | 🟡 MinorReject
port=0during DSN parsing.
ParseUintaccepts0, and this currently passes through as a valid port.💡 Proposed fix
if p, ok := params["port"]; ok && p != "" { v, err := strconv.ParseUint(p, 10, 16) if err != nil { return nil, fmt.Errorf("invalid port %q: %w", p, err) } + if v == 0 { + return nil, fmt.Errorf("invalid port %q: must be between 1 and 65535", p) + } port = uint16(v) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@idp/dex/config.go` around lines 238 - 244, The DSN parser currently accepts port=0 because strconv.ParseUint allows 0; in the port parsing block (the if that checks params["port"] and calls strconv.ParseUint) add a check after parsing that v != 0 and return an error like "invalid port 0" if it is zero, ensuring the parsed uint16 port variable is only set when v>0; update any error message to include the original p value for clarity.combined/cmd/config.go (1)
616-622:⚠️ Potential issue | 🟠 MajorConfigured
auth.storage.fileis still being overwritten.For non-Postgres storage, this block always forces
idp.db, so an explicitly configuredmgmt.Auth.Storage.Fileis ignored.💡 Proposed fix
- authStorageFile := "" + authStorageFile := mgmt.Auth.Storage.File if authStorageType != "postgres" { if authStorageType == "" { authStorageType = "sqlite3" } - authStorageFile = path.Join(mgmt.DataDir, "idp.db") + if authStorageFile == "" { + authStorageFile = path.Join(mgmt.DataDir, "idp.db") + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@combined/cmd/config.go` around lines 616 - 622, The auth storage file is always being set to path.Join(mgmt.DataDir, "idp.db"), overwriting any explicitly configured mgmt.Auth.Storage.File; change the logic in the block using authStorageType and authStorageFile so that for non-postgres backends you only assign the default idp.db when mgmt.Auth.Storage.File is empty—i.e., if authStorageType != "postgres" then set authStorageType default to "sqlite3" if empty and set authStorageFile = mgmt.Auth.Storage.File if that value is non-empty otherwise set authStorageFile = path.Join(mgmt.DataDir, "idp.db"); keep the authStorageFile variable name and authStorageType checks intact so subsequent code using authStorageFile behaves the same.
🧹 Nitpick comments (1)
combined/cmd/config.go (1)
610-615: Add fail-fast validation forserver.authStorecombinations.Consider validating
engine/dsnearlier (e.g., inValidate) so invalid combos fail with a clear config error before startup wiring.💡 Example validation (outside this segment)
func (c *CombinedConfig) Validate() error { @@ + switch c.Server.AuthStore.Engine { + case "", "sqlite3", "postgres": + default: + return fmt.Errorf("invalid server.authStore.engine %q: must be one of sqlite3, postgres", c.Server.AuthStore.Engine) + } + if c.Server.AuthStore.Engine == "postgres" && c.Server.AuthStore.DSN == "" { + return fmt.Errorf("server.authStore.dsn is required when server.authStore.engine is postgres") + } @@ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@combined/cmd/config.go` around lines 610 - 615, The current logic determines authStorageType/DSN at wiring time (authStorageType := mgmt.Auth.Storage.Type; authStorageDSN := c.Server.AuthStore.DSN; if c.Server.AuthStore.Engine != "" { authStorageType = c.Server.AuthStore.Engine }) but doesn’t validate illegal or missing combinations early; add fail-fast validation in the configuration Validate method to check c.Server.AuthStore.Engine and c.Server.AuthStore.DSN against mgmt.Auth.Storage.Type (and any allowed values) and return a clear error when an invalid combo is provided (e.g., engine set without DSN when required, or unsupported engine value), so callers of Validate fail before startup wiring; reference the same symbols (authStorageType, authStorageDSN, mgmt.Auth.Storage.Type, c.Server.AuthStore.Engine, c.Server.AuthStore.DSN) when implementing the checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@idp/dex/config.go`:
- Around line 316-319: The loop that parses DSN tokens currently uses eqIdx :=
strings.IndexByte(s, '=') and silently breaks when eqIdx < 0, which returns
partial params and hides typos; change that behavior so that instead of break
the parser returns a clear error (e.g., fmt.Errorf or wrapped error) indicating
a malformed/trailing DSN token and include the offending token (s) in the
message; update the function containing the loop (the code that declares eqIdx
and calls strings.IndexByte on s) to propagate this error to the caller rather
than continuing with partial data.
---
Duplicate comments:
In `@combined/cmd/config.go`:
- Around line 616-622: The auth storage file is always being set to
path.Join(mgmt.DataDir, "idp.db"), overwriting any explicitly configured
mgmt.Auth.Storage.File; change the logic in the block using authStorageType and
authStorageFile so that for non-postgres backends you only assign the default
idp.db when mgmt.Auth.Storage.File is empty—i.e., if authStorageType !=
"postgres" then set authStorageType default to "sqlite3" if empty and set
authStorageFile = mgmt.Auth.Storage.File if that value is non-empty otherwise
set authStorageFile = path.Join(mgmt.DataDir, "idp.db"); keep the
authStorageFile variable name and authStorageType checks intact so subsequent
code using authStorageFile behaves the same.
In `@idp/dex/config.go`:
- Around line 238-244: The DSN parser currently accepts port=0 because
strconv.ParseUint allows 0; in the port parsing block (the if that checks
params["port"] and calls strconv.ParseUint) add a check after parsing that v !=
0 and return an error like "invalid port 0" if it is zero, ensuring the parsed
uint16 port variable is only set when v>0; update any error message to include
the original p value for clarity.
---
Nitpick comments:
In `@combined/cmd/config.go`:
- Around line 610-615: The current logic determines authStorageType/DSN at
wiring time (authStorageType := mgmt.Auth.Storage.Type; authStorageDSN :=
c.Server.AuthStore.DSN; if c.Server.AuthStore.Engine != "" { authStorageType =
c.Server.AuthStore.Engine }) but doesn’t validate illegal or missing
combinations early; add fail-fast validation in the configuration Validate
method to check c.Server.AuthStore.Engine and c.Server.AuthStore.DSN against
mgmt.Auth.Storage.Type (and any allowed values) and return a clear error when an
invalid combo is provided (e.g., engine set without DSN when required, or
unsupported engine value), so callers of Validate fail before startup wiring;
reference the same symbols (authStorageType, authStorageDSN,
mgmt.Auth.Storage.Type, c.Server.AuthStore.Engine, c.Server.AuthStore.DSN) when
implementing the checks.
Entire-Checkpoint: 80d45e37782f
Entire-Checkpoint: 80d45e37782f
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
idp/dex/config.go (1)
319-322:⚠️ Potential issue | 🟡 MinorReturn an error for malformed key/value tokens instead of silently stopping.
The current
breakcan return partially parsed params and hide DSN typos.💡 Proposed fix
eqIdx := strings.IndexByte(s, '=') if eqIdx < 0 { - break + return nil, fmt.Errorf("invalid key=value DSN segment %q", s) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@idp/dex/config.go` around lines 319 - 322, In the token parsing loop where eqIdx is computed (eqIdx := strings.IndexByte(s, '=')), don't silently break on eqIdx < 0; instead return a descriptive error from the surrounding parsing function (e.g., the function that parses connector DSN params) indicating a malformed key/value token and include the offending token string (s) so callers don't receive partially parsed params; update call sites to handle the returned error as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@idp/dex/config.go`:
- Around line 319-322: In the token parsing loop where eqIdx is computed (eqIdx
:= strings.IndexByte(s, '=')), don't silently break on eqIdx < 0; instead return
a descriptive error from the surrounding parsing function (e.g., the function
that parses connector DSN params) indicating a malformed key/value token and
include the offending token string (s) so callers don't receive partially parsed
params; update call sites to handle the returned error as needed.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
idp/dex/config.go (1)
319-322:⚠️ Potential issue | 🟡 MinorSilently breaks on malformed trailing DSN token — should return an error.
When
eqIdx < 0(no=found in a remaining non-empty token), the parser silently discards the rest of the DSN string, making typos likehost=localhost dbnam mydbimpossible to diagnose.💡 Proposed fix
eqIdx := strings.IndexByte(s, '=') if eqIdx < 0 { - break + return nil, fmt.Errorf("invalid DSN token %q: expected key=value", s) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@idp/dex/config.go` around lines 319 - 322, The DSN parser loop currently does "if eqIdx < 0 { break }" which silently discards a trailing malformed token; change this to return a clear error when a non-empty remainder "s" contains no '='. In the function containing the strings.IndexByte call (the DSN parsing loop), replace the break path for "eqIdx < 0" with a returned error like "malformed DSN: missing '=' in token '<token>'" (include the contents of s or the offending token in the message) so callers can surface and handle the malformed DSN instead of silently continuing; keep existing behavior for empty s.
🧹 Nitpick comments (1)
idp/dex/config.go (1)
353-368: Missing\'backslash-escape handling for quoted DSN values.The libpq spec allows
\'as an alternative to''inside single-quoted values. The current parser will interpret the'in\'as the closing quote and leave\in the output, producing a wrong value or an "unterminated quoted value" error for passwords using that escape style.♻️ Proposed fix
func parseQuotedDSNValue(s string) (value, rest string, err error) { var buf strings.Builder for len(s) > 0 { + if s[0] == '\\' && len(s) > 1 { + buf.WriteByte(s[1]) + s = s[2:] + continue + } if s[0] == '\'' { if len(s) > 1 && s[1] == '\'' { buf.WriteByte('\'') s = s[2:] continue } return buf.String(), s[1:], nil } buf.WriteByte(s[0]) s = s[1:] } return "", "", fmt.Errorf("unterminated quoted value") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@idp/dex/config.go` around lines 353 - 368, The parseQuotedDSNValue function currently only handles doubled single-quote escapes (''), so add handling for backslash-escaped single quotes (\' ) per libpq: when parsing and you see a backslash followed by a single-quote, consume both characters and append a single-quote to buf (just like the '' branch) instead of treating the quote as a terminator; preserve the existing logic for '' and normal characters and still return an unterminated error only if the input ends without a closing quote.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@idp/dex/config.go`:
- Around line 351-352: Update the doc comment for parseQuotedDSNValue to
correctly state that libpq uses two single quotes (''), not a double-quote ("),
to represent a literal single quote inside a single-quoted DSN value; change the
line "// Libpq uses " to represent a literal single quote inside quoted values."
to use "''" so the comment accurately matches the implementation in
parseQuotedDSNValue.
---
Duplicate comments:
In `@idp/dex/config.go`:
- Around line 319-322: The DSN parser loop currently does "if eqIdx < 0 { break
}" which silently discards a trailing malformed token; change this to return a
clear error when a non-empty remainder "s" contains no '='. In the function
containing the strings.IndexByte call (the DSN parsing loop), replace the break
path for "eqIdx < 0" with a returned error like "malformed DSN: missing '=' in
token '<token>'" (include the contents of s or the offending token in the
message) so callers can surface and handle the malformed DSN instead of silently
continuing; keep existing behavior for empty s.
---
Nitpick comments:
In `@idp/dex/config.go`:
- Around line 353-368: The parseQuotedDSNValue function currently only handles
doubled single-quote escapes (''), so add handling for backslash-escaped single
quotes (\' ) per libpq: when parsing and you see a backslash followed by a
single-quote, consume both characters and append a single-quote to buf (just
like the '' branch) instead of treating the quote as a terminator; preserve the
existing logic for '' and normal characters and still return an unterminated
error only if the input ends without a closing quote.
Comment on lines
+351
to
+352
| // parseQuotedDSNValue parses a single-quoted value starting after the opening quote. | ||
| // Libpq uses ” to represent a literal single quote inside quoted values. |
Contributor
There was a problem hiding this comment.
Incorrect doc comment — " should be '' (two single quotes).
Line 352: // Libpq uses " to represent a literal single quote is garbled; libpq uses '' (doubled single quote) for escaping inside a single-quoted value, which is what the implementation correctly does.
📝 Proposed fix
-// Libpq uses " to represent a literal single quote inside quoted values.
+// Libpq uses '' (two consecutive single quotes) to represent a literal single quote inside quoted values.📝 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
| // parseQuotedDSNValue parses a single-quoted value starting after the opening quote. | |
| // Libpq uses ” to represent a literal single quote inside quoted values. | |
| // parseQuotedDSNValue parses a single-quoted value starting after the opening quote. | |
| // Libpq uses '' (two consecutive single quotes) to represent a literal single quote inside quoted values. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@idp/dex/config.go` around lines 351 - 352, Update the doc comment for
parseQuotedDSNValue to correctly state that libpq uses two single quotes (''),
not a double-quote ("), to represent a literal single quote inside a
single-quoted DSN value; change the line "// Libpq uses " to represent a literal
single quote inside quoted values." to use "''" so the comment accurately
matches the implementation in parseQuotedDSNValue.
braginini
changed the title
Entire-Checkpoint: 3eec968003d1
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
combined/cmd/config.go (1)
617-622:authStorageFilealways overrides any explicitly configuredmgmt.Auth.Storage.File.The hardcoded assignment on line 622 ignores
mgmt.Auth.Storage.File, silently relocating the IdP DB for any operator who has set that field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@combined/cmd/config.go` around lines 617 - 622, The code unconditionally sets authStorageFile to path.Join(mgmt.DataDir, "idp.db") which overwrites any explicit mgmt.Auth.Storage.File; change the logic so authStorageFile is only set to the default when no explicit file is provided: ensure authStorageFile is initialized from mgmt.Auth.Storage.File (or check mgmt.Auth.Storage.File != "") and only when that is empty and authStorageType != "postgres" (after defaulting authStorageType to "sqlite3" if blank) assign path.Join(mgmt.DataDir, "idp.db"); update the block that references authStorageFile/authStorageType to preserve mgmt.Auth.Storage.File if present.
🧹 Nitpick comments (1)
combined/cmd/config.go (1)
74-74:StoreConfig.EncryptionKeyis not used for auth storage — reusing the struct exposes a confusing no-op field.
AuthStoreis wired only toEngineandDSNdownstream (lines 614–615, 613). TheEncryptionKeyYAML field ends up in the public config surface with no effect, which can mislead operators.Consider a purpose-specific struct:
♻️ Proposed dedicated type
+// AuthStoreConfig contains auth (embedded IdP) storage settings +type AuthStoreConfig struct { + Engine string `yaml:"engine"` + DSN string `yaml:"dsn"` +} type ServerConfig struct { ... - AuthStore StoreConfig `yaml:"authStore"` + AuthStore AuthStoreConfig `yaml:"authStore"` ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@combined/cmd/config.go` at line 74, AuthStore currently reuses StoreConfig which includes an unused EncryptionKey field, confusing operators; replace the public AuthStore field with a purpose-specific struct (e.g., AuthStoreConfig) that only contains the fields actually used downstream (Engine, DSN) and remove EncryptionKey from that new type, then update references where AuthStore is consumed (code paths that read Engine and DSN) to use the new AuthStoreConfig type instead of StoreConfig so the YAML surface no longer exposes a no-op EncryptionKey.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@combined/cmd/config.go`:
- Around line 613-622: When determining auth storage, the code reads
authStorageDSN from c.Server.AuthStore.DSN but never validates it for the
postgres case, allowing an empty DSN to propagate to
EmbeddedIdP.Storage.Config.DSN; modify the block around authStorageDSN and
authStorageType so that after resolving authStorageType (and defaulting to
"sqlite3" when blank) you explicitly check if authStorageType == "postgres" and
return a clear configuration error if authStorageDSN is empty (or require the
user to supply a DSN), otherwise proceed to set auth storage fields; update any
callers that expect an error on invalid config so misconfiguration fails at
parse time rather than at IdP init.
---
Duplicate comments:
In `@combined/cmd/config.go`:
- Around line 617-622: The code unconditionally sets authStorageFile to
path.Join(mgmt.DataDir, "idp.db") which overwrites any explicit
mgmt.Auth.Storage.File; change the logic so authStorageFile is only set to the
default when no explicit file is provided: ensure authStorageFile is initialized
from mgmt.Auth.Storage.File (or check mgmt.Auth.Storage.File != "") and only
when that is empty and authStorageType != "postgres" (after defaulting
authStorageType to "sqlite3" if blank) assign path.Join(mgmt.DataDir, "idp.db");
update the block that references authStorageFile/authStorageType to preserve
mgmt.Auth.Storage.File if present.
---
Nitpick comments:
In `@combined/cmd/config.go`:
- Line 74: AuthStore currently reuses StoreConfig which includes an unused
EncryptionKey field, confusing operators; replace the public AuthStore field
with a purpose-specific struct (e.g., AuthStoreConfig) that only contains the
fields actually used downstream (Engine, DSN) and remove EncryptionKey from that
new type, then update references where AuthStore is consumed (code paths that
read Engine and DSN) to use the new AuthStoreConfig type instead of StoreConfig
so the YAML surface no longer exposes a no-op EncryptionKey.
Comment on lines
+613
to
+622
| authStorageDSN := c.Server.AuthStore.DSN | ||
| if c.Server.AuthStore.Engine != "" { | ||
| authStorageType = c.Server.AuthStore.Engine | ||
| } | ||
| authStorageFile := "" | ||
| if authStorageType != "postgres" { | ||
| if authStorageType == "" { | ||
| authStorageType = "sqlite3" | ||
| } | ||
| authStorageFile = path.Join(mgmt.DataDir, "idp.db") |
Contributor
There was a problem hiding this comment.
Silent misconfiguration when authStore.engine = postgres but no DSN is provided.
authStorageDSN is sourced from c.Server.AuthStore.DSN without any guard. When the resolved authStorageType is "postgres" and the DSN is empty, no error is returned here — the bad configuration propagates to EmbeddedIdP.Storage.Config.DSN and only fails at IdP initialization, far from the config-parsing site.
🛡️ Proposed fix
authStorageType := mgmt.Auth.Storage.Type
authStorageDSN := c.Server.AuthStore.DSN
if c.Server.AuthStore.Engine != "" {
authStorageType = c.Server.AuthStore.Engine
}
+if authStorageType == "postgres" && authStorageDSN == "" {
+ return nil, fmt.Errorf("authStore.dsn is required when authStore.engine is postgres")
+}
authStorageFile := ""
if authStorageType != "postgres" {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@combined/cmd/config.go` around lines 613 - 622, When determining auth
storage, the code reads authStorageDSN from c.Server.AuthStore.DSN but never
validates it for the postgres case, allowing an empty DSN to propagate to
EmbeddedIdP.Storage.Config.DSN; modify the block around authStorageDSN and
authStorageType so that after resolving authStorageType (and defaulting to
"sqlite3" when blank) you explicitly check if authStorageType == "postgres" and
return a clear configuration error if authStorageDSN is empty (or require the
user to supply a DSN), otherwise proceed to set auth storage fields; update any
callers that expect an error on invalid config so misconfiguration fails at
parse time rather than at IdP init.
Entire-Checkpoint: b17839d3d8c6
Entire-Checkpoint: 0f083effa20e
|
shuuri-labs
approved these changes
Feb 27, 2026
bcmmbaga
approved these changes
Feb 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
Bug Fixes
Documentation