Skip to content

Conversation

@DOsinga
Copy link
Collaborator

@DOsinga DOsinga commented Nov 23, 2025

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements a security fix for unauthenticated WebSocket connections by adding token-based authentication and CORS restrictions. When no auth_token is configured, it generates a random UUID token that must be passed in WebSocket connections and restricts CORS to specific localhost origins.

Key changes:

  • Adds ws_token generation for unauthenticated mode
  • Restricts CORS to localhost origins when unauthenticated
  • Passes token via query parameter in WebSocket connections

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
crates/goose-cli/static/script.js Updated WebSocket connection to include token query parameter from window.GOOSE_WS_TOKEN
crates/goose-cli/src/commands/web.rs Added ws_token generation, CORS restrictions, token injection into HTML, and WebSocket token validation logic

Comment on lines +351 to +357
if state.auth_token.is_none() {
let provided_token = query.token.as_deref().unwrap_or("");
if provided_token != state.ws_token {
tracing::warn!("WebSocket connection rejected: invalid token");
return Err(StatusCode::FORBIDDEN);
}
}
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WebSocket connections will fail when auth_token is configured. The auth_middleware requires an Authorization header (line 99-117), but browsers cannot set custom headers in WebSocket upgrade requests. This means:

  1. When auth_token.is_some(), the auth_middleware will always return 401 for /ws requests
  2. The websocket_handler will never execute
  3. WebSocket connections are broken in authenticated mode

The auth_middleware needs to allow /ws to pass through and let websocket_handler validate the token from the query parameter:

async fn auth_middleware(
    State(state): State<AppState>,
    req: Request,
    next: Next,
) -> Result<Response, StatusCode> {
    // Skip auth for health check and WebSocket
    if req.uri().path() == "/api/health" || req.uri().path() == "/ws" {
        return Ok(next.run(req).await);
    }
    // ... rest of auth logic
}

Then in websocket_handler, validate auth_token when present:

if let Some(ref expected_token) = state.auth_token {
    let provided_token = query.token.as_deref().unwrap_or("");
    if provided_token != expected_token {
        tracing::warn!("WebSocket connection rejected: invalid auth token");
        return Err(StatusCode::FORBIDDEN);
    }
} else if state.auth_token.is_none() {
    // Validate ws_token
    let provided_token = query.token.as_deref().unwrap_or("");
    if provided_token != state.ws_token {
        tracing::warn!("WebSocket connection rejected: invalid token");
        return Err(StatusCode::FORBIDDEN);
    }
}

Copilot uses AI. Check for mistakes.
return Ok(next.run(req).await);
}

// If no auth token is configured, skip authentication entirely
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is actually a helpful and IMO necessary comment, I don't like just blindly deleting oneline coments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interestingly enough, goose deleted this comment. which it probably did because I have general settings that it should delete useless comments.

I would still argue that the comment says the same thing as the code below though.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it never ever wants to with mine - I guess it tends to follow "house style" over what system prompt says! @DOsinga sometimes I wonder if we need to put some logic in the editor tool for it to to return an error if it detects single line comment (error can be "this is an inane comment, please either remove and try again or consider if it is really needed) or something?

Copy link
Collaborator

@michaelneale michaelneale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems reasonable (although gets rid of useful comments.

@michaelneale michaelneale merged commit e566f1e into main Nov 23, 2025
27 of 28 checks passed
@michaelneale michaelneale deleted the cors-and-token branch November 23, 2025 23:58
michaelneale added a commit that referenced this pull request Nov 24, 2025
* main: (48 commits)
  [fix] generic check for gemini compat (#5842)
  Add scheduler to diagnostics (#5849)
  Cors and token (#5850)
  fix sessions coming back with empty messages (#5841)
  markdown export from URL (#5830)
  Next camp refactor live (#5706)
  Add out of context compaction test via error proxy (#5805)
  fix: Add backward compatibility for conversationCompacted message type (#5819)
  Add /agent/stop endpoint, make max active agents configurable (#5826)
  Handle 404s (#5791)
  Persist provider name and model config in the session (#5419)
  Comment out the flaky mcp callers (#5827)
  Slash commands (#5718)
  fix: remove setx calls to not permanently edit the windows shell PATH (#5821)
  fix: Parse maas models for gcp vertex provider (#5816)
  fix: support Gemini 3's thought signatures (#5806)
  chore: Add Adrian Cole to Maintainers (#5815)
  [MCP-UI] Proxy and Better Message Handling (#5487)
  Release 1.15.0
  Document New Window menu in macOS dock (#5811)
  ...
michaelneale added a commit that referenced this pull request Nov 24, 2025
* main:
  feat: implement SKILLS.md - claude compatibility.  (#5760)
  urgent: github broke runners for macos (#5853)
  [fix] generic check for gemini compat (#5842)
  Add scheduler to diagnostics (#5849)
  Cors and token (#5850)
  fix sessions coming back with empty messages (#5841)
kskarthik pushed a commit to kskarthik/goose that referenced this pull request Nov 25, 2025
Co-authored-by: Douwe Osinga <[email protected]>
kskarthik pushed a commit to kskarthik/goose that referenced this pull request Nov 26, 2025
Co-authored-by: Douwe Osinga <[email protected]>
Signed-off-by: Sai Karthik <[email protected]>
BlairAllan pushed a commit to BlairAllan/goose that referenced this pull request Nov 29, 2025
Co-authored-by: Douwe Osinga <[email protected]>
Signed-off-by: Blair Allan <[email protected]>
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.

3 participants