feat(portal): support token in x-authorization header #11596
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
jamilbk
force-pushed
the
feat/authz-header
branch
from
4496171 to
4e9eb30
Compare
There was a problem hiding this comment.
Pull request overview
This pull request adds support for passing authentication tokens via the x-authorization HTTP header (in addition to the existing token query parameter) for WebSocket connections. This change improves security by preventing tokens from being logged in query parameters, which is a common issue with many logging and monitoring tools. The X- prefix is used because Phoenix only exposes x-* and sec-websocket-* headers in WebSocket connection callbacks.
- Introduces a new
extract_token/2function inPortalAPI.Socketsthat checks for tokens in headers first (with "Bearer" prefix), then falls back to query parameters - Refactors client, gateway, and relay socket implementations to use the new token extraction logic
- Adds comprehensive test coverage for token extraction from headers, including precedence tests
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
elixir/lib/portal_api/sockets.ex |
Adds extract_token/2 function to extract tokens from x-authorization header or token parameter |
elixir/lib/portal_api/client/socket.ex |
Refactors connect/3 to use extract_token/2 and extracts authentication logic into do_connect/4 |
elixir/lib/portal_api/gateway/socket.ex |
Refactors connect/3 to use extract_token/2 and extracts authentication logic into do_connect/4 |
elixir/lib/portal_api/relay/socket.ex |
Refactors connect/3 to use extract_token/2 and extracts authentication logic into do_connect/4 |
elixir/test/portal_api/sockets_test.exs |
Adds comprehensive unit tests for the extract_token/2 function |
elixir/test/portal_api/client/socket_test.exs |
Adds integration tests for header-based token authentication and precedence |
elixir/test/portal_api/gateway/socket_test.exs |
Adds integration tests for header-based token authentication and precedence |
elixir/test/portal_api/relay/socket_test.exs |
Adds integration tests for header-based token authentication and precedence |
|
|
||
| defp extract_token_from_header(%{x_headers: x_headers}) when is_list(x_headers) do | ||
| case List.keyfind(x_headers, "x-authorization", 0) do | ||
| {"x-authorization", "Bearer " <> token} -> {:ok, token} |
There was a problem hiding this comment.
The pattern matching "Bearer " <> token will accept an empty string as a valid token when the header value is exactly "Bearer " (with trailing space but no token). This could lead to accepting invalid authentication attempts. Consider adding a guard clause to ensure the token is not empty: when byte_size(token) > 0 or explicitly check for non-empty strings.
| {"x-authorization", "Bearer " <> token} -> {:ok, token} | |
| {"x-authorization", "Bearer " <> token} when byte_size(token) > 0 -> {:ok, token} |
| test "falls back to params when header value is empty" do | ||
| params = %{"token" => "fallback-token"} | ||
| connect_info = %{x_headers: [{"x-authorization", ""}]} | ||
|
|
||
| assert Sockets.extract_token(params, connect_info) == {:ok, "fallback-token"} | ||
| end |
There was a problem hiding this comment.
Missing test case for an edge case where the x-authorization header contains only "Bearer " (with trailing space but no token). The current implementation would accept this as a valid token with an empty string value, which should likely be rejected. Add a test to verify the expected behavior in this scenario.
|
|
||
| defp extract_token_from_header(_connect_info), do: :error | ||
|
|
||
| defp extract_token_from_params(%{"token" => token}) when is_binary(token), do: {:ok, token} |
There was a problem hiding this comment.
The function accepts an empty string as a valid token when the token parameter is present but empty. While the guard clause when is_binary(token) ensures it's a string, it doesn't validate that the token is non-empty. Consider adding and byte_size(token) > 0 to the guard clause to reject empty tokens, consistent with proper token validation.
| defp extract_token_from_params(%{"token" => token}) when is_binary(token), do: {:ok, token} | |
| defp extract_token_from_params(%{"token" => token}) when is_binary(token) and byte_size(token) > 0, | |
| do: {:ok, token} |
Accepts the token in either the
x-authorizationheader or the existingtokenparam, prioritizing the former. TheX-header is used because Phoenix doesn't expose arbitrary HTTP headers in the WebSocket connect callback.Related: #9581
Related: #11595