Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Telegram notifications are not escaped properly #839

Closed
aprosvetova opened this issue Jan 3, 2021 · 10 comments · Fixed by #1148
Closed

Telegram notifications are not escaped properly #839

aprosvetova opened this issue Jan 3, 2021 · 10 comments · Fixed by #1148
Assignees
Milestone

Comments

@aprosvetova
Copy link

Markdown parse_mode is used to make hyperlinks in Telegram notifications:
link = fmt.Sprintf("↦ [%s](%s)", req.Comment.PostTitle, req.Comment.Locator.URL+uiNav+req.Comment.ID)

It breaks if post title contains markdown symbols itself.

Markdown symbols [ ] ( ) must be escaped with backslash.

Screenshot 2021-01-03 at 03 52 45

Post title is [Contest Ended] Looking for authors: let's create a Flipper story in this case.

umputun added a commit that referenced this issue Jan 3, 2021
@umputun
Copy link
Owner

umputun commented Jan 3, 2021

Thx for the report. I have added escape for [ ] ( ), let me know if it worked for you

@aprosvetova
Copy link
Author

Screenshot 2021-01-04 at 13 19 13

Well, this didn't help. The escaping code looks fine though. Will try debugging this later.

@retifrav
Copy link

retifrav commented Aug 5, 2021

In the version I have (v1.8.1-82f8f6c9-20210504T18:25:06) it's all good:

telegram-escaping

@aprosvetova
Copy link
Author

aprosvetova commented Aug 5, 2021 via email

@retifrav
Copy link

retifrav commented Aug 5, 2021

Right, sorry, didn't read the original post carefully. Yes, I got the same result:

telegram-escaping-2

I very vaguely remember having some (similar?) issues with Markdown parsing mode in a couple of my own services, and I think that's why eventually I switched to HTML parsing.

However, I've done some tests just now (not with remark42, just plain requests to Telegram API), and escaping in Markdown parsing mode seems to work fine, actually:

telegram-escaping-3

here's the the cURL command for this request:

curl -X "POST" "https://api.telegram.org/botTOKEN/sendMessage?chat_id=CHATID&text=Here%20goes%20%5Bsome%20%5C%5Btesting%5C%5D%20long%5D(ya.ru)%20link&parse_mode=MarkdownV2&disable_web_page_preview=true"

If I drop escaping (\), then the link is still formed, just [] symbols are lost:

telegram-escaping-4

here's the cURL for it:

curl -X "POST" "https://api.telegram.org/botTOKEN/sendMessage?chat_id=CHATID&text=Here%20goes%20%5Bsome%20%5Btesting%5D%20long%5D(ya.ru)%20link&parse_mode=MarkdownV2&disable_web_page_preview=true"

So there seems to be something wrong with the way remark42 forms such links. I don't know how HTTP requests are in Go, but I assume there are fine, and so I would guess the issue is with the URL-encoding of special characters.

...Then I realized that remark42 sends message in request body (doesn't it?), so I tried that, and it's all good there too, here's the cURL:

curl -X "POST" "https://api.telegram.org/botTOKEN/sendMessage" \
     -H 'Content-Type: application/json; charset=utf-8' \
     -d $'{
  "parse_mode": "MarkdownV2",
  "chat_id": "CHATID",
  "disable_web_page_preview": "true",
  "text": "Here goes [some \\\\[testing\\\\] long](ya.ru) link"
}'

Sooo, perhaps there is not enough \-escaping? :)

@paskal
Copy link
Sponsor Collaborator

paskal commented Aug 5, 2021

If you would like to test it, the function is escapeText. Spoiler: telegram text escaping is complicated, and documentation doesn't match the real behaviour.

retifrav added a commit to retifrav/testing-telegram-api that referenced this issue Aug 7, 2021
@retifrav
Copy link

retifrav commented Aug 7, 2021

Hmm, the escapeText() function seems to work fine (although it misses the . symbol, which also needs to be escaped, according to Telegram documentation). I've tested it in a small CLI utility here. To put it briefly, here's how I formed the message:

postBody, _ := json.Marshal(map[string]string{
    "parse_mode": "MarkdownV2",
    "chat_id": config.TelegramChatID,
    "disable_web_page_preview": "true",
    "text": fmt.Sprintf(
        "Here goes [%s](%s) link",
        escapeText("some [testing] long"),
        "ya.ru",
    ),
    //"text": "Here goes [some \\[testing\\] long](ya.ru) link",
})

and here's the result in Telegram:

telegram-link-escaped-brackets

Which is the expected result.

My Go knowledge is not enough to fully understand what's going on in buildTelegramMessage(), but my next guess is that perhaps there is some redundant escaping there for [] symbols that surround the link (as those don't need to be escaped).

@paskal paskal self-assigned this Aug 7, 2021
@paskal paskal added this to the v1.9 milestone Oct 16, 2021
@paskal
Copy link
Sponsor Collaborator

paskal commented Oct 24, 2021

@retifrav @aprosvetova please let me know if you'll find any problems in the current :master docker image.

@retifrav
Copy link

retifrav commented Oct 24, 2021

A bit unrelated to the issue, I don't use Docker, so I wanted to build the latest master from sources, but that failed:

$ git clone https://github.com/umputun/remark42.git
$ cd remark42
$ make OS=linux ARCH=amd64
docker build -f Dockerfile.artifacts -t remark42.bin .
make: docker: Command not found
make: *** [Makefile:5: bin] Error 127

Is it an error that it tries to build Docker image (or use Docker for something else?), although README says this command should build a binary? It looks like I'm missing some other steps for building.

@paskal
Copy link
Sponsor Collaborator

paskal commented Oct 24, 2021

make uses docker to produce the binary. You can read Dockerfile.artifacts and get instructions on producing the binary using nodejs and go from there, but I strongly suggest installing docker instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants