[webhooks] add destination number in *:received payload#319
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds SubscriptionsHelper.getPhoneNumber(context, simSlotIndex) and changes SMS/MMS flows to populate explicit sender and recipient (plus simNumber) in webhook payloads; updates payload models and the OpenAPI spec to reflect new webhook event/payload shapes and callbacks. Changes
Sequence DiagramsequenceDiagram
participant RC as ReceiverService
participant WH as EventsReceiver
participant SH as SubscriptionsHelper
participant SM as SubscriptionManager
participant WEB as WebhookEndpoint
RC->>RC: Parse incoming message (address, destinations, subscriptionId)
RC->>WH: Emit event (simNumber, destinations, message data)
loop per destination
WH->>SH: getPhoneNumber(context, simNumber - 1)
SH->>SM: query activeSubscriptionInfoList
SM-->>SH: subscription info / phoneNumber
SH-->>WH: sender (nullable)
WH->>WEB: POST webhook payload (sender, recipient, simNumber, event)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
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/modules/webhooks/payload/SmsEventPayload.kt (1)
5-11:⚠️ Potential issue | 🔴 CriticalType mismatch:
simNumber: Int?passed to non-nullIntparameter inMessageEventPayload
SmsEventPayloaddeclaressimNumber: Int?(line 8) but passes it toMessageEventPayloadwhich expectssimNumber: Int(non-null). This is a compilation error on top of the argument count mismatch already noted inMessageEventPayload.kt.If
simNumbertruly needs to be nullable inSmsEventPayload(EventsReceiver passesevent.simNumberwhich appears nullable), then eitherMessageEventPayload.simNumbershould also beInt?, or the coercion to a default (e.g.,simNumber ?: 0as done inReceiverService) should happen here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/me/capcom/smsgateway/modules/webhooks/payload/SmsEventPayload.kt` around lines 5 - 11, SmsEventPayload is declaring simNumber as nullable (simNumber: Int?) but passes it to the base class MessageEventPayload which expects a non-null Int; fix by either changing MessageEventPayload.simNumber to Int? to accept nulls, or coerce the nullable here before calling the superclass (e.g., pass simNumber ?: <default>) so the types align; update the constructor signature in MessageEventPayload or the SmsEventPayload constructor call accordingly and ensure all callers (e.g., EventsReceiver) remain consistent with the chosen approach.
🧹 Nitpick comments (2)
app/src/main/java/me/capcom/smsgateway/modules/webhooks/EventsReceiver.kt (2)
43-49: Sender lookup logic is clean; consider usingnullover empty stringThe fallback chain (subscriptionId → simNumber) is well-structured. However, defaulting
senderto""(lines 59, 68, 78) when the device's own number can't be determined produces an empty string in the JSON payload. Anullvalue would more clearly convey "unknown" to webhook consumers.This would require changing
sendertoString?in the payload classes, so it's a design decision.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/me/capcom/smsgateway/modules/webhooks/EventsReceiver.kt` around lines 43 - 49, Change the sender fallback so it yields null instead of an empty string: when resolving sender in EventsReceiver.kt (the local val sender computed from event.subscriptionId and event.simNumber using SubscriptionsHelper.getPhoneNumberBySubscriptionId / getPhoneNumber), return null if no phone number is found rather than "" and update any webhook payload data classes that hold sender to use a nullable String (String?) so the serialized JSON emits null for unknown senders.
51-88:phoneNumberanddestinationare assigned the same value for outgoing eventsFor all three event types (
SmsSent,SmsDelivered,SmsFailed), bothphoneNumberanddestinationare set to the loop variabledestination(the recipient number fromevent.phoneNumbers). While this maintains backward compatibility forphoneNumber, it's redundant and may confuse consumers.Is the intent that
phoneNumberremains the backward-compatible "other party" field, whiledestinationis the newly named equivalent? If so, this is fine but worth a code comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/me/capcom/smsgateway/modules/webhooks/EventsReceiver.kt` around lines 51 - 88, The loop in EventsReceiver.kt sets both SmsEventPayload.phoneNumber and destination to the same loop variable destination for SmsSent/SmsDelivered/SmsFailed; add a brief code comment above the payload construction (near the event.phoneNumbers.forEach and before SmsEventPayload.SmsSent/Delivered/Failed) clarifying intent—e.g., that phoneNumber is preserved for backward compatibility as the "other party" field while destination is the new explicit recipient field—so future readers know this duplication is intentional (reference webhookEventType, SmsEventPayload.* classes, event.phoneNumbers, and the get<WebHooksService>().emit call).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/me/capcom/smsgateway/helpers/SubscriptionsHelper.kt`:
- Around line 88-121: Both getPhoneNumber and getPhoneNumberBySubscriptionId
risk NPEs because subscriptionManager.activeSubscriptionInfoList can be null and
also use deprecated SubscriptionInfo.number on API 33+; update both functions
(getPhoneNumber and getPhoneNumberBySubscriptionId) to safely handle a null
activeSubscriptionInfoList by using the null-safe operator (e.g.,
activeSubscriptionInfoList?.find { ... }) before calling .find and .number, and
for API 33+ use SubscriptionManager.getPhoneNumber(subscriptionId) when
available (falling back to SubscriptionInfo.number only for older APIs) while
preserving the permission checks and existing return behavior.
In
`@app/src/main/java/me/capcom/smsgateway/modules/webhooks/payload/MessageEventPayload.kt`:
- Around line 3-12: The constructor parameter list of MessageEventPayload does
not match its subclasses: either reintroduce the removed phoneNumber parameter
in MessageEventPayload's constructor (kept as non-val if you only need it for
serialization) and preserve the deprecated phoneNumber getter, or update the
subclass super calls in SmsEventPayload and MmsReceivedPayload to pass only
(messageId, sender, simNumber, destination) and fix simNumber nullability
(change SmsEventPayload.simNumber: Int? to Int or make
MessageEventPayload.simNumber nullable). Locate MessageEventPayload,
SmsEventPayload, and MmsReceivedPayload and choose one approach: (A) add back
phoneNumber parameter to MessageEventPayload signature and keep the deprecated
phoneNumber getter, or (B) remove the extra argument from subclass super(...)
invocations and align simNumber types consistently between SmsEventPayload and
MessageEventPayload.
- Around line 14-16: Update the reference comment in MessageEventPayload.kt to
fix the typo: change "desing.md" to "design.md" in the comment block (the
Javadoc-style reference at the top of the file associated with the
MessageEventPayload class) so the documented path correctly reads
dev-docs/android-app/tasks/2026-02-17_sms-received-webhook-destination-number/01-design.md.
In
`@app/src/main/java/me/capcom/smsgateway/modules/webhooks/payload/MmsReceivedPayload.kt`:
- Around line 5-20: The MmsReceivedPayload constructor currently includes an
unused phoneNumber parameter and calls the superclass with incorrect arguments;
update the MmsReceivedPayload declaration to remove the phoneNumber constructor
parameter and change the superclass invocation to MessageEventPayload(messageId,
sender, simNumber, destination) so the parameter order matches the parent
constructor; keep the deprecated phoneNumber property (val phoneNumber: String
get() = sender) as-is to preserve the intended API.
In
`@app/src/main/java/me/capcom/smsgateway/modules/webhooks/payload/SmsEventPayload.kt`:
- Around line 17-25: The SmsSent payload currently causes phoneNumber and
destination to carry the same recipient value — update the
construction/definition so semantics are consistent: either make
SmsSent.phoneNumber represent the device/sender number (match
MessageEventPayload behavior) or change EventsReceiver.kt to pass the device
number into the phoneNumber parameter. Locate the SmsSent class and the code in
EventsReceiver.kt that constructs SmsSent and choose one fix: (a) alter SmsSent
constructor signature/field ordering so phoneNumber is the device/sender and
destination is the recipient, or (b) keep the current SmsSent signature but
change the call site in EventsReceiver.kt to pass sender for phoneNumber and
recipient for destination; ensure serialized JSON fields reflect the intended
semantics and update any dependent code/tests accordingly.
---
Outside diff comments:
In
`@app/src/main/java/me/capcom/smsgateway/modules/webhooks/payload/SmsEventPayload.kt`:
- Around line 5-11: SmsEventPayload is declaring simNumber as nullable
(simNumber: Int?) but passes it to the base class MessageEventPayload which
expects a non-null Int; fix by either changing MessageEventPayload.simNumber to
Int? to accept nulls, or coerce the nullable here before calling the superclass
(e.g., pass simNumber ?: <default>) so the types align; update the constructor
signature in MessageEventPayload or the SmsEventPayload constructor call
accordingly and ensure all callers (e.g., EventsReceiver) remain consistent with
the chosen approach.
---
Nitpick comments:
In `@app/src/main/java/me/capcom/smsgateway/modules/webhooks/EventsReceiver.kt`:
- Around line 43-49: Change the sender fallback so it yields null instead of an
empty string: when resolving sender in EventsReceiver.kt (the local val sender
computed from event.subscriptionId and event.simNumber using
SubscriptionsHelper.getPhoneNumberBySubscriptionId / getPhoneNumber), return
null if no phone number is found rather than "" and update any webhook payload
data classes that hold sender to use a nullable String (String?) so the
serialized JSON emits null for unknown senders.
- Around line 51-88: The loop in EventsReceiver.kt sets both
SmsEventPayload.phoneNumber and destination to the same loop variable
destination for SmsSent/SmsDelivered/SmsFailed; add a brief code comment above
the payload construction (near the event.phoneNumbers.forEach and before
SmsEventPayload.SmsSent/Delivered/Failed) clarifying intent—e.g., that
phoneNumber is preserved for backward compatibility as the "other party" field
while destination is the new explicit recipient field—so future readers know
this duplication is intentional (reference webhookEventType, SmsEventPayload.*
classes, event.phoneNumbers, and the get<WebHooksService>().emit call).
ffd25b8 to
dbfccd5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/me/capcom/smsgateway/modules/webhooks/payload/MessageEventPayload.kt`:
- Around line 5-10: The Depracation ReplaceWith("recipient") on the
MessageEventPayload property phoneNumber is misleading for incoming events
because some subclasses (e.g., SmsReceived, SmsDataReceived, MmsReceivedPayload)
pass sender into phoneNumber; update the deprecation to avoid incorrect
automated replacements by either removing the ReplaceWith parameter entirely
from the `@Deprecated` annotation on val phoneNumber or change the message to
explicitly state direction-dependent replacement (e.g., "Use sender for received
events or recipient for sent events") and remove the ReplaceWith argument;
locate the phoneNumber property in MessageEventPayload and adjust the
`@Deprecated` annotation accordingly so callers aren’t auto-migrated to recipient
when they should use sender.
---
Duplicate comments:
In `@app/src/main/java/me/capcom/smsgateway/helpers/SubscriptionsHelper.kt`:
- Around line 97-103: subscriptionManager.activeSubscriptionInfoList may be null
on older SDKs; update every occurrence of
subscriptionManager.activeSubscriptionInfoList.find { ... } (the calls in the
methods using the expression subscriptionManager.activeSubscriptionInfoList.find
— e.g., the functions that retrieve the SIM number by simSlotIndex) to use a
safe-call before .find (subscriptionManager.activeSubscriptionInfoList?.find {
... }) and propagate the nullable result (keep the existing ?.number?.takeIf {
it.isNotBlank() } logic) so you don't dereference a null list; apply the same
safe-call fix to the other occurrences mentioned (the calls at the earlier
methods and the one at line 115).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/me/capcom/smsgateway/modules/webhooks/payload/MessageEventPayload.kt`:
- Around line 9-10: The deprecated phoneNumber constructor property on
MessageEventPayload creates a nullability mismatch and allows callers to pass a
divergent value; change phoneNumber from a primary-constructor val into a body
property that delegates to sender and recipient to enforce the invariant (e.g.
annotate phoneNumber with `@Deprecated` and implement it as val phoneNumber:
String? get() = sender ?: recipient, or if downstream requires non-null use
get() = sender ?: recipient ?: ""), or use a backing field initialized to sender
?: recipient if your serializer needs a concrete field; update the
MessageEventPayload declaration to remove phoneNumber from the constructor and
implement the computed/delegated property so sender/recipient remain the single
source of truth.
---
Duplicate comments:
In `@app/src/main/java/me/capcom/smsgateway/helpers/SubscriptionsHelper.kt`:
- Around line 97-103: The code accesses the deprecated SubscriptionInfo.number
(on subscriptionManager.activeSubscriptionInfoList) which triggers a compiler
warning on API 33+; update getSimPhoneNumber to either suppress the deprecation
or preferably branch by API level: for API 33+ call
SubscriptionManager.getPhoneNumber(subscriptionId) after confirming
READ_PHONE_NUMBERS permission (extend or add to hasPhoneStatePermission), and
for older APIs continue using SubscriptionInfo.number; if you only want to
silence the warning immediately, add `@Suppress`("DEPRECATION") to the function
containing the use of SubscriptionInfo.number (referencing getSimPhoneNumber /
subscriptionManager.activeSubscriptionInfoList / simSlotIndex /
hasPhoneStatePermission).
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
app/src/main/assets/api/swagger.json (2)
2207-2209: Inconsistent naming:WebHookEventvssmsgateway.WebhookEvent.The existing event-type enum is named
smsgateway.WebhookEvent(lowercase 'h'), while this callback schema is namedWebHookEvent(uppercase 'H') and drops thesmsgateway.prefix. The casing mismatch (WebhookvsWebHook) is likely to confuse SDK generators and API consumers. Consider aligning casing and prefix convention.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/assets/api/swagger.json` around lines 2207 - 2209, The schema name WebHookEvent is inconsistent with the existing event-type smsgateway.WebhookEvent (difference in casing and missing namespace); rename the schema to match the existing identifier (smsgateway.WebhookEvent with the same camelCasing "WebhookEvent" used elsewhere) and update any references in swagger.json to use smsgateway.WebhookEvent so SDK generators and consumers see a single canonical type name; ensure both the schema key and any $ref strings are updated to the exact same identifier.
1436-1447: Static analysis: basic auth scheme over potentially unencrypted channel.Checkov flags
ApiAuth(HTTP basic) as allowing cleartext credentials when the local server ("url": "/") may run over plain HTTP. All endpoints already carry operation-level security, so the "no global security" warnings (CKV_OPENAPI_4/5) are effectively false positives here. However, for the basic auth concern, consider documenting that the local server is expected to be on a trusted network, or adding a note in the description.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/assets/api/swagger.json` around lines 1436 - 1447, Add guidance to the ApiAuth security scheme to address Checkov's cleartext-credentials warning: update the ApiAuth entry (in swagger.json) to include a descriptive "description" that explicitly states this basic auth is intended only for local/trusted-network use or must be used over HTTPS, or remove/replace ApiAuth if you don't need basic auth; keep the JWTAuth entry unchanged. Ensure the description references ApiAuth and documents the expected transport security (trusted network or TLS) so static analysis clearly understands the constraint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/assets/api/swagger.json`:
- Around line 2281-2309: Mark the phoneNumber property as deprecated and make
sender explicitly nullable: in the SmsEventPayload schema add "deprecated": true
to the phoneNumber property so code generators surface migration guidance, and
add "nullable": true to the sender property (matching destination) while keeping
its description that it may be null.
- Line 13: Update the license URL in the Swagger JSON: locate the licenses "url"
property (the entry currently set to
"http://www.apache.org/licenses/LICENSE-2.0") and change it to use HTTPS by
setting it to "https://www.apache.org/licenses/LICENSE-2.0" so the canonical
Apache 2.0 license link is secure.
- Around line 2222-2254: The WebHookEvent.event enum is missing two values
referenced elsewhere; update the WebHookEvent schema's "event" enum (symbol:
WebHookEvent.event) to include "mms:received" and "sms:data-received" so it
matches the referenced payloads (SmsDataReceivedPayload and any mms payload
handling), ensuring the enum contains
["sms:received","sms:sent","system:ping","sms:delivered","sms:failed","mms:received","sms:data-received"]
(or the equivalent order you prefer) to keep the swagger callback schema
consistent with the oneOf payload references.
- Around line 2233-2254: The payload "oneOf" array is missing the
MmsReceivedPayload reference, so update the payload schema's oneOf (the
"payload" entry) to include a reference to the MmsReceivedPayload schema (i.e.,
add a $ref to "#/components/schemas/MmsReceivedPayload") so that
WebhookEvent.event entries like "mms:received" can be validated and
deserialized.
---
Nitpick comments:
In `@app/src/main/assets/api/swagger.json`:
- Around line 2207-2209: The schema name WebHookEvent is inconsistent with the
existing event-type smsgateway.WebhookEvent (difference in casing and missing
namespace); rename the schema to match the existing identifier
(smsgateway.WebhookEvent with the same camelCasing "WebhookEvent" used
elsewhere) and update any references in swagger.json to use
smsgateway.WebhookEvent so SDK generators and consumers see a single canonical
type name; ensure both the schema key and any $ref strings are updated to the
exact same identifier.
- Around line 1436-1447: Add guidance to the ApiAuth security scheme to address
Checkov's cleartext-credentials warning: update the ApiAuth entry (in
swagger.json) to include a descriptive "description" that explicitly states this
basic auth is intended only for local/trusted-network use or must be used over
HTTPS, or remove/replace ApiAuth if you don't need basic auth; keep the JWTAuth
entry unchanged. Ensure the description references ApiAuth and documents the
expected transport security (trusted network or TLS) so static analysis clearly
understands the constraint.
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 (1)
app/src/main/assets/api/swagger.json (1)
1264-1272:⚠️ Potential issue | 🟡 Minor204 response MUST NOT define a response body.
Per RFC 9110 §15.3.5, a 204 response carries no content. Defining a
contentschema here is a spec violation and may cause code generators to emit dead deserialization logic. The other 204 responses in this file (e.g.,/auth/token/{jti}DELETE at line 130 and/devices/{id}DELETE at line 266) correctly omitcontent.🐛 Proposed fix
"204": { - "description": "Webhook deleted", - "content": { - "application/json": { - "schema": { - "type": "object" - } - } - } + "description": "Webhook deleted" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/assets/api/swagger.json` around lines 1264 - 1272, The 204 response for the "Webhook deleted" operation incorrectly defines a response body; remove the entire "content" object (and its nested "application/json" and "schema") from the 204 response so the response only contains "description": "Webhook deleted". Locate the 204 response block in the swagger.json that contains "Webhook deleted" and update it to match other DELETE 204 responses (e.g., the `/auth/token/{jti}` DELETE), ensuring no content/schema is present.
🧹 Nitpick comments (1)
app/src/main/assets/api/swagger.json (1)
2222-2232:WebHookEvent.eventinline enum duplicatessmsgateway.WebhookEvent; use a$refinstead.The inline
enumat lines 2224-2232 repeats the same seven values already defined insmsgateway.WebhookEvent(lines 2197-2205). Any future addition to the webhook event type must be added in two places.♻️ Proposed refactor
"event": { - "type": "string", - "enum": [ - "sms:received", - "sms:sent", - "system:ping", - "sms:delivered", - "sms:failed", - "mms:received", - "sms:data-received" - ], - "description": "The type of event that triggered the webhook." + "description": "The type of event that triggered the webhook.", + "allOf": [ + { "$ref": "#/components/schemas/smsgateway.WebhookEvent" } + ] },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/assets/api/swagger.json` around lines 2222 - 2232, Replace the inline enum under the WebHookEvent.event property with a $ref to the existing smsgateway.WebhookEvent schema so the event values are defined in one place; locate the WebHookEvent definition (property "event") in swagger.json and change the inline "type":"string" + "enum":[...] to a single "$ref": "#/components/schemas/smsgateway.WebhookEvent" (or equivalent components path used in this file), ensuring the schema now points to smsgateway.WebhookEvent instead of duplicating the enum.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/assets/api/swagger.json`:
- Around line 1437-1439: The ApiAuth HTTP Basic scheme transmits credentials
unencrypted; update the OpenAPI entry for "ApiAuth" in swagger.json to warn
consumers and require secure transport: add a clear "description" on the
"ApiAuth" scheme stating that Basic auth MUST only be used over TLS, and either
change the local server entry (the server with "url": "/") to an HTTPS host or
add an explicit server-level or scheme-level note (e.g., description or vendor
extension) that the endpoint is local-only and must be accessed over a
trusted/encrypted channel; reference the "ApiAuth" object and the server with
"url": "/" when making these edits.
- Around line 2235-2259: The oneOf on WebHookEvent.payload is ambiguous because
payload variants (SmsReceivedPayload, SmsSentPayload, SmsDeliveredPayload,
SmsFailedPayload, SystemPingPayload, SmsDataReceivedPayload, MmsReceivedPayload)
lack discriminating required fields and there is no discriminator; update the
OpenAPI spec by adding a discriminator to WebHookEvent.payload with propertyName
"event" and a mapping for each variant to its schema, and also ensure each
payload schema includes a required "event" property (and where applicable a
unique required field like "message"/"receivedAt"/"sentAt") or add those unique
required fields in their inner allOf so each oneOf branch can be matched
exclusively.
- Around line 2404-2437: Add a "title" and "description" to the
MmsReceivedPayload schema to match other payloads (e.g., SmsReceivedPayload and
SmsSentPayload); update the schema object that references SmsEventPayload
(symbol: MmsReceivedPayload) by inserting a title like "MmsReceivedPayload" and
a concise description such as "Payload for received MMS messages" alongside the
existing properties so generated docs include the same metadata as other payload
schemas.
- Around line 2207-2261: The WebHookEvent schema is missing a required array and
is not using the project namespace; update the schema named "WebHookEvent" to
"smsgateway.WebHookEvent" (and similarly rename payload schemas
SmsReceivedPayload, SmsSentPayload, SmsDeliveredPayload, SmsFailedPayload,
SystemPingPayload, SmsDataReceivedPayload, MmsReceivedPayload to the
smsgateway.* equivalents) and add a required array that at minimum includes
"event" and "payload" (optionally include "id", "webhookId", "deviceId" if they
must be present); also update all $ref references across the spec to point to
the new smsgateway.* schema names so consumers and code generators use the
namespaced schemas.
---
Outside diff comments:
In `@app/src/main/assets/api/swagger.json`:
- Around line 1264-1272: The 204 response for the "Webhook deleted" operation
incorrectly defines a response body; remove the entire "content" object (and its
nested "application/json" and "schema") from the 204 response so the response
only contains "description": "Webhook deleted". Locate the 204 response block in
the swagger.json that contains "Webhook deleted" and update it to match other
DELETE 204 responses (e.g., the `/auth/token/{jti}` DELETE), ensuring no
content/schema is present.
---
Duplicate comments:
In `@app/src/main/assets/api/swagger.json`:
- Around line 2295-2313: The schema for the SMS message object needs two small
fixes: add "deprecated": true to the phoneNumber property and add "nullable":
true to the sender property so its description matches the schema (destination
already has nullable). Locate the object containing "phoneNumber", "sender",
"destination", "simNumber" in swagger.json and update phoneNumber to include
"deprecated": true and update sender to include "nullable": true (ensure valid
JSON with commas in the surrounding properties).
---
Nitpick comments:
In `@app/src/main/assets/api/swagger.json`:
- Around line 2222-2232: Replace the inline enum under the WebHookEvent.event
property with a $ref to the existing smsgateway.WebhookEvent schema so the event
values are defined in one place; locate the WebHookEvent definition (property
"event") in swagger.json and change the inline "type":"string" + "enum":[...] to
a single "$ref": "#/components/schemas/smsgateway.WebhookEvent" (or equivalent
components path used in this file), ensuring the schema now points to
smsgateway.WebhookEvent instead of duplicating the enum.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/assets/api/swagger.json`:
- Around line 2387-2390: The SmsDataReceivedPayload.data schema lacks OpenAPI
annotations for base64 content; update the SmsDataReceivedPayload definition to
add format: byte and the same base64 validation pattern used by
smsgateway.DataMessage.data (use the identical pattern and the format: byte) so
generators validate/encode it consistently; locate SmsDataReceivedPayload.data
and mirror the annotation/pattern from smsgateway.DataMessage.data.
---
Duplicate comments:
In `@app/src/main/assets/api/swagger.json`:
- Around line 1220-1221: The referenced schemas and callback $ref use
inconsistent naming—update all schema names and $ref occurrences to include the
smsgateway. namespace so they match the rest of the file: rename/alias the
definitions for WebHookEvent, SmsEventPayload, SmsReceivedPayload,
SmsSentPayload, SmsDeliveredPayload, SmsFailedPayload, SystemPingPayload,
SmsDataReceivedPayload, and MmsReceivedPayload to smsgateway.WebHookEvent,
smsgateway.SmsEventPayload, etc., and update the callback $ref that points to
WebHookEvent to use "#/components/schemas/smsgateway.WebHookEvent"; ensure all
other references in the file are updated to the new names to avoid broken $ref
links.
- Around line 2282-2310: The SmsEventPayload schema still has inconsistencies:
mark the phoneNumber property as deprecated by adding "deprecated": true and
mark the sender property as nullable by adding "nullable": true; update the
SmsEventPayload object's properties (phoneNumber and sender) so phoneNumber is
explicitly deprecated and sender allows null to match its description.
- Around line 1430-1432: The ApiAuth security scheme (ApiAuth) currently uses
HTTP Basic (type: http, scheme: basic) without guidance; update the swagger.json
ApiAuth entry to include a clear description field stating that HTTP Basic MUST
only be used over TLS (HTTPS) or a trusted local channel and warning about
cleartext credentials, and ensure any references to ApiAuth in security
requirements remain unchanged; keep the description concise and explicit about
TLS requirement and recommended alternatives (e.g., token-based auth) for
external usage.
- Around line 2224-2248: The oneOf payload is ambiguous because variants like
SystemPingPayload are empty and none define an exclusive required discriminator
property; update the parent "payload" schema to include a discriminator (e.g.,
"discriminator": {"propertyName":"type", "mapping":{...}}) and then add a unique
required field (or a shared "type" enum) to each variant schema
(SmsReceivedPayload, SmsSentPayload, SmsDeliveredPayload, SmsFailedPayload,
SystemPingPayload, SmsDataReceivedPayload, MmsReceivedPayload) so validators can
select a single branch — for example add required
["type","message","receivedAt"] for SmsReceivedPayload (or at minimum a required
"type" with a unique value per schema) and ensure the discriminator mapping
points each discriminator value to the corresponding component schema.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/src/main/assets/api/swagger.json (2)
1308-1400: Add explicitsecurity: []to health probe endpoints to signal intentional open access.Omitting
securityis valid, but addingsecurity: []documents the intent and silences the Checkov CKV_OPENAPI_5 finding. The same applies to/health(line 330).♻️ Example for `/health/live` (apply the same to `/health/ready`, `/health/startup`, and `/health`)
"responses": { ... } + "security": [] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/assets/api/swagger.json` around lines 1308 - 1400, Add an explicit empty security requirement to the health endpoints so they document intentional open access and satisfy Checkov: for the GET operations under the paths "/health/live", "/health/ready", "/health/startup" (and the "/health" endpoint referenced in the comment) add a top-level "security: []" entry inside each operation object (the same level as "tags", "summary", "description", and "responses") so the spec clearly indicates these probes require no authentication.
1434-1439:JWTAuthshould usetype: http / scheme: bearerinstead ofapiKey.Using
type: apiKeywithname: Authorizationto represent a Bearer token is a Swagger 2.0 workaround. While it works, it is semantically incorrect as it uses the API key model to represent a Bearer token — a hacky solution from the past. OpenAPI 3.0 has native support for this.♻️ Proposed fix
"JWTAuth": { - "type": "apiKey", - "description": "JWT authentication", - "name": "Authorization", - "in": "header" + "type": "http", + "scheme": "bearer", + "bearerFormat": "JWT", + "description": "JWT Bearer authentication" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/assets/api/swagger.json` around lines 1434 - 1439, Replace the JWTAuth security scheme definition so it uses the OpenAPI HTTP Bearer scheme instead of an apiKey: locate the "JWTAuth" object in swagger.json and change its fields from "type": "apiKey", "name": "Authorization", "in": "header" to "type": "http" and "scheme": "bearer" (optionally add "bearerFormat": "JWT") so the security scheme is semantically correct for bearer tokens.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/assets/api/swagger.json`:
- Around line 2304-2308: The Swagger schema for SmsEventPayload.simNumber is
missing the numeric range; update the simNumber property (same field represented
as smsgateway.Message.simNumber elsewhere) to include "minimum" and "maximum"
constraints matching the documented range (e.g., add "minimum": 0 and "maximum":
3), keep "type": "integer" and "nullable": true, and ensure the description
remains intact so consumers know expected values.
- Around line 1469-1471: The OpenAPI property "expires_at" is defined as type
"string" but missing the "format: date-time", causing generators to produce a
plain string instead of a datetime; update the "expires_at" schema in
swagger.json to include "format": "date-time" alongside "type": "string" (look
for the "expires_at" property in the JSON and add the format field) so codegen
emits a proper datetime type.
---
Duplicate comments:
In `@app/src/main/assets/api/swagger.json`:
- Around line 1430-1432: The ApiAuth security scheme currently declares "type":
"http" and "scheme": "basic" which transmits credentials in cleartext over
non-TLS connections; update the ApiAuth object in swagger.json to include a
descriptive warning (e.g., a "description" property on ApiAuth) stating that
HTTP Basic must only be used over TLS (HTTPS) and recommend using stronger
alternatives (Bearer tokens/OAuth) or ensuring TLS is enforced; locate the
ApiAuth entry by the "ApiAuth" key to make this change.
- Around line 2291-2297: Update the OpenAPI schema entries for the properties
"phoneNumber" and "sender" in swagger.json: mark "phoneNumber" with
"deprecated": true to indicate migration to "sender"/"destination", and add
"nullable": true to the "sender" property so its schema matches the description
that it may be null (same behavior as "destination"). Ensure the keys are added
at the same object level as "type" and "description" for each property.
- Around line 2200-2258: The WebHookEvent schema and its payload variants must
be namespaced and discriminable: change the schema name and all refs to use the
project namespace (e.g., smsgateway.WebHookEvent, smsgateway.SmsReceivedPayload,
smsgateway.SmsSentPayload, etc.), then replace the bare oneOf for
WebHookEvent.payload with a discriminator object (e.g., "discriminator": {
"propertyName": "type", "mapping": {
"sms_received":"#/components/schemas/smsgateway.SmsReceivedPayload", ... } })
and ensure each payload schema declares the discriminator property (type) as a
required field with a constant/enum value unique per variant so validators can
unambiguously select the correct schema (update SmsReceivedPayload,
SmsSentPayload, SmsDeliveredPayload, SmsFailedPayload, SystemPingPayload,
SmsDataReceivedPayload, MmsReceivedPayload accordingly).
---
Nitpick comments:
In `@app/src/main/assets/api/swagger.json`:
- Around line 1308-1400: Add an explicit empty security requirement to the
health endpoints so they document intentional open access and satisfy Checkov:
for the GET operations under the paths "/health/live", "/health/ready",
"/health/startup" (and the "/health" endpoint referenced in the comment) add a
top-level "security: []" entry inside each operation object (the same level as
"tags", "summary", "description", and "responses") so the spec clearly indicates
these probes require no authentication.
- Around line 1434-1439: Replace the JWTAuth security scheme definition so it
uses the OpenAPI HTTP Bearer scheme instead of an apiKey: locate the "JWTAuth"
object in swagger.json and change its fields from "type": "apiKey", "name":
"Authorization", "in": "header" to "type": "http" and "scheme": "bearer"
(optionally add "bearerFormat": "JWT") so the security scheme is semantically
correct for bearer tokens.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/assets/api/swagger.json`:
- Around line 1620-1623: The OpenAPI schema for smsgateway.LogEntry.createdAt is
missing the format annotation so generators produce a plain string; update the
LogEntry schema to add "format: date-time" to the createdAt property (matching
the other timestamp fields like SmsReceivedPayload.receivedAt and
SmsSentPayload.sentAt) so codegens emit a proper datetime type and ensure the
description/documentation remains unchanged.
---
Duplicate comments:
In `@app/src/main/assets/api/swagger.json`:
- Around line 1434-1437: The ApiAuth scheme in swagger.json currently uses
"type": "http" and "scheme": "basic" with no advisory; update the ApiAuth
definition to include a clear description field that warns basic auth transmits
base64-encoded credentials and MUST only be used over TLS (HTTPS) — reference
the "ApiAuth" object and its "scheme": "basic" entry and add the description
text to that object so consumers and scanners are alerted to the TLS
requirement.
- Around line 2295-2312: Update the SmsEventPayload schema: mark the phoneNumber
property as "deprecated": true; add "nullable": true to the sender property (to
match destination) and update its description if needed; and add "minimum": 1
and "maximum": 3 to the simNumber integer property (which is already nullable)
so it matches smsgateway.Message.simNumber. Reference the SmsEventPayload schema
and the properties phoneNumber, sender, destination, and simNumber when making
these edits.
- Around line 2204-2261: The WebHookEvent schema and its payload variants are
using non-namespaced names and the payload oneOf lacks a discriminator which
will break validators; rename WebHookEvent and each payload schema to use the
smsgateway. prefix (e.g., smsgateway.WebHookEvent,
smsgateway.SmsReceivedPayload, smsgateway.SmsSentPayload, etc.) and update the
$ref targets accordingly, and then add a discriminator for the payload oneOf
that uses the outer "event" field (add a discriminator object with propertyName
"event" and mapping entries for each smsgateway.* payload) or alternatively add
unique required fields inside each payload's allOf to guarantee exactly one
branch matches; update the WebHookEvent.schema so "event" remains the
discriminant and the payload oneOf references the namespaced schemas.
3762198 to
0bb0840
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.kt (1)
69-84:simNumber ?: 0masks "unknown SIM" for SMS payloads that acceptInt?.
SmsEventPayload.SmsReceivedandSmsEventPayload.SmsDataReceiveddeclaresimNumber: Int?, so they can carrynullto represent an unknown SIM. Defaulting to0conflates "unknown" with a potentially valid value. OnlyMmsReceivedPayloadrequires non-nullInt.Consider passing
simNumber(nullable) for SMS payloads and reserving the?: 0fallback for MMS only:♻️ Proposed fix
is InboxMessage.Text -> WebHookEvent.SmsReceived to SmsEventPayload.SmsReceived( messageId = message.hashCode().toUInt().toString(16), message = message.text, sender = sender, - simNumber = simNumber ?: 0, + simNumber = simNumber, receivedAt = message.date, recipient = recipient, ) is InboxMessage.Data -> WebHookEvent.SmsDataReceived to SmsEventPayload.SmsDataReceived( messageId = message.hashCode().toUInt().toString(16), data = Base64.encodeToString(message.data, Base64.NO_WRAP), - simNumber = simNumber ?: 0, + simNumber = simNumber, receivedAt = message.date, sender = sender, recipient = recipient, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.kt` around lines 69 - 84, In the message handling branch, don't coerce simNumber to 0 for SMS payloads—pass the nullable simNumber directly to SmsEventPayload.SmsReceived and SmsEventPayload.SmsDataReceived so null represents "unknown SIM"; keep the existing simNumber ?: 0 fallback only when constructing MmsReceivedPayload (which requires a non-null Int). Locate the simNumber variable and the when branch creating SmsEventPayload.SmsReceived, SmsEventPayload.SmsDataReceived and MmsReceivedPayload and change the SMS payloads to use simNumber (nullable) while leaving the MMS construction unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/src/main/java/me/capcom/smsgateway/helpers/SubscriptionsHelper.kt`:
- Around line 88-104: getPhoneNumber currently uses the deprecated
SubscriptionInfo.number (deprecated in API 33); update getPhoneNumber (and the
other sibling methods in this file) to avoid calling SubscriptionInfo.number
directly by using the newer API when available: for API >= 33 call
SubscriptionManager.getPhoneNumber(subscriptionId) (or the appropriate
non-deprecated Telephony/SubscriptionManager API) using the
SubscriptionInfo.subscriptionId, and for older APIs fall back to
SubscriptionInfo.number with proper null/blank checks; ensure calls are guarded
by SDK checks and preserve the existing permission and null-safety logic in
getPhoneNumber and related methods.
In
`@app/src/main/java/me/capcom/smsgateway/modules/webhooks/payload/MessageEventPayload.kt`:
- Around line 3-11: The constructor currently declares phoneNumber as an
independent non-null val which can diverge from sender/recipient; remove
phoneNumber from the constructor parameters in the MessageEventPayload class and
instead add a body property val phoneNumber: String = sender ?: recipient ?: ""
(retain the `@Deprecated` annotation on the property if desired) so the invariant
is enforced at construction time and the nullability gap is eliminated; update
any subclasses/callers that passed phoneNumber to stop providing it and rely on
the new derived property.
---
Nitpick comments:
In `@app/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.kt`:
- Around line 69-84: In the message handling branch, don't coerce simNumber to 0
for SMS payloads—pass the nullable simNumber directly to
SmsEventPayload.SmsReceived and SmsEventPayload.SmsDataReceived so null
represents "unknown SIM"; keep the existing simNumber ?: 0 fallback only when
constructing MmsReceivedPayload (which requires a non-null Int). Locate the
simNumber variable and the when branch creating SmsEventPayload.SmsReceived,
SmsEventPayload.SmsDataReceived and MmsReceivedPayload and change the SMS
payloads to use simNumber (nullable) while leaving the MMS construction
unchanged.
73ff296 to
dfb3cd8
Compare
Contributor
🤖 Pull request artifacts
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfb3cd8d68
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1c76f82 to
b2d7709
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This was referenced Mar 5, 2026
This was referenced Apr 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor
Deprecated
Documentation