External API gateway (services/gateway) + resolve-discord endpoint (#59) - #65
Open
qiuethan wants to merge 17 commits into
Open
External API gateway (services/gateway) + resolve-discord endpoint (#59)#65qiuethan wants to merge 17 commits into
qiuethan wants to merge 17 commits into
Conversation
📝 WalkthroughWalkthroughIntroduces a new public-facing ChangesGateway Service
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Caller as External Caller
participant Gateway as Gateway API (resolve.py)
participant Auth as Auth Middleware (require_scope)
participant RateLimit as RateLimitMiddleware
participant Storage as StorageAdapter
participant Directory as HttpDirectoryClient
participant TeamTracking as team-tracking Directory Service
Caller->>Gateway: GET /v1/resolve/discord/{github_login} + X-API-Key
Gateway->>RateLimit: check per-key request count
RateLimit-->>Gateway: 429 if exceeded, else continue
Gateway->>Auth: require_scope("resolve:discord")
Auth->>Storage: verify key hash and scope
Storage-->>Auth: key valid / invalid
Auth-->>Gateway: authorized or 401/403
Gateway->>Directory: get_person_by_github(github_login)
Directory->>TeamTracking: GET /people/by-identifier/github/{login}
TeamTracking-->>Directory: person JSON or 404/5xx
Directory-->>Gateway: person or raise DirectoryUnavailable
Gateway->>Directory: list_identifiers(person_id)
Directory->>TeamTracking: GET identifiers
TeamTracking-->>Directory: identifiers list
Directory-->>Gateway: identifiers
Gateway-->>Caller: 200 {discord_id} or 404/503
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
services/gateway/contracts/types.py (1)
7-16: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
ApiKeyomits audit fields present in the persisted schema.
services/gateway/src/storage/schema.pypersistscreated_at,updated_at,created_by, andupdated_byfor every key, but this domain model exposes none of them. Given thegateway-keysCLI supportsissue/list/revoke, operators will likely want to see who created/revoked a key and when — that data exists in storage but can't surface through this contract.♻️ Suggested addition
class ApiKey(BaseModel): model_config = ConfigDict(extra="forbid") id: UUID name: str prefix: str scopes: list[str] active: bool = True + created_at: datetime + updated_at: datetime + created_by: str + updated_by: str revoked_at: datetime | None = None last_used_at: datetime | None = None🤖 Prompt for 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. In `@services/gateway/contracts/types.py` around lines 7 - 16, The ApiKey contract is missing audit metadata that already exists in the persisted schema. Update the ApiKey model to include the audit fields from storage—created_at, updated_at, created_by, and updated_by—so the gateway-keys issue/list/revoke flow can expose them. Keep the change localized to ApiKey in the contracts/types model and ensure the new fields are typed consistently with the schema and remain optional or nullable where appropriate.services/gateway/src/api/ratelimit.py (1)
33-39: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueDouble sweep per request once threshold is hit.
_sweepis called both before and after recording the hit (lines 33 and 39). Once_hitsexceeds the threshold, every request pays two O(n) scans instead of one. Consider sweeping only once (e.g., after the increment) since the pre-increment sweep doesn't change the outcome of the post-increment check.🤖 Prompt for 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. In `@services/gateway/src/api/ratelimit.py` around lines 33 - 39, The hit recording path in the rate limiter is sweeping twice per request, which adds unnecessary O(n) work once the threshold is reached. Update the request update flow in the rate-limiting method that touches _sweep, _hits, and the increment logic so it only performs a single sweep per request, preferably after updating the hit count, while preserving the window reset behavior.services/gateway/src/directory/http_client.py (1)
15-28: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winClient is created/torn down on every call unless one is injected.
When
clientisn't supplied,_getbuilds a freshhttpx.Clientand closes it after every single request, losing connection reuse/keep-alive. Since this runs on the resolve endpoint's hot path (multiple internal calls per external request), this adds unnecessary connection overhead under load.♻️ Suggested fix: own a persistent client by default
def __init__(self, base_url: str, api_key: str, client: httpx.Client | None = None) -> None: self._base_url = base_url.rstrip("/") self._api_key = api_key - self._client = client + self._client = client or httpx.Client(timeout=_TIMEOUT) + self._owns_client = client is None def _get(self, path: str): - client = self._client or httpx.Client(timeout=_TIMEOUT) + client = self._client try: resp = client.get(f"{self._base_url}{path}", headers={"X-API-Key": self._api_key}) except httpx.HTTPError as e: raise DirectoryUnavailable(f"directory unreachable: {e}") from e - finally: - if self._client is None: - client.close() if resp.status_code == 404: return NoneAdd a
close()/__enter__/__exit__if lifecycle management is needed by callers.🤖 Prompt for 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. In `@services/gateway/src/directory/http_client.py` around lines 15 - 28, The _get method in http_client.py is recreating and closing an httpx.Client on every call when no client is injected, which prevents connection reuse on the hot path. Update HttpClient to own a persistent default client created in __init__ (or otherwise reused across requests) and only close it when the instance is disposed; keep honoring an injected client, and add lifecycle support such as close() or context manager methods if needed.services/gateway/tests/test_resolve.py (1)
55-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for insufficient-scope (403) case.
test_requires_scope_and_keyonly exercises the missing-header 401 path. Since this endpoint's authorization model hinges on scope enforcement (resolve:discord), add a case issuing a key without that scope and asserting 403.✅ Suggested additional test
+def test_wrong_scope_403(): + store = InMemoryStorageAdapter() + plaintext, prefix, key_hash = generate_key() + store.create_api_key(name="c", prefix=prefix, key_hash=key_hash, + scopes=["other:scope"], actor="t") + app = create_app() + app.dependency_overrides[get_storage] = lambda: store + app.dependency_overrides[get_directory] = lambda: FakeDir(person={"id": "p1"}) + c = TestClient(app) + assert c.get("/v1/resolve/discord/octocat", headers={"X-API-Key": plaintext}).status_code == 403🤖 Prompt for 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. In `@services/gateway/tests/test_resolve.py` around lines 55 - 58, The `test_requires_scope_and_key` test only covers the missing-auth-header 401 path; extend it to also verify the insufficient-scope 403 case for the `/v1/resolve/discord/octocat` endpoint. Use the existing `_client(...)` helper and the `FakeDir`/client setup in `test_resolve.py`, then add a request with a key that lacks the `resolve:discord` scope and assert the response status is 403.
🤖 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 @.github/workflows/ci.yml:
- Line 80: The checkout step currently persists the GitHub token and the job
lacks explicit least-privilege permissions. Update the workflow job that uses
actions/checkout@v4 to set read-only job permissions and configure the checkout
action to disable credential persistence, so the token is not stored for later
git operations.
In `@services/gateway/Dockerfile`:
- Around line 1-11: The gateway container currently runs Uvicorn as root, so
harden the Dockerfile by running the final image under a dedicated non-root
user. Update the service image setup around the existing COPY/uv sync/ENV/CMD
steps to create or switch to a low-privilege user before starting uvicorn, and
ensure the app files and working directory used by services/gateway are
accessible to that user. Keep the startup command unchanged except for executing
it as the non-root user.
In `@services/gateway/pyproject.toml`:
- Around line 5-6: The wheel packaging config currently only includes the src
package, but the gateway also imports contracts.* at runtime. Update the
[tool.hatch.build.targets.wheel] packages setting in pyproject.toml to include
contracts alongside src so the installed wheel contains both package trees and
those imports continue to work.
In `@services/gateway/README.md`:
- Around line 70-72: The remaining fenced code blocks in the README are missing
language tags, which triggers markdownlint. Update the fence examples around the
response example and the repo-layout tree, plus any other remaining fenced
blocks in the referenced sections, by adding appropriate labels such as json and
text so the documentation stays lint-clean. Use the existing README fenced block
structure as the target, and make sure every unannotated fence in that area is
labeled consistently.
In `@services/gateway/src/api/ratelimit.py`:
- Around line 11-42: The RateLimitMiddleware._hits cache is currently unbounded
because dispatch() stores every raw X-API-Key value before auth, which can be
abused to grow memory. Update RateLimitMiddleware to cap this cache with a hard
maximum or LRU-style eviction, and only insert validated/known API keys into
_hits. Keep the fix localized to RateLimitMiddleware._sweep and dispatch so the
per-key rate tracking still works without allowing unlimited growth.
In `@services/gateway/src/api/routers/resolve.py`:
- Around line 16-26: The resolve endpoint currently returns distinguishable 404
details in the github_login lookup flow, which exposes whether a login exists or
only lacks a Discord link. Update the logic in resolve() so both the
get_person_by_github() miss and the missing discord_id branch raise the same
generic HTTPException 404 response, using one shared detail/body message and
keeping the response identical for both paths.
In `@services/gateway/src/storage/postgres.py`:
- Around line 93-102: The revoke_api_key method currently updates revoked_at and
updated_by even when the key is already revoked, which overwrites the original
audit trail. Add a guard in revoke_api_key’s update query so it only applies
when api_keys.active is still true (or otherwise preserves existing
revoked_at/updated_by), and return None or no-op on repeat revocations. Keep the
fix localized to revoke_api_key in the Postgres storage code and ensure the
returning(api_keys) path still maps correctly through _api_key_row_to_model.
---
Nitpick comments:
In `@services/gateway/contracts/types.py`:
- Around line 7-16: The ApiKey contract is missing audit metadata that already
exists in the persisted schema. Update the ApiKey model to include the audit
fields from storage—created_at, updated_at, created_by, and updated_by—so the
gateway-keys issue/list/revoke flow can expose them. Keep the change localized
to ApiKey in the contracts/types model and ensure the new fields are typed
consistently with the schema and remain optional or nullable where appropriate.
In `@services/gateway/src/api/ratelimit.py`:
- Around line 33-39: The hit recording path in the rate limiter is sweeping
twice per request, which adds unnecessary O(n) work once the threshold is
reached. Update the request update flow in the rate-limiting method that touches
_sweep, _hits, and the increment logic so it only performs a single sweep per
request, preferably after updating the hit count, while preserving the window
reset behavior.
In `@services/gateway/src/directory/http_client.py`:
- Around line 15-28: The _get method in http_client.py is recreating and closing
an httpx.Client on every call when no client is injected, which prevents
connection reuse on the hot path. Update HttpClient to own a persistent default
client created in __init__ (or otherwise reused across requests) and only close
it when the instance is disposed; keep honoring an injected client, and add
lifecycle support such as close() or context manager methods if needed.
In `@services/gateway/tests/test_resolve.py`:
- Around line 55-58: The `test_requires_scope_and_key` test only covers the
missing-auth-header 401 path; extend it to also verify the insufficient-scope
403 case for the `/v1/resolve/discord/octocat` endpoint. Use the existing
`_client(...)` helper and the `FakeDir`/client setup in `test_resolve.py`, then
add a request with a key that lacks the `resolve:discord` scope and assert the
response status is 403.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 38d6c055-6694-4915-9f99-debd8a950052
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (43)
.github/workflows/ci.ymlREADME.mddocs/ARCHITECTURE.mdservices/gateway/.dockerignoreservices/gateway/.env.exampleservices/gateway/Dockerfileservices/gateway/README.mdservices/gateway/alembic.iniservices/gateway/contracts/__init__.pyservices/gateway/contracts/directory.pyservices/gateway/contracts/storage.pyservices/gateway/contracts/types.pyservices/gateway/docker-compose.ymlservices/gateway/migrations/env.pyservices/gateway/migrations/script.py.makoservices/gateway/migrations/versions/001_api_keys.pyservices/gateway/pyproject.tomlservices/gateway/railway.jsonservices/gateway/src/__init__.pyservices/gateway/src/api/__init__.pyservices/gateway/src/api/app.pyservices/gateway/src/api/auth.pyservices/gateway/src/api/deps.pyservices/gateway/src/api/hashing.pyservices/gateway/src/api/middleware.pyservices/gateway/src/api/ratelimit.pyservices/gateway/src/api/routers/__init__.pyservices/gateway/src/api/routers/resolve.pyservices/gateway/src/cli.pyservices/gateway/src/config.pyservices/gateway/src/directory/__init__.pyservices/gateway/src/directory/http_client.pyservices/gateway/src/storage/__init__.pyservices/gateway/src/storage/in_memory.pyservices/gateway/src/storage/postgres.pyservices/gateway/src/storage/schema.pyservices/gateway/tests/test_auth.pyservices/gateway/tests/test_cli.pyservices/gateway/tests/test_directory.pyservices/gateway/tests/test_health.pyservices/gateway/tests/test_ratelimit.pyservices/gateway/tests/test_resolve.pyservices/gateway/tests/test_storage.py
| run: | ||
| working-directory: services/gateway | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Tighten checkout credentials and job permissions.
This job doesn’t need a persisted write token. Set read-only permissions for the job and disable credential persistence on checkout.
🔐 Suggested workflow hardening
gateway-test:
runs-on: ubuntu-latest
+ permissions:
+ contents: read
defaults:
run:
working-directory: services/gateway
steps:
- uses: actions/checkout@v4
+ with:
+ persist-credentials: false
- name: Install uv
uses: astral-sh/setup-uv@v5📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - uses: actions/checkout@v4 | |
| gateway-test: | |
| runs-on: ubuntu-latest | |
| permissions: | |
| contents: read | |
| defaults: | |
| run: | |
| working-directory: services/gateway | |
| steps: | |
| - uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false | |
| - name: Install uv | |
| uses: astral-sh/setup-uv@v5 |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 80-80: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for 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.
In @.github/workflows/ci.yml at line 80, The checkout step currently persists
the GitHub token and the job lacks explicit least-privilege permissions. Update
the workflow job that uses actions/checkout@v4 to set read-only job permissions
and configure the checkout action to disable credential persistence, so the
token is not stored for later git operations.
Source: Linters/SAST tools
Comment on lines
+1
to
+11
| FROM python:3.11-slim | ||
| COPY --from=ghcr.io/astral-sh/uv:latest /uv /usr/local/bin/uv | ||
| WORKDIR /app | ||
| COPY pyproject.toml uv.lock ./ | ||
| COPY packages/ ./packages/ | ||
| COPY services/gateway/ ./services/gateway/ | ||
| RUN uv sync --frozen --no-dev --package gateway | ||
| ENV PATH="/app/.venv/bin:$PATH" | ||
| WORKDIR /app/services/gateway | ||
| EXPOSE 8000 | ||
| CMD ["uvicorn", "src.api.app:app", "--host", "0.0.0.0", "--port", "8000"] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Run the gateway as a non-root user.
The container currently starts Uvicorn as root. For a public-facing gateway, that’s unnecessary risk.
🔧 Suggested Dockerfile hardening
FROM python:3.11-slim
COPY --from=ghcr.io/astral-sh/uv:latest /uv /usr/local/bin/uv
WORKDIR /app
COPY pyproject.toml uv.lock ./
COPY packages/ ./packages/
COPY services/gateway/ ./services/gateway/
RUN uv sync --frozen --no-dev --package gateway
ENV PATH="/app/.venv/bin:$PATH"
+RUN addgroup --system gateway && adduser --system --ingroup gateway gateway
+USER gateway
WORKDIR /app/services/gateway
EXPOSE 8000
CMD ["uvicorn", "src.api.app:app", "--host", "0.0.0.0", "--port", "8000"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| FROM python:3.11-slim | |
| COPY --from=ghcr.io/astral-sh/uv:latest /uv /usr/local/bin/uv | |
| WORKDIR /app | |
| COPY pyproject.toml uv.lock ./ | |
| COPY packages/ ./packages/ | |
| COPY services/gateway/ ./services/gateway/ | |
| RUN uv sync --frozen --no-dev --package gateway | |
| ENV PATH="/app/.venv/bin:$PATH" | |
| WORKDIR /app/services/gateway | |
| EXPOSE 8000 | |
| CMD ["uvicorn", "src.api.app:app", "--host", "0.0.0.0", "--port", "8000"] | |
| FROM python:3.11-slim | |
| COPY --from=ghcr.io/astral-sh/uv:latest /uv /usr/local/bin/uv | |
| WORKDIR /app | |
| COPY pyproject.toml uv.lock ./ | |
| COPY packages/ ./packages/ | |
| COPY services/gateway/ ./services/gateway/ | |
| RUN uv sync --frozen --no-dev --package gateway | |
| ENV PATH="/app/.venv/bin:$PATH" | |
| RUN addgroup --system gateway && adduser --system --ingroup gateway gateway | |
| USER gateway | |
| WORKDIR /app/services/gateway | |
| EXPOSE 8000 | |
| CMD ["uvicorn", "src.api.app:app", "--host", "0.0.0.0", "--port", "8000"] |
🧰 Tools
🪛 Trivy (0.69.3)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
🤖 Prompt for 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.
In `@services/gateway/Dockerfile` around lines 1 - 11, The gateway container
currently runs Uvicorn as root, so harden the Dockerfile by running the final
image under a dedicated non-root user. Update the service image setup around the
existing COPY/uv sync/ENV/CMD steps to create or switch to a low-privilege user
before starting uvicorn, and ensure the app files and working directory used by
services/gateway are accessible to that user. Keep the startup command unchanged
except for executing it as the non-root user.
Source: Linters/SAST tools
Comment on lines
+5
to
+6
| [tool.hatch.build.targets.wheel] | ||
| packages = ["src"] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
from pathlib import Path
import tomllib
root = Path("services/gateway")
cfg = tomllib.loads((root / "pyproject.toml").read_text())
print("wheel packages:", cfg["tool"]["hatch"]["build"]["targets"]["wheel"]["packages"])
for rel in ("src", "contracts"):
p = root / rel
print(f"{rel}: exists={p.exists()} __init__.py={(p / '__init__.py').exists()}")
PYRepository: qiuethan/Misty
Length of output: 250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
root="services/gateway"
echo "== pyproject.toml =="
sed -n '1,120p' "$root/pyproject.toml"
echo
echo "== package/import references =="
rg -n --hidden --glob '!**/.git/**' '(^|[^A-Za-z0-9_])contracts([./A-Za-z0-9_]|$)|from contracts|import contracts' "$root" || true
echo
echo "== tree (top-level) =="
find "$root" -maxdepth 2 -type f | sed "s#^$root/##" | sort | head -200Repository: qiuethan/Misty
Length of output: 2398
Include contracts/ in the wheel packages.
packages = ["src"] only packages the src module, but the service imports contracts.* at runtime. Add contracts here or those imports will fail once the wheel is installed.
🤖 Prompt for 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.
In `@services/gateway/pyproject.toml` around lines 5 - 6, The wheel packaging
config currently only includes the src package, but the gateway also imports
contracts.* at runtime. Update the [tool.hatch.build.targets.wheel] packages
setting in pyproject.toml to include contracts alongside src so the installed
wheel contains both package trees and those imports continue to work.
Comment on lines
+70
to
+72
| ``` | ||
| GET /v1/resolve/discord/{github_login} | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Annotate the remaining fenced blocks.
markdownlint is warning on the response example and repo-layout tree because both fences are missing a language tag. Adding json and text (or similar) will keep the docs lint-clean.
Also applies to: 121-147
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 70-70: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for 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.
In `@services/gateway/README.md` around lines 70 - 72, The remaining fenced code
blocks in the README are missing language tags, which triggers markdownlint.
Update the fence examples around the response example and the repo-layout tree,
plus any other remaining fenced blocks in the referenced sections, by adding
appropriate labels such as json and text so the documentation stays lint-clean.
Use the existing README fenced block structure as the target, and make sure
every unannotated fence in that area is labeled consistently.
Source: Linters/SAST tools
Comment on lines
+11
to
+42
| class RateLimitMiddleware(BaseHTTPMiddleware): | ||
| _SWEEP_THRESHOLD = 1024 | ||
|
|
||
| def __init__(self, app, limit: int = 60, window_s: int = 60): | ||
| super().__init__(app) | ||
| self._limit = limit | ||
| self._window = window_s | ||
| self._hits: dict[str, tuple[int, float]] = {} # key -> (count, window_start) | ||
|
|
||
| def _sweep(self, now: float) -> None: | ||
| """Opportunistically evict entries whose window has fully expired. | ||
| Only runs once the dict grows large, so it stays O(1) amortized.""" | ||
| if len(self._hits) < self._SWEEP_THRESHOLD: | ||
| return | ||
| expired = [k for k, (_, start) in self._hits.items() if now - start >= self._window] | ||
| for k in expired: | ||
| del self._hits[k] | ||
|
|
||
| async def dispatch(self, request: Request, call_next): | ||
| key = request.headers.get("X-API-Key") | ||
| if key: | ||
| now = time.monotonic() | ||
| self._sweep(now) | ||
| count, start = self._hits.get(key, (0, now)) | ||
| if now - start >= self._window: | ||
| count, start = 0, now | ||
| count += 1 | ||
| self._hits[key] = (count, start) | ||
| self._sweep(now) | ||
| if count > self._limit: | ||
| return JSONResponse(status_code=429, content={"detail": "rate limit exceeded"}) | ||
| return await call_next(request) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
fd -a 'app.py|ratelimit.py|*auth*|*api_key*|*dependency*|*test*' services/gateway/srcRepository: qiuethan/Misty
Length of output: 508
🏁 Script executed:
fd --fixed-strings -t f 'services/gateway/src/api/app.py'
fd --fixed-strings -t f 'services/gateway/src/api/ratelimit.py'
fd --hidden -t f services/gateway/src | rg 'auth|api_key|dependency|test'Repository: qiuethan/Misty
Length of output: 538
🏁 Script executed:
fd . services/gateway/srcRepository: qiuethan/Misty
Length of output: 955
🏁 Script executed:
wc -l services/gateway/src/api/app.py services/gateway/src/api/auth.py services/gateway/src/api/deps.py services/gateway/src/api/ratelimit.py services/gateway/src/api/middleware.py
ast-grep outline services/gateway/src/api/app.py --view expanded
ast-grep outline services/gateway/src/api/auth.py --view expanded
ast-grep outline services/gateway/src/api/deps.py --view expanded
ast-grep outline services/gateway/src/api/ratelimit.py --view expanded
ast-grep outline services/gateway/src/api/middleware.py --view expanded
fd . services/gateway | rg 'test|spec'Repository: qiuethan/Misty
Length of output: 1440
🏁 Script executed:
cat -n services/gateway/src/api/app.py
printf '\n--- auth.py ---\n'
cat -n services/gateway/src/api/auth.py
printf '\n--- ratelimit.py ---\n'
cat -n services/gateway/src/api/ratelimit.py
printf '\n--- test_ratelimit.py ---\n'
cat -n services/gateway/tests/test_ratelimit.pyRepository: qiuethan/Misty
Length of output: 7490
🏁 Script executed:
rg -n 'X-API-Key|api_key|require_api_key|build_auth|envelope="gw_"|gw_' services/gateway/src services/gateway/testsRepository: qiuethan/Misty
Length of output: 8756
Bound the rate-limit key cache
_hits is keyed by the raw X-API-Key header, so a client can spray unique values and grow this dict until entries expire. Since this middleware runs before auth, that’s an easy memory-DoS path. Add a hard cap/LRU eviction or validate keys before inserting them.
🤖 Prompt for 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.
In `@services/gateway/src/api/ratelimit.py` around lines 11 - 42, The
RateLimitMiddleware._hits cache is currently unbounded because dispatch() stores
every raw X-API-Key value before auth, which can be abused to grow memory.
Update RateLimitMiddleware to cap this cache with a hard maximum or LRU-style
eviction, and only insert validated/known API keys into _hits. Keep the fix
localized to RateLimitMiddleware._sweep and dispatch so the per-key rate
tracking still works without allowing unlimited growth.
Comment on lines
+16
to
+26
| person = directory.get_person_by_github(github_login) | ||
| if person is None: | ||
| raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="github login not found") | ||
| ids = directory.list_identifiers(person["id"]) | ||
| discord_id = next((i["external_id"] for i in ids if i.get("provider") == "discord"), None) | ||
| if discord_id is None: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_404_NOT_FOUND, | ||
| detail="no discord identifier for that github login", | ||
| ) | ||
| return {"discord_id": discord_id} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Distinguishable 404 messages allow GitHub-login enumeration.
Returning a different detail for "login not found" (Line 18) vs. "no discord identifier for that github login" (Line 22-25) lets a caller holding a valid resolve:discord key distinguish between "this GitHub login isn't in our directory" and "it exists but has no linked Discord." That directly leaks directory membership (person data) to external callers, contradicting the PR's own stated goal of not leaking person data through this endpoint.
Use one generic 404 message/body for both cases so external callers can't distinguish "unknown login" from "no discord link."
🔒 Proposed fix: unify 404 responses
person = directory.get_person_by_github(github_login)
- if person is None:
- raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="github login not found")
- ids = directory.list_identifiers(person["id"])
- discord_id = next((i["external_id"] for i in ids if i.get("provider") == "discord"), None)
- if discord_id is None:
- raise HTTPException(
- status_code=status.HTTP_404_NOT_FOUND,
- detail="no discord identifier for that github login",
- )
+ discord_id = None
+ if person is not None:
+ ids = directory.list_identifiers(person["id"])
+ discord_id = next((i["external_id"] for i in ids if i.get("provider") == "discord"), None)
+ if discord_id is None:
+ raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="not found")
return {"discord_id": discord_id}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| person = directory.get_person_by_github(github_login) | |
| if person is None: | |
| raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="github login not found") | |
| ids = directory.list_identifiers(person["id"]) | |
| discord_id = next((i["external_id"] for i in ids if i.get("provider") == "discord"), None) | |
| if discord_id is None: | |
| raise HTTPException( | |
| status_code=status.HTTP_404_NOT_FOUND, | |
| detail="no discord identifier for that github login", | |
| ) | |
| return {"discord_id": discord_id} | |
| person = directory.get_person_by_github(github_login) | |
| discord_id = None | |
| if person is not None: | |
| ids = directory.list_identifiers(person["id"]) | |
| discord_id = next((i["external_id"] for i in ids if i.get("provider") == "discord"), None) | |
| if discord_id is None: | |
| raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="not found") | |
| return {"discord_id": discord_id} |
🤖 Prompt for 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.
In `@services/gateway/src/api/routers/resolve.py` around lines 16 - 26, The
resolve endpoint currently returns distinguishable 404 details in the
github_login lookup flow, which exposes whether a login exists or only lacks a
Discord link. Update the logic in resolve() so both the get_person_by_github()
miss and the missing discord_id branch raise the same generic HTTPException 404
response, using one shared detail/body message and keeping the response
identical for both paths.
Comment on lines
+93
to
+102
| def revoke_api_key(self, api_key_id: UUID, *, actor: str) -> ApiKey | None: | ||
| now = _now() | ||
| with self._engine.begin() as conn: | ||
| row = conn.execute( | ||
| update(api_keys) | ||
| .where(api_keys.c.id == api_key_id) | ||
| .values(active=False, revoked_at=now, updated_at=now, updated_by=actor) | ||
| .returning(api_keys) | ||
| ).one_or_none() | ||
| return _api_key_row_to_model(row) if row else None |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Re-revoking an already-revoked key overwrites the original revoked_at.
The update has no guard on current active state, so calling revoke_api_key twice (e.g. concurrent CLI revocations, or an accidental repeat call) will silently overwrite the original revoked_at/updated_by audit trail with the new call's values. Given the PR's emphasis on auditing, the first revocation timestamp/actor is lost.
♻️ Suggested guard to preserve original revocation record
def revoke_api_key(self, api_key_id: UUID, *, actor: str) -> ApiKey | None:
now = _now()
with self._engine.begin() as conn:
row = conn.execute(
update(api_keys)
- .where(api_keys.c.id == api_key_id)
+ .where(api_keys.c.id == api_key_id, api_keys.c.active.is_(True))
.values(active=False, revoked_at=now, updated_at=now, updated_by=actor)
.returning(api_keys)
).one_or_none()
+ if row is None:
+ with self._engine.connect() as conn:
+ row = conn.execute(select(api_keys).where(api_keys.c.id == api_key_id)).one_or_none()
return _api_key_row_to_model(row) if row else None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def revoke_api_key(self, api_key_id: UUID, *, actor: str) -> ApiKey | None: | |
| now = _now() | |
| with self._engine.begin() as conn: | |
| row = conn.execute( | |
| update(api_keys) | |
| .where(api_keys.c.id == api_key_id) | |
| .values(active=False, revoked_at=now, updated_at=now, updated_by=actor) | |
| .returning(api_keys) | |
| ).one_or_none() | |
| return _api_key_row_to_model(row) if row else None | |
| def revoke_api_key(self, api_key_id: UUID, *, actor: str) -> ApiKey | None: | |
| now = _now() | |
| with self._engine.begin() as conn: | |
| row = conn.execute( | |
| update(api_keys) | |
| .where(api_keys.c.id == api_key_id, api_keys.c.active.is_(True)) | |
| .values(active=False, revoked_at=now, updated_at=now, updated_by=actor) | |
| .returning(api_keys) | |
| ).one_or_none() | |
| if row is None: | |
| with self._engine.connect() as conn: | |
| row = conn.execute(select(api_keys).where(api_keys.c.id == api_key_id)).one_or_none() | |
| return _api_key_row_to_model(row) if row else None |
🤖 Prompt for 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.
In `@services/gateway/src/storage/postgres.py` around lines 93 - 102, The
revoke_api_key method currently updates revoked_at and updated_by even when the
key is already revoked, which overwrites the original audit trail. Add a guard
in revoke_api_key’s update query so it only applies when api_keys.active is
still true (or otherwise preserves existing revoked_at/updated_by), and return
None or no-op on repeat revocations. Keep the fix localized to revoke_api_key in
the Postgres storage code and ensure the returning(api_keys) path still maps
correctly through _api_key_row_to_model.
Collaborator
|
lgtm |
…ng (#59) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
platform_auth's env-bootstrap path mints an AuthedKey carrying ADMIN_SCOPE, and ADMIN_SCOPE is a wildcard — has_scope() returns True for every scope once it is present. On the internal services that is a deliberate grace path: it authenticates you to the admin API that issues the first real key, and it is only reachable from the private network. The gateway has neither half of that justification. Its keys are issued direct-to-DB by gateway-keys, so there is no admin API to bootstrap, and it is the one service exposed to the internet. Wiring API_KEY into it put a single wildcard credential on the public door for no gain. Pass get_env_key=lambda: None to disable the path, and drop the api_key setting so it cannot be quietly re-added. Also wrap directory_api_key in SecretStr, matching the convention the other services adopted after this branch forked: a plain str prints in full on any repr/diff/traceback. verify_production_secrets unwraps it explicitly, since a SecretStr never compares equal to a str and the guard would otherwise stop firing in silence — the failure mode platform_auth's secret_guard docstring asks each service to pin. tests/test_config.py pins it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rs (#59) The old limiter kept an unbounded dict keyed on the raw X-API-Key header, in front of auth. Three problems, all of which matter more here than they would on a private service: - anyone could grow it without limit inside a window, since the sweep only evicted entries whose window had already expired; - past 1024 entries every request paid a full O(n) scan of the dict, and _sweep was called twice per request, so the cost of a flood landed on everyone else; - rotating the header value defeated the limit outright, which is what makes the first two exploitable rather than theoretical — and a well-formed gw_ key forces an argon2 verification, so unmetered attempts are unmetered CPU. Split it into the two things it was conflating. The per-consumer quota is now a dependency running after require_api_key, keyed on AuthedKey.name: that set is bounded by the keys we have issued, so it cannot be grown from outside, and no plaintext key sits in process memory. In front of auth sits a per-IP flood guard, whose only job is to cap how much argon2 work one address can demand; /health is exempt so the liveness probe cannot be throttled. Both use FixedWindowCounter, which is bounded by construction: entries are ordered by window start, so eviction takes the oldest at O(1) instead of scanning. When trusting proxy headers it reads the rightmost X-Forwarded-For hop, the only one a client cannot write itself — the leftmost would have been per-request bucket rotation by design. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…#59) The two misses had different bodies — "github login not found" versus "no discord identifier for that github login" — which let any holder of a resolve:discord key walk a list of GitHub logins and learn which of them belong to UTMIST members. That is a membership oracle on a public endpoint, and it is precisely the disclosure this service exists to prevent. Both now return one body; the distinction survives in the audit log, where only we can read it. While in here: - declare the response as a model, so the "only ever discord_id" contract is stated in the OpenAPI schema rather than only in prose; - read `person["id"]` as `.get("id")` and fail closed to 503 when it is absent. An unrecognised upstream shape is an upstream fault, not a missing record, and blind indexing surfaced it as a 500; - record why `github_login` reaches the audit log, since it is a path segment and the middleware logs the path. That is deliberate — it is what makes abuse of the public door investigable — but the branch had claimed the opposite, so it needed saying where someone will read it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Five small things, none of them worth its own commit: - HttpDirectoryClient now holds one pooled httpx.Client for its lifetime, and deps caches the instance. It built and closed a client per call, and the resolver makes two calls per request, so the hot path was paying two fresh TCP + TLS handshakes. - touch_api_key_last_used logs instead of swallowing silently. Best-effort is right — a DB blip must not fail auth over a bookkeeping write — but in silence the only symptom is a key that looks unused while in daily use. - gateway-keys issue reports a duplicate --name as a sentence and exit 1, matching revoke, rather than an argparse-free traceback. Nothing goes to stdout on that path: a caller piping stdout into a secrets store must not be handed a key that was never persisted. - CI runs `ruff format --check`, like every other Python job in this workflow. It did not, which is why the format pass below touches files this branch had otherwise left alone. - `ruff format` over the service, to make that check pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The branch's docs claimed the audit log "never carries person data". It does: the GitHub login is a path segment, and AuditLogMiddleware records request.url.path, so every call writes the login the caller asked about. That is the right behaviour — an audit trail for a public endpoint that omitted what was requested would be useless for investigating abuse, and the value is public, pseudonymous, and caller-supplied — but the docs said the opposite, and a security claim nobody has checked is worse than none. Now stated, along with what genuinely never appears there: anything the directory answered back. Also documents a constraint the resolver inherits and cannot fix locally: team-tracking matches person_identifiers.external_id exactly for every provider but email, so GitHub logins resolve case-sensitively even though GitHub's are not. The gateway deliberately does not lowercase the login — that only helps if stored values are already lowercase, and breaks the mixed-case links that work today. The fix is upstream (normalise github identifiers on write, plus a migration) and is tracked separately. Plus the fallout from the preceding commits: no API_KEY, the two rate-limit layers, TRUST_PROXY_HEADERS, the single 404 body, and a warning never to issue a gateway key scoped `admin`. The local dev port moves 8002 → 8006, which was colliding with llm. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
qiuethan
force-pushed
the
worktree-external-gateway
branch
from
August 17, 2026 00:31
c92dc64 to
84e651f
Compare
github-actions
Bot
added
zone: services/other
label-consistency fails a services/ directory that has no zone of its own, because it falls through to the services/* catch-all — the bucket that hid the August 2026 drift, where several services shared one zone and a PR spanning two of them looked single-zone. The gateway is a new services/ member and this machinery landed after this branch forked, so it arrived unregistered. All five hand-maintained lists, per docs/CODE-OWNERSHIP.md: zone_for() in pr-zone-check.yml (canonical), CODEOWNERS, labeler.yml (the zone key *and* its negation in the services/other catch-all, or the zone gets two labels), the PR template, and the table in CODE-OWNERSHIP.md. Its CI job is already in this branch. `node scripts/check-labels.mjs` passes. Still needs `gh label create "zone: services/gateway" --color BFD4F2` — nothing checks that the label exists, and an uncreated one is created on first use in a random colour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
qiuethan
added
zone: services/gateway
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #59. Part of the #60 access-architecture epic. Built on #58's
platform_auth(now live).The platform's external door: a new public FastAPI service (
services/gateway) that authenticates external consumers with per-consumer scoped keys, rate-limits + audits them, and exposes a curated read surface composed from the private services. Everything else stays private; this is the one publicly-reachable service.What's in here
Inbound (external): per-consumer scoped API keys in the gateway's own
api_keysDB, issued/revoked at runtime via agateway-keysCLI (nothing in code/config), verified viaplatform_auth; two-layer rate limiting (per-key quota behind auth, per-IP flood guard in front) + shared audit;/v1+ OpenAPI.Outbound (internal): holds one team-tracking key (
identifiers:read) to call the private directory over the private network; returns only curated fields.First endpoint:
Serves #34 (reviewer-ping GitHub→Discord).
Security posture (it's the public door)
API_KEYenv var, whichplatform_authresolves to the wildcardadminscope. That is a sanctioned grace path on the private network — it's how you reach the admin API that issues the first real key. The gateway has no admin API (keys go straight to its DB viagateway-keys) and is the only service on the internet, so it passesget_env_key=lambda: Noneand there is noAPI_KEYsetting at all. Every caller must present an issued, scoped key. Pinned by test.{"discord_id": ...}— nothing else about the person leaves the service, in success or error bodies.AuditLogMiddlewarerecords the path. That's intentional (an audit trail for the public door that omits what was requested is useless for investigating abuse) and the value is public, pseudonymous, and caller-supplied. What never appears there is anything the directory answered back.AuthedKey.name— so it can only ever be keyed on a key we issued, which bounds the counter and keeps plaintext keys out of process memory. The per-IP flood guard (120/60s,/healthexempt) runs in front of auth, because authentication is itself the expensive step: a well-formedgw_key forces an argon2 verification, and per-key limiting can't bound that (an attacker just varies the key). Both use aFixedWindowCounterthat is bounded by construction, with O(1) eviction of the oldest window.platform_authre-checksactive/revoked_at).SecretStr, unwrapped only at the two boundaries that need the raw value. The internal key lives only in the outbound header — never returned or logged;verify_production_secretsfails the service closed if it's still the dev default outside local.github_loginis percent-encoded into the outbound URL (defense-in-depth).Known constraint
GitHub logins resolve case-sensitively. team-tracking matches
person_identifiers.external_idexactly for every provider exceptemail, so a person stored asoctocatis not found by a request forOctoCat. The gateway deliberately does not lowercase the login — that would only help if stored values were already lowercase, and would break the mixed-case links that work today. The fix belongs upstream (normalisegithubidentifiers on write + a migration for existing rows) and is tracked separately. Documented in the service README; relevant to #34.Testing / review
ruff checkandruff format --checkboth clean.staging, and revised after a review pass that found two blocking issues on the public door (the wildcard env key above; a rate limiter that kept an unbounded dict keyed on the rawX-API-Keyheader in front of auth, which was both a memory-growth vector and trivially bypassed by rotating the key).DATABASE_URLper env.team-tracking-keys issue --name gateway --scopes identifiers:read→ set as the gateway'sDIRECTORY_API_KEY; setDIRECTORY_BASE_URLto team-tracking's private URL./+ Railway Config File/services/gateway/railway.json; scale to us-east; generate a public domain.TRUST_PROXY_HEADERS=trueon the Railway service. Railway terminates TLS in front of the gateway, and without this the per-IP flood guard sees the proxy's address and buckets every caller together.gateway-keys issue --name reviewer-ping --scopes resolve:discord→ store as the GitHub Action's secret; point the Action athttps://<gateway-domain>/v1/resolve/discord/{login}. Scope it to exactly that — never issue a gateway key withadmin.🤖 Generated with Claude Code