Copilot commented on code in PR #1565:
URL: https://github.com/apache/dubbo-admin/pull/1565#discussion_r4109809348
##########
pkg/config/console/auth/config_test.go:
##########
@@ -23,19 +23,52 @@ import (
)
func validConfig() *Config {
- return &Config{User: "admin", Password: "secret", ExpirationTime: 3600}
+ return &Config{
+ User: "admin",
+ Password: "secret",
+ ExpirationTime: 3600,
+ SessionSecret: strings.Repeat("s", MinimumSessionSecretLength),
+ }
}
-func TestConfigValidateDefaultsPasswordOnly(t *testing.T) {
+func TestConfigValidatePasswordOnly(t *testing.T) {
cfg := validConfig()
if err := cfg.Validate(); err != nil {
t.Fatalf("Validate() error = %v", err)
}
if len(cfg.Methods) != 1 || cfg.Methods[0] != MethodPassword {
t.Fatalf("Methods = %v, want [%s]", cfg.Methods, MethodPassword)
}
- if cfg.SessionSecret != DefaultSessionSecret {
- t.Fatalf("SessionSecret = %q, want legacy default",
cfg.SessionSecret)
+ if cfg.SessionSecret != strings.Repeat("s", MinimumSessionSecretLength)
{
+ t.Fatalf("SessionSecret = %q, want configured secret",
cfg.SessionSecret)
+ }
+}
+
+func TestConfigValidateRequiresSessionSecret(t *testing.T) {
+ cfg := validConfig()
Review Comment:
This missing-secret test inherits `DUBBO_ADMIN_SESSION_SECRET` from the
process, so it reports no error whenever the test suite runs in an environment
that legitimately sets the new variable. Clear the variable explicitly to make
the missing-secret scenario deterministic.
##########
pkg/config/console/auth/config.go:
##########
@@ -89,8 +91,11 @@ func (c *Config) Validate() error {
if c.ExpirationTime <= 0 || c.ExpirationTime >= 24*60*60 {
return errors.New("auth: expirationTime should be greater than
0 and less than 86400")
}
- if c.SessionSecret == "" {
- c.SessionSecret = DefaultSessionSecret
+ if strings.TrimSpace(c.SessionSecret) == "" {
+ c.SessionSecret = os.Getenv(SessionSecretEnvVar)
+ }
+ if strings.TrimSpace(c.SessionSecret) == "" ||
len([]byte(c.SessionSecret)) < MinimumSessionSecretLength {
+ return fmt.Errorf("auth: sessionSecret must contain at least %d
bytes", MinimumSessionSecretLength)
Review Comment:
This validation can still be bypassed by a top-level `console: null`
configuration. `AdminConfig.Validate` replaces a nil console with
`DefaultConsoleConfig` but does not validate that replacement
(`pkg/config/app/admin.go:156-160`); now that the default has no secret,
startup proceeds to `cookie.NewStore([]byte(""))`, restoring a publicly
forgeable signing key. Validate the newly assigned default (and add a
regression test for a nil console) so every startup path fails closed.
##########
pkg/config/console/config_test.go:
##########
@@ -43,6 +43,18 @@ func TestReleaseProviderRequiresStrongSessionSecret(t
*testing.T) {
}
}
+func TestPasswordAuthenticationRequiresStrongSessionSecret(t *testing.T) {
+ cfg := DefaultConsoleConfig()
Review Comment:
This test does not isolate the newly supported environment fallback. If
`DUBBO_ADMIN_SESSION_SECRET` is present, `DefaultConsoleConfig()` validates
successfully and the asserted missing-secret failure disappears; explicitly
clear the variable before constructing the config.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
