✨(fullstack) include a file scanner to prevent storing dangerous files#9
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 38 minutes and 28 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds end-to-end asynchronous antivirus scanning via ClamAV for file transfers. A new ChangesAntivirus Scanning Workflow
Sequence Diagram(s)sequenceDiagram
participant User as User/Browser
participant Frontend as Frontend App
participant Backend as Backend API
participant Celery as Celery Worker
participant ClamAV as ClamAV Service
User->>Frontend: upload file to draft
Frontend->>Backend: POST complete_upload
Backend->>Backend: set scan_status=PENDING
Backend->>Celery: schedule submit_scan_task (on_commit)
Backend-->>Frontend: HTTP 200
Celery->>Backend: submit_scan_task executes
Backend->>Backend: mint webhook_secret, sign S3 URL
Backend->>ClamAV: POST /v2/scan-async (URL + webhook callback + API key)
ClamAV-->>Backend: job_id
Backend->>Backend: persist scan_job_id
rect rgba(200, 200, 100, 0.5)
Note over Frontend,Backend: Scan polling (useEffect in useTransferDraft)
Frontend->>Backend: GET /drafts/{id}/ (periodic)
Backend-->>Frontend: files with scan_status
Frontend->>Frontend: update local scanStatus per file
end
ClamAV->>Backend: POST /webhooks/scan-result/?file_id=X&secret=S
Backend->>Backend: validate secret, map payload → ScanStatus
Backend->>Backend: UPDATE TransferFile WHERE scan_status=PENDING
User->>Frontend: submit transfer
Frontend->>Backend: POST /drafts/{id}/finalize/
alt any file PENDING
Backend-->>Frontend: HTTP 202 reason:scan_pending
Frontend->>Frontend: setIsScanning(true), wait, retry
else any file INFECTED or file-error
Backend-->>Frontend: HTTP 400 reason:scan_blocked
Frontend->>Frontend: show submitError alert
else all files pass
Backend->>Backend: create Transfer
Backend-->>Frontend: HTTP 200
Frontend->>Frontend: show TransferSuccess with scan badge
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/core/api/viewsets/draft.py`:
- Around line 454-460: The loop unconditionally sets f.scan_status to PENDING
and saves each instance, which can overwrite concurrent updates; instead collect
the IDs from errored, perform a conditional bulk update via the model's queryset
filtered by id__in=ids and scan_status=ScanStatus.ERROR to set
scan_status=ScanStatus.PENDING and updated_at, then only enqueue
submit_scan_task.delay for the IDs actually updated and append those IDs to
scanning; keep transaction.on_commit for enqueueing and reference the
variables/objects used in the diff (errored, f.id, ScanStatus.PENDING/ERROR,
submit_scan_task.delay, scanning) so you only resubmit rows that transitioned
from ERROR→PENDING.
In `@src/backend/core/migrations/0004_transferfile_scan_fields.py`:
- Around line 4-10: The current migration function
grandfather_existing_files_clean blindly sets scan_status="clean" on all
TransferFile rows, including draft/incomplete uploads; change
grandfather_existing_files_clean to only mark finalized/uploaded files as clean
by filtering TransferFile (e.g., exclude draft/incomplete states or require a
non-null file/upload timestamp) before calling .update(scan_status="clean") so
draft or incomplete TransferFile records keep their original/unscanned state;
locate the function grandfather_existing_files_clean and the TransferFile model
usage to implement the filtered queryset.
In `@src/backend/core/models.py`:
- Around line 404-426: The scan_status CharField (and similar workflow-gate
fields like scan_job_id/webhook_secret) isn't indexed which will cause full
table scans as volume grows; update the model to add a database index for
scan_status (e.g., set db_index=True on the scan_status field) or add an
explicit Index in the model Meta.indexes for scan_status (and any other
frequently-filtered fields such as scan_job_id or webhook_secret if queried) and
create the corresponding migration so queries using ScanStatus.choices become
efficient.
In `@src/backend/core/tasks.py`:
- Around line 271-277: The current early-return when settings.CLAMAV_SERVICE_URL
or settings.SCAN_WEBHOOK_BASE_URL are unset leaves the TransferFile with
scan_status="pending"; update the TransferFile to a terminal error state (e.g.,
set transfer_file.scan_status = "error" or "failed", optionally set an error
message/timestamp field) and persist the change (call transfer_file.save() or
the model's update/commit method) before returning; also include the
transfer_file_id in the error log for clarity. Ensure this change is applied in
the same block that checks settings.CLAMAV_SERVICE_URL /
settings.SCAN_WEBHOOK_BASE_URL so the file never remains indefinitely pending.
In `@src/backend/transferts/settings.py`:
- Around line 187-230: When CLAMAV_SCAN_ENABLED is true but required settings
may be empty, add a startup-time validation that raises an error instead of
failing later in async tasks: after the config values (CLAMAV_SCAN_ENABLED,
CLAMAV_SERVICE_URL, CLAMAV_API_KEY, SCAN_WEBHOOK_BASE_URL) are loaded, check if
CLAMAV_SCAN_ENABLED is truthy and any of CLAMAV_SERVICE_URL, CLAMAV_API_KEY or
SCAN_WEBHOOK_BASE_URL is blank, and raise
django.core.exceptions.ImproperlyConfigured with a clear message listing the
missing keys; implement this check in the settings module (e.g. at the end of
the file or in the settings class initialization) so the app fails fast on
misconfiguration.
In `@src/frontend/src/features/transfers/components/_transfers.scss`:
- Line 1049: Add a blank line immediately before the comment starting with "//
The parent `__label` aligns its items on the text baseline..." inside the scan
badge block (the selector containing the __label comment) to satisfy the
scss/double-slash-comment-empty-line-before rule; locate the comment near the
scan badge/__label styles and insert one empty line above it so Stylelint no
longer flags the violation.
In `@src/frontend/src/features/transfers/components/DownloadView.tsx`:
- Around line 80-85: The icon-only “clean” scan badge (the span wrapping
CheckmarkShield in DownloadView.tsx) lacks a reliable accessible name; update
the span to provide an explicit accessible label (for example add
aria-label="Scanned — no virus found" or aria-labelledby pointing to a visually
hidden text) and ensure the icon component (CheckmarkShield) is treated as
decorative if the span supplies the label (e.g., leave aria-hidden on the SVG).
Keep the existing title if desired but rely on aria-label/aria-labelledby for
assistive tech.
- Around line 194-198: In DownloadView.tsx the download button tooltip always
shows the antivirus message even for permanently blocked files; update the title
logic where `downloadable` is used to choose a status-specific message: when the
file's status is "infected" show a permanently-blocked message (e.g., "Download
blocked: file contains a virus"), when the status is "error" show an appropriate
error message (e.g., "Download unavailable due to an error"), and otherwise keep
the existing "Available once the antivirus scan passes" fallback; locate the JSX
that sets `title={ downloadable ? t("Download") : t("Available once the
antivirus scan passes") }` and replace it with branching based on `file.status`
(or the prop that holds status).
In `@src/frontend/src/features/transfers/components/TransferDetail.tsx`:
- Around line 199-204: The icon-only clean scan badge in TransferDetail (the
span with class "file-item__scan file-item__scan--clean" that renders
<CheckmarkShield />) lacks an explicit accessible name; add an aria-label
attribute to that span (using the same translated string used for title, i.e.,
t("Scanned — no virus found")) so screen readers announce the badge consistently
alongside or instead of the title.
- Around line 350-355: The state prop currently maps any file.scan_status other
than "infected" or "error" to "done", which incorrectly treats "pending" as
successful; update the state assignment in TransferDetail.tsx (the JSX using
state={...} with file.scan_status) to explicitly handle "pending" (e.g., map
"pending" to "pending" or "loading"/neutral) and only map completed/clean
statuses to "done", leaving "infected" and "error" as "error".
In `@src/frontend/src/features/transfers/components/TransferForm.tsx`:
- Around line 447-468: The catch block in the submit handler currently ignores
non-antivirus errors, leaving users without feedback; after the existing checks
(SubmitCancelledError, ApiError scan_blocked, scan_timeout) add a default error
path that calls setSubmitError with a user-facing message (e.g., "An error
occurred while sending the transfer. Please try again.") and optionally log the
error for diagnostics (use console.error or the existing logger), ensuring this
is added inside the same catch in TransferForm.tsx so finalize failures that
don't set draft.error still surface to the UI.
In `@src/frontend/src/features/transfers/components/TransferSuccess.tsx`:
- Around line 64-67: The current `scanned` boolean uses `some` on
`transfer.files` which can be true if any single file is "clean"; change it so
`scanned` is true only when every file in `transfer.files` has `scan_status ===
"clean"` (and consider requiring `transfer.files.length > 0` so an empty file
list doesn't mark as scanned). Update the `scanned` assignment to use an
all/every check over `transfer.files` (referencing `scanned`, `transfer.files`,
and `scan_status`) so the "Files scanned — no virus found" message only appears
when all files are actually clean.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cb14e94d-109c-4212-9396-ec38f1c15a9f
📒 Files selected for processing (23)
src/backend/core/api/serializers.pysrc/backend/core/api/viewsets/download.pysrc/backend/core/api/viewsets/draft.pysrc/backend/core/api/viewsets/webhook.pysrc/backend/core/enums.pysrc/backend/core/migrations/0004_transferfile_scan_fields.pysrc/backend/core/migrations/0005_alter_transferfile_scan_status.pysrc/backend/core/migrations/0006_alter_transferfile_scan_status.pysrc/backend/core/models.pysrc/backend/core/services/s3.pysrc/backend/core/tasks.pysrc/backend/core/urls.pysrc/backend/transferts/celery_app.pysrc/backend/transferts/settings.pysrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/src/features/api/types.tssrc/frontend/src/features/transfers/api/useTransferDraft.tssrc/frontend/src/features/transfers/components/DownloadView.tsxsrc/frontend/src/features/transfers/components/TransferDetail.tsxsrc/frontend/src/features/transfers/components/TransferForm.tsxsrc/frontend/src/features/transfers/components/TransferSuccess.tsxsrc/frontend/src/features/transfers/components/_transfers.scss
Comment on lines
+454
to
+460
| for f in errored: | ||
| f.scan_status = ScanStatus.PENDING | ||
| f.save(update_fields=["scan_status", "updated_at"]) | ||
| transaction.on_commit( | ||
| lambda fid=str(f.id): submit_scan_task.delay(fid) | ||
| ) | ||
| scanning.append(str(f.id)) |
There was a problem hiding this comment.
Protect ERROR → PENDING resubmission from lost updates.
At Line 454–Line 460, each stale model instance is saved unconditionally. If a webhook updates the same row concurrently, this can overwrite a newer scan_status back to PENDING and distort finalize gating. Use a conditional queryset update (WHERE scan_status='error') before enqueueing rescan.
Proposed fix
for f in errored:
- f.scan_status = ScanStatus.PENDING
- f.save(update_fields=["scan_status", "updated_at"])
- transaction.on_commit(
- lambda fid=str(f.id): submit_scan_task.delay(fid)
- )
- scanning.append(str(f.id))
+ updated = models.TransferFile.objects.filter(
+ id=f.id, scan_status=ScanStatus.ERROR
+ ).update(scan_status=ScanStatus.PENDING, updated_at=timezone.now())
+ if updated:
+ fid = str(f.id)
+ transaction.on_commit(lambda fid=fid: submit_scan_task.delay(fid))
+ scanning.append(fid)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for f in errored: | |
| f.scan_status = ScanStatus.PENDING | |
| f.save(update_fields=["scan_status", "updated_at"]) | |
| transaction.on_commit( | |
| lambda fid=str(f.id): submit_scan_task.delay(fid) | |
| ) | |
| scanning.append(str(f.id)) | |
| for f in errored: | |
| updated = models.TransferFile.objects.filter( | |
| id=f.id, scan_status=ScanStatus.ERROR | |
| ).update(scan_status=ScanStatus.PENDING, updated_at=timezone.now()) | |
| if updated: | |
| fid = str(f.id) | |
| transaction.on_commit(lambda fid=fid: submit_scan_task.delay(fid)) | |
| scanning.append(fid) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/core/api/viewsets/draft.py` around lines 454 - 460, The loop
unconditionally sets f.scan_status to PENDING and saves each instance, which can
overwrite concurrent updates; instead collect the IDs from errored, perform a
conditional bulk update via the model's queryset filtered by id__in=ids and
scan_status=ScanStatus.ERROR to set scan_status=ScanStatus.PENDING and
updated_at, then only enqueue submit_scan_task.delay for the IDs actually
updated and append those IDs to scanning; keep transaction.on_commit for
enqueueing and reference the variables/objects used in the diff (errored, f.id,
ScanStatus.PENDING/ERROR, submit_scan_task.delay, scanning) so you only resubmit
rows that transitioned from ERROR→PENDING.
Comment on lines
+199
to
+204
| <span | ||
| className="file-item__scan file-item__scan--clean" | ||
| title={t("Scanned — no virus found")} | ||
| > | ||
| <CheckmarkShield /> | ||
| </span> |
There was a problem hiding this comment.
Add an accessible name to the icon-only “clean” scan badge.
The badge is icon-only and currently relies on title; add aria-label so screen readers announce it consistently.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/frontend/src/features/transfers/components/TransferDetail.tsx` around
lines 199 - 204, The icon-only clean scan badge in TransferDetail (the span with
class "file-item__scan file-item__scan--clean" that renders <CheckmarkShield />)
lacks an explicit accessible name; add an aria-label attribute to that span
(using the same translated string used for title, i.e., t("Scanned — no virus
found")) so screen readers announce the badge consistently alongside or instead
of the title.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/core/api/viewsets/webhook.py`:
- Around line 61-63: The TransferFile.objects.filter() update operation
unconditionally overwrites scan_status, allowing stale webhooks to overwrite
terminal states like INFECTED or ERROR. Modify the filter condition in the
update call to include a check that the current scan_status is in a mutable
state (such as PENDING) before allowing the update to proceed, ensuring that
once a terminal state is reached, subsequent callbacks cannot change it.
In `@src/backend/core/enums.py`:
- Around line 58-63: The docstring for the ScanStatus enum states that downloads
fail on anything except CLEAN and SKIPPED, but the TOO_LARGE status is
separately documented as downloadable within the same enum, creating a contract
contradiction. Update the ScanStatus docstring (starting at the description of
PENDING and covering the download policy) to explicitly mention TOO_LARGE as
another downloadable state alongside CLEAN and SKIPPED, or clarify the complete
set of downloadable states to match the enum's documented behavior. Ensure the
updated docstring adheres to the 100 character per line limit per the project's
coding guidelines.
In `@src/backend/core/tasks.py`:
- Around line 313-317: The exception handler is retrying all RequestExceptions,
including non-transient HTTP 4xx client errors that will never succeed on retry.
Modify the exception handling to distinguish between transient errors (5xx
server errors, connection timeouts, etc.) and non-transient errors (4xx client
errors). For non-transient failures like 4xx responses, log the error and raise
without retrying; for transient errors only, call self.retry(exc=exc). This
prevents files from staying in PENDING state while the reaper continuously
re-enqueues terminal scan submission failures.
- Around line 313-317: In the except block handling requests.RequestException,
add Sentry exception capture to track external service failures in production.
Before the self.retry(exc=exc) call in the exception handler, insert a call to
sentry_sdk.capture_exception(exc) to ensure the scanner submission failure is
reported to Sentry for incident visibility, as required by the coding guidelines
for external service request failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e02a6ed7-2257-48c4-b87c-a3ff94f98975
📒 Files selected for processing (23)
src/backend/core/api/serializers.pysrc/backend/core/api/viewsets/download.pysrc/backend/core/api/viewsets/draft.pysrc/backend/core/api/viewsets/webhook.pysrc/backend/core/enums.pysrc/backend/core/migrations/0004_transferfile_scan_fields.pysrc/backend/core/migrations/0005_alter_transferfile_scan_status.pysrc/backend/core/migrations/0006_alter_transferfile_scan_status.pysrc/backend/core/models.pysrc/backend/core/services/s3.pysrc/backend/core/tasks.pysrc/backend/core/urls.pysrc/backend/transferts/celery_app.pysrc/backend/transferts/settings.pysrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/src/features/api/types.tssrc/frontend/src/features/transfers/api/useTransferDraft.tssrc/frontend/src/features/transfers/components/DownloadView.tsxsrc/frontend/src/features/transfers/components/TransferDetail.tsxsrc/frontend/src/features/transfers/components/TransferForm.tsxsrc/frontend/src/features/transfers/components/TransferSuccess.tsxsrc/frontend/src/features/transfers/components/_transfers.scss
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/frontend/src/features/transfers/components/_transfers.scss (1)
1049-1049:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the Stylelint empty-line-before-comment violation.
Line 1049 still violates
scss/double-slash-comment-empty-line-before.💡 Suggested fix
&__scan { display: inline-flex; align-items: center; + // The parent `__label` aligns its items on the text baseline (right for // name / size), but this badge is icon-led — pin it to the row's vertical // centre instead so the icon doesn't ride above the text line.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/features/transfers/components/_transfers.scss` at line 1049, The Stylelint rule `scss/double-slash-comment-empty-line-before` is being violated on the comment that starts with "The parent `__label` aligns its items on the text baseline". Add an empty line immediately before this double-slash comment to comply with the Stylelint configuration requirement.Source: Linters/SAST tools
src/frontend/src/features/transfers/components/TransferForm.tsx (1)
422-443:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle non-antivirus submit failures with a fallback error path.
At Line 422, the
catchonly maps scan-specific failures; other finalize errors are silently ignored, so users can see a no-op submit with no feedback.💡 Suggested fix
} catch (err) { // A cancel is a deliberate user action — stay on the form silently. if (err instanceof SubmitCancelledError) return; // Antivirus gate: a blocked file (virus / scan error) or a scan that // never resolved (scanner down) — surface a clear, dedicated message. if ( err instanceof ApiError && (err.body as { reason?: string })?.reason === "scan_blocked" ) { setSubmitError( t("A virus was detected — this transfer was blocked and not sent."), ); return; } if (err instanceof Error && err.message === "scan_timeout") { setSubmitError( t("The antivirus scan is taking too long. Please try again."), ); return; } - // Other errors already surface via draft.error / per-file state. + setSubmitError(t("Error creating transfer.")); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/features/transfers/components/TransferForm.tsx` around lines 422 - 443, The catch block at the end of the try-catch in TransferForm.tsx currently handles only specific scan-related failures (SubmitCancelledError, scan_blocked reason, and scan_timeout message) but silently ignores all other errors by reaching the end of the catch block without any action. Add a fallback error handler after all the existing specific error checks (after the scan_timeout condition) to catch any remaining errors that don't match the scan-specific conditions, and call setSubmitError with an appropriate generic error message to ensure users receive feedback when non-antivirus related submit failures occur.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/core/models.py`:
- Around line 432-434: The help_text for the scan_status field does not
accurately describe the download gate behavior. Update the help_text to reflect
that files are downloadable not just when CLEAN, but also when the scan_status
is SKIPPED or TOO_LARGE, since those states are explicitly allowed by the
download gate logic in the download.py viewsets file. Revise the description to
be accurate while maintaining compliance with the 100 character per line limit
as per Django coding guidelines.
---
Duplicate comments:
In `@src/frontend/src/features/transfers/components/_transfers.scss`:
- Line 1049: The Stylelint rule `scss/double-slash-comment-empty-line-before` is
being violated on the comment that starts with "The parent `__label` aligns its
items on the text baseline". Add an empty line immediately before this
double-slash comment to comply with the Stylelint configuration requirement.
In `@src/frontend/src/features/transfers/components/TransferForm.tsx`:
- Around line 422-443: The catch block at the end of the try-catch in
TransferForm.tsx currently handles only specific scan-related failures
(SubmitCancelledError, scan_blocked reason, and scan_timeout message) but
silently ignores all other errors by reaching the end of the catch block without
any action. Add a fallback error handler after all the existing specific error
checks (after the scan_timeout condition) to catch any remaining errors that
don't match the scan-specific conditions, and call setSubmitError with an
appropriate generic error message to ensure users receive feedback when
non-antivirus related submit failures occur.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 68756987-0993-4db9-a822-a9a053240b39
📒 Files selected for processing (23)
src/backend/core/api/serializers.pysrc/backend/core/api/viewsets/download.pysrc/backend/core/api/viewsets/draft.pysrc/backend/core/api/viewsets/webhook.pysrc/backend/core/enums.pysrc/backend/core/migrations/0004_transferfile_scan_fields.pysrc/backend/core/migrations/0005_alter_transferfile_scan_status.pysrc/backend/core/migrations/0006_alter_transferfile_scan_status.pysrc/backend/core/models.pysrc/backend/core/services/s3.pysrc/backend/core/tasks.pysrc/backend/core/urls.pysrc/backend/transferts/celery_app.pysrc/backend/transferts/settings.pysrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/src/features/api/types.tssrc/frontend/src/features/transfers/api/useTransferDraft.tssrc/frontend/src/features/transfers/components/DownloadView.tsxsrc/frontend/src/features/transfers/components/TransferDetail.tsxsrc/frontend/src/features/transfers/components/TransferForm.tsxsrc/frontend/src/features/transfers/components/TransferSuccess.tsxsrc/frontend/src/features/transfers/components/_transfers.scss
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)
src/frontend/src/features/transfers/components/TransferDetail.tsx (1)
334-337:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winProvide status-aware tooltip text when download is disabled.
At Line 336, disabled actions still show
Download, which hides the reason (pending vs blocked). Reusing the same status-aware title logic asDownloadViewwould avoid ambiguity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/features/transfers/components/TransferDetail.tsx` around lines 334 - 337, The title attribute in the disabled download button of TransferDetail component currently shows a static "Download" string regardless of why the button is disabled. Instead of using a fixed title, implement status-aware tooltip logic by determining the appropriate message based on the transfer state (checking conditions like isActive, transfer.public_token, and downloadable status). Reference the tooltip logic from DownloadView component and apply similar conditional logic to dynamically set the title attribute so users understand whether the download is pending, blocked, or unavailable due to a specific reason.src/frontend/src/features/transfers/components/TransferForm.tsx (1)
392-401:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMirror blocked-scan checks in
handleSubmitto prevent Enter-key bypass.Line 396–401 does not enforce the same scan blockers used by
submitDisabled(infected / unscannable), so pressing Enter can still call finalize even when the submit button is disabled.💡 Proposed fix
const anyInfected = draft.files.some((f) => f.scanStatus === "infected"); // A file-bound scan error is a hard block too (retry won't help). A // *transient* scan error is not — clicking Create re-submits it. const anyUnscannable = draft.files.some( (f) => f.scanStatus === "error" && f.scanErrorKind === "file", ); + const hasScanBlocker = anyInfected || anyUnscannable; const handleSubmit = async (e: React.FormEvent) => { e.preventDefault(); // Button is disabled during both phases — the form can still submit // via Enter key, so guard here too. if (busy) return; - if (!hasFiles || anyError) return; + if (!hasFiles || anyError || hasScanBlocker) return; if (sharingMode === "email" && recipients.length === 0 && !hasValidPending) { return; } @@ const submitDisabled = busy || !hasFiles || anyError || - anyInfected || - anyUnscannable || + hasScanBlocker || (sharingMode === "email" && recipients.length === 0 && !hasValidPending);Also applies to: 473-485
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/features/transfers/components/TransferForm.tsx` around lines 392 - 401, The handleSubmit function does not include the same scan-related blockers that are checked in submitDisabled, allowing users to bypass the disabled button state by pressing Enter. Add the same infected/unscannable file checks from the submitDisabled logic to the guard clauses in handleSubmit to ensure form submission is blocked when the button would be disabled. Apply this same fix to any other submission handlers or locations mentioned in the review comment.
♻️ Duplicate comments (5)
src/frontend/src/features/transfers/components/TransferSuccess.tsx (1)
41-44:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly show “Files scanned, no virus found” when all files are clean.
At Line 44,
.some(...)can display this message when only one file is clean.Suggested patch
- const scanned = transfer.files.some((f) => f.scan_status === "clean"); + const scanned = + transfer.files.length > 0 && + transfer.files.every((f) => f.scan_status === "clean");Also applies to: 54-59
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/features/transfers/components/TransferSuccess.tsx` around lines 41 - 44, The `scanned` constant in the TransferSuccess component currently uses the `.some()` method to check if at least one file has a scan_status of "clean", but the requirement is to only display the virus scan success message when all files are clean. Replace the `.some()` method with `.every()` on the transfer.files array to ensure the scanned message only displays when all files have been scanned and are clean, not just when one file is clean.src/frontend/src/features/transfers/components/DownloadView.tsx (1)
181-185:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse status-specific disabled tooltip text for blocked files.
At Line 181,
infected/errorfiles still get the “Available once the antivirus scan passes” tooltip, which is incorrect for terminal blocked outcomes.Suggested patch
title={ downloadable ? t("Download") - : t("Available once the antivirus scan passes") + : file.scan_status === "infected" + ? t("Blocked: a virus was detected in this file") + : file.scan_status === "error" + ? t("Blocked: the antivirus scan could not complete") + : t("Available once the antivirus scan passes") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/features/transfers/components/DownloadView.tsx` around lines 181 - 185, The title attribute at lines 181-185 in the DownloadView component only checks the downloadable boolean to determine tooltip text, but does not account for terminal blocked states. Modify the title attribute logic to check for additional status conditions beyond downloadable, such as infected or error states. For terminal blocked outcomes like infected or error files, use a different tooltip message that appropriately indicates the file is permanently blocked rather than waiting for an antivirus scan to complete. Ensure the conditional logic properly prioritizes terminal error states before falling back to the "Available once the antivirus scan passes" message.src/frontend/src/features/transfers/components/TransferDetail.tsx (1)
321-326:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not render
pendingscan status as a successful file state.At Line 321, everything except
infected/erroris treated as"done", sopendingappears visually successful before scan completion.Suggested patch
state={ - file.scan_status === "infected" || - file.scan_status === "error" - ? "error" - : "done" + file.scan_status === "infected" || file.scan_status === "error" + ? "error" + : file.scan_status === "clean" || + file.scan_status === "skipped" || + file.scan_status === "too_large" + ? "done" + : "default" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/features/transfers/components/TransferDetail.tsx` around lines 321 - 326, In the TransferDetail component's file state rendering logic, the current conditional treats any scan_status that is not "infected" or "error" as "done", which incorrectly shows pending scans as completed. Instead of using a ternary that defaults to "done" for all other cases, explicitly check for the actual completion status (likely "clean" or similar) and assign "done" only for that case, while assigning an appropriate pending or loading state for "pending" scan status. Update the state assignment condition to handle the three distinct scan status cases: infected/error as "error", a specific successful scan completion status as "done", and "pending" as its own state value.src/backend/core/api/viewsets/webhook.py (1)
63-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard webhook updates to mutable scan states only.
At Line 63, updating by
idalone allows late callbacks to overwrite already-finalized results (CLEAN,INFECTED, orERROR) from newer events.Suggested fix
- models.TransferFile.objects.filter(id=transfer_file.id).update( - scan_status=new_status, scan_error_kind=error_kind - ) + updated = models.TransferFile.objects.filter( + id=transfer_file.id, + scan_status=ScanStatus.PENDING, + ).update(scan_status=new_status, scan_error_kind=error_kind) + if not updated: + logger.info("Ignoring stale scan callback for non-pending file %s", file_id) + return Response(status=200)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/api/viewsets/webhook.py` around lines 63 - 65, The TransferFile.objects.filter(id=transfer_file.id).update() call at line 63 only filters by ID, allowing late webhook callbacks to overwrite already-finalized scan results. Guard this update by adding an additional filter condition to the .filter() call to exclude records that are already in terminal states (CLEAN, INFECTED, or ERROR). Only permit updates when the current scan_status is in a mutable/pending state, preventing late-arriving callbacks from overwriting finalized results from newer events.src/backend/core/api/viewsets/draft.py (1)
466-479:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid lost updates when resetting transient scan errors.
At Lines 467-475, saving stale in-memory rows can overwrite a newer webhook result (e.g.,
CLEAN/INFECTED) back toPENDING. Transition should be conditional at the DB level before enqueueing a rescan.Suggested fix
- for f in transient_errored: - f.scan_status = ScanStatus.PENDING - f.scan_error_kind = "" - f.save( - update_fields=[ - "scan_status", - "scan_error_kind", - "updated_at", - ] - ) - transaction.on_commit( - lambda fid=str(f.id): submit_scan_task.delay(fid) - ) - scanning.append(str(f.id)) + for f in transient_errored: + updated = models.TransferFile.objects.filter( + id=f.id, + scan_status=ScanStatus.ERROR, + ).update( + scan_status=ScanStatus.PENDING, + scan_error_kind="", + updated_at=timezone.now(), + ) + if updated: + fid = str(f.id) + transaction.on_commit( + lambda fid=fid: submit_scan_task.delay(fid) + ) + scanning.append(fid)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/api/viewsets/draft.py` around lines 466 - 479, Replace the unconditional in-memory update and save pattern in the transient_errored loop with a conditional database-level update to prevent overwriting newer webhook results. Instead of setting scan_status and scan_error_kind on the in-memory object f and calling f.save(), use a Django ORM queryset update with a conditional filter that only resets these fields if the current database state still indicates a transient error. This ensures that if a webhook has concurrently updated the scan status to CLEAN or INFECTED, the conditional update will not execute and the newer status will not be lost. Reference the File model's update method to perform this conditional transition at the database level before enqueueing the rescan task via submit_scan_task.delay().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/core/api/viewsets/draft.py`:
- Around line 431-437: The comment block preceding the CLAMAV_SCAN_ENABLED gate
check incorrectly states that finalization requires all files to be CLEAN, but
the actual gate logic allows files with SKIPPED and TOO_LARGE statuses in
addition to CLEAN. Update the comment to accurately reflect all three allowed
file statuses that pass the finalization check, removing the inaccuracy about
the CLEAN-only requirement.
---
Outside diff comments:
In `@src/frontend/src/features/transfers/components/TransferDetail.tsx`:
- Around line 334-337: The title attribute in the disabled download button of
TransferDetail component currently shows a static "Download" string regardless
of why the button is disabled. Instead of using a fixed title, implement
status-aware tooltip logic by determining the appropriate message based on the
transfer state (checking conditions like isActive, transfer.public_token, and
downloadable status). Reference the tooltip logic from DownloadView component
and apply similar conditional logic to dynamically set the title attribute so
users understand whether the download is pending, blocked, or unavailable due to
a specific reason.
In `@src/frontend/src/features/transfers/components/TransferForm.tsx`:
- Around line 392-401: The handleSubmit function does not include the same
scan-related blockers that are checked in submitDisabled, allowing users to
bypass the disabled button state by pressing Enter. Add the same
infected/unscannable file checks from the submitDisabled logic to the guard
clauses in handleSubmit to ensure form submission is blocked when the button
would be disabled. Apply this same fix to any other submission handlers or
locations mentioned in the review comment.
---
Duplicate comments:
In `@src/backend/core/api/viewsets/draft.py`:
- Around line 466-479: Replace the unconditional in-memory update and save
pattern in the transient_errored loop with a conditional database-level update
to prevent overwriting newer webhook results. Instead of setting scan_status and
scan_error_kind on the in-memory object f and calling f.save(), use a Django ORM
queryset update with a conditional filter that only resets these fields if the
current database state still indicates a transient error. This ensures that if a
webhook has concurrently updated the scan status to CLEAN or INFECTED, the
conditional update will not execute and the newer status will not be lost.
Reference the File model's update method to perform this conditional transition
at the database level before enqueueing the rescan task via
submit_scan_task.delay().
In `@src/backend/core/api/viewsets/webhook.py`:
- Around line 63-65: The
TransferFile.objects.filter(id=transfer_file.id).update() call at line 63 only
filters by ID, allowing late webhook callbacks to overwrite already-finalized
scan results. Guard this update by adding an additional filter condition to the
.filter() call to exclude records that are already in terminal states (CLEAN,
INFECTED, or ERROR). Only permit updates when the current scan_status is in a
mutable/pending state, preventing late-arriving callbacks from overwriting
finalized results from newer events.
In `@src/frontend/src/features/transfers/components/DownloadView.tsx`:
- Around line 181-185: The title attribute at lines 181-185 in the DownloadView
component only checks the downloadable boolean to determine tooltip text, but
does not account for terminal blocked states. Modify the title attribute logic
to check for additional status conditions beyond downloadable, such as infected
or error states. For terminal blocked outcomes like infected or error files, use
a different tooltip message that appropriately indicates the file is permanently
blocked rather than waiting for an antivirus scan to complete. Ensure the
conditional logic properly prioritizes terminal error states before falling back
to the "Available once the antivirus scan passes" message.
In `@src/frontend/src/features/transfers/components/TransferDetail.tsx`:
- Around line 321-326: In the TransferDetail component's file state rendering
logic, the current conditional treats any scan_status that is not "infected" or
"error" as "done", which incorrectly shows pending scans as completed. Instead
of using a ternary that defaults to "done" for all other cases, explicitly check
for the actual completion status (likely "clean" or similar) and assign "done"
only for that case, while assigning an appropriate pending or loading state for
"pending" scan status. Update the state assignment condition to handle the three
distinct scan status cases: infected/error as "error", a specific successful
scan completion status as "done", and "pending" as its own state value.
In `@src/frontend/src/features/transfers/components/TransferSuccess.tsx`:
- Around line 41-44: The `scanned` constant in the TransferSuccess component
currently uses the `.some()` method to check if at least one file has a
scan_status of "clean", but the requirement is to only display the virus scan
success message when all files are clean. Replace the `.some()` method with
`.every()` on the transfer.files array to ensure the scanned message only
displays when all files have been scanned and are clean, not just when one file
is clean.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 912c62e7-b6ab-4f9b-821c-3cb795acb0c8
📒 Files selected for processing (17)
src/backend/core/api/serializers.pysrc/backend/core/api/viewsets/config.pysrc/backend/core/api/viewsets/draft.pysrc/backend/core/api/viewsets/webhook.pysrc/backend/core/migrations/0007_transferfile_scan_error_kind.pysrc/backend/core/models.pysrc/backend/core/tests/test_api_webhook.pysrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/src/features/api/types.tssrc/frontend/src/features/providers/config.tsxsrc/frontend/src/features/transfers/api/useTransferDraft.tssrc/frontend/src/features/transfers/components/DownloadView.tsxsrc/frontend/src/features/transfers/components/TransferDetail.tsxsrc/frontend/src/features/transfers/components/TransferForm.tsxsrc/frontend/src/features/transfers/components/TransferSuccess.tsxsrc/frontend/src/features/transfers/components/_transfers.scss
Nastaliss
changed the title
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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 (1)
src/backend/core/tasks.py (1)
295-299: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMint
webhook_secretatomically.Two concurrent submissions can both see an empty secret, save different values, and submit scan jobs with mismatched callback URLs. The losing job’s callback will be rejected by the webhook’s current-secret comparison, delaying the file in
PENDINGuntil the reaper recovers it.Proposed fix
# Mint the per-file callback secret on first submit; reuse it on retries so # the webhook URL stays stable across attempts. if not tf.webhook_secret: - tf.webhook_secret = secrets.token_urlsafe(32) - tf.save(update_fields=["webhook_secret", "updated_at"]) + new_secret = secrets.token_urlsafe(32) + updated = TransferFile.objects.filter(id=tf.id, webhook_secret="").update( + webhook_secret=new_secret, + updated_at=timezone.now(), + ) + if updated: + tf.webhook_secret = new_secret + else: + current_secret = ( + TransferFile.objects.filter(id=tf.id) + .values_list("webhook_secret", flat=True) + .first() + ) + if not current_secret: + return + tf.webhook_secret = current_secret🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/tasks.py` around lines 295 - 299, The webhook_secret minting is vulnerable to a race condition where multiple concurrent submissions can both see an empty secret and save different values. Replace the non-atomic check-and-set pattern (the if condition followed by tf.save) with an atomic database operation using Django's update_or_create or F() objects with conditional updates to ensure only one concurrent submission wins and sets the webhook_secret. This ensures all submissions for the same task file use the same callback URL regardless of concurrency.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/core/tests/test_api_webhook.py`:
- Around line 100-122: Add a new test method (similar to
test_terminal_state_not_overwritten and
test_duplicate_callback_cannot_flip_clean) to cover the ERROR terminal state
idempotency. The test should create a file with scan_status set to
ScanStatus.ERROR, then post a webhook callback with a success/clean payload
(e.g., status done with malware False), and assert that the file's scan_status
remains ERROR and is not overwritten. This ensures the ERROR state, like
INFECTED and CLEAN, cannot be flipped by duplicate or stale callbacks.
---
Outside diff comments:
In `@src/backend/core/tasks.py`:
- Around line 295-299: The webhook_secret minting is vulnerable to a race
condition where multiple concurrent submissions can both see an empty secret and
save different values. Replace the non-atomic check-and-set pattern (the if
condition followed by tf.save) with an atomic database operation using Django's
update_or_create or F() objects with conditional updates to ensure only one
concurrent submission wins and sets the webhook_secret. This ensures all
submissions for the same task file use the same callback URL regardless of
concurrency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d7e24123-b9fb-443b-9625-845d6bb11f33
📒 Files selected for processing (14)
src/backend/core/api/viewsets/draft.pysrc/backend/core/api/viewsets/webhook.pysrc/backend/core/migrations/0005_transferfile_scan_fields.pysrc/backend/core/models.pysrc/backend/core/tasks.pysrc/backend/core/tests/conftest.pysrc/backend/core/tests/test_api_webhook.pysrc/backend/transferts/settings.pysrc/frontend/caddy/Caddyfilesrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/src/features/transfers/components/TransferForm.tsxsrc/frontend/src/features/transfers/components/TransferSuccess.tsxsrc/frontend/src/features/transfers/components/_transfers.scss
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
User Interface
Localization
Tests