Skip to content

fix: The cookie path for the portal is incorrect. #7847

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.

Sign up for GitHub

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

Merged
shaohuzhang1 merged 1 commit into from
Nov 12, 2026
Merged

fix: The cookie path for the portal is incorrect. #7847

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/chat/views/v3/chat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,7 +495,7 @@ def get(self, request: Request):

def get_chat_user_cookie_path(access_token):
"""对话用户登录时写入文件凭证 cookie 的路径；退出登录时按同一路径删除。"""
return f"/chat/{access_token + '/' if access_token else ''}"
return f"/chat/{access_token or 'portal'}"


def clear_file_auth_cookie(response: HttpResponse, access_token):

@shaohuzhang1 shaohuzhang1 Nov 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The diff changes behavior, not just formatting — both the old and new versions look problematic.

Issues with the change

1. Cookie path no longer matches across login/logout in the same way

The docstring says the path is used both when writing the cookie at login and when deleting it at logout. The two code paths must produce the identical string.

  • Old: f"/chat/{access_token + '/' if access_token else ''}" → /chat/abc/ or /chat/
  • New: f"/chat/{access_token or 'portal'}" → /chat/abc or /chat/portal

The empty-token case now yields /chat/portal instead of /chat/. If any other code (or an older deployed version) still uses /chat/, the cookie won't be cleared at logout, leaving a stale auth cookie. Verify every call site was updated together.

2. Inconsistent trailing slash between branches (old code)

In the original, non-empty tokens get a trailing / (/chat/abc/) but the empty case gives /chat/ with no extra segment. This asymmetry is confusing; make sure login and logout both use this same function so they can't drift apart. (Your change at least keeps them symmetric.)

3. 'portal' magic string

Hardcoding "portal" as a fallback path segment:

  • Couples a generic helper to what looks like a specific product/module name.
  • Creates a path collision risk: a real access token equal to "portal" would resolve to the same cookie path as the anonymous case, causing users to stomp on each other's cookies / log each other out.
  • Makes the fallback opaque to readers. Prefer a named constant (e.g. ANONYMOUS_CHAT_PATH = "portal") and/or document why portal is correct.

4. Path scoping / security considerations

  • If access_token is ever user-controlled, embedding it raw in a cookie Path means it must be URL-safe and free of /, ;, ,, whitespace, etc. A token containing / silently widens or narrows the cookie scope. Consider validating/sanitizing, or better, don't put secrets in a Path at all (paths are visible in requests and shared across subresource loads).
  • Putting the actual token in the cookie path leaks it in every HTTP request line for that scope. If the token here is the secret itself, prefer storing a separate opaque identifier in the cookie and keep the token server-side.
  • Two different tokens scoping cookies by path means a user browsing multiple chats could accumulate many auth cookies; consider whether a single origin-wide cookie plus server-side validation is simpler and safer.

5. Function naming

get_chat_user_cookie_path suggests pure computation; ensure it doesn't start doing I/O. It's fine now, but the docstring-heavy reliance on callers calling it twice (login + logout) is fragile — see #6.

Optimization / refactor suggestions

  • Single source of truth: compute the path once and pass it to both set and clear, or have both use one helper — already the intent, just make sure no other hardcoded /chat/... paths exist (grep for "/chat/").
  • Avoid f-string indirection: 
    def get_chat_user_cookie_path(access_token: str | None) -> str:
    return f"/chat/{access_token or ANONYMOUS_CHAT_PATH}"
    is fine; just introduce ANONYMOUS_CHAT_PATH.
  • Add unit tests covering: non-empty token, empty string, None, and that login-path == logout-path for each.
  • Document contract: note explicitly that the returned value must be identical across login/logout and must be a valid cookie Path (no unescaped special chars).

Verdict

No syntax bug, but the semantic change (fallback "" → "portal", removal of trailing slash) can break cookie cleanup against previously issued cookies and introduces a token-collision edge case. Confirm intent, align all call sites and deployed cookies, and replace the magic string with a constant.

Expand Down
Loading