[mms] add extended support#379
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (44)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (36)
WalkthroughThis PR implements end-to-end MMS support: a vendored AOSP PDU composition library, MMS domain/API models, attachment storage, outbound send orchestration via SmsManager with sent-state broadcast handling, inbound MMS retrieval parsing with permission gating, new local server endpoints for attachment access, Android manifest/FileProvider wiring, and documentation. ChangesMMS send, receive, and API exposure
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant MessagesService
participant MmsSender
participant PduComposer
participant SmsManager
participant EventsReceiver
MessagesService->>MmsSender: send(messageId, phoneNumbers, mms, subscriptionId)
MmsSender->>PduComposer: make()
MmsSender->>SmsManager: sendMultimediaMessage(uri, PendingIntent)
SmsManager-->>EventsReceiver: ACTION_MMS_SENT
EventsReceiver->>MessagesService: processStateIntent(intent, resultCode)
MessagesService->>MmsSender: cleanup(context, messageId)
sequenceDiagram
participant MmsContentObserver
participant ReceiverService
participant MMSRetrieveParser
participant IncomingMessagesService
participant MmsAttachmentStorage
MmsContentObserver->>ReceiverService: processMmsDownloaded(mmsId)
ReceiverService->>MMSRetrieveParser: parse(pduBytes)
MMSRetrieveParser-->>ReceiverService: MRetrieveConf(parts)
ReceiverService->>IncomingMessagesService: save(InboxMessage.MMS)
loop each attachment part
IncomingMessagesService->>MmsAttachmentStorage: store(messageId, partId, bytes)
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/main/java/me/capcom/smsgateway/modules/receiver/MessagesReceiver.kt (1)
77-101:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDuplicate
textFilterdeclaration and double registration will cause compilation error and duplicate message processing.
textFilteris declared twice (lines 77 and 85), which won't compile. Additionally, if this were to compile, the sameINSTANCEwould be registered twice forSMS_RECEIVED_ACTION—once onappContext(line 80) and once oncontext(line 88)—causing each SMS to be processed twice.This appears to be incomplete refactoring or merge conflict residue.
🐛 Proposed fix: Remove the duplicate registration block
fun register(context: Context) { val appContext = context.applicationContext unregister(appContext) val textFilter = IntentFilter().apply { addAction(Intents.SMS_RECEIVED_ACTION) } appContext.registerReceiver( INSTANCE, textFilter ) - val textFilter = IntentFilter().apply { - addAction(Intents.SMS_RECEIVED_ACTION) - } - context.registerReceiver( - INSTANCE, - textFilter - ) - val dataFilter = IntentFilter().apply { addAction(Intents.DATA_SMS_RECEIVED_ACTION) addDataScheme("sms") addDataAuthority("*", "53739") }🤖 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 `@app/src/main/java/me/capcom/smsgateway/modules/receiver/MessagesReceiver.kt` around lines 77 - 101, Remove the duplicate registration block for SMS_RECEIVED_ACTION to fix the compilation error and prevent duplicate message processing. You have two identical textFilter declarations and registrations of the same INSTANCE receiver—one registering on appContext and another on context. Delete the second duplicate textFilter IntentFilter block (the one that registers on context) and keep only the first registration on appContext. This will leave you with one textFilter for SMS_RECEIVED_ACTION on appContext and the separate dataFilter block for DATA_SMS_RECEIVED_ACTION, eliminating both the variable naming conflict and the duplicate receiver registration.app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsContentObserver.kt (1)
130-146:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPermanent processing failure blocks subsequent messages.
If
processMmsDownloaded(mmsId)throws an exception, the high-water mark is not advanced, causing the same failing MMS to be retried on everyprocessNewMessages()call. A single corrupt or unparseable MMS would block processing of all later messages indefinitely.Consider updating the mark after a configurable number of retries, or always advance the mark while logging the failure to prevent one bad message from blocking the pipeline.
Proposed fix: always advance mark, rely on logging for failed items
try { processMmsDownloaded(mmsId) - storage.mmsLastProcessedID = mmsId } catch (e: Exception) { logsService.insert( LogEntry.Priority.ERROR, MODULE_NAME, "Failed processing downloaded MMS (id=$mmsId)", - mapOf("mmsId" to mmsId) + mapOf("mmsId" to mmsId, "error" to (e.message ?: e.toString())) ) } + // Always advance mark to prevent one corrupt MMS from blocking the pipeline + storage.mmsLastProcessedID = mmsId }🤖 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 `@app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsContentObserver.kt` around lines 130 - 146, The issue is that when processMmsDownloaded(mmsId) throws an exception in the while loop of the cursor processing block, the high-water mark storage.mmsLastProcessedID is not advanced because it only gets set inside the try block. This causes the same failing MMS to be retried indefinitely, blocking all subsequent messages. Fix this by moving the storage.mmsLastProcessedID = mmsId assignment outside and after the try-catch block so that it always executes for every MMS, regardless of whether processMmsDownloaded succeeds or fails. This allows the pipeline to advance past problem messages while still logging the failures.
🤖 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 `@app/src/main/assets/api/swagger.json`:
- Around line 56-62: The attachments array in the OpenAPI schema lacks a
maxItems constraint, which allows unrestricted MMS payload sizes and compromises
resource control validation. Add a maxItems property to the attachments array
definition in the swagger.json file to enforce a reasonable upper bound on the
number of attachments that can be included in a single MMS message, preventing
oversized payloads at the request validation level.
- Around line 66-90: The smsgateway.MmsAttachment schema currently describes
data and url as mutually exclusive, but the schema definition does not enforce
this constraint—both properties are optional with no validation ensuring exactly
one is provided. Add OpenAPI schema constraint logic (such as oneOf composition)
to the MmsAttachment object that enforces exactly one of the data or url fields
must be present, not both and not neither. This will align the schema validation
with the documented contract and eliminate runtime ambiguity.
In `@app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.kt`:
- Around line 257-267: The MMS content mapping now includes raw attachment
payload fields (data, url) that are sensitive. The existing error/failure
handling path logs the complete message object, which will leak this sensitive
attachment content and inflate log storage. Locate the error handling path that
logs the full message object and replace it with sanitized logging that excludes
the raw attachment payload fields (data and url) from the
MessageContent.Mms.Attachment objects, or log only safe metadata instead of the
complete message.
In
`@app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/InboxRoutes.kt`:
- Around line 145-165: The attachment response handler loads the entire file
into memory using file.readBytes() before sending it to the client, which can
cause OOM errors for large MMS attachments or concurrent requests. Replace the
call.respondBytes(file.readBytes(), contentType) invocation with
call.respondFile(file, contentType) to stream the file directly to the client
instead of buffering it entirely in memory. This will efficiently handle files
of any size while maintaining the same Content-Disposition header and
content-type settings.
In `@app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesService.kt`:
- Around line 330-371: The code resolves the fromMsisdn variable based on the
SIM slot and subscription details (to address carrier-specific MMS delivery
requirements), but then passes null to the mmsSender.send() method instead of
using the resolved value. The contradictory comment on lines 368-370 conflicts
with the earlier rationale for resolving fromMsisdn. Fix this inconsistency by
passing the resolved fromMsisdn variable to the mmsSender.send() call instead of
null, and update or remove the contradictory comment so the implementation
aligns with the stated strategy of providing explicit MSISDN when available to
avoid carrier-specific delivery failures.
In `@app/src/main/java/me/capcom/smsgateway/modules/mms/MmsAttachmentStorage.kt`:
- Around line 63-67: In the remove function in MmsAttachmentStorage.kt, the
delete() method calls on both the individual files (in the forEach block) and
the directory itself are ignoring their Boolean return values, which means
failed deletions go unreported. Capture and check the return values of these
delete() operations and surface any failures through appropriate logging or
exception handling to prevent silent data retention on disk.
In `@app/src/main/java/me/capcom/smsgateway/modules/mms/MmsSender.kt`:
- Around line 163-169: The code in MmsSender.kt reads the full response body
into memory with it.body?.bytes() without size validation, risking
OutOfMemoryError on large responses. Before calling bytes() on the response
body, check the Content-Length header from the response and validate that the
attachment size is within an acceptable limit (e.g., a reasonable max attachment
size constant). If the size exceeds the limit, throw a RuntimeException with a
descriptive message instead of attempting to allocate the full response into
memory.
- Around line 160-169: The code at the point where httpClient.newCall() is
executed fetches caller-supplied URLs without any validation, creating an SSRF
vulnerability. Add URL validation logic immediately after confirming the URL is
non-null and before making the HTTP request in the withContext block. Validate
that the URL does not point to private IP addresses (localhost, 127.0.0.1,
10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16), link-local addresses
(169.254.0.0/16), or other restricted schemes. If the URL fails validation,
throw an exception with a clear error message instead of proceeding with the
fetch. This prevents malicious senders from using the attachment mechanism to
target private network endpoints and exfiltrate data via MMS.
In
`@app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/ContentType.java`:
- Around line 188-190: The isSupportedAudioType method rejects application/ogg
because the isAudioType check requires the content type to start with audio/,
even though AUDIO_OGG is listed in the supported types. Remove the isAudioType
check from the isSupportedAudioType method since isSupportedType already
validates against the complete list of supported audio types including
AUDIO_OGG, so the additional isAudioType check is unnecessary and overly
restrictive.
In
`@app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/EncodedStringValue.java`:
- Around line 217-218: The split() method in the EncodedStringValue class loses
charset fidelity by using platform-default encoding instead of respecting the
mCharacterSet field. In the location where a new EncodedStringValue is
constructed with temp[i].getBytes(), the getBytes() call needs to explicitly
specify the mCharacterSet parameter instead of relying on the platform default
encoding. This ensures that non-default encoded values maintain their original
charset when being split and re-encoded.
In
`@app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduComposer.java`:
- Around line 796-813: The recipient validation logic incorrectly treats missing
fields as successful. The methods appendHeader at lines 796, 801, and 806 return
PDU_COMPOSE_FIELD_NOT_SET when a field is absent, which is different from
PDU_COMPOSE_CONTENT_ERROR. The current conditions only check for inequality to
PDU_COMPOSE_CONTENT_ERROR, allowing missing fields to be treated as successfully
added. Change the conditional checks for TO, CC, and BCC from checking "not
equal to PDU_COMPOSE_CONTENT_ERROR" to explicitly checking for a success
condition (likely equality to a success constant or checking that the return
value is not one of the error/missing constants). This ensures that recipient is
only set to true when a recipient field is actually present and successfully
added, allowing the validation at line 811 to correctly reject PDUs with no
recipients.
In `@app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduHeaders.java`:
- Around line 235-237: The constants
RESPONSE_STATUS_ERROR_PERMANENT_REPLY_CHARGING_LIMITATIONS_NOT_MET and
RESPONSE_STATUS_ERROR_PERMANENT_REPLY_CHARGING_REQUEST_NOT_ACCEPTED both have
the hex value 0xE6, making them impossible to distinguish at runtime. Fix this
by assigning a unique hex value to
RESPONSE_STATUS_ERROR_PERMANENT_REPLY_CHARGING_REQUEST_NOT_ACCEPTED. Based on
the sequence pattern where
RESPONSE_STATUS_ERROR_PERMANENT_REPLY_CHARGING_FORWARDING_DENIED uses 0xE8,
change RESPONSE_STATUS_ERROR_PERMANENT_REPLY_CHARGING_REQUEST_NOT_ACCEPTED to
use 0xE7 to maintain proper hex value ordering and ensure each constant has a
distinct wire representation.
In `@app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduPart.java`:
- Around line 410-413: The code at lines 410-413 retrieves a `P_CONTENT_ID`
header from `mPartHeader` and immediately constructs a String from the result
without null-checking `contentId`. If the header is not present, `get()` returns
null, causing a NullPointerException when passed to the String constructor. Add
a null guard check after retrieving `contentId` from the map (after line 411) to
verify it is not null before calling `new String(contentId)`. If `contentId` is
null, provide an appropriate fallback value or return value instead of crashing.
In
`@app/src/main/java/me/capcom/smsgateway/modules/ping/services/PingForegroundService.kt`:
- Around line 59-64: The null check for intervalSeconds at line 59 exits the
thread without stopping the foreground service, leaving it running with no work
to do. Add a stopSelf() call before the return@thread statement when
intervalSeconds is null, mirroring the behavior of the interval <= 0 check in
the subsequent lines. This ensures the foreground service is properly cleaned up
when no valid interval is configured.
In
`@app/src/main/java/me/capcom/smsgateway/modules/receiver/SmsContentObserver.kt`:
- Around line 45-48: The constant MODULE_NAME is referenced in the
logsService.insert calls throughout the SmsContentObserver class but is not
defined in the companion object (only TAG is defined). Replace all occurrences
of MODULE_NAME with TAG in the logsService.insert method calls across the class.
This includes the log statements at lines 45-48 (anchor), 104-108, 120-124,
149-154, 177-182, and 194-196 (siblings). Each of these sites should have
MODULE_NAME replaced with TAG to match the defined constant in the companion
object and resolve the compilation error.
In `@docs/MMS.md`:
- Around line 288-315: The documentation describes a separate detail endpoint
`GET /inbox/{messageId}` that is not actually implemented in the code. Remove
this non-existent endpoint from the documentation and update the section to
reflect the two actual endpoints: the list endpoint that returns full message
details including attachment metadata, and the attachment bytes endpoint.
Alternatively, clarify that full message details (including the attachments
array shown in the example JSON) are returned as part of the list response from
`GET /inbox?type=MMS_DOWNLOADED`, eliminating the need for a separate detail
endpoint.
---
Outside diff comments:
In `@app/src/main/java/me/capcom/smsgateway/modules/receiver/MessagesReceiver.kt`:
- Around line 77-101: Remove the duplicate registration block for
SMS_RECEIVED_ACTION to fix the compilation error and prevent duplicate message
processing. You have two identical textFilter declarations and registrations of
the same INSTANCE receiver—one registering on appContext and another on context.
Delete the second duplicate textFilter IntentFilter block (the one that
registers on context) and keep only the first registration on appContext. This
will leave you with one textFilter for SMS_RECEIVED_ACTION on appContext and the
separate dataFilter block for DATA_SMS_RECEIVED_ACTION, eliminating both the
variable naming conflict and the duplicate receiver registration.
In
`@app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsContentObserver.kt`:
- Around line 130-146: The issue is that when processMmsDownloaded(mmsId) throws
an exception in the while loop of the cursor processing block, the high-water
mark storage.mmsLastProcessedID is not advanced because it only gets set inside
the try block. This causes the same failing MMS to be retried indefinitely,
blocking all subsequent messages. Fix this by moving the
storage.mmsLastProcessedID = mmsId assignment outside and after the try-catch
block so that it always executes for every MMS, regardless of whether
processMmsDownloaded succeeds or fails. This allows the pipeline to advance past
problem messages while still logging the 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: CHILL
Plan: Pro
Run ID: ccbb3ea3-ae9f-4980-903d-cbc7584aea66
📒 Files selected for processing (48)
.idea/gradle.xml.idea/misc.xmlREADME.mdapp/src/main/AndroidManifest.xmlapp/src/main/assets/api/swagger.jsonapp/src/main/java/me/capcom/smsgateway/App.ktapp/src/main/java/me/capcom/smsgateway/data/entities/Message.ktapp/src/main/java/me/capcom/smsgateway/domain/MessageContent.ktapp/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayApi.ktapp/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/WebService.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/domain/messages/Message.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/domain/messages/MmsMessage.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/domain/messages/PostMessageRequest.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/routes/InboxRoutes.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/routes/MessagesRoutes.ktapp/src/main/java/me/capcom/smsgateway/modules/messages/MessagesRepository.ktapp/src/main/java/me/capcom/smsgateway/modules/messages/MessagesService.ktapp/src/main/java/me/capcom/smsgateway/modules/mms/MmsAttachmentStorage.ktapp/src/main/java/me/capcom/smsgateway/modules/mms/MmsSender.ktapp/src/main/java/me/capcom/smsgateway/modules/mms/Module.ktapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/CharacterSets.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/ContentType.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/EncodedStringValue.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/GenericPdu.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/InvalidHeaderValueException.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/MmsException.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/MultimediaMessagePdu.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduBody.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduComposer.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduContentTypes.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduHeaders.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduPart.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/SendReq.javaapp/src/main/java/me/capcom/smsgateway/modules/ping/services/PingForegroundService.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/MessagesReceiver.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/MmsContentObserver.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/MmsReceiver.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/SmsContentObserver.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/StateStorage.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/parsers/MMSParser.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/parsers/MMSRetrieveParser.ktapp/src/main/java/me/capcom/smsgateway/receivers/EventsReceiver.ktapp/src/main/res/values-zh/strings.xmlapp/src/main/res/xml/file_paths.xmlapp/src/main/res/xml/root_preferences.xmldocs/MMS.md
| "smsgateway.MmsAttachment": { | ||
| "properties": { | ||
| "contentType": { | ||
| "description": "MIME type. Common values: image/jpeg, image/png, image/gif, video/mp4, audio/amr, audio/mpeg, text/plain.", | ||
| "type": "string" | ||
| }, | ||
| "name": { | ||
| "description": "Optional filename for the part (used as Content-Location).", | ||
| "nullable": true, | ||
| "type": "string" | ||
| }, | ||
| "data": { | ||
| "description": "Base64-encoded bytes. Mutually exclusive with `url`.", | ||
| "format": "byte", | ||
| "nullable": true, | ||
| "type": "string" | ||
| }, | ||
| "url": { | ||
| "description": "HTTP(S) URL the device will fetch immediately before sending. Mutually exclusive with `data`.", | ||
| "nullable": true, | ||
| "type": "string" | ||
| } | ||
| }, | ||
| "required": ["contentType"], | ||
| "type": "object" |
There was a problem hiding this comment.
Enforce data XOR url for smsgateway.MmsAttachment.
The schema text says “exactly one,” but current validation allows both or neither. That creates client/server contract drift and ambiguous runtime behavior.
Suggested fix
"smsgateway.MmsAttachment": {
"properties": {
...
},
+ "oneOf": [
+ { "required": ["data"] },
+ { "required": ["url"] }
+ ],
+ "not": {
+ "required": ["data", "url"]
+ },
"required": ["contentType"],
"type": "object"
}📝 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.
| "smsgateway.MmsAttachment": { | |
| "properties": { | |
| "contentType": { | |
| "description": "MIME type. Common values: image/jpeg, image/png, image/gif, video/mp4, audio/amr, audio/mpeg, text/plain.", | |
| "type": "string" | |
| }, | |
| "name": { | |
| "description": "Optional filename for the part (used as Content-Location).", | |
| "nullable": true, | |
| "type": "string" | |
| }, | |
| "data": { | |
| "description": "Base64-encoded bytes. Mutually exclusive with `url`.", | |
| "format": "byte", | |
| "nullable": true, | |
| "type": "string" | |
| }, | |
| "url": { | |
| "description": "HTTP(S) URL the device will fetch immediately before sending. Mutually exclusive with `data`.", | |
| "nullable": true, | |
| "type": "string" | |
| } | |
| }, | |
| "required": ["contentType"], | |
| "type": "object" | |
| "smsgateway.MmsAttachment": { | |
| "properties": { | |
| "contentType": { | |
| "description": "MIME type. Common values: image/jpeg, image/png, image/gif, video/mp4, audio/amr, audio/mpeg, text/plain.", | |
| "type": "string" | |
| }, | |
| "name": { | |
| "description": "Optional filename for the part (used as Content-Location).", | |
| "nullable": true, | |
| "type": "string" | |
| }, | |
| "data": { | |
| "description": "Base64-encoded bytes. Mutually exclusive with `url`.", | |
| "format": "byte", | |
| "nullable": true, | |
| "type": "string" | |
| }, | |
| "url": { | |
| "description": "HTTP(S) URL the device will fetch immediately before sending. Mutually exclusive with `data`.", | |
| "nullable": true, | |
| "type": "string" | |
| } | |
| }, | |
| "oneOf": [ | |
| { "required": ["data"] }, | |
| { "required": ["url"] } | |
| ], | |
| "not": { | |
| "required": ["data", "url"] | |
| }, | |
| "required": ["contentType"], | |
| "type": "object" | |
| } |
🤖 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 `@app/src/main/assets/api/swagger.json` around lines 66 - 90, The
smsgateway.MmsAttachment schema currently describes data and url as mutually
exclusive, but the schema definition does not enforce this constraint—both
properties are optional with no validation ensuring exactly one is provided. Add
OpenAPI schema constraint logic (such as oneOf composition) to the MmsAttachment
object that enforces exactly one of the data or url fields must be present, not
both and not neither. This will align the schema validation with the documented
contract and eliminate runtime ambiguity.
| public static final int RESPONSE_STATUS_ERROR_PERMANENT_REPLY_CHARGING_LIMITATIONS_NOT_MET = 0xE6; | ||
| public static final int RESPONSE_STATUS_ERROR_PERMANENT_REPLY_CHARGING_REQUEST_NOT_ACCEPTED = 0xE6; | ||
| public static final int RESPONSE_STATUS_ERROR_PERMANENT_REPLY_CHARGING_FORWARDING_DENIED = 0xE8; |
There was a problem hiding this comment.
Two distinct response-status constants collapse to the same wire value.
These two named statuses currently share 0xE6, so they cannot be represented distinctly.
Proposed fix
public static final int RESPONSE_STATUS_ERROR_PERMANENT_REPLY_CHARGING_LIMITATIONS_NOT_MET = 0xE6;
-public static final int RESPONSE_STATUS_ERROR_PERMANENT_REPLY_CHARGING_REQUEST_NOT_ACCEPTED = 0xE6;
+public static final int RESPONSE_STATUS_ERROR_PERMANENT_REPLY_CHARGING_REQUEST_NOT_ACCEPTED = 0xE7;📝 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.
| public static final int RESPONSE_STATUS_ERROR_PERMANENT_REPLY_CHARGING_LIMITATIONS_NOT_MET = 0xE6; | |
| public static final int RESPONSE_STATUS_ERROR_PERMANENT_REPLY_CHARGING_REQUEST_NOT_ACCEPTED = 0xE6; | |
| public static final int RESPONSE_STATUS_ERROR_PERMANENT_REPLY_CHARGING_FORWARDING_DENIED = 0xE8; | |
| public static final int RESPONSE_STATUS_ERROR_PERMANENT_REPLY_CHARGING_LIMITATIONS_NOT_MET = 0xE6; | |
| public static final int RESPONSE_STATUS_ERROR_PERMANENT_REPLY_CHARGING_REQUEST_NOT_ACCEPTED = 0xE7; | |
| public static final int RESPONSE_STATUS_ERROR_PERMANENT_REPLY_CHARGING_FORWARDING_DENIED = 0xE8; |
🤖 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 `@app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduHeaders.java`
around lines 235 - 237, The constants
RESPONSE_STATUS_ERROR_PERMANENT_REPLY_CHARGING_LIMITATIONS_NOT_MET and
RESPONSE_STATUS_ERROR_PERMANENT_REPLY_CHARGING_REQUEST_NOT_ACCEPTED both have
the hex value 0xE6, making them impossible to distinguish at runtime. Fix this
by assigning a unique hex value to
RESPONSE_STATUS_ERROR_PERMANENT_REPLY_CHARGING_REQUEST_NOT_ACCEPTED. Based on
the sequence pattern where
RESPONSE_STATUS_ERROR_PERMANENT_REPLY_CHARGING_FORWARDING_DENIED uses 0xE8,
change RESPONSE_STATUS_ERROR_PERMANENT_REPLY_CHARGING_REQUEST_NOT_ACCEPTED to
use 0xE7 to maintain proper hex value ordering and ensure each constant has a
distinct wire representation.
| if (null == location) { | ||
| byte[] contentId = (byte[]) mPartHeader.get(P_CONTENT_ID); | ||
| return "cid:" + new String(contentId); | ||
| } else { |
There was a problem hiding this comment.
Guard null contentId before building the "cid:" fallback.
At Line 410–413, new String(contentId) can throw when no P_CONTENT_ID header is set, causing a hard crash instead of a safe fallback.
Suggested patch
if (null == location) {
byte[] contentId = (byte[]) mPartHeader.get(P_CONTENT_ID);
- return "cid:" + new String(contentId);
+ if (contentId == null || contentId.length == 0) {
+ return null;
+ }
+ return "cid:" + new String(contentId);
} else {
return new String(location);
}📝 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.
| if (null == location) { | |
| byte[] contentId = (byte[]) mPartHeader.get(P_CONTENT_ID); | |
| return "cid:" + new String(contentId); | |
| } else { | |
| if (null == location) { | |
| byte[] contentId = (byte[]) mPartHeader.get(P_CONTENT_ID); | |
| if (contentId == null || contentId.length == 0) { | |
| return null; | |
| } | |
| return "cid:" + new String(contentId); | |
| } else { |
🤖 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 `@app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduPart.java`
around lines 410 - 413, The code at lines 410-413 retrieves a `P_CONTENT_ID`
header from `mPartHeader` and immediately constructs a String from the result
without null-checking `contentId`. If the header is not present, `get()` returns
null, causing a NullPointerException when passed to the String constructor. Add
a null guard check after retrieving `contentId` from the map (after line 411) to
verify it is not null before calling `new String(contentId)`. If `contentId` is
null, provide an appropriate fallback value or return value instead of crashing.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/main/java/me/capcom/smsgateway/modules/mms/MmsSender.kt (1)
163-166:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a collision-resistant PDU filename.
The lossy sanitizer can map distinct message IDs to the same PDU path, so concurrent or retried MMS sends can overwrite/cleanup each other’s files. Use a hash of the original ID instead of replacement.
Suggested fix
+import java.security.MessageDigest + @@ private fun writePduFile(messageId: String, bytes: ByteArray): File { val dir = File(context.filesDir, "mms-out").apply { mkdirs() } - val safeId = messageId.replace(Regex("[^A-Za-z0-9_.-]"), "_") - val file = File(dir, "$safeId.pdu") + val file = File(dir, "${pduFileName(messageId)}.pdu") file.writeBytes(bytes) return file } @@ /** Clean up any stale PDU files for the given message. */ fun cleanup(context: Context, messageId: String) { - val safeId = messageId.replace(Regex("[^A-Za-z0-9_.-]"), "_") - File(File(context.filesDir, "mms-out"), "$safeId.pdu").delete() + File(File(context.filesDir, "mms-out"), "${pduFileName(messageId)}.pdu").delete() } + + private fun pduFileName(messageId: String): String = + MessageDigest.getInstance("SHA-256") + .digest(messageId.toByteArray(Charsets.UTF_8)) + .joinToString("") { "%02x".format(it) } }Also applies to: 203-205
🤖 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 `@app/src/main/java/me/capcom/smsgateway/modules/mms/MmsSender.kt` around lines 163 - 166, The writePduFile method uses a lossy character replacement sanitizer to generate the safeId variable, which can cause different message IDs to map to the same filename, leading to file collisions and overwrites during concurrent or retried sends. Replace the lossy sanitizer approach with a collision-resistant hash function (such as MD5 or SHA-256) applied to the original messageId to generate a unique and safe filename identifier. This same fix must also be applied at the second location mentioned (around lines 203-205) where similar PDU filename generation occurs.app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsContentObserver.kt (1)
39-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t advance the MMS high-water mark before durable processing.
Setting the mark to the current max before the posted catch-up skips MMS rows that arrived before permission/startup, and the unconditional advance after exceptions makes transient read/save failures unretryable. Only advance after successful processing, or persist a separate failed-ID quarantine for corrupt rows.
Suggested fix
- // Initialize high-water mark to current max ID if not set - if (storage.mmsLastProcessedID == 0L) { - storage.mmsLastProcessedID = queryMaxMmsId() - } - val thread = HandlerThread("MmsContentObserver").apply { start() } @@ try { processMmsDownloaded(mmsId) storage.mmsLastProcessedID = mmsId } catch (e: Exception) { logsService.insert( @@ mapOf("mmsId" to mmsId, "error" to (e.message ?: e.toString())), ) } - // Always advance mark to prevent one corrupt MMS from blocking the pipeline - storage.mmsLastProcessedID = mmsId } }Also applies to: 133-145
🤖 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 `@app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsContentObserver.kt` around lines 39 - 65, The high-water mark storage.mmsLastProcessedID is being set to the current max MMS ID before the catch-up processing starts, which causes rows that arrived before startup or permission grant to be skipped forever. Remove the initialization block that sets storage.mmsLastProcessedID to queryMaxMmsId() at the beginning of the observer setup. Instead, only advance the high-water mark after successfully processing messages, ensuring that transient failures during processNewMessages() remain retryable and no messages are lost. Apply the same fix at the other affected location where the high-water mark is managed.
🤖 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 `@app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.kt`:
- Around line 231-233: The logging fields in the catch block of processMessage()
are accessing message.content multiple times (for getting the class name and
casting to check attachments), which will throw an exception again if the
content is invalid and caused the original error. Wrap the message.content
access in a runCatching block before constructing the log context, store the
result safely, and use that cached result when building the logging map entries
for messageType and attachmentsCount to prevent the catch block from being
aborted by a second exception.
In
`@app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/InboxRoutes.kt`:
- Around line 159-162: The Content-Disposition header in the InboxRoutes.kt file
is directly interpolating the sender-controlled attachment.displayName without
escaping, which allows header injection or malformed headers through quote and
CRLF characters. Before using attachment.displayName in the header value string
(in the call.response.header call), properly escape or encode it to prevent
injection attacks. Apply appropriate escaping such as removing or encoding
problematic characters like quotes and newlines, or use URL encoding for the
filename value.
- Around line 155-163: The current implementation uses call.respondFile(file)
which infers the content type from the filename, causing the stored
attachment.contentType to be lost. Replace the call.respondFile(file) invocation
with LocalFileContent, passing both the file and the attachment.contentType to
preserve the actual MIME type information that was stored when the attachment
was uploaded. The parseContentType() helper function (defined at lines 230-233)
may be useful for properly formatting the content type before passing it to
LocalFileContent.
In `@app/src/main/java/me/capcom/smsgateway/modules/mms/MmsSender.kt`:
- Around line 149-154: The attachmentBytes function in MmsSender.kt has no size
validation for decoded Base64 attachment data, which allows arbitrarily large
attachments to be decoded and allocated in memory, risking Out-of-Memory errors.
Add a maximum size limit constant for MMS attachments and validate the decoded
byte array size immediately after the Base64.decode call in the attachmentBytes
function. If the decoded data exceeds this limit, throw an
IllegalArgumentException with a clear error message before the large allocation
causes issues. This bounds checking should occur within the try block to ensure
proper error handling.
---
Outside diff comments:
In `@app/src/main/java/me/capcom/smsgateway/modules/mms/MmsSender.kt`:
- Around line 163-166: The writePduFile method uses a lossy character
replacement sanitizer to generate the safeId variable, which can cause different
message IDs to map to the same filename, leading to file collisions and
overwrites during concurrent or retried sends. Replace the lossy sanitizer
approach with a collision-resistant hash function (such as MD5 or SHA-256)
applied to the original messageId to generate a unique and safe filename
identifier. This same fix must also be applied at the second location mentioned
(around lines 203-205) where similar PDU filename generation occurs.
In
`@app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsContentObserver.kt`:
- Around line 39-65: The high-water mark storage.mmsLastProcessedID is being set
to the current max MMS ID before the catch-up processing starts, which causes
rows that arrived before startup or permission grant to be skipped forever.
Remove the initialization block that sets storage.mmsLastProcessedID to
queryMaxMmsId() at the beginning of the observer setup. Instead, only advance
the high-water mark after successfully processing messages, ensuring that
transient failures during processNewMessages() remain retryable and no messages
are lost. Apply the same fix at the other affected location where the high-water
mark is managed.
🪄 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: CHILL
Plan: Pro
Run ID: 3c80b2d7-971f-47e6-9ab9-e71e17f5d428
📒 Files selected for processing (12)
app/src/main/java/me/capcom/smsgateway/domain/MessageContent.ktapp/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayApi.ktapp/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/domain/messages/MmsMessage.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/routes/InboxRoutes.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/routes/MessagesRoutes.ktapp/src/main/java/me/capcom/smsgateway/modules/messages/MessagesService.ktapp/src/main/java/me/capcom/smsgateway/modules/mms/MmsAttachmentStorage.ktapp/src/main/java/me/capcom/smsgateway/modules/mms/MmsSender.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/MessagesReceiver.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/MmsContentObserver.ktdocs/MMS.md
💤 Files with no reviewable changes (2)
- app/src/main/java/me/capcom/smsgateway/modules/receiver/MessagesReceiver.kt
- app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/MessagesRoutes.kt
✅ Files skipped from review due to trivial changes (1)
- docs/MMS.md
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/main/java/me/capcom/smsgateway/modules/mms/MmsAttachmentStorage.kt
- app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayApi.kt
- app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesService.kt
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/InboxRoutes.kt (1)
141-141: ⚖️ Poor tradeoffConsider lazy-loading or batch-loading attachments to avoid N+1 I/O.
Each call to
toDomain()triggerslistAttachmentRefs(), resulting in a separateattachmentStorage.list()per message. With the 500-message limit, this could noticeably degrade response times.Options:
- Lazy approach: Add an optional
includeAttachmentsquery parameter (defaultfalse) so clients opt-in only when needed.- Batch approach: Collect all message IDs first, then load attachments in a single call or grouped batches.
🤖 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 `@app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/InboxRoutes.kt` at line 141, The current implementation in the InboxRoutes.kt file has an N+1 I/O problem where each message's toDomain() call triggers a separate listAttachmentRefs() lookup via attachmentStorage.list(). Implement the lazy-loading approach by adding an optional includeAttachments query parameter (default false) to the endpoint, then modify the toDomain() call to conditionally include attachments only when the parameter is true. This allows clients to opt-in to attachment loading only when needed, avoiding unnecessary I/O on the 500-message result set.
🤖 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
`@app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/InboxRoutes.kt`:
- Around line 154-164: Add a file existence check after retrieving the
attachment file to handle the case where attachment metadata exists but the
actual file is missing from disk. After the line val file = attachment.file,
check if the file exists using appropriate file system methods. If the file does
not exist, return call.respond(HttpStatusCode.NotFound) before reaching the
LocalFileContent call to prevent a 500 error and instead properly return a 404
response.
---
Nitpick comments:
In
`@app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/InboxRoutes.kt`:
- Line 141: The current implementation in the InboxRoutes.kt file has an N+1 I/O
problem where each message's toDomain() call triggers a separate
listAttachmentRefs() lookup via attachmentStorage.list(). Implement the
lazy-loading approach by adding an optional includeAttachments query parameter
(default false) to the endpoint, then modify the toDomain() call to
conditionally include attachments only when the parameter is true. This allows
clients to opt-in to attachment loading only when needed, avoiding unnecessary
I/O on the 500-message result set.
🪄 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: CHILL
Plan: Pro
Run ID: 7dce6a8a-d8f0-4e84-9273-33331a3b49f2
📒 Files selected for processing (3)
app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/routes/InboxRoutes.ktapp/src/main/java/me/capcom/smsgateway/modules/mms/MmsSender.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/MmsSender.kt
7d4ab8e to
42cba0d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduComposer.java`:
- Around line 283-297: The appendTextString method accesses text[0] without
first checking if the text array is empty or null, which can cause an
ArrayIndexOutOfBoundsException. Add a null and empty length check before the
conditional that accesses text[0], and handle empty arrays appropriately (either
by returning early or raising a compose error to match the expected behavior).
This same validation issue also exists in the part content-type encoding at
lines 911-921, so ensure the same defensive checks are applied wherever text
arrays are accessed without prior validation.
- Around line 851-855: The code at line 851 calls
mPduHeader.getTextString(PduHeaders.CONTENT_TYPE) which can return null, and
passing null directly to the String constructor will throw a
NullPointerException before your null check on contentTypeIdentifier is reached.
Add a null check immediately after calling
getTextString(PduHeaders.CONTENT_TYPE) and before constructing the String
object; if the returned value is null, return PDU_COMPOSE_CONTENT_ERROR
immediately since content type is mandatory. This ensures the null case is
handled gracefully before attempting String construction.
🪄 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: CHILL
Plan: Pro
Run ID: aedf6e87-0974-4d63-966e-a03ed25292af
📒 Files selected for processing (45)
README.mdapp/src/main/AndroidManifest.xmlapp/src/main/assets/api/swagger.jsonapp/src/main/java/me/capcom/smsgateway/App.ktapp/src/main/java/me/capcom/smsgateway/data/entities/Message.ktapp/src/main/java/me/capcom/smsgateway/domain/MessageContent.ktapp/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayApi.ktapp/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/WebService.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/domain/messages/Message.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/domain/messages/MmsMessage.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/domain/messages/PostMessageRequest.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/routes/InboxRoutes.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/routes/MessagesRoutes.ktapp/src/main/java/me/capcom/smsgateway/modules/messages/MessagesRepository.ktapp/src/main/java/me/capcom/smsgateway/modules/messages/MessagesService.ktapp/src/main/java/me/capcom/smsgateway/modules/mms/MmsAttachmentStorage.ktapp/src/main/java/me/capcom/smsgateway/modules/mms/MmsSender.ktapp/src/main/java/me/capcom/smsgateway/modules/mms/Module.ktapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/CharacterSets.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/ContentType.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/EncodedStringValue.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/GenericPdu.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/InvalidHeaderValueException.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/MmsException.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/MultimediaMessagePdu.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduBody.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduComposer.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduContentTypes.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduHeaders.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduPart.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/SendReq.javaapp/src/main/java/me/capcom/smsgateway/modules/receiver/MessagesReceiver.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/MmsContentObserver.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/MmsReceiver.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/SmsContentObserver.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/StateStorage.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/parsers/MMSParser.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/parsers/MMSRetrieveParser.ktapp/src/main/java/me/capcom/smsgateway/receivers/EventsReceiver.ktapp/src/main/res/values-zh/strings.xmlapp/src/main/res/xml/file_paths.xmlapp/src/main/res/xml/root_preferences.xmldocs/MMS.md
✅ Files skipped from review due to trivial changes (3)
- app/src/main/res/xml/root_preferences.xml
- README.md
- docs/MMS.md
🚧 Files skipped from review as they are similar to previous changes (35)
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/MmsException.java
- app/src/main/java/me/capcom/smsgateway/modules/receiver/MessagesReceiver.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/Module.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/InvalidHeaderValueException.java
- app/src/main/res/values-zh/strings.xml
- app/src/main/java/me/capcom/smsgateway/modules/receiver/parsers/MMSParser.kt
- app/src/main/java/me/capcom/smsgateway/modules/localserver/domain/messages/MmsMessage.kt
- app/src/main/res/xml/file_paths.xml
- app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/MessagesRoutes.kt
- app/src/main/java/me/capcom/smsgateway/modules/localserver/WebService.kt
- app/src/main/java/me/capcom/smsgateway/domain/MessageContent.kt
- app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsReceiver.kt
- app/src/main/AndroidManifest.xml
- app/src/main/java/me/capcom/smsgateway/modules/localserver/domain/messages/PostMessageRequest.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/CharacterSets.java
- app/src/main/java/me/capcom/smsgateway/modules/localserver/domain/messages/Message.kt
- app/src/main/java/me/capcom/smsgateway/modules/receiver/StateStorage.kt
- app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayApi.kt
- app/src/main/java/me/capcom/smsgateway/data/entities/Message.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduContentTypes.java
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/ContentType.java
- app/src/main/java/me/capcom/smsgateway/modules/mms/MmsSender.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduBody.java
- app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsContentObserver.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/MmsAttachmentStorage.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduHeaders.java
- app/src/main/java/me/capcom/smsgateway/modules/receiver/parsers/MMSRetrieveParser.kt
- app/src/main/java/me/capcom/smsgateway/receivers/EventsReceiver.kt
- app/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.kt
- app/src/main/java/me/capcom/smsgateway/modules/receiver/SmsContentObserver.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/SendReq.java
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/EncodedStringValue.java
- app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/InboxRoutes.kt
- app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesService.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduPart.java
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.kt (3)
222-235:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedact full MMS payloads from logs.
messagecan now be anInboxMessage.MMScontaining base64 attachment data, so these log entries can persist media contents and message bodies. Log metadata only.🛡️ Proposed fix
logsService.insert( LogEntry.Priority.DEBUG, MODULE_NAME, "ReceiverService::process - message received", - mapOf("message" to message) + mapOf( + "messageType" to message.javaClass.simpleName, + "date" to message.date, + "subscriptionId" to message.subscriptionId, + ) ) @@ logsService.insert( LogEntry.Priority.DEBUG, MODULE_NAME, "ReceiverService::process - duplicate message, skipping", - mapOf("message" to message) + mapOf( + "messageType" to message.javaClass.simpleName, + "date" to message.date, + "subscriptionId" to message.subscriptionId, + ) ) return }🤖 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 `@app/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.kt` around lines 222 - 235, The logsService.insert calls in ReceiverService::process are logging the full message object which can contain base64 attachment data and message bodies from MMS messages, exposing sensitive media content in logs. Replace the mapOf("message" to message) parameter in both logsService.insert calls with metadata-only information that identifies the message (such as its ID or type) without including the actual message content or attachment data. This ensures sensitive message bodies and media are not persisted in debug logs.
160-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersist MMS attachments once, outside the webhook-only branch.
Attachment storage backs the inbox/download API, but
process()only storesInboxMessage.MMSattachments whentriggerWebhooksis true. The downloaded path compensates by storing at Line 161, causing duplicate writes for downloaded MMS whileprocess(..., false)never creates downloadable files. Move MMS attachment persistence into one helper called after dedup for every MMS, then keep the webhook branch only for emission.♻️ Suggested refactor
- try { - attachmentStorage.store(messageId, part.partId, name, part.contentType, part.data) - } catch (e: Exception) { - logsService.insert( - LogEntry.Priority.WARN, - MODULE_NAME, - "Failed to persist MMS attachment", - mapOf( - "messageId" to messageId, - "partId" to part.partId, - "error" to (e.message ?: e.toString()), - ) - ) - } attachments += InboxMessage.MMS.Attachment( partId = part.partId, contentType = part.contentType, @@ val incoming = incomingMessagesService.save(message) + + if (message is InboxMessage.MMS) { + persistMmsAttachments(message) + } if (triggerWebhooks) { val (type, payload) = when (message) { @@ is InboxMessage.MMS -> { - message.attachments.forEach { att -> - val decoded = att.data?.let { runCatching { - android.util.Base64.decode(it, android.util.Base64.DEFAULT) - }.getOrNull() } - if (decoded != null) { - try { - attachmentStorage.store( - message.messageId, - att.partId, - att.name, - att.contentType, - decoded, - ) - } catch (e: Exception) { - logsService.insert( - LogEntry.Priority.WARN, - MODULE_NAME, - "Failed to persist MMS attachment", - mapOf( - "messageId" to message.messageId, - "partId" to att.partId, - "error" to (e.message ?: e.toString()), - ) - ) - } - } - } - WebHookEvent.MmsDownloaded to MmsDownloadedPayload( messageId = message.messageId,Add the helper outside
process:private fun persistMmsAttachments(message: InboxMessage.MMS) { message.attachments.forEach { att -> val decoded = att.data?.let { runCatching { Base64.decode(it, Base64.DEFAULT) }.getOrNull() } ?: return@forEach try { attachmentStorage.store( message.messageId, att.partId, att.name, att.contentType, decoded, ) } catch (e: Exception) { logsService.insert( LogEntry.Priority.WARN, MODULE_NAME, "Failed to persist MMS attachment", mapOf( "messageId" to message.messageId, "partId" to att.partId, "error" to (e.message ?: e.toString()), ) ) } } }Also applies to: 248-309
🤖 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 `@app/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.kt` around lines 160 - 173, The MMS attachment persistence logic is currently duplicated and only executed within the webhook-triggered branch, causing downloaded attachments to be stored twice while non-webhook calls never create downloadable files. Extract the attachment storage and error handling logic (currently in the try-catch block with attachmentStorage.store and logsService.insert) into a new private helper method called persistMmsAttachments that accepts an InboxMessage.MMS parameter and iterates over its attachments. Call this helper method once after message deduplication for every MMS message, regardless of the triggerWebhooks flag, and remove the attachment persistence code from inside the webhook-only conditional branch so the webhook branch focuses only on emission.
113-125:⚠️ Potential issue | 🔴 CriticalConstrain PDU path before any read or delete operation.
EXTRA_PDU_PATHis extracted from an exported broadcast intent without validation. The code reads the file at line 125 without any canonical path check, allowing an attacker to read arbitrary files. The delete guard at line 206-209 does canonicalize, but it permits deletion of any file underfiles/mms-in/—the same tree where persisted MMS attachments are stored. A spoofed callback can therefore delete legitimate attachment files.Implement a dedicated temporary PDU directory separate from attachment storage and validate the resolved path before any file operation (
exists(),readBytes(), ordelete()).🛡️ Suggested direction
- val file = File(path) + val file = resolvePduFile(context, path) ?: return if (!file.exists()) { logsService.insert( LogEntry.Priority.WARN, @@ private fun deletePduFile(context: Context, path: String?) { if (path.isNullOrBlank()) return runCatching { - val file = File(path).canonicalFile - val root = File(context.filesDir, "mms-in").canonicalFile - if (file.path.startsWith(root.path + File.separator)) { + val file = resolvePduFile(context, path) ?: return@runCatching + if (file.isFile) { file.delete() } else { logsService.insert( @@ } } + + private fun resolvePduFile(context: Context, path: String): File? { + // Use a temp PDU directory that is distinct from MmsAttachmentStorage's + // persisted attachment root, and update the producer to write there too. + val root = File(context.filesDir, "mms-pdu").canonicalFile + val file = File(path).canonicalFile + if (file.parentFile?.canonicalFile != root) { + logsService.insert( + LogEntry.Priority.WARN, + MODULE_NAME, + "Refused MMS PDU path outside temp PDU directory", + mapOf("path" to path) + ) + return null + } + return file + }Update the producer to write PDU files to the new temp-only directory:
- putExtra(EventsReceiver.EXTRA_PDU_PATH, file.absolutePath) + putExtra(EventsReceiver.EXTRA_PDU_PATH, File(context.filesDir, "mms-pdu").apply { mkdirs() }.resolve(file.name).absolutePath)Also applies to: 203–209
🤖 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 `@app/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.kt` around lines 113 - 125, The PDU path extracted from the EXTRA_PDU_PATH broadcast intent is not validated before file operations, allowing arbitrary file reads and deletions. Create a dedicated temporary PDU directory separate from the attachment storage directory used for MMS files. Before any file operation including exists(), readBytes() (at line 125), and delete() (at lines 206-209), validate that the resolved canonical path remains within this temporary PDU directory. This prevents both unauthorized reads of arbitrary files and prevents deletion of legitimate attachment files stored in the mms-in directory. Update the producer to write PDU files to the new temp-only directory, and add canonical path validation checks at both the read operation and the delete operation locations.
🤖 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.
Outside diff comments:
In `@app/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.kt`:
- Around line 222-235: The logsService.insert calls in ReceiverService::process
are logging the full message object which can contain base64 attachment data and
message bodies from MMS messages, exposing sensitive media content in logs.
Replace the mapOf("message" to message) parameter in both logsService.insert
calls with metadata-only information that identifies the message (such as its ID
or type) without including the actual message content or attachment data. This
ensures sensitive message bodies and media are not persisted in debug logs.
- Around line 160-173: The MMS attachment persistence logic is currently
duplicated and only executed within the webhook-triggered branch, causing
downloaded attachments to be stored twice while non-webhook calls never create
downloadable files. Extract the attachment storage and error handling logic
(currently in the try-catch block with attachmentStorage.store and
logsService.insert) into a new private helper method called
persistMmsAttachments that accepts an InboxMessage.MMS parameter and iterates
over its attachments. Call this helper method once after message deduplication
for every MMS message, regardless of the triggerWebhooks flag, and remove the
attachment persistence code from inside the webhook-only conditional branch so
the webhook branch focuses only on emission.
- Around line 113-125: The PDU path extracted from the EXTRA_PDU_PATH broadcast
intent is not validated before file operations, allowing arbitrary file reads
and deletions. Create a dedicated temporary PDU directory separate from the
attachment storage directory used for MMS files. Before any file operation
including exists(), readBytes() (at line 125), and delete() (at lines 206-209),
validate that the resolved canonical path remains within this temporary PDU
directory. This prevents both unauthorized reads of arbitrary files and prevents
deletion of legitimate attachment files stored in the mms-in directory. Update
the producer to write PDU files to the new temp-only directory, and add
canonical path validation checks at both the read operation and the delete
operation locations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f4873861-0201-4bcb-89a7-237b5a533126
📒 Files selected for processing (1)
app/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.kt
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 (1)
app/src/main/java/me/capcom/smsgateway/receivers/EventsReceiver.kt (1)
65-70:⚠️ Potential issue | 🟠 MajorHarden the status-broadcast trust boundary before accepting
ACTION_MMS_SENT.This dynamic receiver accepts custom status actions without sender validation. While explicit intents targeting
EventsReceiver::class.javaprovide some protection, the hardcoded action strings are discoverable in the APK, and message IDs are enumerable. A malicious app could send spoofed broadcasts targeting the app's receiver and tamper with message state viaprocessStateIntent, which updates state and triggers cleanup based only onEXTRA_MESSAGE_IDandresultCodewith no verification of intent authenticity.Consider securing this with: (1) non-exported dynamic registration flags where supported, and (2) a per-message nonce/token validated in
processStateIntentbefore state mutation.🤖 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 `@app/src/main/java/me/capcom/smsgateway/receivers/EventsReceiver.kt` around lines 65 - 70, The dynamic receiver registration in EventsReceiver accepts broadcast actions without sender validation, creating a security risk where malicious apps can spoof broadcasts. Add the RECEIVER_NOT_EXPORTED flag to the context.registerReceiver call (for Android 12+) to restrict which apps can send intents to this receiver. Additionally, modify the processStateIntent method to validate intent authenticity by implementing a per-message verification mechanism (such as a nonce or token) that is checked before any state mutation occurs based on EXTRA_MESSAGE_ID and resultCode, ensuring only legitimate status broadcasts from the system are processed.
🤖 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
`@app/src/main/java/me/capcom/smsgateway/modules/incoming/IncomingMessagesService.kt`:
- Around line 36-38: The persistMmsAttachments method is being called before the
message is inserted into the repository, causing orphaned attachment files if
the database insert fails. Move the persistMmsAttachments call to execute after
the repository.insert operation completes successfully. This ensures attachments
are only persisted when the corresponding inbox message row is guaranteed to
exist in the database. Apply this ordering fix to all MMS attachment persistence
operations in the file, including the location mentioned at lines 49-63.
- Around line 68-73: The Base64 decoding operation in the
IncomingMessagesService uses runCatching with getOrNull() which silently
converts decode failures to null with no logging, making attachment decode
errors invisible for debugging. Modify the decoded variable assignment to
capture and log decode failures by adding a failure handler to the runCatching
block that logs the exception when Base64.decode fails, ensuring attachment
decode errors are tracked and visible in the application logs.
---
Outside diff comments:
In `@app/src/main/java/me/capcom/smsgateway/receivers/EventsReceiver.kt`:
- Around line 65-70: The dynamic receiver registration in EventsReceiver accepts
broadcast actions without sender validation, creating a security risk where
malicious apps can spoof broadcasts. Add the RECEIVER_NOT_EXPORTED flag to the
context.registerReceiver call (for Android 12+) to restrict which apps can send
intents to this receiver. Additionally, modify the processStateIntent method to
validate intent authenticity by implementing a per-message verification
mechanism (such as a nonce or token) that is checked before any state mutation
occurs based on EXTRA_MESSAGE_ID and resultCode, ensuring only legitimate status
broadcasts from the system are processed.
🪄 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: CHILL
Plan: Pro
Run ID: 8b6ad32e-e997-4eab-856d-cade3ab21b0c
📒 Files selected for processing (3)
app/src/main/java/me/capcom/smsgateway/modules/incoming/IncomingMessagesService.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.ktapp/src/main/java/me/capcom/smsgateway/receivers/EventsReceiver.kt
36c029e to
cc0d93f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/assets/api/swagger.json (1)
2272-2274: ⚡ Quick winDocument 404 error response body for consistency.
Other endpoints (e.g.,
DELETE /devices/{id}at lines 1896-1904) return a structuredErrorResponseschema for 404 errors, but this endpoint's 404 response has no content schema defined.📋 Proposed fix for consistent error handling
"404": { - "description": "Attachment not found" + "description": "Attachment not found", + "content": { + "application/json": { + "schema": { + "$ref": "`#/components/schemas/smsgateway.ErrorResponse`" + } + } + } }🤖 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 `@app/src/main/assets/api/swagger.json` around lines 2272 - 2274, The 404 response for this endpoint in the swagger.json file currently only includes a description without defining a response body schema. To fix this, add a content schema definition to the 404 response object (at lines 2272-2274) that references the ErrorResponse schema, matching the same pattern used in other endpoints like the DELETE /devices/{id} endpoint (referenced around lines 1896-1904). Include the appropriate media type (application/json) and schema reference to ensure consistent error response documentation across all endpoints.
🤖 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.
Nitpick comments:
In `@app/src/main/assets/api/swagger.json`:
- Around line 2272-2274: The 404 response for this endpoint in the swagger.json
file currently only includes a description without defining a response body
schema. To fix this, add a content schema definition to the 404 response object
(at lines 2272-2274) that references the ErrorResponse schema, matching the same
pattern used in other endpoints like the DELETE /devices/{id} endpoint
(referenced around lines 1896-1904). Include the appropriate media type
(application/json) and schema reference to ensure consistent error response
documentation across all endpoints.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d35b668-84f7-4170-8d3f-51b17388caae
📒 Files selected for processing (45)
README.mdapp/src/main/AndroidManifest.xmlapp/src/main/assets/api/swagger.jsonapp/src/main/java/me/capcom/smsgateway/App.ktapp/src/main/java/me/capcom/smsgateway/data/entities/Message.ktapp/src/main/java/me/capcom/smsgateway/domain/MessageContent.ktapp/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayApi.ktapp/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.ktapp/src/main/java/me/capcom/smsgateway/modules/incoming/IncomingMessagesService.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/WebService.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/domain/messages/Message.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/domain/messages/MmsMessage.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/domain/messages/PostMessageRequest.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/routes/InboxRoutes.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/routes/MessagesRoutes.ktapp/src/main/java/me/capcom/smsgateway/modules/messages/MessagesRepository.ktapp/src/main/java/me/capcom/smsgateway/modules/messages/MessagesService.ktapp/src/main/java/me/capcom/smsgateway/modules/mms/MmsAttachmentStorage.ktapp/src/main/java/me/capcom/smsgateway/modules/mms/MmsSender.ktapp/src/main/java/me/capcom/smsgateway/modules/mms/Module.ktapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/CharacterSets.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/ContentType.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/EncodedStringValue.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/GenericPdu.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/InvalidHeaderValueException.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/MmsException.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/MultimediaMessagePdu.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduBody.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduComposer.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduContentTypes.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduHeaders.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduPart.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/SendReq.javaapp/src/main/java/me/capcom/smsgateway/modules/receiver/MessagesReceiver.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/MmsContentObserver.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/MmsReceiver.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/StateStorage.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/parsers/MMSParser.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/parsers/MMSRetrieveParser.ktapp/src/main/java/me/capcom/smsgateway/receivers/EventsReceiver.ktapp/src/main/res/values-zh/strings.xmlapp/src/main/res/xml/file_paths.xmlapp/src/main/res/xml/root_preferences.xmldocs/MMS.md
✅ Files skipped from review due to trivial changes (4)
- app/src/main/java/me/capcom/smsgateway/App.kt
- README.md
- app/src/main/res/values-zh/strings.xml
- docs/MMS.md
🚧 Files skipped from review as they are similar to previous changes (38)
- app/src/main/java/me/capcom/smsgateway/modules/receiver/MessagesReceiver.kt
- app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesRepository.kt
- app/src/main/java/me/capcom/smsgateway/modules/localserver/WebService.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/MmsException.java
- app/src/main/java/me/capcom/smsgateway/modules/mms/Module.kt
- app/src/main/java/me/capcom/smsgateway/modules/localserver/domain/messages/MmsMessage.kt
- app/src/main/java/me/capcom/smsgateway/modules/localserver/domain/messages/Message.kt
- app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsReceiver.kt
- app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.kt
- app/src/main/java/me/capcom/smsgateway/data/entities/Message.kt
- app/src/main/AndroidManifest.xml
- app/src/main/java/me/capcom/smsgateway/modules/localserver/domain/messages/PostMessageRequest.kt
- app/src/main/java/me/capcom/smsgateway/domain/MessageContent.kt
- app/src/main/res/xml/root_preferences.xml
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/CharacterSets.java
- app/src/main/res/xml/file_paths.xml
- app/src/main/java/me/capcom/smsgateway/modules/receiver/StateStorage.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/InvalidHeaderValueException.java
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/GenericPdu.java
- app/src/main/java/me/capcom/smsgateway/modules/incoming/IncomingMessagesService.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduContentTypes.java
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduBody.java
- app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesService.kt
- app/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/MmsAttachmentStorage.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/MultimediaMessagePdu.java
- app/src/main/java/me/capcom/smsgateway/modules/receiver/parsers/MMSParser.kt
- app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/InboxRoutes.kt
- app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsContentObserver.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/ContentType.java
- app/src/main/java/me/capcom/smsgateway/modules/receiver/parsers/MMSRetrieveParser.kt
- app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayApi.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduPart.java
- app/src/main/java/me/capcom/smsgateway/modules/mms/MmsSender.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/EncodedStringValue.java
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduComposer.java
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduHeaders.java
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/SendReq.java
cc0d93f to
9744a44
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/assets/api/swagger.json (1)
43-65: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider adding validation to ensure MMS has content.
smsgateway.MmsMessagehas no required fields—a request could technically pass validation withnullsubject,nulltext, and an empty/missingattachmentsarray, resulting in an empty MMS. Consider adding aminItems: 1onattachmentsor using schema composition to ensure at least one ofsubject,text, orattachmentsis provided.🤖 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 `@app/src/main/assets/api/swagger.json` around lines 43 - 65, The smsgateway.MmsMessage schema currently has no required fields, allowing empty or null values for all properties. To ensure valid MMS content, add either a minItems constraint of 1 to the attachments array property to guarantee at least one attachment is present, or alternatively use schema composition with oneOf/anyOf to enforce that at least one of the subject, text, or attachments fields must be provided with non-null values. Choose the approach that best fits the application's requirements for MMS message validation.
🤖 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.
Nitpick comments:
In `@app/src/main/assets/api/swagger.json`:
- Around line 43-65: The smsgateway.MmsMessage schema currently has no required
fields, allowing empty or null values for all properties. To ensure valid MMS
content, add either a minItems constraint of 1 to the attachments array property
to guarantee at least one attachment is present, or alternatively use schema
composition with oneOf/anyOf to enforce that at least one of the subject, text,
or attachments fields must be provided with non-null values. Choose the approach
that best fits the application's requirements for MMS message validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7ac69ba-3152-4397-818e-27eda9ccc932
📒 Files selected for processing (45)
README.mdapp/src/main/AndroidManifest.xmlapp/src/main/assets/api/swagger.jsonapp/src/main/java/me/capcom/smsgateway/App.ktapp/src/main/java/me/capcom/smsgateway/data/entities/Message.ktapp/src/main/java/me/capcom/smsgateway/domain/MessageContent.ktapp/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayApi.ktapp/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.ktapp/src/main/java/me/capcom/smsgateway/modules/incoming/IncomingMessagesService.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/WebService.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/domain/messages/Message.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/domain/messages/MmsMessage.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/domain/messages/PostMessageRequest.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/routes/InboxRoutes.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/routes/MessagesRoutes.ktapp/src/main/java/me/capcom/smsgateway/modules/messages/MessagesRepository.ktapp/src/main/java/me/capcom/smsgateway/modules/messages/MessagesService.ktapp/src/main/java/me/capcom/smsgateway/modules/mms/MmsAttachmentStorage.ktapp/src/main/java/me/capcom/smsgateway/modules/mms/MmsSender.ktapp/src/main/java/me/capcom/smsgateway/modules/mms/Module.ktapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/CharacterSets.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/ContentType.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/EncodedStringValue.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/GenericPdu.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/InvalidHeaderValueException.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/MmsException.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/MultimediaMessagePdu.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduBody.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduComposer.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduContentTypes.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduHeaders.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduPart.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/SendReq.javaapp/src/main/java/me/capcom/smsgateway/modules/receiver/MessagesReceiver.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/MmsContentObserver.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/MmsReceiver.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/StateStorage.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/parsers/MMSParser.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/parsers/MMSRetrieveParser.ktapp/src/main/java/me/capcom/smsgateway/receivers/EventsReceiver.ktapp/src/main/res/values-zh/strings.xmlapp/src/main/res/xml/file_paths.xmlapp/src/main/res/xml/root_preferences.xmldocs/MMS.md
✅ Files skipped from review due to trivial changes (4)
- README.md
- app/src/main/res/xml/root_preferences.xml
- app/src/main/res/xml/file_paths.xml
- app/src/main/res/values-zh/strings.xml
🚧 Files skipped from review as they are similar to previous changes (40)
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/InvalidHeaderValueException.java
- app/src/main/java/me/capcom/smsgateway/modules/mms/Module.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/MmsException.java
- app/src/main/java/me/capcom/smsgateway/modules/receiver/parsers/MMSParser.kt
- app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsReceiver.kt
- app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesRepository.kt
- app/src/main/java/me/capcom/smsgateway/modules/localserver/domain/messages/Message.kt
- app/src/main/java/me/capcom/smsgateway/modules/localserver/domain/messages/MmsMessage.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/MultimediaMessagePdu.java
- app/src/main/java/me/capcom/smsgateway/modules/localserver/WebService.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduContentTypes.java
- app/src/main/java/me/capcom/smsgateway/data/entities/Message.kt
- app/src/main/java/me/capcom/smsgateway/modules/receiver/StateStorage.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/ContentType.java
- app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/MessagesRoutes.kt
- app/src/main/java/me/capcom/smsgateway/App.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/CharacterSets.java
- app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.kt
- docs/MMS.md
- app/src/main/java/me/capcom/smsgateway/modules/localserver/domain/messages/PostMessageRequest.kt
- app/src/main/java/me/capcom/smsgateway/modules/receiver/MessagesReceiver.kt
- app/src/main/AndroidManifest.xml
- app/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduBody.java
- app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayApi.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/MmsAttachmentStorage.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduHeaders.java
- app/src/main/java/me/capcom/smsgateway/receivers/EventsReceiver.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/SendReq.java
- app/src/main/java/me/capcom/smsgateway/modules/receiver/parsers/MMSRetrieveParser.kt
- app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/InboxRoutes.kt
- app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsContentObserver.kt
- app/src/main/java/me/capcom/smsgateway/modules/incoming/IncomingMessagesService.kt
- app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesService.kt
- app/src/main/java/me/capcom/smsgateway/domain/MessageContent.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/GenericPdu.java
- app/src/main/java/me/capcom/smsgateway/modules/mms/MmsSender.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/EncodedStringValue.java
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduPart.java
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduComposer.java
9744a44 to
eb09679
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/assets/api/swagger.json (1)
2272-2274: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd an error body schema to the
404response for consistency.The
400and401responses referencesmsgateway.ErrorResponse, but404returns no content. Aligning it keeps error handling consistent for clients.♻️ Proposed change
"404": { - "description": "Attachment not found" + "description": "Attachment not found", + "content": { + "application/json": { + "schema": { + "$ref": "`#/components/schemas/smsgateway.ErrorResponse`" + } + } + } }🤖 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 `@app/src/main/assets/api/swagger.json` around lines 2272 - 2274, The `404` response definition for the Attachment not found endpoint is missing a schema reference, while the `400` and `401` responses properly reference the smsgateway.ErrorResponse schema. Add a schema property to the `404` response object that references smsgateway.ErrorResponse, matching the pattern used in the `400` and `401` response definitions to ensure consistent error response structure across all error statuses.
🤖 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.
Nitpick comments:
In `@app/src/main/assets/api/swagger.json`:
- Around line 2272-2274: The `404` response definition for the Attachment not
found endpoint is missing a schema reference, while the `400` and `401`
responses properly reference the smsgateway.ErrorResponse schema. Add a schema
property to the `404` response object that references smsgateway.ErrorResponse,
matching the pattern used in the `400` and `401` response definitions to ensure
consistent error response structure across all error statuses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c8c2d963-e24b-4dd5-adb1-1bcdb043e4a7
📒 Files selected for processing (44)
README.mdapp/src/main/AndroidManifest.xmlapp/src/main/assets/api/swagger.jsonapp/src/main/java/me/capcom/smsgateway/App.ktapp/src/main/java/me/capcom/smsgateway/data/entities/Message.ktapp/src/main/java/me/capcom/smsgateway/domain/MessageContent.ktapp/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayApi.ktapp/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.ktapp/src/main/java/me/capcom/smsgateway/modules/incoming/IncomingMessagesService.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/WebService.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/domain/messages/Message.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/domain/messages/MmsMessage.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/domain/messages/PostMessageRequest.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/routes/InboxRoutes.ktapp/src/main/java/me/capcom/smsgateway/modules/localserver/routes/MessagesRoutes.ktapp/src/main/java/me/capcom/smsgateway/modules/messages/MessagesRepository.ktapp/src/main/java/me/capcom/smsgateway/modules/messages/MessagesService.ktapp/src/main/java/me/capcom/smsgateway/modules/mms/MmsAttachmentStorage.ktapp/src/main/java/me/capcom/smsgateway/modules/mms/MmsSender.ktapp/src/main/java/me/capcom/smsgateway/modules/mms/Module.ktapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/CharacterSets.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/ContentType.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/EncodedStringValue.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/GenericPdu.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/InvalidHeaderValueException.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/MmsException.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/MultimediaMessagePdu.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduBody.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduComposer.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduContentTypes.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduHeaders.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduPart.javaapp/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/SendReq.javaapp/src/main/java/me/capcom/smsgateway/modules/receiver/MessagesReceiver.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/MmsContentObserver.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/MmsReceiver.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/parsers/MMSParser.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/parsers/MMSRetrieveParser.ktapp/src/main/java/me/capcom/smsgateway/receivers/EventsReceiver.ktapp/src/main/res/values-zh/strings.xmlapp/src/main/res/xml/file_paths.xmlapp/src/main/res/xml/root_preferences.xmldocs/MMS.md
✅ Files skipped from review due to trivial changes (7)
- app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsReceiver.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduContentTypes.java
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/GenericPdu.java
- README.md
- app/src/main/res/xml/root_preferences.xml
- app/src/main/res/values-zh/strings.xml
- docs/MMS.md
🚧 Files skipped from review as they are similar to previous changes (35)
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/InvalidHeaderValueException.java
- app/src/main/java/me/capcom/smsgateway/modules/localserver/domain/messages/MmsMessage.kt
- app/src/main/res/xml/file_paths.xml
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/MmsException.java
- app/src/main/java/me/capcom/smsgateway/modules/localserver/domain/messages/Message.kt
- app/src/main/java/me/capcom/smsgateway/modules/receiver/parsers/MMSParser.kt
- app/src/main/java/me/capcom/smsgateway/App.kt
- app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesRepository.kt
- app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/CharacterSets.java
- app/src/main/java/me/capcom/smsgateway/modules/localserver/WebService.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/Module.kt
- app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayApi.kt
- app/src/main/java/me/capcom/smsgateway/modules/receiver/MessagesReceiver.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduBody.java
- app/src/main/java/me/capcom/smsgateway/modules/localserver/domain/messages/PostMessageRequest.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/MultimediaMessagePdu.java
- app/src/main/java/me/capcom/smsgateway/receivers/EventsReceiver.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/SendReq.java
- app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsContentObserver.kt
- app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesService.kt
- app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/InboxRoutes.kt
- app/src/main/java/me/capcom/smsgateway/data/entities/Message.kt
- app/src/main/java/me/capcom/smsgateway/modules/receiver/parsers/MMSRetrieveParser.kt
- app/src/main/AndroidManifest.xml
- app/src/main/java/me/capcom/smsgateway/domain/MessageContent.kt
- app/src/main/java/me/capcom/smsgateway/modules/localserver/routes/MessagesRoutes.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduComposer.java
- app/src/main/java/me/capcom/smsgateway/modules/incoming/IncomingMessagesService.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduPart.java
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/EncodedStringValue.java
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/PduHeaders.java
- app/src/main/java/me/capcom/smsgateway/modules/mms/MmsAttachmentStorage.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/MmsSender.kt
- app/src/main/java/me/capcom/smsgateway/modules/mms/pdu/aosp/ContentType.java
eb09679 to
07f1b2d
Compare
|
This PR is stale because it has been open for 7 days with no activity. |
Add complete MMS handling without requiring default SMS app role:
Send path:
- PduComposer from AOSP for building MMS PDUs
- MmsSender dispatches via platform SmsManager.sendMultimediaMessage
- Outgoing PDU cleanup after send result
Receive path:
- MmsReceiver intercepts WAP_PUSH_RECEIVED and parses M-Notification.ind
- ReceiverService orchestrates MMS retrieval via carrier MMSC
- MMSRetrieveParser decodes retrieved M-Retrieve.conf PDUs
- MmsAttachmentStorage persists parts under files/mms-in/<hash>/
Inbox & API:
- SmsContentObserver polls content://sms for incoming SMS (replaces
BroadcastReceiver path that carriers can suppress)
- MmsContentObserver watches content://mms for platform-downloaded MMS
- InboxRoutes serves attachment bytes via GET /inbox/{id}/attachments/{partId}
- Webhook payloads include inline base64 attachment data
Infrastructure:
- PingForegroundService made idempotent to avoid crash on rapid restarts
- StateStorage tracks high-water marks for content observers
07f1b2d to
883ee5d
Compare
Summary by CodeRabbit
New Features
Bug Fixes
Documentation