10 Commits

Author SHA1 Message Date
22f19c849b style: use the config key error-message convention for signing_key
All checks were successful
check / check (push) Successful in 1m48s
The signing_key errors used bare phrasing while every other validation
error follows the 'config key %q' convention; align them. The secret
value itself is still never echoed.
2026-08-07 17:04:08 +00:00
83fa22871e fix: reject allowlist_hosts entries containing no hostname labels
Entries consisting only of dots (".", "..") are now a startup error
naming the key and entry. Previously a bare "." passed validation and
became a suffix pattern matching every trailing-dot FQDN upstream,
bypassing URL signing.
2026-08-07 17:03:50 +00:00
808356f142 test: allowlist_hosts must reject dot-only entries (PR #53 rework)
A bare "." entry becomes a HasSuffix suffix pattern that matches any
upstream written in FQDN trailing-dot form (evil.com.), effectively
disabling URL signing with one character. Failing test first, fix
follows.
2026-08-07 17:03:32 +00:00
5297e6033a fix: abort startup on explicitly empty db_url instead of deriving
The derived file:...state.sqlite3 URL is a default and defaults apply
only to omitted keys: db_url set to an empty string now aborts naming
the key, matching the existing behavior of state_dir: "".
2026-08-07 17:03:12 +00:00
813ff63153 test: explicitly empty db_url must abort startup (PR #53 rework)
db_url: "" currently silently derives the state_dir-based sqlite URL,
which is a default applied to a SET value; state_dir: "" already
aborts. Failing test first, fix follows.
2026-08-07 17:02:44 +00:00
c0d325156f fix: abort startup when a config key is explicitly set to null
An explicitly-null key (port: null, bare port:, debug: ~, metrics
subkeys, and every other known key) previously fell through the ok/nil
check in the strict getters and silently took the default, violating
the no-silent-fallback rule and contradicting metrics: null which
already aborted. validateKnownKeys now collects null-valued keys (top
level and metrics subkeys) and aborts naming each one, and the strict
getters and validateAllowlistHostsValue error on null instead of
defaulting as defense in depth. This also replaces the unhelpful
'value <nil> is not a map of metrics settings' rendering for
metrics: null with the null-specific message.
2026-08-07 17:02:16 +00:00
370545997f test: explicit null config values must abort startup (PR #53 rework)
An explicitly-null key (port: null, bare port:, debug: ~, and every
other config key including metrics subkeys) is a SET value under the
no-silent-fallback rule and must abort startup naming the key, instead
of silently taking the default as it does today. All 13 subtests fail
against the current behavior; the fix follows.
2026-08-07 17:01:16 +00:00
11e9206c21 fix: suppress gosec G703 false positive on state_dir probe removal
The pinned CI linter (golangci-lint v2.10.1) flags os.Remove(probePath)
in ensureStateDirWritable as G703 path traversal via taint analysis.
probePath comes from os.CreateTemp inside the StateDir that the probe
just validated, so the taint finding is a false positive; suppress it
with a justified nolint comment matching the existing precedent in
loadConfigFile. Verified against the pinned linter version via the
Dockerfile lint stage (0 issues).
2026-08-07 17:00:20 +00:00
745a461688 Merge origin/main into feature/config-validation (TODO.md bookkeeping) 2026-08-07 16:57:44 +00:00
5d0b5f864e docs: record manual test pass of auth and encrypted URL flows (closes #49) (#50)
All checks were successful
check / check (push) Successful in 4s
closes #49

Records the P0 manual test pass in `TODO.md` per its Workflow section
(checked-off results into Completed Steps; cache size management and
eviction promoted to Next Step). `TODO.md` is the only changed file —
no production code changes, as the issue requires.

## Test setup

`pixad` built from `main` at `6573b9d` via `make build`, run on port
18099 with a throwaway local config (temp state dir, known
`signing_key`, `allowlist_hosts` including `s3.sneak.cloud`), driven
with curl using explicit cookie replay (session cookies are
`Secure`/`HttpOnly`/`SameSite=Strict`).

## Results — all six checks PASS

1. **Login form**: GET `/` → HTTP 200, `Pixa - Login` page with
   `name="key"` password form.
2. **Wrong key error**: POST `/` with `key=wrong-key` → HTTP 200 login
   page containing "Invalid signing key".
3. **Generator form**: POST `/` with the correct signing key → HTTP 303
   to `/` with `Set-Cookie: pixa_session=...; HttpOnly; Secure;
   SameSite=Strict`; GET `/` with that cookie → `Pixa - URL Generator`
   with the `/generate` form and logout link.
4. **Encrypted URL serves image**: POST `/generate` (ttl=3600) produced
   a `/v1/e/<token>/img.jpeg` URL → HTTP 200, `Content-Type:
   image/jpeg`, 800x600 baseline JPEG, 61706 bytes.
5. **Expired URL → 410**: a ttl=1 URL fetched after 3 s → HTTP 410 Gone
   with `{"error":"URL has expired","status":410,...}`.
6. **Logout**: GET `/logout` → HTTP 303 to `/` with `Set-Cookie:
   pixa_session=; Max-Age=0`; subsequent GET `/` → login form again.

Additionally, all nine checks in `scripts/manual-test.sh` passed
against the same server instance.

## Verification

`make check` green on the branch head (all tests, golangci-lint 0
issues, fmt-check clean) — the first fully green `make check` on a
`main`-derived branch under the current linter, confirming the #47/#48
fix on merged `main`.

Note for review: the test execution was performed this session; the
adversarial re-review (independently re-running the six flows) is still
pending and should happen before merge.

Co-authored-by: sneak <sneak@sneak.berlin>
Reviewed-on: #50
Co-authored-by: clawbot <clawbot@noreply.example.org>
Co-committed-by: clawbot <clawbot@noreply.example.org>
2026-08-07 18:44:01 +02:00
3 changed files with 267 additions and 27 deletions

32
TODO.md
View File

@@ -19,11 +19,8 @@ on main.
# Next Step
P0: manual test pass of the auth and encrypted URL flows, then commit
the checked-off results to TODO.md: visit / and see the login form;
wrong key shows an error; correct signing key shows the generator form;
a generated encrypted URL serves the image; an expired URL (short TTL)
returns 410; logout redirects back to login
P0: implement cache size management and eviction so the disk cannot
fill up
# Completed Steps
@@ -33,6 +30,29 @@ returns 410; logout redirects back to login
to omitted keys), unknown config keys abort startup, a malformed
config file aborts instead of being skipped, and `state_dir` is
verified creatable and writable before the listener binds
- 2026-08-07 manual test pass of the auth and encrypted URL flows
against a locally built and running `pixad` (built from `main` at
`6573b9d`, port 18099, local throwaway config); all six checks
passed, plus all nine tests in `scripts/manual-test.sh` (closes #49):
- [x] visit `/` and see the login form: HTTP 200, `Pixa - Login`
page with `name="key"` password form
- [x] wrong key shows an error: POST `/` with `key=wrong-key`
returned HTTP 200 login page containing "Invalid signing key"
- [x] correct signing key shows the generator form: POST `/`
returned HTTP 303 to `/` with
`Set-Cookie: pixa_session=...; HttpOnly; Secure; SameSite=Strict`;
GET `/` with that cookie rendered `Pixa - URL Generator` with the
`/generate` form and logout link
- [x] a generated encrypted URL serves the image: POST `/generate`
(ttl=3600) produced a `/v1/e/<token>/img.jpeg` URL that returned
HTTP 200, `Content-Type: image/jpeg`, an 800x600 baseline JPEG of
61706 bytes
- [x] an expired URL (short TTL) returns 410: a ttl=1 URL fetched
after 3 s returned HTTP 410 Gone with
`{"error":"URL has expired","status":410,...}`
- [x] logout redirects back to login: GET `/logout` returned HTTP
303 to `/` with `Set-Cookie: pixa_session=; Max-Age=0`;
subsequent GET `/` rendered the login form again
- 2026-08-07 fix the two remaining gosec findings (G124 in
internal/session): session cookies now always carry
Secure/HttpOnly/SameSite=Strict on both the set and clear paths;
@@ -59,8 +79,6 @@ returns 410; logout redirects back to login
# Future Steps
- P0: implement cache size management and eviction so the disk cannot
fill up
- P1: implement blocked networks configuration to extend SSRF
protection
- P1: rate limit global concurrent upstream fetches to prevent

View File

@@ -113,9 +113,19 @@ func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) {
"upstream_connections_per_host", DefaultUpstreamConnectionsPerHost),
}
// Build DBURL from StateDir if not explicitly set
// Build DBURL from StateDir if not explicitly set. The derived URL
// is a default: it applies only when db_url is omitted, never to an
// explicitly empty value.
c.DBURL = loader.stringVal("db_url", "")
if c.DBURL == "" {
if c.DBURL == "" && loader.err == nil {
if sc != nil {
if _, present := sc.Get("db_url"); present {
return nil, fmt.Errorf(
"config key %q: value must not be empty; omit the key to derive it from state_dir",
"db_url")
}
}
c.DBURL = fmt.Sprintf("file:%s/state.sqlite3?_journal_mode=WAL", c.StateDir)
}
@@ -132,10 +142,12 @@ func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) {
// validateKnownKeys rejects configuration files containing keys the
// application does not understand, so typos fail at startup instead of
// being silently ignored. The env section is permitted because
// being silently ignored, and rejects keys that are explicitly set to
// null: a null is a SET value, never an omission, so it must not
// silently take the default. The env section is permitted because
// smartconfig consumes it for environment variable injection.
func validateKnownKeys(sc *smartconfig.Config) error {
var unknown []string
var unknown, nullKeys []string
for key, value := range sc.Data() {
if !isKnownConfigKey(key) {
@@ -144,6 +156,12 @@ func validateKnownKeys(sc *smartconfig.Config) error {
continue
}
if value == nil {
nullKeys = append(nullKeys, key)
continue
}
if key == "metrics" {
metricsMap, ok := value.(map[string]interface{})
if !ok {
@@ -152,9 +170,15 @@ func validateKnownKeys(sc *smartconfig.Config) error {
"metrics", value)
}
for subkey := range metricsMap {
for subkey, subvalue := range metricsMap {
if subkey != "username" && subkey != "password" {
unknown = append(unknown, "metrics."+subkey)
continue
}
if subvalue == nil {
nullKeys = append(nullKeys, "metrics."+subkey)
}
}
}
@@ -166,9 +190,29 @@ func validateKnownKeys(sc *smartconfig.Config) error {
return fmt.Errorf("unknown config keys: %s", strings.Join(unknown, ", "))
}
if len(nullKeys) > 0 {
sort.Strings(nullKeys)
if len(nullKeys) == 1 {
return errNullConfigValue(nullKeys[0])
}
return fmt.Errorf(
"config keys %s: value is null; omit a key entirely to use its default",
strings.Join(nullKeys, ", "))
}
return nil
}
// errNullConfigValue reports a config key that is explicitly set to
// null (including the bare "key:" form and the "~" alias). Silently
// applying the default would mask a truncated or typo'd config entry.
func errNullConfigValue(key string) error {
return fmt.Errorf(
"config key %q: value is null; omit the key entirely to use the default", key)
}
// isKnownConfigKey reports whether key is a permitted top-level
// configuration key.
func isKnownConfigKey(key string) bool {
@@ -206,6 +250,7 @@ func (c *Config) ensureStateDirWritable() error {
"state_dir", probePath, err)
}
//nolint:gosec // G703: probePath comes from os.CreateTemp inside the just-validated StateDir
if err := os.Remove(probePath); err != nil {
return fmt.Errorf("config key %q: cannot remove probe file %q: %w",
"state_dir", probePath, err)
@@ -217,15 +262,16 @@ func (c *Config) ensureStateDirWritable() error {
// validate checks that all required configuration values are set and
// that every value is within its valid range.
func (c *Config) validate() error {
// The signing key value is never echoed in error messages.
if c.SigningKey == "" {
return fmt.Errorf("signing_key is required")
return fmt.Errorf("config key %q: a value is required", "signing_key")
}
// Minimum key length for security (32 bytes = 256 bits)
const minKeyLength = 32
if len(c.SigningKey) < minKeyLength {
return fmt.Errorf("signing_key must be at least %d characters, got %d",
minKeyLength, len(c.SigningKey))
return fmt.Errorf("config key %q: value must be at least %d characters, got %d",
"signing_key", minKeyLength, len(c.SigningKey))
}
const maxPort = 65535
@@ -267,7 +313,11 @@ func (c *Config) validate() error {
// validateAllowlistHost checks that an allowlist_hosts entry is a bare
// hostname, optionally with a leading dot for suffix matching. URLs,
// paths, and whitespace indicate a misconfigured entry.
// paths, and whitespace indicate a misconfigured entry. An entry with
// no hostname labels (such as ".") is rejected: the allowlist matcher
// treats a leading dot as a suffix pattern, so a bare "." would match
// any upstream host written in FQDN trailing-dot form and effectively
// disable URL signing.
func validateAllowlistHost(host string) error {
if strings.Contains(host, "://") || strings.ContainsAny(host, "/ \t") {
return fmt.Errorf(
@@ -275,6 +325,12 @@ func validateAllowlistHost(host string) error {
"allowlist_hosts", host)
}
if strings.Trim(host, ".") == "" {
return fmt.Errorf(
"config key %q: entry %q contains no hostname labels",
"allowlist_hosts", host)
}
return nil
}
@@ -370,17 +426,22 @@ func (l *strictLoader) boolVal(key string, defaultVal bool) bool {
}
// getString returns the string value for key, or defaultVal if the key
// is omitted. A present value that is not a string is an error.
// is omitted. A present value that is not a string, or is explicitly
// null, is an error.
func getString(sc *smartconfig.Config, key, defaultVal string) (string, error) {
if sc == nil {
return defaultVal, nil
}
raw, ok := sc.Get(key)
if !ok || raw == nil {
if !ok {
return defaultVal, nil
}
if raw == nil {
return "", errNullConfigValue(key)
}
str, ok := raw.(string)
if !ok {
return "", fmt.Errorf("config key %q: value %v (%T) is not a string",
@@ -391,18 +452,22 @@ func getString(sc *smartconfig.Config, key, defaultVal string) (string, error) {
}
// getInt returns the integer value for key, or defaultVal if the key is
// omitted. A present value that is not a whole number is an error;
// fractional values are never truncated.
// omitted. A present value that is not a whole number, or is explicitly
// null, is an error; fractional values are never truncated.
func getInt(sc *smartconfig.Config, key string, defaultVal int) (int, error) {
if sc == nil {
return defaultVal, nil
}
raw, ok := sc.Get(key)
if !ok || raw == nil {
if !ok {
return defaultVal, nil
}
if raw == nil {
return 0, errNullConfigValue(key)
}
switch val := raw.(type) {
case int:
return val, nil
@@ -429,17 +494,22 @@ func getInt(sc *smartconfig.Config, key string, defaultVal int) (int, error) {
// getBool returns the boolean value for key, or defaultVal if the key
// is omitted. A present value that is not a boolean (or a ParseBool-able
// string) is an error; numbers are not accepted as booleans.
// string), or is explicitly null, is an error; numbers are not accepted
// as booleans.
func getBool(sc *smartconfig.Config, key string, defaultVal bool) (bool, error) {
if sc == nil {
return defaultVal, nil
}
raw, ok := sc.Get(key)
if !ok || raw == nil {
if !ok {
return defaultVal, nil
}
if raw == nil {
return false, errNullConfigValue(key)
}
switch val := raw.(type) {
case bool:
return val, nil
@@ -458,17 +528,21 @@ func getBool(sc *smartconfig.Config, key string, defaultVal bool) (bool, error)
// validateAllowlistHostsValue checks the raw shape of the
// allowlist_hosts value before the lenient extraction in getStringSlice
// runs: a value that is not a list of strings (or a comma-separated
// string), a non-string entry, or an empty entry is an error, never
// silently skipped.
// runs: an explicitly null value, a value that is not a list of strings
// (or a comma-separated string), a non-string entry, or an empty entry
// is an error, never silently skipped.
func validateAllowlistHostsValue(sc *smartconfig.Config) error {
const key = "allowlist_hosts"
raw, ok := sc.Get(key)
if !ok || raw == nil {
if !ok {
return nil
}
if raw == nil {
return errNullConfigValue(key)
}
switch val := raw.(type) {
case []interface{}:
for _, item := range val {

View File

@@ -295,6 +295,154 @@ func TestSetButInvalidValueAbortsStartup(t *testing.T) {
}
}
// TestExplicitNullValueAbortsStartup verifies that a key explicitly
// set to null (including the bare "key:" form and the "~" alias) aborts
// startup naming the key. An explicit null is a SET value: it must
// never silently fall back to the default the way an omitted key does.
func TestExplicitNullValueAbortsStartup(t *testing.T) {
signingKeyLine := "signing_key: " + validTestSigningKey + "\n"
cases := []struct {
name string
yaml string
// wantErrSubstrings must all appear in the error message.
wantErrSubstrings []string
}{
{
name: "port explicit null",
yaml: signingKeyLine + "port: null\n",
wantErrSubstrings: []string{"port", "null"},
},
{
name: "port bare key no value",
yaml: signingKeyLine + "port:\n",
wantErrSubstrings: []string{"port", "null"},
},
{
name: "debug tilde null",
yaml: signingKeyLine + "debug: ~\n",
wantErrSubstrings: []string{"debug", "null"},
},
{
name: "maintenance_mode null",
yaml: signingKeyLine + "maintenance_mode: null\n",
wantErrSubstrings: []string{"maintenance_mode", "null"},
},
{
name: "allow_http null",
yaml: signingKeyLine + "allow_http: null\n",
wantErrSubstrings: []string{"allow_http", "null"},
},
{
name: "state_dir null",
yaml: signingKeyLine + "state_dir: null\n",
wantErrSubstrings: []string{"state_dir", "null"},
},
{
name: "db_url null",
yaml: signingKeyLine + "db_url: null\n",
wantErrSubstrings: []string{"db_url", "null"},
},
{
name: "sentry_dsn null",
yaml: signingKeyLine + "sentry_dsn: null\n",
wantErrSubstrings: []string{"sentry_dsn", "null"},
},
{
name: "upstream_connections_per_host null",
yaml: signingKeyLine + "upstream_connections_per_host: null\n",
wantErrSubstrings: []string{"upstream_connections_per_host", "null"},
},
{
name: "allowlist_hosts null",
yaml: signingKeyLine + "allowlist_hosts: null\n",
wantErrSubstrings: []string{"allowlist_hosts", "null"},
},
{
name: "signing_key null",
yaml: "signing_key: null\n",
wantErrSubstrings: []string{"signing_key", "null"},
},
{
name: "metrics null",
yaml: signingKeyLine + "metrics: null\n",
wantErrSubstrings: []string{"metrics", "null"},
},
{
name: "metrics subkeys null",
yaml: signingKeyLine + "metrics:\n username: null\n password: null\n",
wantErrSubstrings: []string{
"metrics.username", "metrics.password", "null",
},
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
c, err := configFromYAML(t, tc.yaml)
if err == nil {
t.Fatalf("config with %s must abort startup, got config: %+v", tc.name, c)
}
t.Logf("got expected error: %v", err)
for _, want := range tc.wantErrSubstrings {
if !strings.Contains(err.Error(), want) {
t.Errorf("error %q does not mention %q", err.Error(), want)
}
}
})
}
}
// TestExplicitlyEmptyDBURLAbortsStartup verifies that db_url set to an
// empty string aborts startup: the derived file:...state.sqlite3 URL is
// a default, and defaults apply only to omitted keys. This matches
// state_dir, where an explicitly empty value already aborts.
func TestExplicitlyEmptyDBURLAbortsStartup(t *testing.T) {
yamlContent := "signing_key: " + validTestSigningKey + "\ndb_url: \"\"\n"
c, err := configFromYAML(t, yamlContent)
if err == nil {
t.Fatalf("explicitly empty db_url must abort startup, got config: %+v", c)
}
t.Logf("got expected error: %v", err)
if !strings.Contains(err.Error(), "db_url") {
t.Errorf("error %q does not name the offending key db_url", err.Error())
}
}
// TestAllowlistHostsRejectsDotOnlyEntries verifies that entries with no
// hostname labels are rejected. The allowlist matcher treats a leading
// dot as a suffix pattern, so a bare "." entry would match any upstream
// host written in FQDN trailing-dot form (e.g. evil.com.) and
// effectively disable URL signing with a single character.
func TestAllowlistHostsRejectsDotOnlyEntries(t *testing.T) {
signingKeyLine := "signing_key: " + validTestSigningKey + "\n"
for _, entry := range []string{".", ".."} {
t.Run(entry, func(t *testing.T) {
yamlContent := signingKeyLine +
"allowlist_hosts:\n - \"" + entry + "\"\n"
c, err := configFromYAML(t, yamlContent)
if err == nil {
t.Fatalf("allowlist entry %q must abort startup, got config: %+v",
entry, c)
}
t.Logf("got expected error: %v", err)
if !strings.Contains(err.Error(), "allowlist_hosts") {
t.Errorf("error %q does not name the offending key allowlist_hosts",
err.Error())
}
})
}
}
func TestUnknownTopLevelKeyAbortsStartup(t *testing.T) {
yamlContent := `signing_key: ` + validTestSigningKey + `
whitelist_hosts: