feat: require bearer token for facilitator endpoints#36
feat: require bearer token for facilitator endpoints#36ponderingdemocritus merged 2 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughThe PR implements Bearer token authentication for the facilitator server by introducing a reusable Elysia module system, adding a bearer token middleware module, and enhancing CI workflows with improved container readiness checks and token-based configuration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🤖 Fix all issues with AI agents
In `@examples/facilitator-server/src/modules/bearer-token.ts`:
- Around line 44-46: The escapeRealm function currently only escapes double
quotes and misses backslashes; update escapeRealm(realm: string) to first
replace backslashes with double-backslashes and then escape double quotes (i.e.,
realm.replace(/\\/g, '\\\\').replace(/"/g, '\\"')) so both '\' and '"' are
properly escaped per RFC 7230 quoted-string rules; ensure you modify the
escapeRealm function accordingly.
🧹 Nitpick comments (4)
.github/workflows/ci.yml (1)
111-126: Redundant final curl; if it fails, no logs are dumped.Line 126 re-runs the same health check that already succeeded inside the loop. It's mostly harmless, but if the container happens to crash between the loop exit and this call, the script fails without dumping logs (unlike the
ready != "true"branch). Consider removing it or, if you want to keep a final assertion, wrapping it in similar error-reporting logic.Suggested simplification
if [ "$ready" != "true" ]; then echo "Container did not become ready. Dumping logs:" docker logs test-facilitator || true exit 1 fi - - curl -fsS http://localhost:8090/supported >/dev/nullexamples/facilitator-server/src/app.ts (1)
103-103:AppModule = unknowndiscards type safety.This forces
as anyon line 152 and allows callers to pass anything (numbers, strings, etc.) in themodulesarray without compile-time feedback. Consider a minimal function signature that matches the Elysia plugin contract.Suggested improvement
-export type AppModule = unknown; +export type AppModule = (app: Elysia) => Elysia;This would also let you drop the
as anycast on line 152.examples/facilitator-server/src/modules/bearer-token.ts (1)
48-50: Return typeunknownloses the Elysia plugin contract.Same as
AppModuleinapp.ts— returningunknownhides the actual(app: Elysia) => Elysiasignature. Typing it properly would give callers andapp.use()better compile-time checks.examples/facilitator-server/src/index.ts (1)
30-44: Token loading logic is sound.The precedence (
BEARER_TOKENS→BEARER_TOKEN→ error) is clear, and the fail-fast throw on missing tokens prevents starting without auth. This is a good security posture.One minor nit: the error message reads awkwardly — "to require bearer auth for facilitator startup" suggests the purpose is to require auth, rather than conveying that auth is required for startup.
Suggested wording
throw new Error( - "Set BEARER_TOKEN or BEARER_TOKENS to require bearer auth for facilitator startup." + "BEARER_TOKEN or BEARER_TOKENS environment variable is required." );
| function escapeRealm(realm: string): string { | ||
| return realm.replace(/"/g, '\\"'); | ||
| } |
There was a problem hiding this comment.
escapeRealm doesn't escape backslashes.
Per RFC 7230's quoted-string grammar, both " and \ must be escaped in a quoted string value. A realm containing a backslash would produce a malformed WWW-Authenticate header.
Suggested fix
function escapeRealm(realm: string): string {
- return realm.replace(/"/g, '\\"');
+ return realm.replace(/[\\"]/g, '\\$&');
}📝 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.
| function escapeRealm(realm: string): string { | |
| return realm.replace(/"/g, '\\"'); | |
| } | |
| function escapeRealm(realm: string): string { | |
| return realm.replace(/[\\"]/g, '\\$&'); | |
| } |
🤖 Prompt for AI Agents
In `@examples/facilitator-server/src/modules/bearer-token.ts` around lines 44 -
46, The escapeRealm function currently only escapes double quotes and misses
backslashes; update escapeRealm(realm: string) to first replace backslashes with
double-backslashes and then escape double quotes (i.e., realm.replace(/\\/g,
'\\\\').replace(/"/g, '\\"')) so both '\' and '"' are properly escaped per RFC
7230 quoted-string rules; ensure you modify the escapeRealm function
accordingly.
1 participant
Summary
Adds mandatory bearer token authentication to protect the /verify and /settle endpoints. The facilitator now requires either a BEARER_TOKEN or BEARER_TOKENS environment variable to start, ensuring secure access to payment processing endpoints.
Changes
This is a breaking change requiring BEARER_TOKEN to be set in production deployments.
Summary by CodeRabbit
New Features
Tests