Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Authorization header #560

Merged
merged 3 commits into from
Jan 6, 2025
Merged

Authorization header #560

merged 3 commits into from
Jan 6, 2025

Conversation

amirmalka
Copy link
Contributor

@amirmalka amirmalka commented Jan 6, 2025

PR Type

Bug fix


Description

  • Fixed the formatting of the Authorization header string.

  • Ensured proper connection setup in ws_export_open method.


Changes walkthrough 📝

Relevant files
Bug fix
backend_api.py
Fix `Authorization` header formatting in WebSocket connection

infrastructure/backend_api.py

  • Corrected the Authorization header string by removing an unintended
    comma.
  • Ensured the WebSocket connection uses properly formatted headers.
  • +1/-1     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Code Style

    The cookie variable is being reassigned inside the for loop, which means only the last cookie will be used in the WebSocket connection. Consider collecting all cookies into a list.

    for cookie in self.selected_tenant_cookie:
        cookie = "Cookie: {}={}".format(cookie.name, cookie.value)

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Fix cookie handling in WebSocket connection to include all cookies instead of just the last one

    The cookie variable is being reassigned inside the loop, causing only the last
    cookie to be used in the WebSocket connection. Store all cookies in a list to
    include them all in the connection headers.

    infrastructure/backend_api.py [2059-2060]

    +cookies = []
     for cookie in self.selected_tenant_cookie:
    -    cookie = "Cookie: {}={}".format(cookie.name, cookie.value)
    +    cookies.append("Cookie: {}={}".format(cookie.name, cookie.value))
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The current implementation overwrites the cookie variable in each loop iteration, causing only the last cookie to be used. This is a critical bug that could break authentication when multiple cookies are needed.

    9
    Ensure all authentication headers are properly included in the WebSocket connection

    The WebSocket connection uses an undefined 'cookie' variable (which is overwritten
    in the loop) and should use the list of cookies instead.

    infrastructure/backend_api.py [2064]

    -ws.connect(server, header=[cookie, authorization])
    +ws.connect(server, header=cookies + [authorization])
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion correctly addresses the issue of using a single overwritten cookie variable in the header list, proposing to use the full list of cookies. This fix is essential for proper WebSocket authentication.

    9

    @amirmalka amirmalka merged commit a705a2c into master Jan 6, 2025
    2 checks passed
    Copy link

    github-actions bot commented Jan 6, 2025

    Failed to generate code suggestions for PR

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

    Successfully merging this pull request may close these issues.

    2 participants