Skip to content

fix(config): enforce full validation on CLI flags#511

Merged
aaronbrethorst merged 1 commit intoOneBusAway:mainfrom
3rabiii:fix/config-validation
Mar 1, 2026
Merged

fix(config): enforce full validation on CLI flags#511
aaronbrethorst merged 1 commit intoOneBusAway:mainfrom
3rabiii:fix/config-validation

Conversation

@3rabiii
Copy link
Copy Markdown
Contributor

@3rabiii 3rabiii commented Feb 28, 2026

Description
This PR addresses the configuration validation bypass issue where validation rules were only applied when loading configurations from a JSON file, leaving CLI flags unchecked.

The Problem:
Previously, an operator could start the server via CLI flags with misconfigured values (e.g., port=0, empty API keys, invalid rate limits) and receive no warnings or errors, potentially leading to insecure or unstable states.

The Solution:
Instead of duplicating the validation logic, this PR enforces the existing robust validation rules on CLI inputs by:

  • Exporting the validate() method to Validate() in internal/appconf/json_config.go.

  • Packing the parsed CLI flags into a temporary JSONConfig struct in cmd/api/main.go.

  • Running the shared Validate() method before converting the struct to the internal app configurations.

Impact:
CLI inputs are now strictly protected by the same rules as JSON configurations, including:

  • Port range enforcement (1-65535)

  • Valid environment name checks

  • Rate limit minimums

  • Non-empty and duplicate API key detection

  • Path traversal prevention for data paths

@aaronbrethorst
fixes : #498

Extracted the validation logic by exporting JSONConfig.Validate() and applying it to CLI-provided configuration. This ensures that parameters like port ranges, rate limits, API keys, and file paths are strictly validated regardless of whether the app is configured via JSON or CLI flags, closing the validation bypass gap
Copy link
Copy Markdown
Member

@aaronbrethorst aaronbrethorst left a comment

Choose a reason for hiding this comment

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

Hey Adel, clean solution here — reusing Validate() instead of duplicating rules is exactly the right call. The restructuring of the CLI path to pack flags into a JSONConfig and run the shared validation + conversion pipeline makes the two config paths symmetrical, which is much easier to reason about.

Fit and Finish

1. Remove the // DRY! and // This allows us to... comments (follow-up PR)

The two inline comments at the top of the CLI block are more noise than signal:

// Pack the CLI flags into a temporary JSONConfig struct
// This allows us to run the exact same robust validation logic as the JSON path!

and

// Convert to internal app configs (DRY!)

The code is self-explanatory — a JSONConfig being validated and converted speaks for itself. Could you remove both in a quick follow-up?

Verification

  • Tests: all pass
  • Lint: 0 issues
  • Validate() export is clean — JSONConfig is already exported, so making its validation method public is consistent
  • Flag defaults match setDefaults() values, so skipping setDefaults() in the CLI path is correct
  • ToGtfsConfigData() correctly defaults Enabled to true when feed.Enabled is nil, so the omitted Enabled: true on GtfsRtFeed in the CLI struct is fine
  • ParseAPIKeys("") returns []string{} which Validate() correctly rejects as "api-keys cannot be empty"
  • CLA: signed

Strengths

  • DRY in practice, not just in theory: Instead of copying validation rules or adding a second validation pass, this funnels CLI flags through the existing validated pipeline. One set of rules, two entry points.
  • Net deletion: 41 lines removed, 56 added, but the added lines include the JSONConfig struct literal which replaces scattered manual assignments. The result is structurally simpler.
  • Correct defaults: The flag defaults (port 4000, env "development", rate-limit 100, etc.) align with setDefaults(), so the CLI path doesn't need to call setDefaults() — the values are already populated from flag.Parse().

Merging now. Thanks Adel! Please take care of the followups sometime soon!

@aaronbrethorst aaronbrethorst merged commit 1b10520 into OneBusAway:main Mar 1, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CLI flag config mode bypasses all validation

2 participants