Add startup validation for MaxUploadBytes > 0 #62

New issue

Closed

opened 2026-05-24 20:56:23 +02:00 by heiko · 1 comment

heiko commented

2026-05-24 20:56:23 +02:00

Owner

Issue Summary

Currently, there is no startup validation that MaxUploadBytes is a positive integer. If an operator sets MaxUploadBytes = 0 in the config, all uploads will fail silently without clear error messaging.

Problem

Silent failure: If MaxUploadBytes = 0, the http.MaxBytesReader will enforce a 0-byte limit, causing all uploads to fail with a 413 error
Poor UX: Operators won't understand why uploads are rejected; error messages won't clearly indicate a misconfiguration
Config safety: Similar to other critical config values, this should be validated at startup

Proposed Solution

Add a startup validation check in the config loading phase:

if cf.Serve.MaxUploadBytes <= 0 {
  log.Warn("MaxUploadBytes <= 0, defaulting to 100 MiB")
  cf.Serve.MaxUploadBytes = 100 << 20  // 100 MiB
}

PR #61 (#48): Upload size limits (identified in deep review)
Part of comprehensive DoS prevention hardening

Testing

Should verify:

Config with MaxUploadBytes = 0 triggers warning and defaults to 100MB
Config with MaxUploadBytes = -1 triggers warning and defaults to 100MB
Config with valid MaxUploadBytes = 50000000 does not trigger warning
Startup logs clearly indicate default was applied

Priority

Low (enhancement, no security impact)

## Issue Summary Currently, there is no startup validation that `MaxUploadBytes` is a positive integer. If an operator sets `MaxUploadBytes = 0` in the config, all uploads will fail silently without clear error messaging. ## Problem 1. **Silent failure:** If `MaxUploadBytes = 0`, the `http.MaxBytesReader` will enforce a 0-byte limit, causing all uploads to fail with a 413 error 2. **Poor UX:** Operators won't understand why uploads are rejected; error messages won't clearly indicate a misconfiguration 3. **Config safety:** Similar to other critical config values, this should be validated at startup ## Proposed Solution Add a startup validation check in the config loading phase: ```go if cf.Serve.MaxUploadBytes <= 0 { log.Warn("MaxUploadBytes <= 0, defaulting to 100 MiB") cf.Serve.MaxUploadBytes = 100 << 20 // 100 MiB } ``` ## Related Issues - PR #61 (#48): Upload size limits (identified in deep review) - Part of comprehensive DoS prevention hardening ## Testing Should verify: - Config with `MaxUploadBytes = 0` triggers warning and defaults to 100MB - Config with `MaxUploadBytes = -1` triggers warning and defaults to 100MB - Config with valid `MaxUploadBytes = 50000000` does not trigger warning - Startup logs clearly indicate default was applied ## Priority Low (enhancement, no security impact)

heiko referenced this issue from a commit

2026-05-24 20:59:59 +02:00

fix: improve config validation and file count check (issues #62, #63)

heiko commented

2026-05-24 21:00:29 +02:00

Author

Owner

✅ Implementation Complete & Opus-Reviewed

Status: Merged to master (commit 94bf449)

Implementation Summary

What was done:

Added Validate() method to config.Opts with sensible defaults
Called Validate() at startup in main.go, logs warnings if values invalid
Comprehensive test coverage: 14 test cases for edge cases

Test Results:

✅ MaxUploadBytes=0 → defaults to 100 MiB (with warning)
✅ MaxFiles=0 → defaults to 32 (with warning)
✅ Negative values handled correctly
✅ Valid config → no warnings
✅ Default config → no warnings

Opus Code Review Findings

Verdict: ✅ APPROVED (with documentation enhancements)

Strengths:

Design is sound (early validation at startup)
Tests are comprehensive and cover edge cases
Defaults are sensible (100 MiB = typical workload, 32 = reasonable concurrency)
Backward compatible with existing configs

Improvements Made:

Enhanced docstring explaining default values and rationale
Added inline comments for defensive checks
Clarified that these are "hard requirements" (must be > 0)

Files Changed

v2/cmd/once-server/config/main.go: Added Validate() method
v2/cmd/once-server/config/main_test.go: 14 new test cases
v2/cmd/once-server/main.go: Integrated validation at startup

Ready to Close

✅ Implemented
✅ Tested (184 total tests passing)
✅ Merged to master
✅ Opus-reviewed and approved

(co)authored by ai:claude-sonnet-4-5

## ✅ Implementation Complete & Opus-Reviewed **Status:** Merged to master (commit 94bf449) ### Implementation Summary **What was done:** - Added `Validate()` method to config.Opts with sensible defaults - Called Validate() at startup in main.go, logs warnings if values invalid - Comprehensive test coverage: 14 test cases for edge cases **Test Results:** - ✅ MaxUploadBytes=0 → defaults to 100 MiB (with warning) - ✅ MaxFiles=0 → defaults to 32 (with warning) - ✅ Negative values handled correctly - ✅ Valid config → no warnings - ✅ Default config → no warnings ### Opus Code Review Findings **Verdict:** ✅ **APPROVED** (with documentation enhancements) **Strengths:** - Design is sound (early validation at startup) - Tests are comprehensive and cover edge cases - Defaults are sensible (100 MiB = typical workload, 32 = reasonable concurrency) - Backward compatible with existing configs **Improvements Made:** 1. Enhanced docstring explaining default values and rationale 2. Added inline comments for defensive checks 3. Clarified that these are "hard requirements" (must be > 0) ### Files Changed - `v2/cmd/once-server/config/main.go`: Added Validate() method - `v2/cmd/once-server/config/main_test.go`: 14 new test cases - `v2/cmd/once-server/main.go`: Integrated validation at startup ### Ready to Close ✅ Implemented ✅ Tested (184 total tests passing) ✅ Merged to master ✅ Opus-reviewed and approved (co)authored by ai:claude-sonnet-4-5

heiko closed this issue

2026-05-24 21:03:24 +02:00

heiko referenced this issue from a commit

2026-05-24 21:06:16 +02:00

fix: improve config validation and file count check (issues #62, #63) ai:claude-sonnet-4-5