Skip to content

Fix Unauthenticated Access to Bot Proxy Endpoints (/bot/v1/chat, /bot/v1/chat/stream)#996

Merged
yeshion23333 merged 3 commits intovolcengine:mainfrom
13ernkastel:main
Mar 27, 2026
Merged

Fix Unauthenticated Access to Bot Proxy Endpoints (/bot/v1/chat, /bot/v1/chat/stream)#996
yeshion23333 merged 3 commits intovolcengine:mainfrom
13ernkastel:main

Conversation

@13ernkastel
Copy link
Copy Markdown
Contributor

@13ernkastel 13ernkastel commented Mar 26, 2026

This PR fixes a Broken Access Control issue on the bot proxy endpoints by requiring authentication before proxying requests upstream.

Previously, POST /bot/v1/chat and POST /bot/v1/chat/stream could accept unauthenticated requests because token extraction was optional and non-blocking. This change enforces authentication with 401 Unauthorized when credentials are missing.

Changes

  • Renamed verify_auth() to extract_auth_token() and made it synchronous.
  • Added require_auth_token(request) to enforce auth before proxying.
  • Updated /bot/v1/chat and /bot/v1/chat/stream to require auth.
  • Added tests for both supported auth headers:
    • X-API-Key
    • Authorization: Bearer ...
  • Added regression tests for unauthenticated requests to both endpoints.
  • Restored BOT_API_URL after each test to avoid leaking module-global state.

Testing

  • Added helper tests for auth token extraction and auth enforcement.
  • Added endpoint regression tests verifying unauthenticated requests return 401.

@github-actions
Copy link
Copy Markdown

Failed to generate code suggestions for PR

@yeshion23333
Copy link
Copy Markdown
Collaborator

Thanks for the security fix — enforcing auth on the bot proxy endpoints makes sense. One suggestion: could we merge the “required token” check into verify_auth() instead of introducing require_auth_token()?

Right now the call sites read as require_auth_token(await verify_auth(request)), which is a bit harder to follow, especially since verify_auth() is only used within this module. If we change verify_auth() to return str and raise HTTPException(status_code=401, ...) when the token is missing, the handlers become simpler and the intent is clearer.

Alternatively, if you want to keep the separation, renaming verify_auth() to something like extract_auth_token() would better reflect that it doesn’t enforce auth.

@13ernkastel
Copy link
Copy Markdown
Contributor Author

Thanks @yeshion23333, that makes sense. I pushed a follow-up that keeps extraction and enforcement separate, but cleans up the API by renaming verify_auth() to extract_auth_token(), making it sync, and having require_auth_token() take the Request directly. That makes the handlers read a lot more naturally.

I also added tests for both supported auth header formats and restored BOT_API_URL in the fixture so the tests don’t leak state.

@yeshion23333 yeshion23333 merged commit 27acda8 into volcengine:main Mar 27, 2026
7 checks passed
@github-project-automation github-project-automation bot moved this from Backlog to Done in OpenViking project Mar 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants