Conversation
…gelog Replace plain SHA-256 API key hashing with HMAC-SHA256 using the server-side JWT secret, preventing offline brute-force of stored key hashes (resolves CodeQL alert #6). The broken Copilot autofix (PR #291, Argon2 — non-deterministic, broke all API key auth) was closed and replaced with the correct approach. Also removes the duplicate legacy 0.1.1 section from CHANGELOG.md left over from the ai-company → SynthOrg rename. Closes #291
Pre-reviewed by 10 agents, 21 findings addressed: - Handle RuntimeError from hash_api_key/decode_token in middleware - Use hmac.compare_digest for timing-safe pwd_sig comparison - Add error logging before RuntimeError raises (_require_secret) - Upgrade hmac.new to hmac.digest for one-shot usage - Change API key not found log level from DEBUG to WARNING - Use NotBlankStr for ApiKey.key_hash field - Document jwt_secret dual-use on AuthConfig - Update stale SHA-256 docstrings in SQLite repo - Document pwd_sig mechanism in create_token/middleware - Add release.yml to DESIGN_SPEC workflow listing - Update DESIGN_SPEC and README for HMAC-SHA256 - Move deferred imports to module level in tests - Add tests for unknown key, RuntimeError propagation, hex format - Extract shared test secret to conftest import - Extract _require_secret helper to deduplicate validation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughSwitched API key hashing from plain SHA‑256 to HMAC‑SHA256 keyed with the JWT secret; Changes
Sequence DiagramsequenceDiagram
participant Client
participant Middleware as Middleware\r\n(_try_api_key_auth/_try_jwt_auth)
participant AuthService
participant Config as Config\r\n(jwt_secret)
participant Storage as Storage\r\n(ApiKey/User)
Client->>Middleware: Request with Authorization header
Middleware->>Middleware: _validate_auth_header(connection)
alt Authorization = API Key
Middleware->>AuthService: _try_api_key_auth(token, auth_service, path)
AuthService->>Config: _require_secret("hash_api_key")
Config-->>AuthService: secret (or missing)
alt Secret present
AuthService->>AuthService: hash_api_key(token) using HMAC-SHA256(secret)
AuthService->>Storage: get_by_hash(computed_hash)
Storage-->>AuthService: ApiKey or None
alt Key found & valid
AuthService-->>Middleware: AuthenticatedUser
else Not found/invalid
AuthService-->>Middleware: None
end
else Secret missing
AuthService-->>Middleware: None (SecretNotConfiguredError logged)
end
else Authorization = JWT
Middleware->>AuthService: _try_jwt_auth(token, auth_service, app_state, path)
AuthService->>Config: _require_secret("decode_token")
Config-->>AuthService: secret (or missing)
alt Secret present
AuthService->>AuthService: decode_token -> claims
AuthService->>Storage: resolve user by claims
Storage-->>AuthService: User or None
AuthService-->>Middleware: AuthenticatedUser or None
else Secret missing
AuthService-->>Middleware: None (SecretNotConfiguredError logged)
end
end
Middleware-->>Client: 200 (authenticated) or 401/unauthorized
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Greptile SummaryThis PR hardens API key storage by replacing plain SHA-256 with HMAC-SHA256 (using the server-side JWT secret as the HMAC key), adds timing-safe Key changes:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Middleware as ApiAuthMiddleware
participant AuthService
participant DB as Persistence
Client->>Middleware: Bearer <token>
Middleware->>Middleware: _validate_auth_header()
alt token contains "."
Middleware->>AuthService: decode_token(token)
AuthService->>AuthService: _require_secret("decode_token")
alt secret missing
AuthService-->>Middleware: SecretNotConfiguredError
Middleware-->>Client: 401
else valid secret
AuthService-->>Middleware: claims dict
Middleware->>DB: users.get(claims["sub"])
DB-->>Middleware: db_user
Middleware->>Middleware: expected_sig = sha256(db_user.password_hash)[:16]
Middleware->>Middleware: hmac.compare_digest(claims["pwd_sig"], expected_sig)
alt pwd_sig mismatch
Middleware-->>Client: 401 (password changed)
else match
Middleware-->>Client: 200 + AuthenticatedUser (JWT)
end
end
else no "." in token (API key path)
Middleware->>AuthService: hash_api_key(token)
AuthService->>AuthService: _require_secret("hash_api_key")
alt secret missing
AuthService-->>Middleware: SecretNotConfiguredError
Middleware-->>Client: 401
else valid secret
AuthService->>AuthService: hmac.digest(secret, raw_key, "sha256")
AuthService-->>Middleware: key_hash (hex)
Middleware->>DB: api_keys.get_by_hash(key_hash)
DB-->>Middleware: api_key | None
alt not found / revoked / expired
Middleware-->>Client: 401
else valid key
Middleware->>DB: users.get(api_key.user_id)
DB-->>Middleware: db_user
Middleware-->>Client: 200 + AuthenticatedUser (API key)
end
end
end
Last reviewed commit: 3eaab42 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ai_company/api/auth/middleware.py (1)
46-99: 🛠️ Refactor suggestion | 🟠 MajorSplit the auth flow functions back under the 50-line cap.
authenticate_request(),_try_jwt_auth(), and_try_api_key_auth()are all above the repo limit after this change. Extract the lookup / mapping / failure-reporting pieces so the new secret-handling branches stay easy to audit.As per coding guidelines, "Functions: < 50 lines".
Also applies to: 110-193, 196-274
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ai_company/api/auth/middleware.py` around lines 46 - 99, The authenticate_request function (and its helpers _try_jwt_auth and _try_api_key_auth) exceed the 50-line limit; refactor by extracting the lookup/mapping and failure-reporting pieces into small helper functions (e.g., extract_and_validate_header, map_token_to_user, log_auth_failure) so that authenticate_request, _try_jwt_auth, and _try_api_key_auth each remain under 50 lines; ensure the new helpers handle the header/token extraction, user lookup mapping, and all logger.warning calls (API_AUTH_FAILED with reason/path) so the secret-handling branches stay compact and easy to audit while preserving existing behavior and exceptions (NotAuthorizedException).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ai_company/api/auth/middleware.py`:
- Around line 212-220: The helper currently logs API_AUTH_FAILED when
auth_service.hash_api_key(token) fails, causing duplicate logs because
authenticate_request also logs invalid_credentials; remove the logger.exception
calls in the except RuntimeError handlers around auth_service.hash_api_key (and
the similar block at lines 224-228) so the helper simply returns None (or an
error sentinel) and let authenticate_request perform the single authoritative
API_AUTH_FAILED/invalid_credentials log; reference auth_service.hash_api_key,
authenticate_request, and API_AUTH_FAILED to locate and update the code paths.
In `@src/ai_company/api/auth/service.py`:
- Around line 202-223: The hash_api_key implementation uses _config.jwt_secret
which couples JWT rotation to API-key validation; change it to use a dedicated
secret on the config (e.g., add api_key_hmac_secret to AuthConfig) and update
hash_api_key to call self._require_secret for the new key name and use
self._config.api_key_hmac_secret.encode() instead of
self._config.jwt_secret.encode(); also update any verification logic that
computes/verifies API-key HMACs to reference api_key_hmac_secret so JWT signing
and API-key HMACing can be rotated independently.
---
Outside diff comments:
In `@src/ai_company/api/auth/middleware.py`:
- Around line 46-99: The authenticate_request function (and its helpers
_try_jwt_auth and _try_api_key_auth) exceed the 50-line limit; refactor by
extracting the lookup/mapping and failure-reporting pieces into small helper
functions (e.g., extract_and_validate_header, map_token_to_user,
log_auth_failure) so that authenticate_request, _try_jwt_auth, and
_try_api_key_auth each remain under 50 lines; ensure the new helpers handle the
header/token extraction, user lookup mapping, and all logger.warning calls
(API_AUTH_FAILED with reason/path) so the secret-handling branches stay compact
and easy to audit while preserving existing behavior and exceptions
(NotAuthorizedException).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4348d06d-3d0a-4abd-ae40-d4f648ac619f
📒 Files selected for processing (11)
CHANGELOG.mdDESIGN_SPEC.mdREADME.mdsrc/ai_company/api/auth/config.pysrc/ai_company/api/auth/middleware.pysrc/ai_company/api/auth/models.pysrc/ai_company/api/auth/service.pysrc/ai_company/persistence/repositories.pysrc/ai_company/persistence/sqlite/user_repo.pytests/unit/api/auth/test_middleware.pytests/unit/api/auth/test_service.py
💤 Files with no reviewable changes (1)
- CHANGELOG.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Nofrom __future__ import annotations— Python 3.14 has PEP 649
Useexcept A, B:(no parentheses) — ruff enforces this on Python 3.14
Type hints: all public functions, mypy strict mode
Docstrings: Google style, required on public classes/functions (enforced by ruff D rules)
Pydantic v2: use@computed_fieldfor derived values instead of storing + validating redundant fields (e.g.TokenUsage.total_tokens).
UseNotBlankStr(fromcore.types) for all identifier/name fields — including optional (NotBlankStr | None) and tuple (tuple[NotBlankStr, ...]) variants — instead of manual whitespace validators.
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (e.g. multiple tool invocations, parallel agent calls). Prefer structured concurrency over barecreate_task.
Line length: 88 characters (ruff)
Functions: < 50 lines, files < 800 lines
Files:
src/ai_company/persistence/sqlite/user_repo.pysrc/ai_company/api/auth/models.pysrc/ai_company/api/auth/config.pysrc/ai_company/api/auth/service.pysrc/ai_company/persistence/repositories.pysrc/ai_company/api/auth/middleware.pytests/unit/api/auth/test_middleware.pytests/unit/api/auth/test_service.py
src/ai_company/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/ai_company/**/*.py: Every module with business logic MUST have:from ai_company.observability import get_loggerthenlogger = get_logger(__name__)
Never useimport logging/logging.getLogger()/print()in application code
Always useloggeras the variable name (not_logger, notlog)
Always use constants from domain-specific modules underai_company.observability.eventsfor event names (e.g.PROVIDER_CALL_STARTfromevents.provider). Import directly:from ai_company.observability.events.<domain> import EVENT_CONSTANT
Always use structured logging kwargs:logger.info(EVENT, key=value)— neverlogger.info("msg %s", val)
All error paths must log at WARNING or ERROR with context before raising
All state transitions must log at INFO
DEBUG logging for object creation, internal flow, entry/exit of key functions
Pure data models, enums, and re-exports do NOT need logging
Files:
src/ai_company/persistence/sqlite/user_repo.pysrc/ai_company/api/auth/models.pysrc/ai_company/api/auth/config.pysrc/ai_company/api/auth/service.pysrc/ai_company/persistence/repositories.pysrc/ai_company/api/auth/middleware.py
{src,tests}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Vendor-agnostic everywhere: NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples. Use generic names:
example-provider,example-large-001,example-medium-001,example-small-001,large/medium/smallas aliases.
Files:
src/ai_company/persistence/sqlite/user_repo.pysrc/ai_company/api/auth/models.pysrc/ai_company/api/auth/config.pysrc/ai_company/api/auth/service.pysrc/ai_company/persistence/repositories.pysrc/ai_company/api/auth/middleware.pytests/unit/api/auth/test_middleware.pytests/unit/api/auth/test_service.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use test markers:@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slow
Async tests:asyncio_mode = "auto"— no manual@pytest.mark.asyncioneeded
Prefer@pytest.mark.parametrizefor testing similar cases
Usetest-provider,test-small-001, etc. for tests
Files:
tests/unit/api/auth/test_middleware.pytests/unit/api/auth/test_service.py
🧠 Learnings (7)
📚 Learning: 2026-03-11T10:12:27.146Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-11T10:12:27.146Z
Learning: Use frozen Pydantic models for config/identity; separate mutable-via-copy models (using `model_copy(update=...)`) for runtime state that evolves.
Applied to files:
src/ai_company/api/auth/models.py
📚 Learning: 2026-03-11T10:12:27.146Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-11T10:12:27.146Z
Learning: For `dict`/`list` fields in frozen Pydantic models, rely on `frozen=True` for field reassignment prevention and `copy.deepcopy()` at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, serializing for persistence).
Applied to files:
src/ai_company/api/auth/models.py
📚 Learning: 2026-03-11T10:12:27.145Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-11T10:12:27.145Z
Learning: Applies to src/ai_company/**/*.py : Every module with business logic MUST have: `from ai_company.observability import get_logger` then `logger = get_logger(__name__)`
Applied to files:
src/ai_company/api/auth/middleware.py
📚 Learning: 2026-03-11T10:12:27.145Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-11T10:12:27.145Z
Learning: Applies to .github/workflows/docker.yml : Docker CI: `.github/workflows/docker.yml` — builds backend + web images, pushes to GHCR, signs with cosign. Scans: Trivy (CRITICAL = hard fail, HIGH = warn-only) + Grype (critical cutoff). CVE triage via `.trivyignore.yaml` and `.grype.yaml`. Images only pushed after scans pass.
Applied to files:
DESIGN_SPEC.md
📚 Learning: 2026-03-11T10:12:27.145Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-11T10:12:27.145Z
Learning: Applies to .github/workflows/*.yml : CI jobs: lint (ruff) + type-check (mypy src/ tests/) + test (pytest + coverage) run in parallel → ci-pass (gate)
Applied to files:
DESIGN_SPEC.md
📚 Learning: 2026-03-11T10:12:27.145Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-11T10:12:27.145Z
Learning: Applies to .github/workflows/release.yml : Release: `.github/workflows/release.yml` — Release Please (Google) auto-creates a release PR on every push to main. Merging the release PR creates a git tag (`vX.Y.Z`) + GitHub Release with changelog. Uses `RELEASE_PLEASE_TOKEN` secret.
Applied to files:
DESIGN_SPEC.md
📚 Learning: 2026-03-11T10:12:27.146Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-11T10:12:27.146Z
Learning: Pre-commit hooks: trailing-whitespace, end-of-file-fixer, check-yaml, check-toml, check-json, check-merge-conflict, check-added-large-files, no-commit-to-branch (main), ruff check+format, gitleaks
Applied to files:
DESIGN_SPEC.md
🧬 Code graph analysis (2)
src/ai_company/api/auth/middleware.py (3)
src/ai_company/api/auth/service.py (2)
AuthService(31-232)hash_api_key(202-223)src/ai_company/api/state.py (3)
auth_service(98-100)AppState(21-122)persistence(83-85)src/ai_company/persistence/protocol.py (1)
api_keys(137-139)
tests/unit/api/auth/test_middleware.py (1)
src/ai_company/api/auth/service.py (1)
hash_api_key(202-223)
🪛 GitHub Check: CodeQL
src/ai_company/api/auth/service.py
[failure] 221-221: Use of a broken or weak cryptographic hashing algorithm on sensitive data
Sensitive data (password) is used in a hashing algorithm (SHA256) that is insecure for password hashing, since it is not a computationally expensive hash function.
Sensitive data (password) is used in a hashing algorithm (SHA256) that is insecure for password hashing, since it is not a computationally expensive hash function.
Sensitive data (password) is used in a hashing algorithm (SHA256) that is insecure for password hashing, since it is not a computationally expensive hash function.
Sensitive data (password) is used in a hashing algorithm (SHA256) that is insecure for password hashing, since it is not a computationally expensive hash function.
Sensitive data (password) is used in a hashing algorithm (SHA256) that is insecure for password hashing, since it is not a computationally expensive hash function.
🔇 Additional comments (2)
tests/unit/api/auth/test_middleware.py (1)
267-295: Good edge-case coverage for the secret-dependent API-key path.These two tests pin the new failure modes that matter most here: unknown keys and an unconfigured HMAC secret returning 401 instead of bubbling a runtime error.
tests/unit/api/auth/test_service.py (1)
149-181: Nice coverage of the new instance-based HMAC hashing behavior.These assertions pin the secret requirement, secret sensitivity, determinism, and hex output without hard-coding a digest value.
| try: | ||
| key_hash = auth_service.hash_api_key(token) | ||
| except RuntimeError: | ||
| logger.exception( | ||
| API_AUTH_FAILED, | ||
| reason="api_key_hash_failed_secret_not_configured", | ||
| path=str(connection.url.path), | ||
| ) | ||
| return None |
There was a problem hiding this comment.
Avoid double-logging API key failures.
These new branches now emit API_AUTH_FAILED here, but authenticate_request() still logs invalid_credentials when this helper returns None. A single rejected API key request will therefore produce two failure events, which will skew auth-failure metrics and warning/error counts.
Also applies to: 224-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ai_company/api/auth/middleware.py` around lines 212 - 220, The helper
currently logs API_AUTH_FAILED when auth_service.hash_api_key(token) fails,
causing duplicate logs because authenticate_request also logs
invalid_credentials; remove the logger.exception calls in the except
RuntimeError handlers around auth_service.hash_api_key (and the similar block at
lines 224-228) so the helper simply returns None (or an error sentinel) and let
authenticate_request perform the single authoritative
API_AUTH_FAILED/invalid_credentials log; reference auth_service.hash_api_key,
authenticate_request, and API_AUTH_FAILED to locate and update the code paths.
| def hash_api_key(self, raw_key: str) -> str: | ||
| """Compute HMAC-SHA256 hex digest of a raw API key. | ||
|
|
||
| Uses the server-side JWT secret as the HMAC key so that | ||
| an attacker with read access to stored hashes cannot | ||
| brute-force API keys offline. | ||
|
|
||
| Args: | ||
| raw_key: The plaintext API key. | ||
|
|
||
| Returns: | ||
| Lowercase hex digest. | ||
|
|
||
| Raises: | ||
| RuntimeError: If the JWT secret is empty. | ||
| """ | ||
| return hashlib.sha256(raw_key.encode()).hexdigest() | ||
| self._require_secret("hash_api_key") | ||
| return hmac.digest( | ||
| self._config.jwt_secret.encode(), | ||
| raw_key.encode(), | ||
| "sha256", | ||
| ).hex() |
There was a problem hiding this comment.
Separate JWT signing and API-key HMAC secrets.
This now keys persisted API-key hashes with jwt_secret, so any normal JWT signing-key rotation immediately invalidates every stored API key. That couples two unrelated secret lifecycles and turns a routine token rollover into a full API-key outage.
Suggested direction
- self._require_secret("hash_api_key")
+ secret = self._require_api_key_secret("hash_api_key")
return hmac.digest(
- self._config.jwt_secret.encode(),
+ secret.encode(),
raw_key.encode(),
"sha256",
).hex()You'd also want a dedicated api_key_hmac_secret (or versioned key ring) on AuthConfig so JWT rotation and API-key verification can evolve independently.
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 221-221: Use of a broken or weak cryptographic hashing algorithm on sensitive data
Sensitive data (password) is used in a hashing algorithm (SHA256) that is insecure for password hashing, since it is not a computationally expensive hash function.
Sensitive data (password) is used in a hashing algorithm (SHA256) that is insecure for password hashing, since it is not a computationally expensive hash function.
Sensitive data (password) is used in a hashing algorithm (SHA256) that is insecure for password hashing, since it is not a computationally expensive hash function.
Sensitive data (password) is used in a hashing algorithm (SHA256) that is insecure for password hashing, since it is not a computationally expensive hash function.
Sensitive data (password) is used in a hashing algorithm (SHA256) that is insecure for password hashing, since it is not a computationally expensive hash function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ai_company/api/auth/service.py` around lines 202 - 223, The hash_api_key
implementation uses _config.jwt_secret which couples JWT rotation to API-key
validation; change it to use a dedicated secret on the config (e.g., add
api_key_hmac_secret to AuthConfig) and update hash_api_key to call
self._require_secret for the new key name and use
self._config.api_key_hmac_secret.encode() instead of
self._config.jwt_secret.encode(); also update any verification logic that
computes/verifies API-key HMACs to reference api_key_hmac_secret so JWT signing
and API-key HMACing can be rotated independently.
There was a problem hiding this comment.
Pull request overview
This PR strengthens the authentication subsystem by switching API key hashing from plain SHA-256 to HMAC-SHA256 (keyed by the server secret), adds timing-safe comparison for JWT pwd_sig, and updates tests/docs accordingly. It also removes a duplicated legacy section from the changelog.
Changes:
- Replace API key hashing with HMAC-SHA256 and route API key hashing through
AuthServiceinstance config. - Add timing-safe
hmac.compare_digestforpwd_sigchecks and improve missing-secret handling in middleware paths. - Expand unit tests for API key edge cases and update documentation to reflect the hashing scheme.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/api/auth/test_service.py | Updates tests for instance-based API key hashing and adds new hashing/secret validation cases. |
| tests/unit/api/auth/test_middleware.py | Updates middleware tests for instance hashing and adds API key edge-case coverage. |
| src/ai_company/persistence/sqlite/user_repo.py | Updates repository docstring to reflect HMAC-SHA256 hash semantics. |
| src/ai_company/persistence/repositories.py | Updates API key hash parameter docs to HMAC-SHA256. |
| src/ai_company/api/auth/service.py | Introduces _require_secret() and implements HMAC-SHA256 API key hashing. |
| src/ai_company/api/auth/models.py | Tightens ApiKey.key_hash typing and updates hash documentation. |
| src/ai_company/api/auth/middleware.py | Uses timing-safe comparison for pwd_sig and routes API key hashing through configured AuthService. |
| src/ai_company/api/auth/config.py | Documents jwt_secret dual-use for API key hashing and rotation implications. |
| README.md | Updates security section to distinguish password vs API key hashing algorithms. |
| DESIGN_SPEC.md | Updates auth tech stack/project structure docs (HMAC-SHA256, workflow listing). |
| CHANGELOG.md | Removes duplicated legacy ai-company 0.1.1 section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| key_hash = auth_service.hash_api_key(token) | ||
| except RuntimeError: | ||
| logger.exception( | ||
| API_AUTH_FAILED, | ||
| reason="api_key_hash_failed_secret_not_configured", | ||
| path=str(connection.url.path), | ||
| ) | ||
| return None |
There was a problem hiding this comment.
Switching API key lookup to only HMAC-SHA256 will cause all existing stored API keys (previously hashed with plain SHA-256) to stop authenticating after deploy. Consider a backwards-compatible migration strategy (e.g., attempt legacy SHA-256 lookup if HMAC lookup misses, and on success re-write the stored hash to the new HMAC form), and/or add an explicit upgrade note that all API keys must be re-issued.
| logger.error( | ||
| API_AUTH_FAILED, | ||
| reason="jwt_secret_missing", | ||
| operation=operation, | ||
| ) |
There was a problem hiding this comment.
_require_secret() logs an error and then callers (e.g., middleware) also log the caught RuntimeError with logger.exception, which will typically produce duplicate log entries per failure. Consider either removing logging from _require_secret (let the boundary layer log once), or raising a dedicated exception type that callers can handle without re-logging stack traces.
| logger.error( | |
| API_AUTH_FAILED, | |
| reason="jwt_secret_missing", | |
| operation=operation, | |
| ) |
| description=( | ||
| "JWT signing secret (resolved at startup). " | ||
| "Also used as the HMAC key for API key hash computation — " | ||
| "rotating this secret invalidates all stored API key hashes." |
There was a problem hiding this comment.
Using jwt_secret as the HMAC key only protects API key hashes if that secret is stored separately from the leaked data. In the default setup, the JWT secret is persisted in the same persistence backend (settings table), so a full DB dump would still enable offline guessing. Consider introducing a separate, non-persisted API-key hashing secret (env/KMS) or explicitly documenting the threat model/assumptions here.
| "rotating this secret invalidates all stored API key hashes." | |
| "rotating this secret invalidates all stored API key hashes. " | |
| "Security note: API key hashes are only protected against offline " | |
| "guessing if this secret is not persisted alongside them (for " | |
| "example, it should be provided via an environment variable or KMS " | |
| "rather than stored in the same database/settings backend)." |
- Define SecretNotConfiguredError(RuntimeError) to replace broad except RuntimeError catches in middleware (silent-failure-hunter) - Use _require_secret return value in create_token, decode_token, hash_api_key instead of re-accessing config (type-design, Greptile) - Fix verify_password to re-raise on InvalidHashError and non-mismatch VerificationError instead of hiding data corruption (silent-failure) - Refactor middleware: extract _validate_auth_header, _resolve_jwt_user, _resolve_api_key_user to keep all functions under 50-line cap (CodeRabbit) - Remove double-logging in API key failure path (CodeRabbit) - Fix "Assumes" docstring wording in _try_api_key_auth (comment-analyzer) - Add pwd_sig clarification: plain SHA-256, not HMAC (comment-analyzer) - Update config.py Attributes docstring for jwt_secret dual-purpose - Fix "HMAC algorithm" ambiguity in DESIGN_SPEC.md project structure - Add tests: pwd_sig mismatch after password change, JWT auth with unconfigured secret (pr-test-analyzer) - Update existing tests for SecretNotConfiguredError and verify_password raising behavior 15 findings addressed from 7 agents + 3 external reviewers. 6835 tests pass, 9 skipped (platform-specific).
| expected_sig = hashlib.sha256(db_user.password_hash.encode()).hexdigest()[:16] | ||
| if not _hmac.compare_digest(claims.get("pwd_sig", ""), expected_sig): |
There was a problem hiding this comment.
hmac.compare_digest raises TypeError on non-string pwd_sig
dict.get("pwd_sig", "") returns the stored value when the key is present — the default "" only kicks in when the key is absent. If an attacker crafts a valid JWT (signed with the real secret) that contains "pwd_sig": null or "pwd_sig": 42, claims.get("pwd_sig", "") returns None or 42 respectively. hmac.compare_digest then raises TypeError because both arguments must be the same type (str or bytes).
This TypeError is not caught in _resolve_jwt_user or in _try_jwt_auth, so it propagates as an unhandled exception and results in a 500 response instead of a clean 401.
| expected_sig = hashlib.sha256(db_user.password_hash.encode()).hexdigest()[:16] | |
| if not _hmac.compare_digest(claims.get("pwd_sig", ""), expected_sig): | |
| expected_sig = hashlib.sha256(db_user.password_hash.encode()).hexdigest()[:16] | |
| pwd_sig = claims.get("pwd_sig") | |
| if not isinstance(pwd_sig, str) or not _hmac.compare_digest(pwd_sig, expected_sig): |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ai_company/api/auth/middleware.py
Line: 194-195
Comment:
**`hmac.compare_digest` raises `TypeError` on non-string `pwd_sig`**
`dict.get("pwd_sig", "")` returns the *stored value* when the key is present — the default `""` only kicks in when the key is **absent**. If an attacker crafts a valid JWT (signed with the real secret) that contains `"pwd_sig": null` or `"pwd_sig": 42`, `claims.get("pwd_sig", "")` returns `None` or `42` respectively. `hmac.compare_digest` then raises `TypeError` because both arguments must be the same type (`str` or `bytes`).
This `TypeError` is not caught in `_resolve_jwt_user` or in `_try_jwt_auth`, so it propagates as an unhandled exception and results in a 500 response instead of a clean 401.
```suggestion
expected_sig = hashlib.sha256(db_user.password_hash.encode()).hexdigest()[:16]
pwd_sig = claims.get("pwd_sig")
if not isinstance(pwd_sig, str) or not _hmac.compare_digest(pwd_sig, expected_sig):
```
How can I resolve this? If you propose a fix, please make it concise.| secret = self._config.jwt_secret | ||
| if not secret: | ||
| msg = "JWT secret not configured" | ||
| logger.error( | ||
| API_AUTH_FAILED, | ||
| reason="jwt_secret_missing", | ||
| operation=operation, | ||
| ) | ||
| raise SecretNotConfiguredError(msg) |
There was a problem hiding this comment.
Double-logging on unconfigured secret
_require_secret logs logger.error(API_AUTH_FAILED, reason="jwt_secret_missing", ...) before raising SecretNotConfiguredError. Every call site in the middleware then catches the exception and calls logger.exception(API_AUTH_FAILED, ...) again. This means each "secret not configured" event emits two log entries — one from _require_secret and one from the middleware handler — with slightly different reason values, which can be confusing for operators correlating alerts.
Consider removing the logger.error call from _require_secret and letting each middleware catch-site own the log, or vice versa — pick one place to log and drop the other.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ai_company/api/auth/service.py
Line: 58-66
Comment:
**Double-logging on unconfigured secret**
`_require_secret` logs `logger.error(API_AUTH_FAILED, reason="jwt_secret_missing", ...)` before raising `SecretNotConfiguredError`. Every call site in the middleware then catches the exception and calls `logger.exception(API_AUTH_FAILED, ...)` again. This means each "secret not configured" event emits **two** log entries — one from `_require_secret` and one from the middleware handler — with slightly different `reason` values, which can be confusing for operators correlating alerts.
Consider removing the `logger.error` call from `_require_secret` and letting each middleware catch-site own the log, or vice versa — pick one place to log and drop the other.
How can I resolve this? If you propose a fix, please make it concise.| jwt_secret: str = Field( | ||
| default="", | ||
| repr=False, | ||
| description="JWT signing secret (resolved at startup)", | ||
| description=( | ||
| "JWT signing secret (resolved at startup). " | ||
| "Also used as the HMAC key for API key hash computation — " | ||
| "rotating this secret invalidates all stored API key hashes." | ||
| ), | ||
| ) |
There was a problem hiding this comment.
JWT secret rotation silently breaks all API key authentication
jwt_secret now serves two distinct security purposes: JWT signing/verification and HMAC key for API key hashing. The field description documents this, but the operational consequence is a sharp footgun: a developer who rotates jwt_secret specifically to invalidate all active JWT sessions (a routine post-incident response) will also silently invalidate every stored API key hash in the database — without any warning at startup or rotation time.
Consider raising a startup warning (or at minimum a logger.warning) when the secret changes relative to the previously persisted value, so operators are aware that API keys will need to be regenerated. Alternatively, a separate api_key_hmac_secret field would fully decouple the two rotation lifecycles.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ai_company/api/auth/config.py
Line: 54-62
Comment:
**JWT secret rotation silently breaks all API key authentication**
`jwt_secret` now serves two distinct security purposes: JWT signing/verification and HMAC key for API key hashing. The field description documents this, but the operational consequence is a sharp footgun: a developer who rotates `jwt_secret` specifically to invalidate all active JWT sessions (a routine post-incident response) will also silently invalidate every stored API key hash in the database — without any warning at startup or rotation time.
Consider raising a startup warning (or at minimum a `logger.warning`) when the secret changes relative to the previously persisted value, so operators are aware that API keys will need to be regenerated. Alternatively, a separate `api_key_hmac_secret` field would fully decouple the two rotation lifecycles.
How can I resolve this? If you propose a fix, please make it concise.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the security and maintainability of the project. It hardens API key hashing using HMAC-SHA256, cleans up the changelog, and incorporates numerous improvements identified by review agents. The changes also include documentation updates and testing enhancements to ensure code quality and reliability. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request significantly improves security by replacing plain SHA-256 hashing for API keys with HMAC-SHA256, keyed with the server's JWT secret. This prevents offline brute-force attacks on leaked API key hashes. The changes are well-implemented, including the use of hmac.compare_digest for timing-safe comparisons, refactoring to centralize secret validation, and comprehensive updates to documentation and tests. The changelog cleanup is also a welcome improvement. I have one suggestion to improve logging by removing a redundant log entry.
|
|
||
| Raises: | ||
| SecretNotConfiguredError: If the JWT secret is empty. | ||
| """ | ||
| secret = self._config.jwt_secret | ||
| if not secret: | ||
| msg = "JWT secret not configured" | ||
| logger.error( |
There was a problem hiding this comment.
To avoid redundant logging, consider removing the logger.error call from this helper function. The callers of this function in src/ai_company/api/auth/middleware.py already catch the RuntimeError and log it using logger.exception, which includes the stack trace and request-specific context like the path. Raising the exception here is sufficient. I've also suggested enhancing the exception message to include the operation name for better context in the logs.
if not secret:
msg = f"JWT secret not configured; this is required for the '{operation}' operation."
raise RuntimeError(msg)🤖 I have created a release *beep* *boop* --- ## [0.1.1](v0.1.0...v0.1.1) (2026-03-11) ### Features * add PR preview deployments via Cloudflare Pages ([#302](#302)) ([b73c45a](b73c45a)) ### Bug Fixes * correct deploy-pages SHA and improve preview cleanup reliability ([#304](#304)) ([584d64a](584d64a)) * harden API key hashing with HMAC-SHA256 and clean up legacy changelog ([#292](#292)) ([5e85353](5e85353)) * upgrade upload-pages-artifact to v4 and add zizmor workflow linting ([#299](#299)) ([2eac571](2eac571)) * use Cloudflare Pages API default per_page for pagination ([#305](#305)) ([9fec245](9fec245)) ### Documentation * remove milestone references and rebrand to SynthOrg ([#289](#289)) ([57a03e0](57a03e0)) * set up documentation site, release CI, and sandbox hardening ([#298](#298)) ([0dec9da](0dec9da)) * split DESIGN_SPEC.md into 7 focused design pages ([#308](#308)) ([9ea0788](9ea0788)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Signed-off-by: Aurelio <19254254+Aureliolo@users.noreply.github.com>
Summary
ai-company0.1.1 section from CHANGELOG.mdChanges
Security hardening
hash_api_keychanged from@staticmethodSHA-256 to instance method HMAC-SHA256hmac.compare_digestfor timing-safepwd_sigcomparison in JWT middlewareRuntimeErrorfrom unconfigured secret in both JWT and API key auth paths_require_secrethelper with consistent error loggingDocumentation
release.ymlto workflow listingpwd_sigmechanism andjwt_secretdual-useTests
Code quality
key_hashfield upgraded toNotBlankStrhmac.new→hmac.digestfor one-shot usageTest plan
Review coverage
10 agents run: code-reviewer, python-reviewer, pr-test-analyzer, silent-failure-hunter, comment-analyzer, type-design-analyzer, logging-audit, resilience-audit, security-reviewer, docs-consistency. 22 findings total, 21 valid, all implemented.