Add Base64 encoding support for profile content and improve edge-to-edge mobile UI#106
Conversation
Server: add base64 profile decoding utilities and errors, extend ProfileRequest with content_b64 and encoding, and use decoded content when saving profiles and creating history; add unit tests for decodeProfileContent. Web: add UTF-8 → base64 encoder and payload builder, switch profiles API to send base64 payloads, and add tests for encoding/building payloads; add an error rule to surface 403s when saving profiles. Mobile: bump Capacitor deps to v8 and iOS platform to 15.0, add Android edge-to-edge support (WindowCompat.setDecorFitsSystemWindows + transparent system bars and updated styles), remove deprecated overlaysWebView/backgroundColor calls, update main.tsx comments, and add a placeholder Android test file.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds end-to-end base64 profile update support (frontend encode, backend decode, persist, tests, and error mapping) and mobile changes for edge-to-edge rendering, Capacitor 8.x upgrades, Android/iOS SDK/target bumps, and a Kotlin resolution rule. ChangesProfile Content Base64 Encoding
Mobile Edge-to-Edge Rendering and Dependency Updates
Sequence Diagram(s)sequenceDiagram
participant UI as Web UI
participant API as profilesAPI
participant Server as CrowdSec Manager (UpdateProfiles)
participant Decoder as decodeProfileContent
participant Storage as DB+FS (history + profiles.yaml)
UI->>API: buildProfileUpdatePayload(content) (content_b64, encoding=base64, restart)
API->>Server: POST /profiles (payload)
Server->>Decoder: decodeProfileContent(req)
Decoder-->>Server: decoded YAML or error
alt decoded
Server->>Storage: save profile history (decoded)
Server->>Storage: write profiles.yaml (decoded)
Storage-->>Server: success
Server-->>API: 200 OK
else decode error
Server-->>API: 400 BadRequest (error)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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. ✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65a0ef36be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| require_relative '../../node_modules/@capacitor/ios/scripts/pods_helpers' | ||
|
|
||
| platform :ios, '14.0' | ||
| platform :ios, '15.0' |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@internal/api/handlers/profiles_test.go`:
- Around line 11-88: Update the TestDecodeProfileContent test to (1) replace the
strict comparison to base64.CorruptInputError(3) with an errors.As check for a
base64.CorruptInputError when the case name is "invalid base64" (use
decodeProfileContent's returned error and assert the error type), and (2) remove
the redundant loop-variable shadowing line `tt := tt` inside the t.Run closure;
keep existing errors.Is checks for sentinel errors like errProfileContentMissing
and errProfileEncodingUnsupported unchanged.
In
`@mobile/android/app/src/main/java/com/crowdsec/manager/mobile/MainActivity.java`:
- Around line 10-11: In MainActivity's onCreate method, swap the calls so that
super.onCreate(savedInstanceState) is invoked before
WindowCompat.setDecorFitsSystemWindows(getWindow(), false); update the onCreate
sequence to call super.onCreate(...) first, then call
WindowCompat.setDecorFitsSystemWindows(...) to follow Android
lifecycle/edge-to-edge guidance.
In `@mobile/android/build.gradle`:
- Around line 26-32: The pinned Kotlin version in the configurations block
(resolutionStrategy.eachDependency with DependencyResolveDetails and
details.requested.group check) is set via details.useVersion "1.8.22" and needs
updating to a Capacitor-8-compatible version; change the useVersion string from
"1.8.22" to at least "2.2.20" (preferably "2.3.20" or latest stable), keeping
the same conditional that matches org.jetbrains.kotlin so the dependency
resolution enforces the newer Kotlin version.
In `@mobile/android/MainActivity.test.java`:
- Around line 1-4: The placeholder MainActivity.test.java file is a
non-compiling Java file left outside the Gradle source sets; replace it with
either a documentation file or actual test scaffolding: remove
MainActivity.test.java and add a TESTING.md or TODO.md in the mobile/android/
directory describing test intent and next steps, or create proper test
scaffolding under app/src/test/java/com/crowdsec/manager/mobile/ with a minimal
JUnit test class for MainActivity (or BridgeActivity bootstrap behavior) so IDEs
and CI won’t be confused by a stray .java file; ensure any new test class uses
the correct package and imports so it compiles.
In `@mobile/package.json`:
- Around line 24-29: Update the platform/version settings required by the
Capacitor 8 migration: in the Xcode project (.pbxproj) replace all occurrences
of IPHONEOS_DEPLOYMENT_TARGET set to 14.0 with 15.0; in Android's
variables.gradle bump minSdkVersion from 23 to 24 and
compileSdkVersion/targetSdkVersion from 35 to 36 (look for the minSdkVersion,
compileSdkVersion, targetSdkVersion symbols); and in AndroidManifest.xml add
"density" to the android:configChanges attribute for the relevant <activity>
entries to include this breaking change. Ensure each change is applied
consistently across all occurrences.
🪄 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: b4026927-54c1-41af-84dc-8fed3befb7d9
⛔ Files ignored due to path filters (2)
mobile/package-lock.jsonis excluded by!**/package-lock.jsonweb/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
internal/api/handlers/profiles.gointernal/api/handlers/profiles_test.gointernal/models/models.gomobile/android/MainActivity.test.javamobile/android/app/src/main/java/com/crowdsec/manager/mobile/MainActivity.javamobile/android/app/src/main/res/values/styles.xmlmobile/android/build.gradlemobile/capacitor.config.tsmobile/ios/App/Podfilemobile/package.jsonmobile/src/main.tsxweb/package.jsonweb/src/lib/api/errors.tsweb/src/lib/api/profiles.test.tsweb/src/lib/api/profiles.ts
💤 Files with no reviewable changes (1)
- mobile/capacitor.config.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-and-push
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
🪛 golangci-lint (2.12.1)
internal/api/handlers/profiles_test.go
[error] 69-69: The copy of the 'for' variable "tt" can be deleted (Go 1.22+)
(copyloopvar)
🔇 Additional comments (13)
mobile/ios/App/Podfile (1)
3-3: No issues found. iOS 15.0 is the minimum required version for Capacitor 8.x, so the deployment target change is appropriate and necessary.web/package.json (1)
3-3: LGTM: Version bump aligns with mobile package.The version increment from 2.4.0 to 2.4.1 is consistent with the mobile package version bump in this PR.
mobile/android/app/src/main/res/values/styles.xml (1)
16-23: LGTM: Excellent documentation of edge-to-edge approach.The comment clearly explains the edge-to-edge setup, the rationale for removing deprecated
windowTranslucentStatus, and how the pieces (MainActivity, WebView CSS, system bars) work together. This documentation will be valuable for future maintainers.mobile/src/main.tsx (1)
14-24: LGTM: Simplified StatusBar configuration aligns with native edge-to-edge setup.The removal of deprecated
setOverlaysWebViewandsetBackgroundColorcalls is appropriate given that edge-to-edge layout is now handled natively (viaWindowCompat.setDecorFitsSystemWindowsin MainActivity and transparent system bars in styles.xml). The inline comments clearly document this intentional simplification.internal/models/models.go (1)
437-441: LGTM! Well-designed backward-compatible extension.The addition of
ContentB64alongside the existingContentfield enables a smooth migration path: legacy clients can continue using plaincontent, while new clients can send base64-encoded content viacontent_b64with theencodingfield. This design maintains full backward compatibility.internal/api/handlers/profiles.go (3)
49-52: LGTM! Clear and idiomatic error declarations.Using package-level
errors.New()for reusable sentinel errors is a Go best practice. These errors can be checked witherrors.Isand provide clear failure reasons.
59-86: LGTM! Well-structured decoding logic with excellent backward compatibility.The decoding strategy is well-designed:
- Backward compatible: When
encodingis empty,Content(legacy field) takes priority, then falls back to decodingContentB64.- Explicit base64 mode: When
encodingis"base64", onlyContentB64is used, enforcing the new contract.- Clear error handling: Missing content and unsupported encodings return distinct, actionable errors.
The helper
decodeProfileContentBase64properly wraps base64 decode errors with%wfor error chain inspection.
208-215: LGTM! Correct integration of content decoding.Decoding is performed early before persistence, ensuring:
- Invalid payloads are rejected with
400 Bad Requestbefore any side effects.- Decoded content is validated by the subsequent write operations.
- Error messages from decoding (including base64 corruption) bubble up to the client.
web/src/lib/api/errors.ts (1)
60-64: LGTM! Helpful error mapping for profile saves.This rule provides clear guidance when profile saves are blocked by Traefik or AppSec before reaching the backend. The patterns comprehensively match various 403 representations, and the message directs users to the right diagnostic logs.
web/src/lib/api/profiles.test.ts (1)
1-26: LGTM! Solid test coverage for encoding and payload construction.The tests verify:
- UTF-8 correctness: Uses a Unicode marker
"✓"to ensure proper UTF-8 handling viaTextEncoder.- Encoding output: Matches against a reference
utf8Base64implementation usingbtoa.- Payload structure: Confirms
buildProfileUpdatePayloadproduces the exact shape expected by the backend (content_b64,encoding: 'base64',restart).The helper
utf8Base64provides a clear reference implementation for validation.web/src/lib/api/profiles.ts (3)
4-8: LGTM! Precise type definition with literal encoding.The
encoding: 'base64'literal type (rather thanstring) provides compile-time enforcement that only base64 encoding is used. This matches the backend's decoding expectations ininternal/api/handlers/profiles.go.
10-20: LGTM! Correct UTF-8 to base64 encoding with stack-overflow prevention.The implementation correctly:
- Encodes the string to UTF-8 bytes via
TextEncoder- Converts bytes to a binary string in chunks (avoiding "Maximum call stack size exceeded" with large content)
- Base64-encodes the binary string with
btoaThe chunk size of
0x8000(32768 bytes) is a reasonable balance between performance and stack safety. The backend'sbase64.StdEncoding.DecodeString()followed bystring(decoded)correctly reverses this transformation.
22-28: LGTM! Clean payload builder.
buildProfileUpdatePayloadencapsulates the encoding step and ensures the payload structure matchesmodels.ProfileRequeston the backend. This separation of concerns makes the API call inprofilesAPI.updatecleaner and easier to test.
| // Placeholder for the project's TDD-gate hook. Sits outside any Gradle source | ||
| // set so it is not compiled. The real test for MainActivity belongs in | ||
| // app/src/test/java/com/crowdsec/manager/mobile/ — add when the activity grows | ||
| // non-trivial behavior beyond the BridgeActivity edge-to-edge bootstrap. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider replacing this placeholder with a documentation file or actual test scaffolding.
A non-compiling .java file outside the source set is an unconventional way to track testing intent. Consider alternatives:
- A
TESTING.mdorTODO.mdin themobile/android/directory - Actual test scaffolding under the correct test path
- Removing the placeholder if tests aren't immediately needed
The current approach may confuse developers or trigger IDE warnings.
🤖 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 `@mobile/android/MainActivity.test.java` around lines 1 - 4, The placeholder
MainActivity.test.java file is a non-compiling Java file left outside the Gradle
source sets; replace it with either a documentation file or actual test
scaffolding: remove MainActivity.test.java and add a TESTING.md or TODO.md in
the mobile/android/ directory describing test intent and next steps, or create
proper test scaffolding under app/src/test/java/com/crowdsec/manager/mobile/
with a minimal JUnit test class for MainActivity (or BridgeActivity bootstrap
behavior) so IDEs and CI won’t be confused by a stray .java file; ensure any new
test class uses the correct package and imports so it compiles.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements base64 encoding for profile updates to bypass potential WAF/Bouncer blocks and upgrades the mobile application to Capacitor 8. It also adds edge-to-edge display support for Android and updates the web UI to handle 403 errors during profile saves. The review feedback highlights a risk in hardcoding the Kotlin version in the Android build, suggests reducing the chunk size for base64 encoding to prevent potential stack overflow issues, and recommends generalizing the new error message for profile save failures.
| configurations.all { | ||
| resolutionStrategy.eachDependency { DependencyResolveDetails details -> | ||
| if (details.requested.group == 'org.jetbrains.kotlin') { | ||
| details.useVersion "1.8.22" | ||
| } | ||
| } | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| export function encodeProfileContent(content: string): string { | ||
| const bytes = new TextEncoder().encode(content) | ||
| let binary = '' | ||
| const chunkSize = 0x8000 |
There was a problem hiding this comment.
The chunk size of 0x8000 (32,768) for the spread operator in String.fromCharCode(...chunk) might still exceed the stack size limit in some JavaScript environments (e.g., older mobile browsers or specific WebView versions), leading to a RangeError. A safer and more conservative chunk size would be 8192 or 16384 to ensure compatibility across a wider range of devices.
| const chunkSize = 0x8000 | |
| const chunkSize = 0x4000 |
| { | ||
| contexts: [ErrorContexts.ProfilesSave], | ||
| patterns: [/status code 403/i, /\b403\b/i, /forbidden/i], | ||
| message: 'Profile save was blocked before it reached CrowdSec Manager. Check Traefik/CrowdSec AppSec logs for a bouncer or WAF block.', |
There was a problem hiding this comment.
The error message is very specific to a deployment using Traefik and CrowdSec AppSec. While helpful for that specific stack, it might be misleading for users running different reverse proxies or WAFs. Consider making the message slightly more generic while still providing examples.
| message: 'Profile save was blocked before it reached CrowdSec Manager. Check Traefik/CrowdSec AppSec logs for a bouncer or WAF block.', | |
| message: 'Profile save was blocked before it reached CrowdSec Manager. Check your reverse proxy or WAF logs (e.g., Traefik/CrowdSec AppSec) for a bouncer or WAF block.', |
hhftechnology
changed the title
Upgrade mobile platform versions and adjust related Android/iOS settings; improve Go test handling for base64 errors. - internal/api/handlers/profiles_test.go: Add wantBase64Err flag and assert base64.CorruptInputError via errors.As; adjust test flow for parallel subtests. - mobile/android/app/src/main/AndroidManifest.xml: Add "density" to activity android:configChanges. - mobile/android/app/src/main/java/com/crowdsec/manager/mobile/MainActivity.java: Move WindowCompat.setDecorFitsSystemWindows call to after super.onCreate. - mobile/android/build.gradle: Change Kotlin resolution version from 1.8.22 to 2.3.21. - mobile/android/variables.gradle: Bump minSdkVersion 23->24 and compile/target SDK 35->36. - mobile/ios/App/App.xcodeproj/project.pbxproj: Raise IPHONEOS_DEPLOYMENT_TARGET from 14.0 to 15.0 in multiple build settings. These changes prepare the mobile apps for newer platform SDKs and make the profile decoding tests properly detect base64 corruption errors.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@mobile/android/app/src/main/AndroidManifest.xml`:
- Line 13: The manifest currently opts BridgeActivity/MainActivity into handling
android:configChanges including density and navigation, which prevents Activity
recreation and can leave UI/resources stale; remove the density and navigation
tokens from the android:configChanges attribute in AndroidManifest.xml unless
you implement explicit handling, or alternatively implement
onConfigurationChanged in BridgeActivity/MainActivity to properly refresh
resources/UI (e.g., reload density-dependent assets, reapply layouts/styles and
handle navigation mode changes) before keeping those flags.
🪄 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: ba917a6a-3223-4c09-94b7-4a407d8443ed
📒 Files selected for processing (6)
internal/api/handlers/profiles_test.gomobile/android/app/src/main/AndroidManifest.xmlmobile/android/app/src/main/java/com/crowdsec/manager/mobile/MainActivity.javamobile/android/build.gradlemobile/android/variables.gradlemobile/ios/App/App.xcodeproj/project.pbxproj
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-and-push
- GitHub Check: Analyze (go)
- GitHub Check: build-and-push
🔇 Additional comments (6)
mobile/android/build.gradle (1)
26-32: Kotlin override update looks correct.This resolves the earlier outdated Kotlin pin and is consistent with the Capacitor 8 migration intent.
mobile/android/variables.gradle (1)
2-4: SDK level bump is clean and internally consistent.
minSdkVersion,compileSdkVersion, andtargetSdkVersionupdates are coherent for the Android platform uplift.mobile/android/app/src/main/java/com/crowdsec/manager/mobile/MainActivity.java (1)
4-4: Edge-to-edge initialization is correctly ordered.
super.onCreate(...)runs first, thenWindowCompat.setDecorFitsSystemWindows(getWindow(), false), which is the right lifecycle order.Also applies to: 11-11
internal/api/handlers/profiles_test.go (1)
11-95: Excellent test implementation! Previous feedback has been addressed.The test suite is comprehensive and well-structured:
- ✅ Uses
errors.Asto checkbase64.CorruptInputErrortype (lines 75-76), making the test robust to input variations- ✅ No unnecessary loop variable shadowing (the previous
tt := tthas been removed)- ✅ Covers all decoding paths: legacy plain content, explicit base64, implicit base64, invalid base64, missing content, and unsupported encoding
- ✅ Appropriate use of
errors.Isfor sentinel errors anderrors.Asfor type-based errors- ✅ Parallel execution enabled for better performance
The separation of
wantBase64ErrfromwantErris a clean pattern that distinguishes type-based error assertions from sentinel error checks.mobile/ios/App/App.xcodeproj/project.pbxproj (2)
353-353: LGTM: Target-level deployment target is consistent with project settings.The App target's
IPHONEOS_DEPLOYMENT_TARGETis correctly synchronized with the project-level settings, ensuring consistent deployment target across all build configurations.Also applies to: 373-373
286-286: Capacitor 8.x requires iOS 15.0 — this deployment target change is necessary and correct.The project uses Capacitor 8.3.3, which requires a minimum iOS deployment target of iOS 15.0. This change aligns with the Capacitor 8.0 upgrade guide. The increase from iOS 14.0 to 15.0 is required for compatibility with the current Capacitor version.
Note: This will prevent iOS 14.x users from installing or updating the app, which is an expected consequence of this upgrade.
| android:theme="@style/AppTheme"> | ||
| <activity | ||
| android:configChanges="orientation|keyboardHidden|keyboard|screenSize|locale|smallestScreenSize|screenLayout|uiMode|navigation" | ||
| android:configChanges="orientation|keyboardHidden|keyboard|screenSize|locale|smallestScreenSize|screenLayout|uiMode|navigation|density" |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Change getProfilesPath to construct profiles.yaml under ConfigDir + CrowdSecConfigSubdir (use constants) instead of deriving from acquis.yaml location. Update error wrapping in createDefaultProfilesYaml to use %w. Add tests (profiles_path_test.go) verifying the new path selection and that UpdateProfiles writes to the crowdsec config directory and records history.
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 `@internal/api/handlers/profiles_path_test.go`:
- Around line 33-90: Add an integration test in profiles_path_test.go to
exercise Base64 input through the HTTP handler: create a test (e.g.,
TestUpdateProfilesHandlesBase64Content) that builds a config/db like
TestUpdateProfilesWritesToCrowdSecConfigDir, prepares a models.ProfileRequest
with ContentB64 set to the base64-encoded profile string and Encoding set to
"base64", marshal it to JSON, send it via an httptest PUT to the UpdateProfiles
handler (the same r.PUT("/profiles", UpdateProfiles(db, cfg, &docker.Client{}))
setup), then assert the written file at filepath.Join(configDir, "crowdsec",
"profiles.yaml") contains the decoded content and that
db.GetLatestProfileHistory() returns the decoded content; mirror the response
and file/DB assertions from the existing
TestUpdateProfilesWritesToCrowdSecConfigDir.
In `@internal/api/handlers/profiles.go`:
- Around line 59-78: In decodeProfileContent, after calling
decodeProfileContentBase64 or returning req.Content, validate the decoded string
is not empty or whitespace-only: trim the result (strings.TrimSpace) and if it
is empty return errProfileContentMissing; apply this check for both the ""-case
path that returns req.Content and the "base64" case that returns
decodeProfileContentBase64 so neither decodeProfileContentBase64 nor direct
content can produce an empty payload.
- Around line 59-78: In decodeProfileContent, add explicit validation to reject
requests where both req.Content and req.ContentB64 are non-empty: at the top of
the function check if both fields are set and return a clear error (e.g.,
errProfileContentAmbiguous or fmt.Errorf("%w", ...)) instead of silently
preferring one; then keep the existing switch behavior for the single-field
cases and the base64 decoding via decodeProfileContentBase64. Ensure the new
error is used when both fields are provided in any encoding branch so clients
get an explicit rejection rather than ambiguous selection.
- Around line 208-215: The handler currently returns err.Error() directly to
clients when decodeProfileContent(req) fails; change this to send a sanitized,
user-friendly message instead. In the error branch after calling
decodeProfileContent, replace the raw err.Error() in the models.Response payload
with a generic message (e.g., "invalid profile content" or "bad request") and
log the original err locally using the request/context logger so internal
details remain recorded; update the c.JSON(models.Response{Success: false,
Error: ...}) accordingly and ensure decodeProfileContent errors are still logged
(not returned) for debugging.
🪄 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: 7dcd1182-8f04-4882-b567-5f33f6da95fa
📒 Files selected for processing (2)
internal/api/handlers/profiles.gointernal/api/handlers/profiles_path_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-and-push
- GitHub Check: build-and-push
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
internal/api/handlers/profiles.go (6)
4-5: LGTM! Import additions support Base64 decoding.The new imports (
encoding/base64anderrors) correctly support the Base64 decoding functionality added in this PR.Also applies to: 12-17
50-52: LGTM! Path resolution now uses ConfigDir.The change to resolve
profiles.yamlfromcfg.ConfigDir + constants.CrowdSecConfigSubdiris cleaner and aligns with the test validation inprofiles_path_test.go.
54-57: LGTM! Well-defined sentinel errors.The sentinel errors provide clear, typed error handling for content validation and unsupported encoding scenarios.
80-86: LGTM! Clean Base64 decoding implementation.The function correctly uses
base64.StdEncodingand provides clear error wrapping for decode failures.
112-112: LGTM! Improved error wrapping with %w.The change from
%vto%wenables proper error chain unwrapping witherrors.Isanderrors.As, which is a best practice improvement.Also applies to: 117-117
230-230: LGTM! Decoded content used consistently.The decoded content is correctly used for both profile history persistence (line 230) and file writing (line 241), ensuring the Base64-decoded or plain text content is stored properly.
Also applies to: 241-241
internal/api/handlers/profiles_path_test.go (2)
1-31: LGTM! Clean test validating path resolution.The test correctly validates that
getProfilesPathnow resolves toConfigDir/crowdsec/profiles.yaml, with good use oft.Parallel()and cross-platform path construction.
33-90: LGTM! Comprehensive integration test for path behavior.The test thoroughly validates that
UpdateProfiles:
- Writes to the correct
ConfigDir/crowdsec/profiles.yamlpath- Does not create files at the legacy acquis-based path
- Records the content in profile history
The test structure is clean with proper cleanup and good use of temporary directories.
Refactor decodeProfileContent to centralize decoding logic for raw and base64 inputs, ensure decoded content is non-empty, and return a clear error when profile content is missing or invalid. decodeProfileContentBase64 errors are propagated and decodeProfileContent now returns a unified decoded string. Update UpdateProfiles to log decoding failures and return a generic "invalid profile content" error message to clients (avoids exposing internal error details). Add TestUpdateProfilesHandlesBase64Content to verify base64-encoded profiles are accepted, written to the config dir, and recorded in the profile history database. Also add the necessary base64 import in the tests.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests