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 webhook #4227

Merged
merged 41 commits into from
Apr 19, 2019
Merged

Telegram webhook #4227

merged 41 commits into from
Apr 19, 2019

Conversation

techknowlogick
Copy link
Member

As title

@techknowlogick techknowlogick added type/feature Completely new functionality. Can only be merged if feature freeze is not active. pr/wip This PR is not ready for review labels Jun 11, 2018
@techknowlogick techknowlogick removed the pr/wip This PR is not ready for review label Jun 11, 2018
@codecov-io
Copy link

codecov-io commented Jun 11, 2018

Codecov Report

Merging #4227 into master will decrease coverage by 0.25%.
The diff coverage is 1.98%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4227      +/-   ##
==========================================
- Coverage   41.67%   41.42%   -0.26%     
==========================================
  Files         414      415       +1     
  Lines       55985    56337     +352     
==========================================
+ Hits        23331    23335       +4     
- Misses      29524    29873     +349     
+ Partials     3130     3129       -1
Impacted Files Coverage Δ
models/webhook_telegram.go 0% <0%> (ø)
modules/auth/repo_form.go 42.47% <0%> (-0.77%) ⬇️
routers/repo/webhook.go 1.52% <0%> (-0.22%) ⬇️
modules/setting/webhook.go 100% <100%> (ø) ⬆️
routers/routes/routes.go 82.09% <100%> (+0.09%) ⬆️
models/webhook.go 61.07% <15.38%> (-1.32%) ⬇️
models/unit.go 0% <0%> (-14.29%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6dbd261...14712ec. Read the comment docs.

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 11, 2018
@lafriks lafriks added this to the 1.6.0 milestone Jun 12, 2018
Copy link
Member

@sapk sapk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add some tests ?

@techknowlogick
Copy link
Member Author

@sapk updated tests

@techknowlogick
Copy link
Member Author

@sapk / @axifive do you have time for a review of this PR? If not that's ok, no rush.

@lunny
Copy link
Member

lunny commented Aug 15, 2018

When created an issue, the webhook failed:

{"ok":false,"error_code":400,"description":"Bad Request: can't parse entities: Unsupported start tag \"!--\" at byte offset 154"}

@techknowlogick
Copy link
Member Author

thanks for testing @lunny. Can you post the the request that was made to telegram, I wonder if I need to escape anything.

@lunny
Copy link
Member

lunny commented Aug 15, 2018

headers

Access-Control-Allow-Origin: *
Access-Control-Expose-Headers: Content-Length,Content-Type,Date,Server,Connection
Connection: keep-alive
Content-Length: 129
Content-Type: application/json
Date: Wed, 15 Aug 2018 04:09:49 GMT
Server: nginx/1.12.2

body

{
  "text": "[\u003ca href=\"http://localhost:3000/lunny/git\"\u003elunny/git\u003c/a\u003e] Issue opened: \u003ca href=\"http://localhost:3000/api/v1/repos/lunny/git/issues/4\"\u003e#4 new issue\u003c/a\u003e\n\n\u003c!--\r\n    1. Please speak English, this is the language all of us can speak and write.\r\n    2. Please ask questions or configuration/deploy problems on our Discord \r\n       server (https://discord.gg/NsatcWJ) or forum (https://discourse.gitea.io).\r\n    3. Please take a moment to check that your issue doesn't already exist.\r\n    4. Please give all relevant information below for bug reports, because \r\n       incomplete details will be handled as an invalid report.\r\n--\u003e\r\n",
  "parse_mode": "HTML"
}

@techknowlogick techknowlogick modified the milestones: 1.6.0, 1.7.0 Sep 3, 2018
@techknowlogick
Copy link
Member Author

Looking at docs the only supported HTML tags are: b, strong, i, em, a, code, and pre. All <,> and & symbols that are not a part of a tag or an HTML entity must be replaced with the corresponding HTML entities (&lt;, &gt;, and &amp; which along with &quot; are the only named HTML entities allowed). All numerical HTML entities are supported.

So I'm going to try to instead send HTML to the API, I will try to transform this into markdown, and send it that way.

models/webhook_telegram.go Outdated Show resolved Hide resolved
@techknowlogick techknowlogick modified the milestones: 1.8.0, 1.9.0 Feb 7, 2019
@techknowlogick
Copy link
Member Author

@lunny @jonasfranz I'm now passing all messages through the sanitizer, and messages are successfully sending.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 16, 2019
@lafriks
Copy link
Member

lafriks commented Apr 17, 2019

@jonasfranz please review

@techknowlogick techknowlogick merged commit 56da256 into go-gitea:master Apr 19, 2019
@techknowlogick techknowlogick deleted the telegram_webhook branch April 19, 2019 02:45
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
@delvh delvh removed the type/changelog Adds the changelog for a new Gitea version label Oct 7, 2023
@wxiaoguang
Copy link
Contributor

Looking at docs the only supported HTML tags are: b, strong, i, em, a, code, and pre. All <,> and & symbols that are not a part of a tag or an HTML entity must be replaced with the corresponding HTML entities (&lt;, &gt;, and &amp; which along with &quot; are the only named HTML entities allowed). All numerical HTML entities are supported.

So I'm going to try to instead send HTML to the API, I will try to transform this into markdown, and send it that way.

The root problem is that markdown shouldn't be mixed with HTML. The complete fix could be this: Refactor webhook #31587

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.