docs: expand admin interface page (MIR-1176)#831
Conversation
Audit the admin-interface doc against the live runtime/CLI and fill in the missing details users need to expose and call admin methods safely: - Security: token format/rotation, reserved ADMIN_TOKEN env var, and the X-Miren-Access trust model. - Auditing: admin calls land in the app's log stream. - JSON-RPC shape: object vs positional params, 30s call timeout, and HTTP status semantics. - Introspection: $-prefixed methods ($methods, $type) are reserved. - CLI: --func-help, --params-file (incl. stdin), mixed input styles, type-aware parsing, kebab→snake flag normalization. Move the per-language samples (Python, Node.js, Bun) into a single "More Implementation Examples" section below CLI usage, with Other Languages linking down to it. Switches the Go and Node.js auth checks to constant-time comparison.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR expands the admin-interface docs: removes the initial “Labs Feature” callout; extends security guidance with ADMIN_TOKEN handling and constant-time bearer-token validation in Go; documents X-Miren-Access routing and auditing (log-stream entries); specifies JSON-RPC contract details (params shapes, 30s timeout, HTTP/JSON semantics) and reserved 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/docs/admin-interface.md (1)
458-504:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate Python bearer-token validation to use constant-time comparison
The Python example uses direct string equality (
auth != f'Bearer {ADMIN_TOKEN}'); switch it tohmac.compare_digestto avoid timing side channels and match the security guidance used elsewhere on the page.import hmac # ... auth = request.headers.get('Authorization', '') expected = f'Bearer {ADMIN_TOKEN}' if ADMIN_TOKEN and not hmac.compare_digest(auth, expected): return 'Unauthorized', 401🤖 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 `@docs/docs/admin-interface.md` around lines 458 - 504, Replace the direct string equality check in admin_endpoint that compares the Authorization header to f'Bearer {ADMIN_TOKEN}' with a constant-time comparison: import hmac at the top, build expected = f'Bearer {ADMIN_TOKEN}', and use hmac.compare_digest(auth, expected) when ADMIN_TOKEN is set to decide unauthorized responses; update the auth validation block (variables: ADMIN_TOKEN, auth, expected) accordingly.
🤖 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 `@docs/docs/admin-interface.md`:
- Line 82: Replace the typo "httpingress" in the sentence "Miren's admin proxy
talks to your app over its **internal** httpingress path, never via the public
route." with a clearer term like "HTTP ingress" or "internal ingress" so the
line reads e.g. "Miren's admin proxy talks to your app over its internal HTTP
ingress path, never via the public route."; update only that token while
preserving emphasis and surrounding wording.
---
Outside diff comments:
In `@docs/docs/admin-interface.md`:
- Around line 458-504: Replace the direct string equality check in
admin_endpoint that compares the Authorization header to f'Bearer {ADMIN_TOKEN}'
with a constant-time comparison: import hmac at the top, build expected =
f'Bearer {ADMIN_TOKEN}', and use hmac.compare_digest(auth, expected) when
ADMIN_TOKEN is set to decide unauthorized responses; update the auth validation
block (variables: ADMIN_TOKEN, auth, expected) accordingly.
🪄 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: dc96118c-3905-4ca4-86d2-185555e51eb7
📒 Files selected for processing (1)
docs/docs/admin-interface.md
phinze
left a comment
phinze
left a comment
There was a problem hiding this comment.
We reviewed this as an audit: every runtime claim on the page, traced back to the code. They all check out — token generation and env-injection in launcher.go, the X-Miren-Access strip/set in httpingress.go, the audit-log shape (source=admin, params size, status, duration_ms), the 30s timeout, the verbatim status-error string, and the whole CLI surface in admin.go (param channels, type coercion, kebab→snake, --no-validate). For a doc this dense with specific claims, 100% accuracy is rare. Nice work.
One suggestion inline about the public-route block, and +1 to CodeRabbit's "httpingress" → "internal HTTP ingress" on line 82. Neither blocking.
CodeRabbit feedback on #831: - Fix "httpingress" jargon → "internal HTTP ingress". - Spell out that the ingress returns 404 for any public request to /.well-known/miren/admin before it reaches the app (httpingress.go:488), so the X-Miren-Access check is defense-in-depth rather than the sole guard.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/docs/admin-interface.md (1)
474-476:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse constant-time comparison to prevent timing attacks.
The Python example uses standard string comparison (
auth != f'Bearer {ADMIN_TOKEN}'), which is vulnerable to timing attacks. An attacker could measure response times to extract the token byte-by-byte. All other language examples (Go, Node.js, Bun) correctly use constant-time comparison functions.🔒 Proposed fix using constant-time comparison
from flask import Flask, request, jsonify +import hmac import os app = Flask(__name__) ADMIN_TOKEN = os.environ.get('ADMIN_TOKEN', '') `@app.route`('/.well-known/miren/admin', methods=['POST']) def admin_endpoint(): # Validate token auth = request.headers.get('Authorization', '') - if ADMIN_TOKEN and auth != f'Bearer {ADMIN_TOKEN}': + expected = f'Bearer {ADMIN_TOKEN}' + if ADMIN_TOKEN and not hmac.compare_digest(auth, expected): return 'Unauthorized', 401🤖 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 `@docs/docs/admin-interface.md` around lines 474 - 476, The current comparison auth != f'Bearer {ADMIN_TOKEN}' is vulnerable to timing attacks; update the check to use a constant-time comparison using hmac.compare_digest by importing hmac and comparing auth to expected = f'Bearer {ADMIN_TOKEN}' with hmac.compare_digest(expected, auth) (and ensure behavior returns 'Unauthorized', 401 when the compare_digest is False or when ADMIN_TOKEN is empty); reference the ADMIN_TOKEN variable and the auth value and replace the direct != check with the hmac.compare_digest-based boolean test.
🤖 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.
Outside diff comments:
In `@docs/docs/admin-interface.md`:
- Around line 474-476: The current comparison auth != f'Bearer {ADMIN_TOKEN}' is
vulnerable to timing attacks; update the check to use a constant-time comparison
using hmac.compare_digest by importing hmac and comparing auth to expected =
f'Bearer {ADMIN_TOKEN}' with hmac.compare_digest(expected, auth) (and ensure
behavior returns 'Unauthorized', 401 when the compare_digest is False or when
ADMIN_TOKEN is empty); reference the ADMIN_TOKEN variable and the auth value and
replace the direct != check with the hmac.compare_digest-based boolean test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1726f90b-60c8-40e6-8e8d-0268221e4103
📒 Files selected for processing (1)
docs/docs/admin-interface.md
Match the constant-time comparison the page recommends and that the Go, Node.js, and Bun examples already use. hmac.compare_digest has been in the stdlib since Python 3.3.
![miren-code-agent[bot]](https://avatars.githubusercontent.com/u/174767976?s=60&v=4){kind=small}
There was a problem hiding this comment.
Re-review: docs: expand admin interface page (MIR-1176)
Both issues I flagged in my previous review have been addressed — cleanly and completely. Here's what I found:
✅ Previous concerns: fully resolved
1. httpingress wording (line 82)
The term is gone from the docs entirely. The new text reads:
"Miren's admin proxy talks to your app over its internal HTTP ingress path, never via the public route."
That's exactly the kind of user-facing clarity I was looking for. Nicely done.
2. Python example timing-safe comparison (line 476)
import hmac was added on line 466, and the naive != check was replaced with:
if ADMIN_TOKEN and not hmac.compare_digest(auth, f'Bearer {ADMIN_TOKEN}'):The Python example now matches the security bar set by the Go and JS/TS examples. The page is internally consistent. This was the one thing I felt strongly about, and it's fixed exactly as I suggested.
What's new since my last review
The PR has also grown meaningfully — the "Calling Admin Methods" section was substantially restructured and expanded, moving it after the implementation examples and adding real depth: discovery (--list, --func-help), all three parameter channels with examples, the type coercion table, kebab→snake normalization, output format switching, and --no-validate. The new full Node.js/Express and Bun/TypeScript examples are complete, idiomatic, and both use crypto.timingSafeEqual consistently. CodeRabbit flagged the Python issue again on the third commit, so it's worth noting that it was addressed in the head commit (line 476) — both bots and I raised it, and the author fixed it.
Overall
This is a high-quality, well-reviewed documentation page. @phinze traced every runtime claim to source code and approved. All security guidance is now consistent across all four language examples. The content is accurate, complete, and helpful. I'm happy to recommend this for merge.
evanphx
deleted the
mir-1176-document-how-to-expose-and-access-an-admin-api
branch
3 participants
Summary
Audits
docs/docs/admin-interface.mdagainst the live runtime and CLI, and fills in the missing details users need to expose and call admin methods safely.ADMIN_TOKENas a reserved env var, and theX-Miren-Accesstrust model.source=adminentries.$-prefixed method names ($methods,$type) are reserved.--func-help,--params-file(including stdin via-), mixed input styles, type-aware parsing, and kebab→snake flag normalization.Linear: MIR-1176
Test plan
cd docs && bun run build— builds cleanly (no MDX/markdown errors)./admin-interfaceend-to-end:<CliCommand>blocks render, tables render, headings link correctly, and the in-page link from "Other Languages" jumps to the examples at the bottom.