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.

Sign up for GitHub

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

Jump to bottom

Fix bug to go to 404 when creating pull request #29059

Closed
wants to merge 11 commits into from
Closed

Fix bug to go to 404 when creating pull request #29059

wants to merge 11 commits into from

Conversation

lunny
Copy link
Member

@lunny lunny commented Feb 5, 2024 •
edited
Loading

The issuePull doesn't exist if creating pull request failed. So redirect to issuePull.Link is wrong.

image

@lunny lunny added type/bug backport/v1.21 This PR should be backported to Gitea 1.21 labels Feb 5, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 5, 2024
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 5, 2024
wxiaoguang
wxiaoguang previously approved these changes Feb 5, 2024
View reviewed changes
@GiteaBot GiteaBot 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 Feb 5, 2024
delvh
delvh approved these changes Feb 5, 2024
View reviewed changes
options/locale/locale_en-US.ini Show resolved Hide resolved
@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 Feb 5, 2024
@lunny
Copy link
Member Author

lunny commented Feb 5, 2024

This is still not right.

  1. the json error includes HTML tags
  2. after click the close window, the loading doesn't dispear.
图片

@wxiaoguang
Copy link
Contributor

This is still not right.

1. the json error includes HTML tags

2. after click the close window, the loading doesn't dispear.

OK, I remember now, that's the reason why I left a "FIXME" there but didn't change it to JSONError.

@silverwind
Copy link
Member

silverwind commented Feb 6, 2024 •
edited
Loading

Toast always escapes HTML in the message. If you still want to render HTML in it, a option like noEscape would need to be added, but care needs to be taken so that we don't introduce XSS.

@wxiaoguang wxiaoguang dismissed their stale review February 7, 2024 02:17

dismiss old review

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels Feb 7, 2024
@wxiaoguang wxiaoguang mentioned this pull request Feb 14, 2024
Merged
@wxiaoguang
Copy link
Contributor

OK, I remember now, that's the reason why I left a "FIXME" there but didn't change it to JSONError.

-> Refactor locale&string&template related code #29165

wxiaoguang added a commit that referenced this pull request Feb 14, 2024
@wxiaoguang
Refactor locale&string&template related code (#29165)
f3eb835 
Clarify when "string" should be used (and be escaped), and when
"template.HTML" should be used (no need to escape)

And help PRs like  #29059 , to render the error messages correctly.
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 15, 2024
@wxiaoguang @silverwind
Refactor locale&string&template related code (go-gitea#29165)
21c428f 
Clarify when "string" should be used (and be escaped), and when
"template.HTML" should be used (no need to escape)

And help PRs like  go-gitea#29059 , to render the error messages correctly.
@yp05327
Copy link
Contributor

yp05327 commented Feb 16, 2024 •
edited
Loading

After checked the discussion above and #29165, I remember the fix of #26129 is not enough, as it will remove the dirty data and redirect to 404 when git hook check failed, but user will not know what happened as no error info will be displayed.

@CaiCandong did some fix of display the error info in #26258, but at that time we can not handle these escaped html codes.
It seems that it is possible to implement it now?

silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
@wxiaoguang @silverwind
Refactor locale&string&template related code (go-gitea#29165)
2bbd9ac 
Clarify when "string" should be used (and be escaped), and when
"template.HTML" should be used (no need to escape)

And help PRs like  go-gitea#29059 , to render the error messages correctly.
@lunny lunny marked this pull request as draft February 23, 2024 03:01
6543 pushed a commit to 6543-forks/gitea that referenced this pull request Feb 26, 2024
@wxiaoguang @earl-warren
Refactor locale&string&template related code (go-gitea#29165)
6524894 
Clarify when "string" should be used (and be escaped), and when
"template.HTML" should be used (no need to escape)

And help PRs like  go-gitea#29059 , to render the error messages correctly.

(cherry picked from commit f3eb835)

Conflicts:
	modules/web/middleware/binding.go
	routers/web/feed/convert.go
	tests/integration/branches_test.go
	tests/integration/repo_branch_test.go
	trivial context conflicts
@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 Feb 29, 2024
@wxiaoguang
Copy link
Contributor

I think the approvals are too quick, at least, the HTML problem hasn't been resolved here.

I only added necessary support in #29165 , but this PR doesn't really make the toast use HTML content correctly.

@silverwind
Copy link
Member

How to reproduce the HTML toast? We could make it render HTML if all content in the HTML is trusted, e.g. no user content.

@lunny
Copy link
Member Author

lunny commented Feb 29, 2024

I moved it back to flash but not Toast.

@silverwind
Copy link
Member

Ok, but flash also needs to ensure to correctly escape user content 😉.

@lunny
Copy link
Member Author

lunny commented Mar 1, 2024

Ok, but flash also needs to ensure to correctly escape user content 😉.

Should we do that, the output comes from the hooks scripts. Maybe administrators should guarantee that since he has got root permission of Gitea.

wxiaoguang
wxiaoguang reviewed Mar 1, 2024
View reviewed changes
routers/web/repo/pull.go
ctx.Flash.Error(flashError)
ctx.JSONRedirect(pullIssue.Link()) // FIXME: it's unfriendly, and will make the content lost
ctx.Flash.Warning(flashError)
ctx.JSONRedirect(ctx.Link)
Copy link
Contributor

@wxiaoguang wxiaoguang Mar 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Well, the code just went back to the old. So why need this PR then?

(Oh I see, fix the 404 .... use a ctx.Link now)

Copy link
Contributor

Choose a reason for hiding this comment

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

But why not keep the "Error" and keep the comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

The UI very special, when using Error, the error prompt section will displayed on the left of the input box.

Copy link
Contributor

@wxiaoguang wxiaoguang Mar 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So when developing Gitea, when there is a unclear problem, should it always be hacky patched, but not really resolve it clearly?

At least, from the framework's view, Flash.Error and Flash.Warning should behave similarly?

@wxiaoguang
Copy link
Contributor

I have made some changes, but no idea why I can't push changes to the PR which has enabled "Maintainers are allowed to edit this pull request". Maybe I need to open a new PR.

@lunny
Copy link
Member Author

lunny commented Mar 2, 2024

I have made some changes, but no idea why I can't push changes to the PR which has enabled "Maintainers are allowed to edit this pull request". Maybe I need to open a new PR.

Please, then I will close this one.

@wxiaoguang
Copy link
Contributor

-> Fix incorrect redirection when create PR fails #29537

And I figured out that the old layout for "alert" is wrong, that's why you see "strange special UI".

@wxiaoguang
Copy link
Contributor

Ok, but flash also needs to ensure to correctly escape user content 😉.

Should we do that, the output comes from the hooks scripts. Maybe administrators should guarantee that since he has got root permission of Gitea.

The answer is: the "message" is fully escaped and sanitized, correctly. See the related code.

@lunny
Copy link
Member Author

lunny commented Mar 2, 2024

replace by #29537

@lunny lunny closed this Mar 2, 2024
@lunny lunny deleted the lunny/fix_pr_hook2 branch March 2, 2024 11:03
@wxiaoguang
Copy link
Contributor

OK, I remember now, that's the reason why I left a "FIXME" there but didn't change it to JSONError.

-> Refactor locale&string&template related code #29165

-> Make PR form use toast to show error message #29545

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 10, 2024
@lunny lunny removed the backport/v1.21 This PR should be backported to Gitea 1.21 label Mar 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wxiaoguang wxiaoguang wxiaoguang left review comments

@delvh delvh delvh approved these changes

@silverwind silverwind silverwind approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/translation size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. type/bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@lunny @wxiaoguang @silverwind @yp05327 @delvh @GiteaBot @6543