Update component exported attribute #2930
Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant modifications to the Android application's manifest and string resources, focusing on enhancing permission management and activity/receiver configurations. The changes involve creating a new permission group called Multiple activities and receivers, including Sequence DiagramsequenceDiagram
participant App
participant AndroidSystem
participant Activity
participant Receiver
App->>AndroidSystem: Request Permission
AndroidSystem-->>App: Permission Check
alt Permission Granted
App->>Activity: Launch DrawingBoundaryActivity
Activity-->>App: Perform Boundary Drawing
App->>Activity: Launch NfcWriteActivity
Activity-->>App: Write NFC Data
App->>Activity: Launch NfcReadActivity
Activity-->>App: Read NFC Data
App->>Receiver: Trigger RefreshToLatestBuildReceiver
Receiver-->>App: Update Application
else Permission Denied
AndroidSystem-->>App: Access Restricted
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
app/AndroidManifest.xml (3)
Input Validation Present but Documentation Needed for Template Format
Based on the verification results:
- Input validation exists in
TemplatePrinterTask.javathrough thevalidateString()method which checks for well-formed{{ }}and< >pairs.- Security measures are implemented with encryption for file operations in
TemplatePrinterUtils.java.- However, there's no user-facing documentation about the expected template format and intent structure.
Required actions:
- Add documentation in the codebase (e.g., README.md) describing:
- The expected intent format for
org.commcare.dalvik.action.PRINT- Template format requirements (e.g.,
{{ }}for variables,< >for sections)- Supported template styles (
TEMPLATE_HTML,TEMPLATE_ZPL)The existing validation and security measures appear sufficient, so rate limiting isn't necessary for this use case.
🔗 Analysis chain
Line range hint
506-512: Document Printer Integration RequirementsSince TemplatePrinterActivity is now exposed, please:
- Document the expected intent format
- Add input validation for template data
- Consider rate limiting if necessary
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing documentation fd README.md | xargs rg -l "TemplatePrinter|Print.*Activity" # Check for input validation ast-grep --pattern 'validateTemplate($_)' -l javaLength of output: 114
Script:
#!/bin/bash # Search for the TemplatePrinterActivity class implementation ast-grep --pattern 'class TemplatePrinterActivity' -l # Look for any input handling in printer-related activities rg -l "getIntent|getExtra.*" --type java | xargs rg "class.*Print.*Activity" -A 5 # Search for any existing validation or security checks in printer activities rg -l "Print.*Activity" --type java | xargs rg "validate|check|security" -A 3 # Look for any documentation in comments rg -l "Print.*Activity" --type java | xargs rg "/\*\*|//.*print" -B 1 -A 3Length of output: 20133
Based on the evidence gathered from the codebase, I can now generate the final response:
Security concerns confirmed for RefreshToLatestBuildReceiver
The exposed broadcast receiver lacks proper security controls:
- No custom permission requirement in the manifest unlike other receivers (e.g., CommCareLogoutReceiver uses
org.commcare.dalvik.permission.COMMCARE_LOGOUT)- No source validation or build verification in the implementation
- Only basic checks for app existence and developer preferences
- Directly launches update activity with
FLAG_ACTIVITY_NEW_TASKwithout user confirmationRecommendations:
- Add a custom permission requirement in AndroidManifest.xml
- Implement source package validation in onReceive()
- Add build signature verification before proceeding with the update
- Consider requiring user confirmation before initiating the update process
🔗 Analysis chain
Line range hint
441-447: Verify Build Refresh SecurityThe RefreshToLatestBuildReceiver being exposed could potentially be used to force app updates. Ensure:
- Proper verification of the source application
- Validation of the build being installed
- User consent mechanisms
Let's search for the receiver implementation and related security measures to verify how the build refresh functionality is secured.
From the initial results, we can see the receiver implementation, but we need to understand the security controls around it, particularly through developer preferences and permissions.
From the previous results, we see the receiver launches RefreshToLatestBuildActivity and uses developer preferences. Let's check the activity implementation and any security validations in the update process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for build verification rg -A 10 "verifyBuild|validateBuild" # Check for user consent implementation rg -A 5 "showUpdatePrompt|confirmUpdate"Length of output: 81
Script:
#!/bin/bash # Search for the RefreshToLatestBuildReceiver implementation rg -A 20 "class.*RefreshToLatestBuildReceiver" # Search for build-related security checks rg -A 10 "checkPermission|validateSource|validateIntent" # Look for update-related code rg -A 10 "updateTo|installBuild|processBuild" # Search for the action constant usage rg "org.commcare.dalvik.api.action.RefreshToLatestBuildAction"Length of output: 10222
Script:
#!/bin/bash # Let's check for any security-related code in the RefreshToLatestBuildReceiver rg -A 30 "DeveloperPreferences|MainConfigurablePreferences" app/src/org/commcare/provider/RefreshToLatestBuildReceiver.java # Check for any permission declarations related to this receiver rg -B 5 -A 5 "org.commcare.dalvik.permission" app/AndroidManifest.xml # Look for any security-related preferences rg -A 5 "DeveloperPreferences.*enable|UPDATE_TARGET" app/src/org/commcare/Length of output: 13832
Script:
#!/bin/bash # Check for the RefreshToLatestBuildActivity implementation rg -A 20 "class.*RefreshToLatestBuildActivity" # Look for any update validation or security checks in the update process rg -A 10 "validateUpdate|verifyUpdate|checkUpdate" app/src/org/commcare/ # Check for any developer mode or privilege checks rg -A 5 "isDeveloperModeEnabled|requiresDeveloperPrivilege" app/src/org/commcare/Length of output: 12142
Based on the gathered information, I can now provide a final response:
Additional NFC Security Controls Required
The NFC implementation shows basic structure but lacks comprehensive security controls:
- The base
NfcActivityclass has encryption key support (NFC_ENCRYPTION_KEY_ARG) but implementation details are not visible- Error handling exists but appears minimal (
finishWithErrorToast)- No clear data validation mechanisms are present in the NFC read/write activities
Required security improvements:
- Implement robust input validation in
NfcWriteActivitybefore writing NDEF messages- Add data sanitization in
NfcReadActivity.onNewIntentfor incoming NFC data- Enhance error handling beyond basic toast messages
- Document the encryption implementation if present, or add if missing
🔗 Analysis chain
Line range hint
402-408: Verify NFC Security ImplementationThe NFC activities being exposed need additional security considerations:
- Ensure proper validation of NFC data
- Consider encryption for sensitive data
- Implement proper error handling for malformed inputs
Also applies to: 411-417
Let me gather more specific information about the NFC implementation.
Let me check one more aspect of the NFC implementation to see how the data is handled.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for NFC security implementations echo "Checking for NFC data validation..." rg -A 5 "getNfcAdapter|createNdefMessage" # Look for encryption usage rg -l "encrypt.*NFC|decrypt.*NFC"Length of output: 276
Script:
#!/bin/bash # Check NFC activity implementations echo "Checking NFC activity implementations..." rg -A 10 "class.*NfcWriteActivity|class.*NfcReadActivity" # Check for data validation and error handling in NFC related files echo "Checking NFC data validation and error handling..." fd -e java -e kt --exec rg -l "try.*catch.*NFC|validate.*NFC|isValid|checkNfc" {} \; # Look for specific NFC security measures echo "Checking NFC security implementations..." ast-grep --pattern 'try { $$$ getNfcAdapter() $$$ } catch ($$$) { $$$ }'Length of output: 2873
Script:
#!/bin/bash # Check NFC base class and data handling echo "Checking NFC base class implementation..." rg -A 15 "class NfcActivity" # Check for NDEF message handling and validation echo "Checking NDEF message handling..." rg -A 10 "handleNdefMessage|onNewIntent|createNdefMessage" # Look for error handling in NFC activities echo "Checking error handling..." rg -A 5 "catch.*Exception.*NFC|onNfcError|handleError" app/src/org/commcare/android/nfc/Length of output: 3907
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/AndroidManifest.xml(5 hunks)app/res/layout/progress_dialog_cancel_button.xml(1 hunks)
🔇 Additional comments (2)
app/res/layout/progress_dialog_cancel_button.xml (1)
17-17: Verify the UX impact of hiding the cancel button
Changing the button's visibility to "gone" removes the ability for users to cancel operations. Please ensure this won't negatively impact user experience, especially in scenarios where operations might take longer than expected.
app/AndroidManifest.xml (1)
Line range hint 349-355: Verify Drawing Boundary Integration
For DrawingBoundaryActivity:
- Ensure proper validation of boundary data
- Implement size limits for boundary data
- Add proper error handling for invalid inputs
✅ Verification successful
Let's search for the actual implementation and boundary-related code to better understand the validation mechanisms in place.
Let's examine the actual implementation of DrawingBoundaryActivity to understand the current validation and error handling mechanisms.
Boundary validation and limits are partially implemented
The code review reveals that DrawingBoundaryActivity already implements several validation mechanisms:
-
Location accuracy validation:
- Enforces minimum accuracy between 10-50 meters (
LOCATION_MIN_MIN_ACCURACYandLOCATION_MIN_MAX_ACCURACY) - Validates location accuracy against
locationMinAccuracythreshold
- Enforces minimum accuracy between 10-50 meters (
-
Boundary data validation:
- Validates coordinates parsing with error handling
- Implements distance and time interval checks for tracking points
- Includes error handling for invalid boundary coordinates
-
Size/Area handling:
- Calculates and validates area using
AreaCalculator - Provides perimeter calculations
- Includes proper error handling for geometry calculations
- Calculates and validates area using
However, there's no explicit maximum size limit for the boundary area, which could be a potential improvement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for boundary validation
rg -A 5 "validateBoundary|checkBounds"
# Look for size limit implementations
ast-grep --pattern 'getMaxSize|getLimitSize' -l java
Length of output: 95
Script:
#!/bin/bash
# Search for DrawingBoundaryActivity implementation
rg -A 10 "class DrawingBoundaryActivity"
# Search for boundary-related methods in the codebase
rg -A 5 "boundary|Boundary" --type java
# Look for input validation patterns
ast-grep --pattern 'if ($boundary $_) {
$$$
}'
Length of output: 1759
Script:
#!/bin/bash
# Search for content of DrawingBoundaryActivity.kt
cat app/src/org/commcare/gis/DrawingBoundaryActivity.kt
# Look for any validation or error handling patterns in the GIS module
rg -A 5 "throw|error|exception|validate" app/src/org/commcare/gis/
Length of output: 14625
| <activity | ||
| android:name="org.commcare.gis.DrawingBoundaryActivity" | ||
| android:exported="false"> | ||
| android:exported="true"> |
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered information, I can now provide the final response:
Security Review Completed: Additional Protection Required
The exposed components require additional security measures:
- NFC activities (NfcReadActivity, NfcWriteActivity) handle sensitive data but lack custom permission protection. Add a custom permission requirement similar to
CommCareLogoutReceiver. - DrawingBoundaryActivity and TemplatePrinterActivity are exposed without permission checks. Implement custom permissions or
checkCallingOrSelfPermission(). - RefreshToLatestBuildReceiver lacks security validation before processing broadcasts. Add signature/system permission protection.
Required changes:
- Add custom permission declarations in AndroidManifest.xml for each component
- Add
android:permissionattribute to each component's manifest entry - Implement runtime permission validation in RefreshToLatestBuildReceiver's onReceive()
🔗 Analysis chain
Security Review Required: Components exposed to external applications
Multiple components are being made accessible to external applications. This change requires careful security review:
- Ensure each component has proper permission checks
- Validate input from external applications
- Consider adding custom permissions if needed
- Document the security implications for future maintenance
Also applies to: 402-402, 411-411, 441-441, 506-506
Let me gather more information about security checks in these components.
Let me gather one final piece of information about the base NFC activity class since both NFC activities inherit from it.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for permission checks in these components
echo "Checking DrawingBoundaryActivity for permission checks..."
ast-grep --pattern 'checkCallingOrSelfPermission($_)' -l java
echo "Checking for input validation in NFC activities..."
ast-grep --pattern 'getIntent()' -l java
echo "Checking RefreshToLatestBuildReceiver for security checks..."
rg -A 10 "class RefreshToLatestBuildReceiver"
Length of output: 15095
Script:
#!/bin/bash
# Check for permission declarations in manifest
echo "Checking permission declarations in AndroidManifest.xml..."
rg -A 2 "permission" app/AndroidManifest.xml
# Check for security checks in the activities
echo "Checking NfcWriteActivity for security measures..."
rg -A 10 "class NfcWriteActivity" app/src/org/commcare/android/nfc/
echo "Checking NfcReadActivity for security measures..."
rg -A 10 "class NfcReadActivity" app/src/org/commcare/android/nfc/
echo "Checking DrawingBoundaryActivity for security measures..."
rg -A 10 "class DrawingBoundaryActivity"
echo "Checking TemplatePrinterActivity for security measures..."
rg -A 10 "class TemplatePrinterActivity"
# Check for intent filters
echo "Checking intent filters for these components..."
rg -A 5 "<intent-filter" app/AndroidManifest.xml
Length of output: 15036
Script:
#!/bin/bash
# Check NfcActivity base class for security measures
echo "Checking NfcActivity base class security..."
rg -A 20 "class NfcActivity" app/src/org/commcare/android/nfc/
# Check for custom permissions usage
echo "Checking for custom permission usage..."
rg -B 2 -A 2 "android:permission=" app/AndroidManifest.xml
Length of output: 2656
avazirna
changed the title
|
@avazirna can you link the Android docs reg. restriction you are talking about in PR description. |
done @shubham1g5, we might need to consider having an option to allow app callout questions to use explicit intents. |
|
@avazirna I think we need to implement a permission at the very least for these exported components as we don't really want to allow other apps to have access to these. |
|
@coderabbitai review |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
app/res/values/strings.xml (1)
Line range hint
390-391: Consider adding runtime permission checks.While the manifest-level permissions are properly set up, consider adding runtime permission validation in these components for an additional layer of security. This is particularly important for components that handle sensitive operations like NFC data and system updates.
Example for NFC activities:
+ // Add to NfcReadActivity/NfcWriteActivity + @Override + protected void onCreate(Bundle savedInstanceState) { + super.onCreate(savedInstanceState); + if (checkCallingOrSelfPermission(getPackageName() + ".permission.READ_NFC_DATA") + != PackageManager.PERMISSION_GRANTED) { + throw new SecurityException("Caller must have permission " + + getPackageName() + ".permission.READ_NFC_DATA"); + } + // ... rest of the onCreate + }Also applies to: 444-446, 454-456, 475-476, 485-486, 551-552
🧹 Nitpick comments (1)
app/AndroidManifest.xml (1)
95-129: Consider usingandroid:protectionLevel="signature|privileged"for internal permissions.The new permissions are well-structured, but for internal actions that are signature-protected, consider using
signature|privilegedto also allow system apps with the appropriate permission to access these components.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/AndroidManifest.xml(7 hunks)app/res/values/strings.xml(1 hunks)
🔇 Additional comments (3)
app/AndroidManifest.xml (2)
74-77: LGTM: Permission group for internal actions.The new permission group
commcare.permission-group.INTERNAL_ACTIONis well-defined and appropriately labeled.
390-391: Verify intent-filter actions match permission protection.The exported components are properly protected with custom permissions, which is good. However, let's verify that the intent-filter actions are consistent with the permission protection:
DrawingBoundaryActivity: action="org.commcare.dalvik.action.DrawBoundary"NfcWriteActivity: action="org.commcare.nfc.WRITE"NfcReadActivity: action="org.commcare.nfc.READ"CommCareLogoutReceiver: action="org.commcare.dalvik.api.action.CommCareLogoutAction"RefreshToLatestBuildReceiver: action="org.commcare.dalvik.api.action.RefreshToLatestBuildAction"TemplatePrinterActivity: action="org.commcare.dalvik.action.PRINT"Also applies to: 444-446, 454-456, 475-476, 485-486, 551-552
✅ Verification successful
Intent actions are properly protected and consistently used
The verification confirms that all intent actions are:
- Properly declared in AndroidManifest.xml with corresponding permission protection
- Used consistently in expected locations (tests, form definitions, implementation)
- No unprotected or unexpected usages found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that these actions aren't used elsewhere in the codebase echo "Checking for usage of these actions in the codebase..." rg "org\.commcare\.(dalvik\.action\.(DrawBoundary|PRINT)|nfc\.(WRITE|READ)|dalvik\.api\.action\.(CommCareLogoutAction|RefreshToLatestBuildAction))"Length of output: 1965
app/res/values/strings.xml (1)
156-164: LGTM: Permission strings are clear and well-defined.The new permission-related strings are properly defined and provide clear descriptions of their purpose. The strings correspond correctly to the permissions defined in the AndroidManifest.xml.
app/AndroidManifest.xml
Outdated
| </permission> | ||
|
|
||
| <permission | ||
| android:name="${applicationId}.permission.READ_NFC_DATA" |
There was a problem hiding this comment.
@avazirna Here the permission READ_NFC_DATA doesn't make sense since the purpose of this permission is not to give an external app permission to read nfc data (we don't have that functionality for external apps) but merely to protect the app's exposed components that should not ideally be exposed in the first place. As such we should have one single permission like PROTECT_FALSELY_EXPOSED_COMPONENTS and apply it to all such components.
There was a problem hiding this comment.
Makes sense having an umbrella permission for these, so this will be applied to READ_NFC_DATA, WRITE_NFC_DATA, DRAW_BOUNDARY_AREA and PRINT
avazirna
force-pushed
the
review-activities-export-attribute
branch
from
1673e7a to
6226c5e
Compare
|
Applied the same permission to |
app/AndroidManifest.xml
Outdated
| <receiver | ||
| android:name="org.commcare.provider.RefreshToLatestBuildReceiver" | ||
| android:exported="false"> | ||
| android:permission="org.commcare.dalvik.permission.UPDATE_TO_LATEST_BUILD" |
There was a problem hiding this comment.
RefreshToLatestBuildReceiver is a internal dev only tool and introducing a permission here will break the underlying scripts. Given it was marked as exported=false earlier, is there a good reason to change it to true now ? FWIW I think the underlying scripts would already be broken because it was marked as un-exported and as such I think the best way forward here is to just delete the RefreshToLatestBuildReceiver
There was a problem hiding this comment.
I might not have enough context here, but my understanding is that this receiver is for external use, its documentation also states that it should respond to requests via adb, in that case it needs to be exported.
Trigger from command line with:
- adb shell am broadcast -a org.commcare.dalvik.api.action.RefreshToLatestBuildAction
Considering that this is for dev use only, what's the usage? To make sure the feature is working as expected?
There was a problem hiding this comment.
It's a adb command to update the app to latest without interacting with UI and was meant to increase dev efficiency - https://github.com/dimagi/commcare-android/blob/master/scripts/ccc#L63
There was a problem hiding this comment.
Deleting this receiver then, thanks
app/AndroidManifest.xml
Outdated
| android:label="@string/title_data_change_logs_activity" | ||
| android:exported="false" | ||
| android:name="org.commcare.activities.DataChangeLogsActivity"> | ||
| android:exported="true" |
There was a problem hiding this comment.
why do we need to export this activity ? (I don't recall using it in callouts)
There was a problem hiding this comment.
It's not, but it's supposed to respond to ACTION_VIEW actions, so I think it needs to be exported. I didn't test this though, let me do that.
There was a problem hiding this comment.
Btw from the Spec, I don't see the reason for this activity to respond o ACTION_VIEW actions, can you provide some context here?
There was a problem hiding this comment.
I think that's again because of dev tooling - https://github.com/dimagi/commcare-android/blob/master/scripts/ccc#L217 which I am fine with killing.
There was a problem hiding this comment.
@shubham1g5 I've reverted the exported status of these components, I will kill them or move to a different manifest file in a different PR.
avazirna
force-pushed
the
review-activities-export-attribute
branch
from
6226c5e to
d0d1c6b
Compare
app/res/values/strings.xml
Outdated
| <string name="permission_protect_falsely_exposed_components_label">Protect falsely exposed components</string> | ||
| <string name="permission_protect_falsely_exposed_components_description">Due to implicit intent constrains, certain intent-aware components are now exposed, this permission is to prevent abusive use by external apps</string> | ||
|
|
||
| <string name="permission_group_internal_action_label">Permission group for CommCare Internal Actions</string> |
There was a problem hiding this comment.
can we try to simply these strings and make them user-friendly (remove any tech jargain like intents/callouts) as these may be presented to the users.
Summary
Android 14 brought new restrictions around implicit intents and CC app callouts are based on implicit intents, this PR reviews the
exportedattribute of activities targeted by predefined app callout options.Ticket: https://dimagi.atlassian.net/browse/QA-7356
PR Checklist