Repository navigation
fix(shared-task): enable full attachment access with share tokens - #413
Conversation
…asks - Add share_token query parameter to attachment download endpoint - Support dual authentication: JWT for logged-in users, share_token for public access - Add get_current_user_optional helper for optional JWT authentication - Update frontend attachment APIs to accept optional shareToken parameter - Pass shareToken through MessageBubble -> ContextBadgeList -> AttachmentPreview - Validate share_token against task ownership before allowing attachment access
This commit enables attachments in shared tasks to be accessed without authentication using share tokens. Both user-uploaded attachments and AI-generated attachments now work correctly on shared task pages. Backend changes: - Add share_token query parameter support to get_attachment endpoint - Add share_token support to get_attachment_preview endpoint (already had it for download) - Update _validate_share_token_access to handle attachments with subtask_id=0 by verifying attachment owner matches task owner - Add detailed logging for debugging share token validation - Change 404 to 403 for denied share token access for better error clarity Frontend changes: - Create ShareTokenContext for passing share token through component tree - Update MessageBubble to wrap content with ShareTokenProvider - Update AttachmentCard to use useShareToken and pass to API calls - Update AttachmentEmbed to use useShareToken and pass to API calls - Update getAttachment and getAttachmentPreview APIs to accept optional shareToken - Update useAttachmentPreview hook to accept and pass shareToken parameter - Maintain simple caching strategy using attachmentId as key (no token in cache key) This fixes the issue where attachments in AI-generated messages were returning 404/401 errors when viewed through shared task links. Co-Authored-By: Claude <noreply@anthropic.com>
The TokenCounter implementation was updated in commit 522a58a to use LiteLLM for unified token counting across all providers, removing the explicit provider detection logic and `provider` attribute. This commit updates the test suite to reflect the implementation changes: - Remove test_detect_provider_openai (obsolete) - Remove test_detect_provider_anthropic (obsolete) - Remove test_detect_provider_google (obsolete) - Fix test_count_message_multimodal assertion (100 -> 50 tokens) The multimodal test was adjusted because LiteLLM accurately counts tokens for the tiny test base64 image, resulting in ~98 tokens instead of the previously estimated 100+ tokens. All 38 compression tests now pass.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds share-token based public access to attachment endpoints and an optional JWT auth path; frontend receives and propagates a shareToken via a new ShareTokenContext and updated attachment APIs/hooks/components to support token-based access and conditional Authorization omission. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser (UI)
participant Frontend as Frontend (React)
participant Backend as API Server
participant DB as Database / Services
Browser->>Frontend: Load page with ?share_token=SHARE
Frontend->>Frontend: ShareTokenProvider saves token in context
Frontend->>Backend: GET /attachments/:id?share_token=SHARE
Backend->>DB: validate share token -> find SharedTask / Attachment / Subtask
DB-->>Backend: validation result (allowed / denied)
alt allowed
Backend-->>Frontend: 200 + attachment data / stream
Frontend->>Browser: render attachment (no Auth header used)
else denied
Backend-->>Frontend: 403 / 404
Frontend->>Browser: show access denied
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Previously, the shared task page only displayed the text output but not the tool blocks (Bash, Read, Edit, Write, etc.) because the Message object was missing the result field containing blocks data. This commit passes the full result object from PublicSubtaskData to MessageBubble component, enabling MixedContentView to render tool blocks for shared tasks. Co-Authored-By: Claude <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/features/tasks/components/message/MessageBubble.tsx (1)
1726-1752:⚠️ Potential issue | 🟠 MajorInclude shareToken in memo equality check.
MessageBubblecan skip re-render whenshareTokenchanges, leavingShareTokenProviderwith stale token. AddshareTokento the comparison.✅ Proposed fix
- prevProps.isRegenerating === nextProps.isRegenerating + prevProps.isRegenerating === nextProps.isRegenerating && + prevProps.shareToken === nextProps.shareTokenfrontend/src/components/common/AttachmentEmbed.tsx (1)
101-103:⚠️ Potential issue | 🟠 MajorPass
shareTokentoHtmlPreview.
HtmlPreviewis rendered withoutshareTokenat line 102, butshareTokenis available inAttachmentEmbedviauseShareToken(). SinceHtmlPreviewdownloads the attachment and currently uses only the user token (notshareToken), shared-task access will fail. UpdateHtmlPreviewto accept and useshareToken.🔧 Suggested fix
- ) : isHtml ? ( - <HtmlPreview attachmentId={attachmentId} filename={data.filename} /> + ) : isHtml ? ( + <HtmlPreview attachmentId={attachmentId} filename={data.filename} shareToken={shareToken} /> ) : previewText ? (
🤖 Fix all issues with AI agents
In `@backend/app/api/endpoints/adapter/attachments.py`:
- Around line 419-439: In download_attachment, when a share_token is provided
and _validate_share_token_access(db, attachment_id, share_token) returns False
you must raise HTTPException(status_code=403, detail="Invalid share token")
instead of letting it fall through to the final 404; update the branch where
share_token is checked (the block that calls _validate_share_token_access and
context_service.get_context_optional) to explicitly return a 403 on invalid
token before continuing, leaving the existing _get_attachment_context and other
authentication branches unchanged.
🧹 Nitpick comments (6)
frontend/src/contexts/ShareTokenContext.tsx (1)
16-24: Consider memoizing the context value to prevent unnecessary re-renders.The context value
{ shareToken }creates a new object on each render, which could cause unnecessary re-renders in consumers. For a simple string value this is a minor optimization, but worth considering if many components consume this context.♻️ Optional: Memoize context value
+'use client' + +import React, { createContext, useContext, useMemo } from 'react' + interface ShareTokenContextValue { /** Share token for public access (no login required) */ shareToken?: string } const ShareTokenContext = createContext<ShareTokenContextValue>({}) export function ShareTokenProvider({ children, shareToken, }: { children: React.ReactNode shareToken?: string }) { - return <ShareTokenContext.Provider value={{ shareToken }}>{children}</ShareTokenContext.Provider> + const value = useMemo(() => ({ shareToken }), [shareToken]) + return <ShareTokenContext.Provider value={value}>{children}</ShareTokenContext.Provider> }frontend/src/apis/attachments.ts (1)
337-343:getAttachmentPreviewUrlandgetAttachmentDownloadUrlare identical.Both functions construct the same URL (
/api/attachments/${attachmentId}/download). Consider consolidating to avoid duplication.♻️ Consolidate duplicate functions
/** - * Get image preview URL for an attachment + * Get attachment download/preview URL * * `@param` attachmentId - Attachment ID * `@param` shareToken - Optional share token for public access - * `@returns` Preview URL + * `@returns` Download URL (used for both preview and download) */ -export function getAttachmentPreviewUrl(attachmentId: number, shareToken?: string): string { +export function getAttachmentDownloadUrl(attachmentId: number, shareToken?: string): string { const baseUrl = `${API_BASE_URL}/api/attachments/${attachmentId}/download` if (shareToken) { return `${baseUrl}?share_token=${encodeURIComponent(shareToken)}` } return baseUrl } -/** - * Get attachment download URL - * - * `@param` attachmentId - Attachment ID - * `@param` shareToken - Optional share token for public access - * `@returns` Download URL - */ -export function getAttachmentDownloadUrl(attachmentId: number, shareToken?: string): string { - const baseUrl = `${API_BASE_URL}/api/attachments/${attachmentId}/download` - if (shareToken) { - return `${baseUrl}?share_token=${encodeURIComponent(shareToken)}` - } - return baseUrl -} +/** `@deprecated` Use getAttachmentDownloadUrl instead */ +export const getAttachmentPreviewUrl = getAttachmentDownloadUrlfrontend/src/hooks/useAttachmentPreview.ts (1)
17-20: Cache key doesn't account forshareToken.The cache uses only
attachmentIdas the key (line 42), but the same attachment could theoretically return different data based on authentication method. If the response differs between authenticated and share-token access, cached data could be incorrect.In practice this is likely fine since:
- Attachment content is the same regardless of auth method
- A user typically accesses via one method consistently
Consider documenting this assumption or adding
shareTokento the cache key if responses could differ.Also applies to: 41-42, 64-64
frontend/src/hooks/useAttachmentImage.ts (1)
66-72: Stale closure in cleanup -blobUrlrevocation is ineffective.The cleanup function captures
blobUrlfrom the closure at the time the effect runs. SinceblobUrlis initiallynulland only set asynchronously, this cleanup will always seenullor a stale value.The second
useEffect(lines 74-80) correctly handles cleanup, so this isn't a bug, but the revocation logic in the first effect's cleanup is misleading/dead code.♻️ Remove ineffective cleanup logic
return () => { isMounted = false - if (blobUrl) { - URL.revokeObjectURL(blobUrl) - } } }, [attachmentId, enabled, shareToken])The second
useEffect(lines 74-80) already handles proper cleanup whenblobUrlchanges or on unmount.backend/app/core/security.py (1)
676-737: Trimget_current_user_optionaland align tracing utilities.This function is ~60 lines and manually manages spans. Consider extracting token verification/user lookup into a helper (or reuse
get_current_user_from_token) and switching to the standard tracing decorators/helpers so it stays under 50 lines and follows the project’s tracing conventions.As per coding guidelines, “Function length: Max 50 lines per function (preferred)” and “OpenTelemetry tracing: Use
@trace_async()for async functions,@trace_sync()for sync functions,add_span_event()to add events,set_span_attribute()to set attributes”.backend/app/api/endpoints/adapter/attachments.py (1)
121-193: Move share-token validation into a service and split helpers.This helper now embeds substantial access-control logic (~70 lines) in the endpoint layer. Consider relocating it to
app/services/(e.g., shared_task or attachment service) and splitting subtask vs unlinked-attachment validation into smaller helpers to keep the API layer thin.As per coding guidelines, “Backend: Add endpoint in
app/api/, schema inapp/schemas/, logic inapp/services/” and “Function length: Max 50 lines per function (preferred)”.
| # Method 1: Share token authentication (no login required) | ||
| if share_token: | ||
| has_access = _validate_share_token_access(db, attachment_id, share_token) | ||
| if has_access: | ||
| # Get context for share token access | ||
| context = context_service.get_context_optional( | ||
| db=db, | ||
| context_id=attachment_id, | ||
| ) | ||
| if context is None: | ||
| raise HTTPException(status_code=404, detail="Attachment not found") | ||
| # Method 2: JWT token authentication (existing logic) | ||
| elif current_user: | ||
| context = _get_attachment_context(db, attachment_id, current_user) | ||
| has_access = True | ||
| else: | ||
| # No authentication provided | ||
| raise HTTPException(status_code=401, detail="Authentication required") | ||
|
|
||
| if not has_access: | ||
| raise HTTPException(status_code=404, detail="Attachment not found") |
There was a problem hiding this comment.
Return 403 when share_token is invalid.
In download_attachment, an invalid share token currently falls through to the final 404, unlike the 403 used in the other endpoints. Add the explicit 403 branch to keep error semantics consistent.
🛠️ Suggested fix
if share_token:
has_access = _validate_share_token_access(db, attachment_id, share_token)
if has_access:
# Get context for share token access
context = context_service.get_context_optional(
db=db,
context_id=attachment_id,
)
if context is None:
raise HTTPException(status_code=404, detail="Attachment not found")
+ else:
+ raise HTTPException(status_code=403, detail="Share token access denied")🤖 Prompt for AI Agents
In `@backend/app/api/endpoints/adapter/attachments.py` around lines 419 - 439, In
download_attachment, when a share_token is provided and
_validate_share_token_access(db, attachment_id, share_token) returns False you
must raise HTTPException(status_code=403, detail="Invalid share token") instead
of letting it fall through to the final 404; update the branch where share_token
is checked (the block that calls _validate_share_token_access and
context_service.get_context_optional) to explicitly return a 403 on invalid
token before continuing, leaving the existing _get_attachment_context and other
authentication branches unchanged.
Merge main branch changes including: - Share token for attachments (#413) - Skill selector for chat interface (#402) - Subscription monitor fix (#408) Conflict resolved in backend/app/core/security.py by keeping both: - get_current_user_flexible_for_executor (from this branch) - get_current_user_optional (from main) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* fix(shared-task): enable attachment preview and download for shared tasks - Add share_token query parameter to attachment download endpoint - Support dual authentication: JWT for logged-in users, share_token for public access - Add get_current_user_optional helper for optional JWT authentication - Update frontend attachment APIs to accept optional shareToken parameter - Pass shareToken through MessageBubble -> ContextBadgeList -> AttachmentPreview - Validate share_token against task ownership before allowing attachment access * fix(attachments): add share token support for public attachment access This commit enables attachments in shared tasks to be accessed without authentication using share tokens. Both user-uploaded attachments and AI-generated attachments now work correctly on shared task pages. Backend changes: - Add share_token query parameter support to get_attachment endpoint - Add share_token support to get_attachment_preview endpoint (already had it for download) - Update _validate_share_token_access to handle attachments with subtask_id=0 by verifying attachment owner matches task owner - Add detailed logging for debugging share token validation - Change 404 to 403 for denied share token access for better error clarity Frontend changes: - Create ShareTokenContext for passing share token through component tree - Update MessageBubble to wrap content with ShareTokenProvider - Update AttachmentCard to use useShareToken and pass to API calls - Update AttachmentEmbed to use useShareToken and pass to API calls - Update getAttachment and getAttachmentPreview APIs to accept optional shareToken - Update useAttachmentPreview hook to accept and pass shareToken parameter - Maintain simple caching strategy using attachmentId as key (no token in cache key) This fixes the issue where attachments in AI-generated messages were returning 404/401 errors when viewed through shared task links. Co-Authored-By: Claude <noreply@anthropic.com> * test(backend): remove obsolete provider detection tests in compression The TokenCounter implementation was updated in commit 522a58a to use LiteLLM for unified token counting across all providers, removing the explicit provider detection logic and `provider` attribute. This commit updates the test suite to reflect the implementation changes: - Remove test_detect_provider_openai (obsolete) - Remove test_detect_provider_anthropic (obsolete) - Remove test_detect_provider_google (obsolete) - Fix test_count_message_multimodal assertion (100 -> 50 tokens) The multimodal test was adjusted because LiteLLM accurately counts tokens for the tiny test base64 image, resulting in ~98 tokens instead of the previously estimated 100+ tokens. All 38 compression tests now pass. * fix(shared-task): display tool usage in shared task page Previously, the shared task page only displayed the text output but not the tool blocks (Bash, Read, Edit, Write, etc.) because the Message object was missing the result field containing blocks data. This commit passes the full result object from PublicSubtaskData to MessageBubble component, enabling MixedContentView to render tool blocks for shared tasks. Co-Authored-By: Claude <noreply@anthropic.com> * feat: 添加OpenAI Anthropic Google模型提供者检测测试 --------- Co-authored-by: yixiangxx <3120662+yixiangxx@users.noreply.github.com> Co-authored-by: yixiangxx <yixiang1@staff.weibo.com> Co-authored-by: Claude <noreply@anthropic.com>
fix(shared-task): enable full attachment access with share tokens (#413) See merge request weibo_rd/common/wecode/wegent!1353
Summary
This PR enables complete attachment access on shared task pages using share tokens, allowing users to view and download both user-uploaded
and AI-generated attachments without authentication.
Changes
Backend (Python)
backend/app/api/endpoints/adapter/attachments.pyshare_tokenquery parameter support to all attachment endpoints:GET /api/attachments/{id}- Attachment metadataGET /api/attachments/{id}/preview- Preview dataGET /api/attachments/{id}/download- File download_validate_share_token_accessto handle two cases:backend/app/core/security.pyget_current_user_optionaldependency for optional JWT authenticationbackend/tests/chat_shell/test_compression.pyFrontend (React/TypeScript)
Context Pattern for Share Token Propagation
ShareTokenContextfor clean share token passing through component treeMessageBubblecontent withShareTokenProvideruseShareToken()hook to access share tokenAPI Layer Updates
getAttachment()- Added optionalshareTokenparametergetAttachmentPreview()- Added optionalshareTokenparameterdownloadAttachment()- Already hadshareTokensupportshare_tokenquery param and omit Authorization header when using share tokenComponent Updates
AttachmentCard- UsesuseShareToken()and passes to API callsAttachmentEmbed- UsesuseShareToken()and passes to API callsAttachmentPreview- PassesshareTokenprop through to componentsContextBadgeList- Renders attachments for both user and AI messagesMessageBubble- Provides share token context to all child componentsHook Updates
useAttachmentPreview- Accepts optionalshareTokenparameteruseAttachmentImage- Accepts optionalshareTokenparameterattachmentIdkey (no token in cache key)Problem Solved
Before this PR, attachments in shared tasks would fail with 401/404 errors because:
subtask_id=0couldn't be validatedNow:
Summary by CodeRabbit
New Features
Tests