#### Pre-Submission Checklist - [X] Commit message has the format required by CONTRIBUTING guide - [X] Commits are split per component (core, individual modules, libs, utils, ...) - [X] Each component has a single commit (if not, squash them into one commit) - [X] No commits to README files for modules (changes must be done to docbook files in `doc/` subfolder, the README file is autogenerated)
#### Type Of Change - [ ] Small bug fix (non-breaking change which fixes an issue) - [X] New feature (non-breaking change which adds new functionality) - [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist: <!-- Go over all points below, and after creating the PR, tick the checkboxes that apply --> - [ ] PR should be backported to stable branches - [X] Tested changes locally - [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description this is a tiny module which provides integration with slack over webhooks. currently it exports only one function which accepts formatted string as an argument, evaluates pseudo-variables if any and sends the msg to slack channel (channel name, username and emoji are set as a module parameters). Kemi function is exported as well.
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/2838
-- Commit Summary --
* slack: new module, send message to slack channel
-- File Changes --
A src/modules/slack/Makefile (31) A src/modules/slack/README (154) A src/modules/slack/doc/Makefile (4) A src/modules/slack/doc/slack.html (468) A src/modules/slack/doc/slack.xml (28) A src/modules/slack/doc/slack_admin.xml (175) A src/modules/slack/slack.c (298) A src/modules/slack/slack.h (58)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/2838.patch https://github.com/kamailio/kamailio/pull/2838.diff
@henningw commented on this pull request.
Thanks for the pull request. I just had a quick look and added a few comments, related to parameter naming and error checking.
return -1;
+ } + + if(strncmp(val, "https://hooks.slack.com", 23)) { + LM_ERR("slack invalid webhook url [%s]\n", val); + return -1; + } + + // TODO: parse webhook to multiple channels? eg.: chan1=>https://AAA/BBB/CC, chan2=>... + + slack_url = (char*)pkg_malloc(len + 1); + if (slack_url==NULL) { + PKG_MEM_ERROR; + return -1; + } + strcpy(slack_url, val);
would be good to use strncpy, as you already have the len.
- return;
+} + +/** + * send message with curl + * @return 0 on success, -1 on error + */ +static int _curl_send(const char* uri, str *post_data) +{ + int datasz; + char* send_data; + CURL *curl_handle; + CURLcode res; + // LM_DBG("sending to[%s]\n", uri); + + datasz = snprintf(NULL, 0, BODY_FMT, slack_channel, slack_username, post_data->s, slack_icon);
snprintf could fail, so you should check for error (negative return value).
- */
+static int _curl_send(const char* uri, str *post_data) +{ + int datasz; + char* send_data; + CURL *curl_handle; + CURLcode res; + // LM_DBG("sending to[%s]\n", uri); + + datasz = snprintf(NULL, 0, BODY_FMT, slack_channel, slack_username, post_data->s, slack_icon); + send_data = (char*)pkg_malloc((datasz+1)*sizeof(char)); + if(send_data==NULL) { + LM_ERR("Error: can not allocate pkg memory [%d] bytes\n", datasz); + return -1; + } + snprintf(send_data, datasz+1, BODY_FMT, slack_channel, slack_username, post_data->s, slack_icon);
see above
<section id="slack.p.icon_emogi">
+ <title><varname>icon_emogi</varname> (str)</title> + <para> + specify an emoji (using colon shortcodes, eg. :white_check_mark:) + to use as the profile photo alongside the message. + </para> + <para> + <emphasis> + Default value is :ghost: + </emphasis> + </para> + <example> + <title>Set <varname>icon_emogi</varname> parameter</title> + <programlisting format="linespecific"> +... +modparam("slack", "icon_emogi", ":ghost:")
I think the parameter _icon_emogi_ is mispelled, i think it should be named _icon_emoji_. Would be good to fix this in code and docs.
- Exported functions
+ */ +static cmd_export_t cmds[] = { + {"slack_send", (cmd_function)slack_send1, 1, slack_fixup, 0, ANY_ROUTE}, + {0, 0, 0, 0, 0, 0} +}; + + +/** + * Exported parameters + */ +static param_export_t mod_params[] = { + { "slack_url", PARAM_STRING|USE_FUNC_PARAM, (void*)_slack_url_param}, + { "channel", PARAM_STRING, &slack_channel }, // channel starts with # + { "username", PARAM_STRING, &slack_username }, + { "icon_emogi", PARAM_STRING, &slack_icon },
I think the parameter _icon_emogi_ is mispelled, i think it should be named _icon_emoji_. Would be good to fix this in code and docs.
@arsperger pushed 1 commit.
3438be7b095453d129babba1eb92936f2be63cd9 slack: fix param typo; strncpy, snprintf error check; removed pre-built html doc
@arsperger commented on this pull request.
- */
+static int _curl_send(const char* uri, str *post_data) +{ + int datasz; + char* send_data; + CURL *curl_handle; + CURLcode res; + // LM_DBG("sending to[%s]\n", uri); + + datasz = snprintf(NULL, 0, BODY_FMT, slack_channel, slack_username, post_data->s, slack_icon); + send_data = (char*)pkg_malloc((datasz+1)*sizeof(char)); + if(send_data==NULL) { + LM_ERR("Error: can not allocate pkg memory [%d] bytes\n", datasz); + return -1; + } + snprintf(send_data, datasz+1, BODY_FMT, slack_channel, slack_username, post_data->s, slack_icon);
thank you for your notes. I am wondering, if the above snprintf check is passed, what could be a reason for this one to fail? do we really need to error check this one?
@henningw commented on this pull request.
- */
+static int _curl_send(const char* uri, str *post_data) +{ + int datasz; + char* send_data; + CURL *curl_handle; + CURLcode res; + // LM_DBG("sending to[%s]\n", uri); + + datasz = snprintf(NULL, 0, BODY_FMT, slack_channel, slack_username, post_data->s, slack_icon); + send_data = (char*)pkg_malloc((datasz+1)*sizeof(char)); + if(send_data==NULL) { + LM_ERR("Error: can not allocate pkg memory [%d] bytes\n", datasz); + return -1; + } + snprintf(send_data, datasz+1, BODY_FMT, slack_channel, slack_username, post_data->s, slack_icon);
This of course depends on the working of snprintf, the docs unfortunately do not give much more details. But generally it is good "defensive" programming practice to check for errors on outside library calls that you do not control/own.
We have too many modules who call the curl library, which can lead to issues with the TLS initialisation. This is why we created an API in the curl module. Please look into using that API instead of calling the curl library directly.
The snprintf() for getting datasz should be checked, because can fail on unexpected characters - from the manual:
``` [EILSEQ] An invalid wide character code was encountered. ```
That will make it safer when wanting to send data taken from SIP traffic (e.g., body of requests).
On negative return code, likely pkg_malloc() will fail, requested size being big.
The second snprintf() is unlikely to fail, but I would use pkg_mallocxz() or set send_data[0] = '\0' before it so in case it fails for what so ever unknown reason, then the CURL library doesn't get a buffer with garbage and does not go beyond allocated buffer to find the '\0'.
For reusing http_client, it would be good to review all modules. Can be done for this one also after merging, it is easier to work directly in repo than as PR.
@arsperger pushed 1 commit.
4d71d13d02afdc51865f44669874ec95b538de7d slack: allocate zeroed memory for send_data
Thanks @arsperger! I am going to merge the PR and invite you to get commit access so you can develop and maintain the module in Kamailio repo. You can still do pull requests for this module, if you prefer it.
If you want to push code to other components or module, it is highly recommended to do pull requests, so other developers can review and be sure there is no conflict with ongoing work on those components.
Do consider using the API provided by http_client module as suggested by @oej -- we should do it for the other modules as well.
Merged #2838 into master.
-
Arsen Semenov -
Daniel-Constantin Mierla
-
Henning Westerholt
-
Olle E. Johansson