[shaohuzhang1]

fix: [AI Agent] The same workflow functions correctly during backend debugging; however, when accessed via a web link, images fail to load during conversation, and certain nodes are missing from the execution details.

[shaohuzhang1]

I can only review the shown diff/surrounding lines, so I’m assuming the missing imports/settings are already correct.

Potential bugs / correctness issues

1. Cookie deletion depends on an exact accessToken match

   • Login sets the cookie at:

     /chat/<access_token>/

   • Logout and change-password clear it using:

     request.query_params.get("accessToken")

   • If the query parameter is missing, empty, or different from the token used at login, delete_cookie() will use the wrong path and the HttpOnly cookie will not be deleted.
   • This is the biggest issue. Consider deriving the token from the authenticated request instead of a query parameter, e.g. from Authorization: Bearer <token>, or store the cookie path server-side.

2. Access token in query string is risky

   • accessToken in the query string can leak via access logs, browser history, Referer headers, proxies, etc.
   • Prefer the Authorization header or server-side session/state. If backward compatibility requires the query param, validate it strictly and avoid logging it.

3. Cache deletion inconsistency in ChangePassword

   • In change-password:

     auth = request.META.get("HTTP_AUTHORIZATION")
     cache.delete(get_key(token=auth), version=version)

   • In logout:

     cache.delete(get_key(token=auth[7:]), version=version)

   • If get_key() expects the raw token, change-password is deleting with "Bearer <token>" instead of <token>, so the cache entry may not be removed.
   • Also, auth[7:] will raise TypeError if HTTP_AUTHORIZATION is missing. Validate authentication before slicing.

4. Cookie deletion must match the original cookie’s domain and path

   • Browser cookie deletion requires matching name, domain, and path.
   • If generate() sets a domain, clear_file_auth_cookie() must pass the same domain.
   • secure, httponly, and samesite are not used for cookie matching, but keeping them consistent avoids confusion.

5. Multiple path-scoped cookies may accumulate

   • Because the path contains the access token, each login with a different token can create a separate mk_file_auth cookie:

     /chat/token-a/
     /chat/token-b/
     

   • Logout only clears the current path. Old cookies remain until expiry.
   • If possible, use a fixed path such as /chat/ and rely on server-side validation, or explicitly clean up old paths.

6. Raw token is embedded in the cookie path

   • Cookie paths are not a strong security boundary, and the token may contain characters that are awkward in a Set-Cookie path.
   • Prefer a fixed cookie path, or at minimum validate/encode the token and keep it reasonably bounded.

Compatibility / implementation notes

7. delete_cookie(..., samesite="Lax") depends on Django version

   • Modern Django supports samesite in delete_cookie(), but older versions may not.
   • If the project supports older Django, verify this. Otherwise it is harmless, though not required for deletion.

8. Ensure HttpResponse is imported

   • The new function uses HttpResponse in the annotation. If it is not imported and annotations are evaluated at runtime, this will raise NameError.

9. clear_file_auth_cookie() assumes result.success() returns an object with delete_cookie()

   • DRF Response generally inherits from Django’s response classes and should support delete_cookie(), but if result.success() returns a custom wrapper, this will fail.
   • Consider typing it as the actual response type used by your project.

Suggested cleanup / optimization

10. Centralize cookie set/clear logic

    • Use one helper for setting and one for clearing so path, domain, secure, httponly, and samesite cannot drift:

    def set_file_auth_cookie(response, token, path, secure):
        response.set_cookie(
            FILE_AUTH_COOKIE,
            value=token,
            max_age=7 * 24 * 3600,
            path=path,
            secure=secure,
            httponly=True,
            samesite="Lax",
        )
        return response
    
    def clear_file_auth_cookie(response, path):
        response.delete_cookie(FILE_AUTH_COOKIE, path=path, samesite="Lax")
        return response

    • Then compute the path once and pass it to both.

11. Prefer deriving the token from the authenticated request

    • Example, if the cache token is the Bearer token:

    auth = request.META.get("HTTP_AUTHORIZATION", "")
    if not auth.startswith("Bearer "):
        return result.error(...)
    access_token = auth[7:]

    • Then use access_token for both cache invalidation and cookie-path clearing. This avoids relying on a query parameter and reduces leakage.

Summary

The main functional risk is that the cookie is only cleared if accessToken is supplied correctly as a query parameter. Missing or mismatched values leave the HttpOnly cookie in place. The main security concern is putting the access token in the query string. Also fix the "Bearer " handling inconsistency in change-password and add guards for missing Authorization headers.

[shaohuzhang1]

Code Review

1. Potential issues (bugs)

• Changed source_id/source_type semantics silently: The file is now attributed to the chat rather than the application. If any existing logic queries files by source_id = application_id with FileSourceType.APPLICATION (e.g., cleanup, permissions, display), it will break or lose access to previously-uploaded files. Verify all consumers before merging.
• Lost debug flag: debug was removed from meta. If downstream code reads meta["debug"] (e.g., to skip persistence or apply debug behavior), this is a regression. Confirm it's now handled elsewhere; if not, keep it.
• Missing keys are not validated: If chat_id, application_id, or chat_user_id is absent from workflow_params, None gets written into metadata/source_id silently. Add validation:

  if not chat_id or not application_id:
      raise ValueError("application_id and chat_id are required")

• Naming inconsistency: the param is chat_user_id but stored as user_id. Fine, but document it, since key mismatch is a common bug source.

2. Minor style/robustness

• Consider .get() defaults for consistency: workflow_params.get("chat_user_id", "") if empty strings are preferable to None in storage.
• The meta dict fits on one line longer than PEP8 allows; current formatting is fine, just be consistent with _upload_tool_file (which presumably should get the same user_id treatment — check for symmetry).

3. Optimization suggestions

• Minimal, but you could avoid rebuilding the dict inline:

  meta = {
      "chat_id": chat_id,
      "application_id": application_id,
      "user_id": chat_user_id,
  }

  for readability/greppability.
• Consider adding logging when required params are missing instead of only failing at serialization time.

Summary: No syntax problems, but the semantic change of source_id/source_type and removal of debug are behavior changes that must be verified against all consumers of this metadata.

[shaohuzhang1]

Issues & Suggestions

1. Missing validation — application_id, chat_id, and chat_user_id are all used without checking for None. A missing chat_id (now used as source_id) could create Files with null/incorrect source linkage. Consider raising an error if they're absent:

   if not chat_id or not application_id:
       raise ValueError("chat_id and application_id are required")

2. Semantic change of source_id — source_id changed from application_id to chat_id. If any existing logic (queries, permissions, cleanup) keys off source_id = application_id, this is a breaking change for previously uploaded files. Migration/backfill may be needed.

3. Key mismatch: user_id vs chat_user_id — meta stores chat_user_id under "user_id". That's fine, but confirm downstream consumers expect user_id, not chat_user_id.

4. Removed debug handling — anything relying on meta["debug"] will now get None/KeyError. Verify no caller depends on it.

5. Hard requirement on chat_user_id — if old callers don't pass it, user_id will silently be None. Add validation or a default.

6. Minor/style — dict literal fits fine; consider f-string consistency elsewhere. No performance concerns here; changes are trivial cost-wise.

Overall: logic looks intentional (shifting file ownership from application-level to chat-level + user attribution), but items 1–4 deserve verification before merging.