Fix some issues with special chars in branch names #3767

nubenum · 2018-04-07T12:51:53Z

Finally some fixes for the special char issues with branch and tag names (#3681):

Adding EscapePound where necessary for #?
Adding Escape where Str2Html is used to avoid " being stripped
Fix last references to legacy URL scheme where possible
Fix legacy redirect (ctx.Req.URL.String() is escaped, ctx.Params("*") is not, that is why TrimSuffix doesn't work)

Some i18n strings use the same argument for the branch name and the branch link. I've chosen to have the URL escapes displayed rather than getting a 404, but left them at the legacy URL scheme.

In some places, there are still issues, because JS or the server-side meddles with the escapes:

Create new pull request if default branch contains #?
Create commit on branch with #?
Define default branch with "
...and possibly others. For the time being, I deem them to be too irrelevant to be worth the effort.

I hope I haven't destroyed other things on my way.

* Adding `EscapePound` where necessary for `#?` * Adding `Escape` where `Str2Html` is used to avoid `"` being stripped * Fix last references to legacy URL scheme where possible * Fix legacy redirect Signed-off-by: Robin Durner <github@nubenum.de>

lafriks · 2018-04-09T09:46:30Z

modules/context/repo.go

-					strings.TrimSuffix(ctx.Req.URL.String(), ctx.Params("*")),
-					ctx.Repo.BranchNameSubURL(),
+					path.Dir(ctx.Req.URL.String()),
+					strings.NewReplacer("%", "%25", "#", "%23", " ", "%20", "?", "%3F").Replace(ctx.Repo.BranchNameSubURL()),


Please do not use custom escaping here, you can use go std lib escaping: (&url.URL{Path: ctx.Repo.BranchNameSubURL()}).String()

Yeah that makes more sense. I'm not sure how and where EscapePound is used, otherwise it would maybe be good to change that as well: helper.go#L114

Signed-off-by: Robin Durner <github@nubenum.de>

jonasfranz · 2018-05-26T10:59:09Z

@nubenum Please resolve conflicts
@lafriks Please add this to 1.5.0 milestone

Signed-off-by: Robin Durner <github@nubenum.de>

nubenum · 2018-05-27T11:37:01Z

Is it normal that the drone build fails? I didn't get it to run locally either, neither for my branch nor for the master.

lafriks · 2018-05-27T17:43:59Z

It should not

daviian · 2018-06-13T15:12:41Z

@nubenum Can you please update your PR so we can proceed as planned to release version 1.5? :)

Signed-off-by: Robin Durner <github@nubenum.de>

nubenum · 2018-06-13T21:07:19Z

I'm sorry, I realized only just now that there was an update to the DroneCI procedure and that there actually were tests failing.
With these fixes, the tests are now passing locally for me, but for some reason they don't pass here. I'll try to look into that soon, maybe someone else has got an idea. Also concerning the Unescape thingy, which is quite hacky. My knowledge of Go and this project is too bad at this time to find a better solution.

techknowlogick · 2018-06-13T21:28:46Z

Sometimes GitHub sends old commits to drone, and it takes a restart to get new files. I've just restarted the build

codecov-io · 2018-06-13T21:35:13Z

Codecov Report

❗ No coverage uploaded for pull request base (master@7dd93b2). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #3767   +/-   ##
=========================================
  Coverage          ?   37.38%           
=========================================
  Files             ?      305           
  Lines             ?    45211           
  Branches          ?        0           
=========================================
  Hits              ?    16903           
  Misses            ?    25865           
  Partials          ?     2443

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 7dd93b2...ea10994. Read the comment docs.

daviian · 2018-06-14T07:06:42Z

modules/context/repo.go

-					strings.TrimSuffix(ctx.Req.URL.String(), ctx.Params("*")),
-					ctx.Repo.BranchNameSubURL(),
+					strings.TrimSuffix(unescaped, ctx.Params("*")),
+					(&url.URL{Path: ctx.Repo.BranchNameSubURL()}).String(),


Can't we just use url.PathEscape here instead? Just a thought. Didn't try it myself.

When I tried that the last time, I got back into the endless loop that I was trying to fix in the first place. I have no idea what exactly the difference is.

I looked into that again:
String(): branch/s%3Cscript%3Ealert%28%27XSS%27%29;%3C/script%3Es
PathEscape(): branch%2Fs%3Cscript%3Ealert%28%27XSS%27%29%3B%3C%2Fscript%3Es

PathEscape also escapes slashes, which makes sense but is not wanted here because BranchNameSubURL already contains the branch/ prefix.

daviian · 2018-06-16T20:15:23Z

templates/user/dashboard/feeds.tmpl

@@ -24,7 +24,8 @@
 						{{else if eq .GetOpType 8}}
 							{{$.i18n.Tr "action.transfer_repo" .GetContent .GetRepoLink .ShortRepoPath | Str2html}}
 						{{else if eq .GetOpType 9}}
-							{{$.i18n.Tr "action.push_tag" .GetRepoLink .GetBranch .ShortRepoPath | Str2html}}
+							{{ $branchLink := .GetBranch | EscapePound | Escape}}


Shouldn't this be also the non-legacy path like at https://github.com/go-gitea/gitea/pull/3767/files#diff-06961515e08a5e52a731517765266c80R16 ?

Comment from above also applies here.

daviian · 2018-06-16T20:19:06Z

templates/user/dashboard/feeds.tmpl

@@ -13,8 +13,8 @@
 						{{else if eq .GetOpType 2}}
 							{{$.i18n.Tr "action.rename_repo" .GetContent .GetRepoLink .ShortRepoPath | Str2html}}
 						{{else if eq .GetOpType 5}}
-							{{ $branchLink := .GetBranch | EscapePound}}
-							{{$.i18n.Tr "action.commit_repo" .GetRepoLink $branchLink .GetBranch .ShortRepoPath | Str2html}}
+							{{ $branchLink := (printf "branch/%s" .GetBranch) | EscapePound | Escape}}


I think this should go into the translation file that already holds a part of the url.

Is it really a good idea to put even more code there that is redundant across all translations? If so, am I supposed to edit the en-US file so that all translations have to be adapted via crowdin?

I just don't like that one part is in the locale file and another one in code. It should be either or and not both.
Yes, just commit the en-US file with the changes and crowdin will spread the word that there are new changes that need translations

Ok moved it to the translations file. Maybe it would be a good idea to pass the entire links as parameters: pushed to %s at %s. But I think this is something for another PR.

daviian · 2018-06-16T20:19:29Z

templates/user/dashboard/feeds.tmpl

@@ -24,7 +24,8 @@
 						{{else if eq .GetOpType 8}}
 							{{$.i18n.Tr "action.transfer_repo" .GetContent .GetRepoLink .ShortRepoPath | Str2html}}
 						{{else if eq .GetOpType 9}}
-							{{$.i18n.Tr "action.push_tag" .GetRepoLink .GetBranch .ShortRepoPath | Str2html}}
+							{{ $branchLink := .GetBranch | EscapePound | Escape}}


Comment from above also applies here.

techknowlogick · 2018-06-20T17:40:20Z

This PR does not contain any migrations. Going to move it to 1.6, label it to backport to 1.5. This leaves only two PRs left for 1.5.

If anyone disagrees with this please feel free to change it back.

Signed-off-by: Robin Durner <github@nubenum.de>

jonasfranz · 2018-06-21T10:18:59Z

@techknowlogick you didn't changed the milestone

lunny · 2018-07-20T02:31:36Z

conflict

techknowlogick · 2018-09-12T04:34:26Z

templates/user/dashboard/feeds.tmpl

-							{{ $branchLink := .GetBranch | EscapePound}}
-							{{$.i18n.Tr "action.commit_repo" .GetRepoLink $branchLink .GetBranch .ShortRepoPath | Str2html}}
+							{{ $branchLink := .GetBranch | EscapePound | Escape}}
+							{{$.i18n.Tr "action.commit_repo" .GetRepoLink $branchLink (Escape .GetBranch) .ShortRepoPath | Str2html}}


Why not use $branchLink twice instead of (Escape .GetBranch)?

I think I chose to do that because EscapePound does url escaping and in general you would not want the branch names to be displayed like that (unless you want to punish users for using chars like ?# and whitespace in their branch names :)

makes sense.

nubenum · 2018-09-12T17:55:37Z

@techknowlogick So now the tests fail apparently because of different capitalization of the hex escapes? And maybe after all it is sufficient to use ctx.Req.URL.Path instead of String() and leave out the unescape? TBH, I've lost track of all the different ways to escape and unescape. Yet I think that #4764 should have been resolved with this PR as well.

techknowlogick · 2018-09-12T20:09:05Z

@nubenum I've just sent an update through to hopefully make the tests pass. Thanks for the PR, and sorry that it took so long to get it merged (hopefully that'll happen soon)

techknowlogick · 2018-09-12T20:15:40Z

@daviian would it be possible to get another review?

lafriks · 2018-09-16T17:51:49Z

@daviian please review

lafriks · 2018-09-16T22:27:38Z

Make LG-TM to work

Prepare for wild/v1.6 branch * BREAKING * Respect email privacy option in user search via API (go-gitea#4512) * Simply remove tidb and deps (go-gitea#3993) * Swagger.v1.json template (go-gitea#3572) * FEATURE * Pull request review/approval and comment on code (go-gitea#3748) * Added dependencies for issues (go-gitea#2196) (go-gitea#2531) * Add the ability to have built in themes in Gitea and provide dark theme arc-green (go-gitea#4198) * Add sudo functionality to the API (go-gitea#4809) * Add oauth providers via cli (go-gitea#4591) * Disable merging a WIP Pull request (go-gitea#4529) * Force user to change password (go-gitea#4489) * Add letsencrypt to Gitea (go-gitea#4189) * Add push webhook support for mirrored repositories (go-gitea#4127) * Add csv file render support defaultly (go-gitea#4105) * Add Recaptcha functionality to Gitea (go-gitea#4044) * BUGFIXES * Fix release creation via API (go-gitea#5076) * Remove links from topics in edit mode (go-gitea#5026) * Fix missing AppSubUrl in few more templates (fixup) (go-gitea#5021) * Fix missing AppSubUrl in some templates (go-gitea#5020) * Hide outdated comments in file view (go-gitea#5017) * Upgrade gopkg.in/testfixtures.v2 (go-gitea#4999) * Disable debug routes unless PPROF is enabled in configuration (go-gitea#4995) * Fix user menu item styling (go-gitea#4985) * Fix layout of the topics editing form (go-gitea#4971) * Fix null pointer dereference in ParseCommitWithSignature (go-gitea#4962) * Fix url in discord webhook (go-gitea#4953) * Detect charset and convert non UTF-8 files for display (go-gitea#4950) * Make sure to catch the right error so it is displayed on the UI (go-gitea#4945) * Fix(topics): don't redirect to explore page. (go-gitea#4938) * Fix bug forget to remove Stopwatch when remove repository (go-gitea#4928) * Fix bug when repo remained bare if multiple branches pushed in single push (go-gitea#4923) * Fix: Let's Encrypt configuration settings (go-gitea#4911) * Fix: Crippled diff (go-gitea#4726) (go-gitea#4900) * Fix trimming of markup section names (go-gitea#4863) * Issues api allow pulls and fix go-gitea#4832 (go-gitea#4852) * Do not autocreate directory for new users/orgs (go-gitea#4828) (go-gitea#4849) * Fix redirect with non-ascii branch names (go-gitea#4764) (go-gitea#4810) * Fix missing release title in webhook (go-gitea#4783) (go-gitea#4796) * User shouldn't be able to approve or reject his/her own PR (go-gitea#4729) * Make sure to reset commit count in the cache on mirror syncing (go-gitea#4720) * Fixed bug where team with admin privelege type doesn't get any unit (go-gitea#4719) * Fix incorrect caption of webhook setting (go-gitea#4701) (go-gitea#4717) * Allow WIP marker to contains < or > (go-gitea#4709) * Hide org/create menu item in Dashboard if user has no rights (go-gitea#4678) (go-gitea#4680) * Site admin could create repos even MAX_CREATION_LIMIT=0 (go-gitea#4645) * Fix custom templates being ignored (go-gitea#4638) * Fix starring icon after semantic ui update (go-gitea#4628) * Fix Split-View line adjustment (go-gitea#4622) * Fix integer constant overflows in tests (go-gitea#4616) * Push whitelist now doesn't apply to branch deletion (go-gitea#4601) (go-gitea#4607) * Fix bugs when too many IN variables (go-gitea#4594) * Fix failure on creating pull request with assignees (go-gitea#4419) (go-gitea#4583) * Fix panic issue on update avatar email (go-gitea#4580) (go-gitea#4581) * Fix status code label for a successful webhook (go-gitea#4540) * An inactive user shouldn't be able to be added as a collaborator (go-gitea#4535) * Don't fail silently if trying to add a collaborator twice (go-gitea#4533) * Fix incorrect MergeWhitelistTeamIDs check in CanUserMerge function (go-gitea#4519) (go-gitea#4525) * Fix out-of-transaction query in removeOrgUser (go-gitea#4521) (go-gitea#4522) * Fix migration from older releases (go-gitea#4495) * Accept 'Data:' in commit graph (go-gitea#4487) * Update xorm to latest version and fix correct `user` table referencing in sql (go-gitea#4473) * Relative URLs for LibreJS page (go-gitea#4460) * Redirect to correct page after using scratch token (go-gitea#4458) * Fix column droping for MSSQL that need new transaction for that (go-gitea#4440) * Replace src with raw to fix image paths (go-gitea#4377) * Add default merge options when creating new repository (go-gitea#4369) * Fix docker build (go-gitea#4358) * Fixes repo membership check in API (go-gitea#4341) * Dep upgrade mysql lib (go-gitea#4161) * Fix some issues with special chars in branch names (go-gitea#3767) * Responsive design fixes (go-gitea#4508) * ENHANCEMENT * Fix milestones sorted wrongly (go-gitea#4987) * Allow api to create tags for releases if they don't exist (go-gitea#4890) * Fix go-gitea#4877 to follow the OpenID Connect Audiences spec (go-gitea#4878) * Enforce token on api routes [fixed critical security issue go-gitea#4357] (go-gitea#4840) * Update legacy branch and tag URLs in dashboard to new format (go-gitea#4812) * Slack webhook channel name cannot be empty or just contain an hashtag (go-gitea#4786) * Add whitespace handling to PR-comparsion (go-gitea#4683) * Make reverse proxy auth optional (go-gitea#4643) * MySQL TLS (go-gitea#4642) * Make sure to set PR split view when creating/previewing a pull request (go-gitea#4617) * Log user in after a successful sign up (go-gitea#4615) * Fix typo IsPullReuqestBroken -> IsPullRequestBroken (go-gitea#4578) * Allow admin toggle forcing a password change for newly created users (go-gitea#4563) * Update jQuery to v1.12.4 (go-gitea#4551) * Env var GITEA_PUSHER_EMAIL (go-gitea#4516) * Feat(repo): support search repository by topic name (go-gitea#4505) * Small improvements to dependency UI (go-gitea#4503) * Make max commits in graph configurable (go-gitea#4498) * Add valid for lfs oid (go-gitea#4461) * Add shortcut to save wiki page (go-gitea#4452) * Allow administrator to create repository for any organization (go-gitea#4368) * Fix repository last updated time update when delete a user who watched the repo (go-gitea#4363) * Switch plaintext scratch tokens to use hash instead (go-gitea#4331) * Increase default TOTP secret size to 320 bits (go-gitea#4287) * Keep preseeded database password (go-gitea#4284) * Implemented hover text showing user FullName (go-gitea#4261) * Add ability to delete a token (go-gitea#4235) * Fix typos in i18n variable names. (go-gitea#4080) * Api: repos/search: add parameters to control the sort order (go-gitea#3964) * Add missing path in the Docker app.ini template (go-gitea#2181) * Add file name and branch to page title (go-gitea#4902) * Offline use of google fonts (go-gitea#4872) * Add missing History link to directory listings v2 (go-gitea#4829) * Locale for Edit and Remove due date issue (go-gitea#4802) * Disable 'May Import Local Repository' when is disabled by setting (Is… (go-gitea#4780) * API /admin/users/{username} missing parameter (go-gitea#4775) * Display error when adding a user to a team twice (go-gitea#4746) * Remove UsePrivilegeSeparation from the Docker sshd_config, see go-gitea#2876 (go-gitea#4722) * Focus title input when clicking helper link (go-gitea#4696) * Add vendor to user reserved words and format words list according alphabet (go-gitea#4685) * Add gitea/issues link to 500 page (go-gitea#4654) * Hide home button when landing page is not set to home (go-gitea#4651) * Remove link to GitHub issues in 404 template (go-gitea#4639) * Cmd/serve: pprof cpu and memory profile dumps to disk (go-gitea#4560) * Add flash message after an account has been successfully activated (go-gitea#4510) * Prevent html entity escaping on delete branch (go-gitea#4471) * Locale for button Edit on protected branch (go-gitea#4442) * Update notification icon (go-gitea#4343) * Added front-end topics validation (go-gitea#4316) * Don't display buttons if there are no system notifications (go-gitea#4280) * Issue due date api (go-gitea#3890) * SECURITY * Improve URL validation for external wiki and external issues (go-gitea#4710) * Make cookies HttpOnly and obey COOKIE_SECURE flag (go-gitea#4706) * Don't disclose emails of all users when sending out emails (go-gitea#4664) * Check that repositories can only be migrated to own user or organizations (go-gitea#4366) * TRANSLATION * Fix punctuation in English translation (go-gitea#4958) * Fix translation (go-gitea#4355)

lafriks requested changes Apr 9, 2018

View reviewed changes

bkcsoft added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 9, 2018

lafriks added the type/bug label Apr 9, 2018

switch to stdlib url escaping

d9c1a01

Signed-off-by: Robin Durner <github@nubenum.de>

Merge branch 'master' into branch-special-chars

f03bbf0

Signed-off-by: Robin Durner <github@nubenum.de>

techknowlogick added this to the 1.5.0 milestone May 26, 2018

Merge branch 'master' into branch-special-chars

f11e90e

nubenum added 2 commits June 13, 2018 20:35

Merge branch 'master' into branch-special-chars

06991df

Fix legacy redirect for tree, fix code tab escape

1237382

Signed-off-by: Robin Durner <github@nubenum.de>

daviian reviewed Jun 14, 2018

View reviewed changes

techknowlogick approved these changes Jun 15, 2018

View reviewed changes

bkcsoft added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jun 15, 2018

Merge branch 'master' into branch-special-chars

e45e2e3

daviian approved these changes Jun 16, 2018

View reviewed changes

bkcsoft 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 Jun 16, 2018

daviian reviewed Jun 16, 2018

View reviewed changes

daviian suggested changes Jun 16, 2018

View reviewed changes

techknowlogick added the backport/v1.5 label Jun 20, 2018

move link parts to translations

e4db446

Signed-off-by: Robin Durner <github@nubenum.de>

techknowlogick modified the milestones: 1.5.0, 1.6.0 Jun 21, 2018

Merge branch 'master' into branch-special-chars

c0ba41a

techknowlogick removed the backport/v1.5 label Sep 12, 2018

Merge branch 'master' into branch-special-chars

7e70b0e

techknowlogick reviewed Sep 12, 2018

View reviewed changes

Update repo.go

7d6d5f4

lafriks approved these changes Sep 12, 2018

View reviewed changes

techknowlogick added 2 commits September 12, 2018 16:16

Merge branch 'master' into branch-special-chars

ea10994

Merge branch 'master' into branch-special-chars

db2d7bc

daviian approved these changes Sep 16, 2018

View reviewed changes

Merge branch 'master' into branch-special-chars

52237fd

lafriks merged commit 756eafa into go-gitea:master Sep 16, 2018

nubenum deleted the branch-special-chars branch September 17, 2018 17:41

go-gitea locked and limited conversation to collaborators Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some issues with special chars in branch names #3767

Fix some issues with special chars in branch names #3767

nubenum commented Apr 7, 2018

lafriks Apr 9, 2018

nubenum Apr 9, 2018

jonasfranz commented May 26, 2018

nubenum commented May 27, 2018

lafriks commented May 27, 2018

daviian commented Jun 13, 2018

nubenum commented Jun 13, 2018

techknowlogick commented Jun 13, 2018

codecov-io commented Jun 13, 2018 •

edited

Loading

daviian Jun 14, 2018

nubenum Jun 14, 2018

nubenum Jun 21, 2018

daviian Jun 16, 2018 •

edited

Loading

daviian Jun 16, 2018

daviian Jun 16, 2018

nubenum Jun 20, 2018

daviian Jun 21, 2018 •

edited

Loading

nubenum Jun 21, 2018

daviian Jun 16, 2018

techknowlogick commented Jun 20, 2018

jonasfranz commented Jun 21, 2018

lunny commented Jul 20, 2018

techknowlogick Sep 12, 2018

nubenum Sep 12, 2018

techknowlogick Sep 12, 2018

nubenum commented Sep 12, 2018

techknowlogick commented Sep 12, 2018

techknowlogick commented Sep 12, 2018

lafriks commented Sep 16, 2018

lafriks commented Sep 16, 2018

Fix some issues with special chars in branch names #3767

Fix some issues with special chars in branch names #3767

Conversation

nubenum commented Apr 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasfranz commented May 26, 2018

nubenum commented May 27, 2018

lafriks commented May 27, 2018

daviian commented Jun 13, 2018

nubenum commented Jun 13, 2018

techknowlogick commented Jun 13, 2018

codecov-io commented Jun 13, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviian Jun 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviian Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

techknowlogick commented Jun 20, 2018

jonasfranz commented Jun 21, 2018

lunny commented Jul 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nubenum commented Sep 12, 2018

techknowlogick commented Sep 12, 2018

techknowlogick commented Sep 12, 2018

lafriks commented Sep 16, 2018

lafriks commented Sep 16, 2018

codecov-io commented Jun 13, 2018 •

edited

Loading

daviian Jun 16, 2018 •

edited

Loading

daviian Jun 21, 2018 •

edited

Loading