Skip to content

feat(security): redact API keys from logs and enforce SHA-256 hashing#471

Closed
3rabiii wants to merge 1 commit intoOneBusAway:mainfrom
3rabiii:fix/redact-api-keys
Closed

feat(security): redact API keys from logs and enforce SHA-256 hashing#471
3rabiii wants to merge 1 commit intoOneBusAway:mainfrom
3rabiii:fix/redact-api-keys

Conversation

@3rabiii
Copy link
Contributor

@3rabiii 3rabiii commented Feb 24, 2026

Resolves #470

What changed?

This PR addresses the critical security issues regarding API key visibility to ensure keys are not leaked in server logs or compromised via static configuration files.

  • Request Log Redaction:
    • Refactored the logging middleware to redact the key parameter to [REDACTED].
    • Ensures sensitive keys are never written to Datadog/CloudWatch or local stdout.
  • Configuration Key Hashing:
    • The server now expects appconf.Config.ApiKeys to be stored as SHA-256 hex hashes rather than plaintext.
    • Incoming request keys are hashed in-memory and securely matched to prevent timing attacks while eliminating plaintext secrets from server memory/configs.
    • Updated config.example.json and default fallback configurations to reflect this requirement.
  • Test Suite Adaptation:
    • Refactored integration tests to transparently hash plaintext test keys on the fly, ensuring all 75+ endpoints pass the new authentication middleware.
  • Documentation:
    • Added a new security section to README.md guiding developers on how to properly hash their keys.
image

@aaronbrethorst
fixes : #470

@aaronbrethorst
Copy link
Member

Hey Adel, thanks for putting time and thought into this -- I appreciate you thinking about the security posture of the project. The log redaction work is a nice touch, and the code itself is well-written.

However, after reviewing this, I've decided not to move forward with these changes. Here's my reasoning:

OneBusAway API keys aren't secrets. The iOS and Android apps' API keys are freely available to anyone online -- they're embedded in open-source client apps and can be used on every OBA server in existence. They function more as client identifiers for rate limiting than as authentication secrets. Hashing them in the config adds operational complexity (operators now need to run echo -n 'key' | sha256sum to configure their server) without a meaningful security benefit, since the keys themselves are public knowledge.

This change would also be a breaking change for every existing deployment -- anyone upgrading with plaintext keys in their config would get silent 401s on all requests, which is a significant risk for the ecosystem.

API key security is something we should think about in the future, but when we do, it'll likely be part of a larger effort (e.g., moving to a proper authentication system, bearer tokens, or OAuth). Introducing SHA-256 hashing for keys that are already public feels like premature optimization for a problem we haven't fully scoped yet.

Thanks again for the initiative, and I hope this doesn't discourage you from contributing. There's plenty of impactful work to be done on the project!

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.

Security Vulnerability: API key leakage in request logs and plaintext configuration

2 participants