fix(security): remove client-side UploadThing credentials - #300
Conversation
📝 WalkthroughWalkthroughCloud UploadThing credential injection and client-side upload/delete flows were removed. Upload controls are disabled, upload history changes are local-only, public-link imports remain supported, and tests and documentation describe the fail-closed behavior. ChangesUploadThing security shutdown
Estimated code review effort: 4 (Complex) | ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| /// <summary> | ||
| /// Removes a history item. | ||
| /// Removes an item from local history without deleting the hosted file. |
There was a problem hiding this comment.
WARNING: Missing parameter validation
The RemoveHistoryItemAsync method accepts a string url parameter but doesn't validate that the URL is not null or empty. This could cause issues if called with invalid input.
| /// Removes an item from local history without deleting the hosted file. | |
| Task RemoveHistoryItemAsync(string url); |
There was a problem hiding this comment.
Keeping this unchanged. The parameter is non-nullable, and the implementation’s equality check safely treats null or empty input as a no-op. The suggested interface change is identical and adds no runtime validation.
| <StackPanel Orientation="Horizontal" Spacing="0"> | ||
| <Button Command="{Binding UploadAndShareCommand}" | ||
| Classes="Primary" IsEnabled="{Binding SelectedMaps.Count}" | ||
| Classes="Primary" IsEnabled="False" |
There was a problem hiding this comment.
SUGGESTION: Consider binding IsEnabled to a property instead of hardcoded value
Hardcoding IsEnabled="False" bypasses MVVM command-binding patterns. Consider binding to a ViewModel property like IsUploadEnabled so the state can be managed centrally and the button can be re-enabled programmatically when short-lived credentials are implemented.
| Classes="Primary" IsEnabled="False" | |
| Classes="Primary" IsEnabled="{Binding IsUploadEnabled}" |
There was a problem hiding this comment.
Keeping the hardcoded disabled state intentionally. This is a fail-closed security switch; re-enabling uploads should require an explicit code change alongside the future authorization implementation, rather than a mutable ViewModel property.
| <StackPanel Orientation="Horizontal" Spacing="0"> | ||
| <Button Command="{Binding UploadAndShareCommand}" | ||
| Classes="Primary" IsEnabled="{Binding SelectedReplays.Count}" | ||
| Classes="Primary" IsEnabled="False" |
There was a problem hiding this comment.
SUGGESTION: Consider binding IsEnabled to a property instead of hardcoded value
Hardcoding IsEnabled="False" bypasses MVVM command-binding patterns. Consider binding to a ViewModel property like IsUploadEnabled so the state can be managed centrally and the button can be re-enabled programmatically when short-lived credentials are implemented.
| Classes="Primary" IsEnabled="False" | |
| Classes="Primary" IsEnabled="{Binding IsUploadEnabled}" |
There was a problem hiding this comment.
Keeping the hardcoded disabled state intentionally. This is a fail-closed security switch; re-enabling uploads should require an explicit code change alongside the future authorization implementation, rather than a mutable ViewModel property.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Resolved Issues (click to expand)The following issues from the previous review have been resolved in this commit:
Files Reviewed (18 files)
Security AssessmentOverall: ✅ Good security improvements with migration fix This PR effectively removes the reversible XOR obfuscation of UploadThing credentials from the build pipeline and desktop binaries. The fail-closed approach for Migration Fix: The pending deletion records issue has been properly resolved. Legacy Note: Repository owners should revoke and rotate the exposed UploadThing token as mentioned in the documentation. Positive Observations
Fix these issues in Kilo Cloud Previous Review Summary (commit c69a6cc)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit c69a6cc)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (18 files)
Security AssessmentOverall: ✅ Good security improvements This PR effectively removes the reversible XOR obfuscation of UploadThing credentials from the build pipeline and desktop binaries. The fail-closed approach for Note: Repository owners should revoke and rotate the exposed UploadThing token as mentioned in the documentation. Positive Observations
Reply with Reviewed by glm-4.7 · Input: 50.2K · Output: 8.6K · Cached: 252.4K |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
GenHub/GenHub/Features/Tools/MapManager/Views/MapManagerView.axaml (2)
190-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale "(max 10MB per file)" hint left next to permanently disabled Upload buttons. Both views correctly hard-disable the Upload button and update its tooltip to explain the credential-shutdown, but the adjacent size-limit caption was left untouched, so it now advertises a limit for a feature users can't use at all.
GenHub/GenHub/Features/Tools/MapManager/Views/MapManagerView.axaml#L190-L207: remove the"(max 10MB per file)"TextBlock at Line 207 (or bind itsIsVisibleto false/an "uploads enabled" flag) since the Upload button is now permanently disabled.GenHub/GenHub/Features/Tools/ReplayManager/Views/ReplayManagerView.axaml#L168-L185: remove the"(max 10MB per file)"TextBlock at Line 185 (or bind itsIsVisiblesimilarly) for the same reason.🤖 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 `@GenHub/GenHub/Features/Tools/MapManager/Views/MapManagerView.axaml` around lines 190 - 207, Remove or hide the stale “(max 10MB per file)” caption beside the disabled Upload controls in MapManagerView.axaml (lines 190-207) and ReplayManagerView.axaml (lines 168-185), using the existing uploads-enabled state if binding visibility instead of removing the TextBlock.
190-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDisabling looks correct; stale size hint next to disabled button.
The permanent
IsEnabled="False"and updated tooltip correctly reflect that cloud uploads are disabled. However, the adjacent"(max 10MB per file)"hint at Line 207 still implies an active upload limit for a control that can never be used, which is confusing. This shares one root cause with the identical text inReplayManagerView.axaml; see the consolidated comment.Also applies to: 207-207
🤖 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 `@GenHub/GenHub/Features/Tools/MapManager/Views/MapManagerView.axaml` around lines 190 - 199, The disabled upload controls should not display the stale “(max 10MB per file)” hint. Remove that hint from the upload section in MapManagerView and the corresponding upload section in ReplayManagerView, while preserving the disabled buttons and their existing tooltips.GenHub/GenHub/Features/Tools/ReplayManager/Views/ReplayManagerView.axaml (1)
168-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDisabling looks correct; stale size hint next to disabled button.
Same pattern as
MapManagerView.axaml: the disable + tooltip change is correct, but"(max 10MB per file)"at Line 185 is now stale next to a permanently disabled Upload button. See the consolidated comment.Also applies to: 185-185
🤖 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 `@GenHub/GenHub/Features/Tools/ReplayManager/Views/ReplayManagerView.axaml` around lines 168 - 177, Remove the stale “(max 10MB per file)” size hint adjacent to the disabled Upload button in the ReplayManagerView upload controls. Keep the existing UploadAndShareCommand button, disabled state, and tooltip unchanged.
🤖 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
`@GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Tools/Services/UploadHistoryServiceTests.cs`:
- Around line 42-68: Extend the UploadHistoryService tests around
RemoveHistoryItemAsync and ClearHistoryAsync to cover partial removal and no-op
behavior. Add a case proving removing one URL preserves unrelated history
entries, plus cases confirming removing a non-matching URL and clearing an empty
history complete without throwing and leave history empty.
In `@GenHub/GenHub/Features/Tools/Services/UploadThingService.cs`:
- Around line 19-37: Move the two disabled-state warning strings used by
UploadFileAsync and DeleteFileAsync into named constants in LogMessages, then
update both logger.LogWarning calls to reference those constants. Preserve the
existing warning text and behavior while following the established LogMessages
convention.
---
Outside diff comments:
In `@GenHub/GenHub/Features/Tools/MapManager/Views/MapManagerView.axaml`:
- Around line 190-207: Remove or hide the stale “(max 10MB per file)” caption
beside the disabled Upload controls in MapManagerView.axaml (lines 190-207) and
ReplayManagerView.axaml (lines 168-185), using the existing uploads-enabled
state if binding visibility instead of removing the TextBlock.
- Around line 190-199: The disabled upload controls should not display the stale
“(max 10MB per file)” hint. Remove that hint from the upload section in
MapManagerView and the corresponding upload section in ReplayManagerView, while
preserving the disabled buttons and their existing tooltips.
In `@GenHub/GenHub/Features/Tools/ReplayManager/Views/ReplayManagerView.axaml`:
- Around line 168-177: Remove the stale “(max 10MB per file)” size hint adjacent
to the disabled Upload button in the ReplayManagerView upload controls. Keep the
existing UploadAndShareCommand button, disabled state, and tooltip unchanged.
🪄 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 Plus
Run ID: 8c45dee5-4809-44f8-a149-21804c1932bd
📒 Files selected for processing (18)
.github/scripts/inject-token.ps1.github/workflows/ci.yml.github/workflows/release.ymlGenHub/GenHub.Core/Constants/ApiConstants.csGenHub/GenHub.Core/Constants/ErrorMessages.csGenHub/GenHub.Core/Constants/LogMessages.csGenHub/GenHub.Core/Interfaces/Common/IUploadHistoryService.csGenHub/GenHub.Core/Models/Tools/UploadRecord.csGenHub/GenHub.Tests/GenHub.Tests.Core/Features/Tools/Services/UploadHistoryServiceTests.csGenHub/GenHub.Tests/GenHub.Tests.Core/Features/Tools/Services/UploadThingServiceTests.csGenHub/GenHub/Features/Tools/MapManager/ViewModels/MapManagerViewModel.csGenHub/GenHub/Features/Tools/MapManager/Views/MapManagerView.axamlGenHub/GenHub/Features/Tools/ReplayManager/ViewModels/ReplayManagerViewModel.csGenHub/GenHub/Features/Tools/ReplayManager/Views/ReplayManagerView.axamlGenHub/GenHub/Features/Tools/Services/UploadHistoryService.csGenHub/GenHub/Features/Tools/Services/UploadThingService.csdocs/dev/constants.mddocs/dev/uploading-api.md
💤 Files with no reviewable changes (5)
- GenHub/GenHub.Core/Constants/LogMessages.cs
- GenHub/GenHub.Core/Constants/ErrorMessages.cs
- .github/scripts/inject-token.ps1
- .github/workflows/ci.yml
- .github/workflows/release.yml
| public async Task RemoveHistoryItemAsync_WhenItemExists_RemovesLocalRecord() | ||
| { | ||
| var service = CreateService(); | ||
| service.RecordUpload(1024, "https://utfs.io/f/example", "example.zip"); | ||
|
|
||
| await service.RemoveHistoryItemAsync("https://utfs.io/f/example"); | ||
|
|
||
| var reloadedService = CreateService(); | ||
| Assert.Empty(await reloadedService.GetUploadHistoryAsync()); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Verifies that clearing history deletes every local record immediately. | ||
| /// </summary> | ||
| /// <returns>A task representing the asynchronous test operation.</returns> | ||
| [Fact] | ||
| public async Task ClearHistoryAsync_WhenItemsExist_RemovesAllLocalRecords() | ||
| { | ||
| var service = CreateService(); | ||
| service.RecordUpload(1024, "https://utfs.io/f/first", "first.zip"); | ||
| service.RecordUpload(2048, "https://utfs.io/f/second", "second.zip"); | ||
|
|
||
| await service.ClearHistoryAsync(); | ||
|
|
||
| var reloadedService = CreateService(); | ||
| Assert.Empty(await reloadedService.GetUploadHistoryAsync()); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add coverage for partial removal and no-op edge cases.
Current tests only cover "remove the only item" and "clear all items." Consider adding a case verifying that removing one URL leaves other unrelated history entries intact, and that calling RemoveHistoryItemAsync/ClearHistoryAsync on an empty/non-matching history doesn't throw.
✅ Example additional test
[Fact]
public async Task RemoveHistoryItemAsync_WhenOtherItemsExist_LeavesThemIntact()
{
var service = CreateService();
service.RecordUpload(1024, "https://utfs.io/f/first", "first.zip");
service.RecordUpload(2048, "https://utfs.io/f/second", "second.zip");
await service.RemoveHistoryItemAsync("https://utfs.io/f/first");
var reloadedService = CreateService();
var remaining = await reloadedService.GetUploadHistoryAsync();
Assert.Single(remaining);
}🤖 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
`@GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Tools/Services/UploadHistoryServiceTests.cs`
around lines 42 - 68, Extend the UploadHistoryService tests around
RemoveHistoryItemAsync and ClearHistoryAsync to cover partial removal and no-op
behavior. Add a case proving removing one URL preserves unrelated history
entries, plus cases confirming removing a non-matching URL and clearing an empty
history complete without throwing and leave history empty.
1 participant
Summary
Remove the legacy UploadThing credential flow and disable authenticated uploads and deletion. Existing public-link imports remain available.
Changes
Testing
git diff --check origin/developmentdocs/features/manifest.md:497Risks and rollback
Cloud upload and deletion remain unavailable until secure authorization is implemented. Public-link imports are unaffected.
Related issues
Closes #298
Related: #233, #238
Greptile Summary
This follow-up completes the secure removal of the legacy UploadThing credential flow.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Reviews (2): Last reviewed commit: "fix(tools): migrate upload history and r..." | Re-trigger Greptile