Skip to content

Return uniform 200 response for new and existing API key registrations#110

Merged
KevinPayravi merged 5 commits into
mainfrom
uniform-api-key-response
May 13, 2026
Merged

Return uniform 200 response for new and existing API key registrations#110
KevinPayravi merged 5 commits into
mainfrom
uniform-api-key-response

Conversation

@DominicBM
Copy link
Copy Markdown
Contributor

@DominicBM DominicBM commented May 13, 2026

Summary

The POST /api_key/:email endpoint currently returns HTTP 200 when registering a new account and HTTP 409 when the email already exists. This difference is a user enumeration oracle — any caller can determine whether an arbitrary email address is registered with DPLA by observing the status code.

This PR closes that gap by returning HTTP 200 with the same response body ("Your API key has been sent to $email.") for both the new-account and existing-account cases.

Changes

  • Routes.scala: ExistingApiKey now calls apiKeyMessage instead of existingKeyResponse (which returned 409 Conflict)
  • existingKeyResponse method removed (no longer referenced)
  • newKeyMessage renamed to apiKeyMessage with neutral wording ("Your API key has been sent to..." rather than "API key created and sent to...")

Background

Discovered during a scanner investigation (2026-05-12): an automated scanner was calling POST /api_key/ at scale against lists of email addresses — including real @openai.com addresses — and using the 200 vs 409 response to enumerate registered users. The uniform response removes the signal without affecting any legitimate use of the endpoint.

The DisabledApiKey (409) path is unchanged — disabled-account handling is a separate concern.

Security Fix: User Enumeration Prevention

This PR fixes a user-enumeration vulnerability in the API key registration endpoint. Previously POST /api_key/:email returned 200 for new accounts and 409 Conflict for existing accounts, allowing attackers to distinguish registered emails by status code.

Changes

  • Both NewApiKey(email) and ExistingApiKey(email) now return HTTP 200 with the same response body: "Your API key has been sent to $email."
  • Renamed newKeyMessage() to apiKeyMessage() and used for both new and existing-key paths.
  • Removed existingKeyResponse(), which previously returned 409 for existing keys.
  • DisabledApiKey(email) behavior unchanged (still returns 409).
  • Tests updated:
    • src/test/scala/dpla/api/v2/endToEnd/PostgresErrorTest.scala now expects 200 and the unified message for the existing-key case.
    • src/test/scala/dpla/api/v2/authentication/MockPostgresClientExistingKey.scala mock now returns AccountFound(account.copy(email = email)) for ValidEmail to reflect the email echo in the message.

Files changed (high-level):

  • src/main/scala/dpla/api/Routes.scala
  • src/test/scala/dpla/api/v2/endToEnd/PostgresErrorTest.scala
  • src/test/scala/dpla/api/v2/authentication/MockPostgresClientExistingKey.scala

Impact Notes

  • Public-facing API change: The endpoint's status code for existing emails changed from 409 -> 200 while keeping the same message body. Clients that rely on a 409 to detect existing accounts must be updated.
  • Security: This is a security fix to prevent account enumeration and should be treated accordingly.
  • Deploy/Operational: Requires deploying the updated service (CI pipeline/ECS task redeploy) for the change to take effect. No changes to pipelines or deployment definitions themselves.
  • Infrastructure and Secrets: No environment variables, AWS Secrets Manager keys, IAM policies, CodePipeline/CodeBuild/ECS task definitions, or other shared infra configurations were added, removed, or modified.
  • Database: No database migrations were introduced.
  • Backwards compatibility: Behavior is intentionally changed to close an information leak; review client code that inspects HTTP status codes for this endpoint.

Review Change Stack

The /api_key/:email endpoint previously returned HTTP 200 for new accounts
and HTTP 409 for existing ones. This difference acts as a user enumeration
oracle — a caller can determine whether any email address is registered
with DPLA. Return 200 with the same body in both cases to close that gap.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 13, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3455d7cb-bf72-4319-b4c9-38acccf0da60

📥 Commits

Reviewing files that changed from the base of the PR and between 634a50c and 3079091.

📒 Files selected for processing (1)
  • src/test/scala/dpla/api/v2/authentication/MockPostgresClientExistingKey.scala

Walkthrough

This PR consolidates API key response handling so both new and existing API key outcomes return the same "Your API key has been sent to $email." message and a 200 OK; tests and a mock Postgres client were updated accordingly.

Changes

API key response unification

Layer / File(s) Summary
Unified API key response handling
src/main/scala/dpla/api/Routes.scala, src/test/scala/dpla/api/v2/endToEnd/PostgresErrorTest.scala, src/test/scala/dpla/api/v2/authentication/MockPostgresClientExistingKey.scala
renderRegistryResponse now returns apiKeyMessage(email) for both NewApiKey and ExistingApiKey; the helper was renamed/rewritten to apiKeyMessage with the unified "Your API key has been sent to $email." text. The end-to-end test was changed to expect 200 OK and the sent-message body. The mock Postgres client now echoes the requested email in AccountFound.
sequenceDiagram
  participant Client
  participant Routes
  participant Registry
  Client->>Routes: POST /api_key/<email>
  Routes->>Registry: request API key (new or existing)
  Registry-->>Routes: NewApiKey(email) / ExistingApiKey(email)
  Routes-->>Client: apiKeyMessage(email) (200 OK)
Loading

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs:

  • dpla/api#104: Modifies Routes.scala around existing API key handling and error payloads; related to how existing-key responses are formed.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely describes the main change: returning a uniform 200 response for both new and existing API key registrations, which directly addresses the core objective of eliminating user enumeration.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch uniform-api-key-response

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/test/scala/dpla/api/v2/endToEnd/PostgresErrorTest.scala`:
- Around line 47-56: The test PostgresErrorTest has a flaky assertion: the
request uses Post("/v2/api_key/email@example.com") but the
apiKeyRegistryExistingKey mock returns x@example.org, so the expected string
"Your API key has been sent to email@example.com." is wrong; fix by making the
test derive the request email and the expected response from a single variable
(e.g., val testEmail = "email@example.com") and use that in the Post call and
the entityAs assertion, or alternatively modify the apiKeyRegistryExistingKey
mock to echo the requested email so Routes.applicationRoutes (constructed via
new Routes(..., apiKeyRegistryExistingKey, ...)) produces deterministic output
matching the test.
🪄 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: CHILL

Plan: Pro

Run ID: 9d0961c9-972e-4830-9631-1e06fbf5b188

📥 Commits

Reviewing files that changed from the base of the PR and between 505bf2f and 634a50c.

📒 Files selected for processing (1)
  • src/test/scala/dpla/api/v2/endToEnd/PostgresErrorTest.scala

Comment thread src/test/scala/dpla/api/v2/endToEnd/PostgresErrorTest.scala
@KevinPayravi KevinPayravi merged commit 98a79d6 into main May 13, 2026
5 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.

2 participants