Skip to content

Support bearer/query API keys (toggle), normalize/deduplicate memory imports, and use auth key for rate-limiting#2

Closed
Skiipy11 wants to merge 1 commit intomainfrom
codex/audit-codebase-for-issues-and-propose-fixes
Closed

Support bearer/query API keys (toggle), normalize/deduplicate memory imports, and use auth key for rate-limiting#2
Skiipy11 wants to merge 1 commit intomainfrom
codex/audit-codebase-for-issues-and-propose-fixes

Conversation

@Skiipy11
Copy link
Copy Markdown
Contributor

Motivation

  • Allow more flexible API key delivery (header, Bearer token, and an optional query-key compatibility toggle) while discouraging insecure default behavior.
  • Centralize and enforce normalization, credential-scrubbing, validation, and deduplication for memory writes and imports to reduce duplication and mismatches.
  • Ensure rate limiting buckets use the resolved authenticated key instead of raw headers so usage is tracked correctly per identity.

Description

  • Add ALLOW_QUERY_API_KEY env flag and document it in README.md and .env.example, and wire it into authentication behavior.
  • Refactor auth middleware (api/src/middleware/auth.js) to use a new extractKey flow that accepts x-api-key, Authorization: Bearer ..., and optionally ?key=..., sets req.authSource and req.rateLimitKey, and returns a clear 401 when query keys are disabled.
  • Update rate-limit middleware (api/src/middleware/ratelimit.js) to prefer req.rateLimitKey for bucketing so rate limits apply to the resolved authenticated identity.
  • Introduce api/src/services/memory-write-utils.js providing normalizeMemoryRecord, normalizeImportRecord, and buildDedupExtraFilter, and update memory and export routes to use these utilities for normalization, content hashing, deduplication (scoped by client_id and type), embedding, and structured-store writes.
  • Add client_id filtering to listStatuses implementations in both Postgres and SQLite stores and remove duplicated SQLite schema block.
  • Add unit tests covering auth behavior, rate-limiting key bucketing, and memory normalization/dedup logic in api/tests/auth.test.js, api/tests/ratelimit.test.js, and api/tests/memory-write-utils.test.js.

Testing

  • Ran the test suite (unit tests) including api/tests/auth.test.js, api/tests/ratelimit.test.js, and api/tests/memory-write-utils.test.js, and all tests passed.
  • Verified that authMiddleware accepts x-api-key, Bearer tokens, and conditionally accepts query keys when ALLOW_QUERY_API_KEY=true via unit tests.
  • Verified that rateLimitMiddleware buckets requests by req.rateLimitKey and enforces limits per resolved identity via unit tests.

Codex Task

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c5bd0a369d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +31 to +35
const validationError = validateMemoryInput({
type: normalized.type,
content: normalized.content,
source_agent: normalized.source_agent,
importance: normalized.importance,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Preserve metadata validation in normalization helper

This new centralized validation path does not pass metadata into validateMemoryInput, so POST /memory no longer enforces the metadata type/size/depth checks that were previously applied to req.body. In practice, malformed or deeply nested metadata now reaches scrubObject(metadata) and can trigger avoidable runtime failures or heavy recursive work instead of returning a deterministic 400 validation error.

Useful? React with 👍 / 👎.

Comment on lines +61 to +63
if (typeof authHeader === 'string' && authHeader.startsWith('Bearer ')) {
const bearerToken = authHeader.slice('Bearer '.length).trim();
if (bearerToken) return { key: bearerToken, source: 'bearer' };
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Parse Bearer auth scheme case-insensitively

Bearer scheme matching is currently case-sensitive (startsWith('Bearer ')), but HTTP auth schemes are case-insensitive, so valid headers like authorization: bearer <token> are rejected with 401. Since this commit introduces Bearer-token support, this causes legitimate clients/proxies using lowercase schemes to fail authentication unexpectedly.

Useful? React with 👍 / 👎.

@Skiipy11 Skiipy11 closed this Mar 27, 2026
@Skiipy11 Skiipy11 deleted the codex/audit-codebase-for-issues-and-propose-fixes branch March 27, 2026 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant