fix(security): fail closed on missing production registry token - #40
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
reprewindai-dev
commented
Aug 10, 2026
@coderabbitai review Please review exact head |
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe registry registration route now fails with HTTP 503 when authentication configuration is missing outside local, development, and test environments. Configured tokens require Bearer authentication. Tests cover environment handling, authentication, and registry mutation. ChangesRegistry authentication guard
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/app/api/v1/registry/register/route.test.tsOops! Something went wrong! :( ESLint: 10.8.1 TypeError [ERR_IMPORT_ATTRIBUTE_MISSING]: Module "file:///.eslintrc.json?mtime=1786454203074" needs an import attribute of "type: json" src/app/api/v1/registry/register/route.tsOops! Something went wrong! :( ESLint: 10.8.1 TypeError [ERR_IMPORT_ATTRIBUTE_MISSING]: Module "file:///.eslintrc.json?mtime=1786454203074" needs an import attribute of "type: json" 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 |
🧩 Analysis chain🏁 Script executed: #!/bin/bashset -euo pipefail
target='d90dd9c57629a93364519b5f8ee5c8a58dd8006e'echo'== Repository revision =='
git rev-parse HEAD
git cat-file -t "$target"
git status --short
echo'== Target commit metadata and changed files =='
git show --no-ext-diff --format=fuller --stat "$target"
git diff-tree --no-commit-id --name-status -r "$target"echo'== Target commit patch =='
git show --no-ext-diff --format= --find-renames "$target"echo'== Candidate route/auth/registry/test files =='
fd -t f .| rg -i '(registry|register|route|router|auth|token|test|spec)'| head -n 250
echo'== Source references with context =='
rg -n -i -C 5 \
'(POST|/api/v1/registry/register|CAPI_REGISTRY_TOKEN|NODE_ENV|register(?:Service)?\s*\(|registry\.(?:set|register|add)|console\.(?:log|error|warn)|logger\.)' \
-g '!node_modules' -g '!dist' -g '!build'.echo'== Test references with context =='
rg -n -i -C 8 \
'(CAPI_REGISTRY_TOKEN|NODE_ENV|/api/v1/registry/register|registry/register|registered service|authorization|bearer)' \
-g '*test*' -g '*spec*' -g '!node_modules' -g '!dist' -g '!build'.||trueLength of output: 197680 🏁 Script executed: #!/bin/bashset -euo pipefail
target='d90dd9c57629a93364519b5f8ee5c8a58dd8006e'
parent="${target}^"echo'== Exact revision verification =='printf'HEAD: '; git rev-parse HEAD
printf'TARGET: '; git rev-parse "$target"printf'PARENT: '; git rev-parse "$parent"
git merge-base --is-ancestor "$target" HEAD &&echo'TARGET_IS_ANCESTOR_OF_HEAD=true'||echo'TARGET_IS_ANCESTOR_OF_HEAD=false'echo'== Exact target diff: registry route and test =='
git diff --no-ext-diff --unified=80 "$parent""$target" -- \
src/app/api/v1/registry/register/route.ts \
src/app/api/v1/registry/register/route.test.ts
echo'== Route at target =='
git show "${target}:src/app/api/v1/registry/register/route.ts"| nl -ba
echo'== Route test at target =='
git show "${target}:src/app/api/v1/registry/register/route.test.ts"| nl -ba
echo'== Engine registration call chain at target =='
git show "${target}:src/lib/covenant/engine.ts"| sed -n '110,155p'| nl -ba -v110
git show "${target}:src/lib/covenant/service-registry.ts"| sed -n '195,275p'| nl -ba -v195
echo'== Token identifiers and response/log statements in changed route =='
git show "${target}:src/app/api/v1/registry/register/route.ts"|
rg -n -i -C 3 '(CAPI_REGISTRY_TOKEN|authorization|bearer|expected|presented|NextResponse|console\.|logger\.|error)'echo'== All target-commit changed paths =='
git diff-tree --no-commit-id --name-status -r "$target"Length of output: 6842 🏁 Script executed: #!/bin/bashset -euo pipefail
echo'== Registry route at HEAD =='
cat -n src/app/api/v1/registry/register/route.ts
echo'== Registry route tests at HEAD =='
cat -n src/app/api/v1/registry/register/route.test.ts
echo'== Registration mutation call chain at HEAD =='
sed -n '115,145p' src/lib/covenant/engine.ts | cat -n
sed -n '195,270p' src/lib/covenant/service-registry.ts | cat -n
echo'== Token, response, and logging paths in the registry route =='
rg -n -i -C 4 \
'(CAPI_REGISTRY_TOKEN|authorization|bearer|expected|presented|NextResponse|console\.|logger\.|error)' \
src/app/api/v1/registry/register/route.ts
echo'== Pull request base and complete changed-file list =='
gh pr view 40 --repo reprewindai-dev/cAPI --json baseRefOid,headRefOid,commits,files \
--jq '{baseRefOid,headRefOid,commits:[.commits[].oid],files:[.files[].path]}'
base="$(gh pr view 40 --repo reprewindai-dev/cAPI --json baseRefOid --jq .baseRefOid)"echo"BASE=$base"echo'== Pull request diff for registry route and tests =='
git diff --no-ext-diff --unified=100 "$base"...HEAD -- \
src/app/api/v1/registry/register/route.ts \
src/app/api/v1/registry/register/route.test.tsLength of output: 28549
Static source review confirms:
I did not execute repository tests. The sandbox instructions prohibit executing repository files. ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/app/api/v1/registry/register/route.test.ts (2)
68-79: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCover whitespace-only token configuration.
The route treats
" "as missing configuration, but this test covers only an absent variable. Add a production case with a whitespace-onlyCAPI_REGISTRY_TOKENand assert the same503response and absence of registry state.🤖 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 `@src/app/api/v1/registry/register/route.test.ts` around lines 68 - 79, Add a production test case alongside the existing missing-token test that sets CAPI_REGISTRY_TOKEN to whitespace only, then verifies register returns 503 with the same authentication error and that the submitted service is absent from services().
45-45: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMake the environment policy explicit in the tests.
The successful unauthenticated test relies on ambient
NODE_ENV, and the negative test covers only"production". Set the success case to an explicit test/development environment and add an unknown environment case, such as"staging", that expects503and no registry mutation.Also applies to: 68-79
🤖 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 `@src/app/api/v1/registry/register/route.test.ts` at line 45, Make the registry route tests explicitly control NODE_ENV instead of relying on ambient state: set the successful unauthenticated case to a test/development environment, and extend the negative cases with an unknown environment such as staging that expects status 503 and verifies no registry mutation. Update the relevant test setup and assertions around the existing authenticated/unauthenticated cases while preserving the production behavior.
🤖 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/app/api/v1/registry/register/route.ts`:
- Around line 49-53: Update the authorization parsing around the presented token
so authentication proceeds only when the trimmed header uses the Bearer scheme;
reject or treat non-Bearer headers as invalid instead of copying them into
presented. Preserve the existing token trimming and comparison behavior for
valid Bearer headers.
- Around line 11-12: Update the environment check in the registration handler
around the production authentication guard to allow unauthenticated registration
only for the repository’s explicit local, development, and test environment
values. Treat unset, staging, and every other unknown NODE_ENV value as
authenticated-required, while preserving the existing rejection behavior when
authentication is not configured.
---
Nitpick comments:
In `@src/app/api/v1/registry/register/route.test.ts`:
- Around line 68-79: Add a production test case alongside the existing
missing-token test that sets CAPI_REGISTRY_TOKEN to whitespace only, then
verifies register returns 503 with the same authentication error and that the
submitted service is absent from services().
- Line 45: Make the registry route tests explicitly control NODE_ENV instead of
relying on ambient state: set the successful unauthenticated case to a
test/development environment, and extend the negative cases with an unknown
environment such as staging that expects status 503 and verifies no registry
mutation. Update the relevant test setup and assertions around the existing
authenticated/unauthenticated cases while preserving the production behavior.
🪄 Autofix
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: 00517795-5318-470d-bbb0-16026b63998e
📒 Files selected for processing (2)
src/app/api/v1/registry/register/route.test.tssrc/app/api/v1/registry/register/route.ts
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
reprewindai-dev
left a comment
There was a problem hiding this comment.
Blocking security follow-up on the current head before this can leave draft or merge. The intended fail-closed production registry posture is not yet fail-closed for deployment ambiguity: unauthenticated registration is allowed for unset/unknown NODE_ENV values (including staging/typos) because only the literal production value is denied. Make unauthenticated registration an explicit allowlist for known local/development/test values and require authentication for unset or every other environment. Also require the Bearer scheme; Authorization: <raw token> must not authenticate. Add negative tests for unset NODE_ENV, staging, an unknown value, and a correct raw token without Bearer. Keep the existing 503 behavior when auth is required but CAPI_REGISTRY_TOKEN is absent. Exact-head cAPI CI is currently failed, so do not promote or merge until these fixes are pushed and executable checks pass.
Uh oh!
There was an error while loading. Please reload this page.
Closes#39.
POST /api/v1/registry/registerpreviously allowed unauthenticated registration wheneverCAPI_REGISTRY_TOKENwas absent outside the literalNODE_ENV=productioncase. That left unset, staging, and unknown environments fail-open, and configured tokens could also be presented without theBearerscheme.This PR now:
local,development, andtestenvironment allowlist;Authorization: Bearer <token>whenCAPI_REGISTRY_TOKENis configured; raw-token Authorization headers are rejected;Current exact head:
623baa8d0d338060f8c4aa039d426be2fa358284.Review findings for environment allowlisting and strict Bearer parsing are fixed on this head and their threads are resolved.
Verification boundary
This is a source/authentication correction only.
reported_runtime_stateremains cAPI3003;verified_runtime_stateremains empty until deployed SHA + HTTP/protocol identity + container listener + Traefik routing agree.Exact-head merge gate
Do not merge solely from source inspection. GitHub Actions run
31464619305has now failed on two attempts before executable steps were exposed. The latestverifyjob is93694991424, completedfailurewithsteps: null, and its job-log download returns404 BlobNotFound. The checked-in.github/workflows/ci.ymlis a conventional Ubuntu/Node workflow (checkout,setup-node,npm ci,npm test,npm run build), so there is no source evidence that the application tests themselves ran or failed. Treat this as an unresolved Actions/pre-step execution gate, not a test result.Required before merge: an exact-head workflow must actually execute and expose successful install/test/build steps, with no blocking review. Runtime verification remains separate and still requires deployed SHA + listener
3003+ protocol identity + Traefik agreement.Summary by CodeRabbit
New Features
Bug Fixes