Skip to content

security: require user identity for feedback API submissions#1453

Open
yossiovadia wants to merge 1 commit intovllm-project:mainfrom
yossiovadia:fix/feedback-api-auth
Open

security: require user identity for feedback API submissions#1453
yossiovadia wants to merge 1 commit intovllm-project:mainfrom
yossiovadia:fix/feedback-api-auth

Conversation

@yossiovadia
Copy link
Copy Markdown
Collaborator

Summary

Fixes #1452 — the feedback API (POST /api/v1/feedback) accepted unauthenticated requests. Anyone reaching port 8080 could submit fake feedback to manipulate Elo ratings, influencing model selection for all users.

Fix

Require user identity for every feedback submission:

  1. Auth header (x-authz-user-id): trusted source from auth backend — takes precedence
  2. Request body (user_id): fallback for development/testing
  3. Neither present: reject with 401 USER_ID_REQUIRED

The resolved userID overrides the body value when the auth header is present, preventing spoofing via the request body when an auth backend is configured.

Changes

File Change
pkg/apiserver/route_feedback.go Add user identity resolution and 401 rejection

1 file changed, 18 insertions, 3 deletions.

Test plan

  • make build-router passes
  • golangci-lint — 0 issues on changed files
  • Anonymous feedback (no user_id, no auth header) returns 401
  • Feedback with user_id in body accepted (dev/testing path)
  • Auth header takes precedence over body user_id

@netlify
Copy link
Copy Markdown

netlify bot commented Mar 6, 2026

Deploy Preview for vllm-semantic-router ready!

Name Link
🔨 Latest commit 4d6dda8
🔍 Latest deploy log https://app.netlify.com/projects/vllm-semantic-router/deploys/69ab3bc2b38d9f0007698ade
😎 Deploy Preview https://deploy-preview-1453--vllm-semantic-router.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

…oject#1452)

The feedback API (POST /api/v1/feedback) accepted unauthenticated
requests. Anyone reaching port 8080 could submit fake feedback to
manipulate Elo ratings, influencing model selection for all users.

Fix: require user identity for every feedback submission.

- Read x-authz-user-id header (trusted, from auth backend) first
- Fall back to user_id in request body (for dev/testing)
- Reject with 401 USER_ID_REQUIRED if neither is present
- Use resolved userID in the Feedback object (overrides body value
  when auth header is present, preventing spoofing)

This ties every feedback entry to an authenticated user, enabling
per-user audit trails and rate limiting.

Fixes vllm-project#1452

Signed-off-by: Yossi Ovadia <yovadia@redhat.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 6, 2026

👥 vLLM Semantic Team Notification

The following members have been identified for the changed files in this PR and have been automatically assigned:

📁 src

Owners: @rootfs, @Xunzhuo, @wangchen615
Files changed:

  • src/semantic-router/pkg/apiserver/route_feedback.go
  • src/semantic-router/pkg/apiserver/route_feedback_test.go

vLLM

🎉 Thanks for your contributions!

This comment was automatically generated based on the OWNER files in the repository.

@rootfs
Copy link
Copy Markdown
Collaborator

rootfs commented Mar 9, 2026

@yossiovadia would that break the privacy?

@yossiovadia
Copy link
Copy Markdown
Collaborator Author

The user_id field already existed in the FeedbackRequest struct and was already stored in the Feedback object when provided — this change just makes it required instead of optional. No new data is being collected.

Without requiring identity, anyone who can reach port 8080 can submit thousands of fake feedback entries and shift Elo ratings for all users. We demonstrated this in the security audit.

Some options if privacy is a concern:

  1. Hash the user_id before storing in Elo ratings
  2. Make it configurable (feedback.require_identity: true/false)
  3. Don't implement the fix, but document the risk as a known limitation

Open to other ideas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

security: feedback API accepts unauthenticated requests — Elo rating manipulation

4 participants