Notify the user when the file path contains leading or trailing spaces and fix the error message for invalid file names. #31507

charles7668 · 2024-06-27T01:50:35Z

silverwind

Seems ok. Personally I would have tried to do it in frontend, but this is also fine and probably simpler than frontend stuff.

wxiaoguang · 2024-06-27T13:41:28Z

routers/web/repo/editor.go

+	for i, v := range treePath {
+		treePath[i] = strings.TrimSpace(v)
+	}
+	form.TreePath = strings.Join(treePath, "/")


Hmm, the path could be something like / / / /, just press Space Slash Space Slash on the editor page 🤣

Then the tree path becomes "////", which doesn't seem good.

Then the tree path becomes "////", which doesn't seem good.

I assume the backend would reject that? Seems like right behaviour to me.

There are more edge cases: for example, what happens to foo/ .. /bar? Will it be rejected or become foo/../bar then become bar?

GitHub has another good behavior: if ../ is inputted, then it changes the path to parent, not sure whether it is implemented in Gitea's UI.

Tested with GitHub, I think GitHub also uses my suggestion: do not do too much for end users, only warn them.

On GitHub, it shows a warning but does not take any action for users. This is my test: 't2 / t3 / t4 /test'

This problem is the same as #31478.

I'm not sure if we need to follow this behavior or not.

Given that GitHub has this warning, then I agree, we should implement it too.

Both approaches sound good to me.
Both get an LGTM from me.

I added some warnings for users similar to GitHub.
However, I have kept backend processing for spaces.
On GitHub, it only removes spaces from filenames, not from parent directories. Since Gitea can run on different operating systems, leading or trailing spaces may cause some problems. Therefore, I have retained space processing.

delvh · 2024-06-28T20:41:48Z

routers/web/repo/editor.go

+	for i, v := range treePath {
+		treePath[i] = strings.TrimSpace(v)
+	}
+	form.TreePath = strings.Join(treePath, "/")


Both approaches sound good to me.
Both get an LGTM from me.

… the "Add New File" page.

delvh · 2024-07-18T13:07:20Z

@silverwind could you review again?

wxiaoguang · 2024-07-18T13:46:43Z

IMO trimming spaces in backend is not right (well, just my personal option, not a blocker if most people like this behavior)
Coding style: showElem vs .style.display = ...
There are still bugs, for example:
- input something like foo/../bar causes incorrect result.
- document.querySelector('.ui.warning.message.flash-message.flash-warning') may query other "flash message" incorrectly but not the "space-related" one.
- the concern in https://github.com/go-gitea/gitea/pull/31507/files#r1657163784 is still not resolved, I do not think making / / / / to //// is right for backend path handling.

charles7668 · 2024-07-19T02:41:54Z

IMO trimming spaces in backend is not right (well, just my personal option, not a blocker if most people like this behavior)

Coding style: showElem vs .style.display = ...

There are still bugs, for example:

input something like foo/../bar causes incorrect result.

How can this bug be reproduced? I tried it but couldn't reproduce it.

   * `document.querySelector('.ui.warning.message.flash-message.flash-warning')` may query other "flash message" incorrectly but not the "space-related" one.
   * the concern in https://github.com/go-gitea/gitea/pull/31507/files#r1657163784 is still not resolved, I do not think making `/ / / /` to `////` is right for backend path handling.

I will fix the other bug.

wxiaoguang · 2024-07-19T02:46:42Z

How can this bug be reproduced? I tried it but couldn't reproduce it.

Paste foo/../bar

# Conflicts: # web_src/js/features/repo-editor.ts

charles7668 · 2024-09-10T07:00:12Z

As an alternative to trimming spaces, I fixed the error message when the file can't be created.

Could you also change the title and resolve the conflicts?

Updated and CI error seems not related

lunny · 2024-09-18T01:43:33Z

It looks like I can still create a file name with trailing whitespace.

charles7668 · 2024-09-18T02:54:25Z

It looks like I can still create a file name with trailing whitespace.

Based on this comment, I have changed the approach to not prevent users from creating spaces in the file name. Instead, a warning message will be shown if there are trailing or leading spaces, and an error message will be displayed if the file cannot be created on the server OS.

I think whether or not to process spaces in the backend can be proposed for discussion.

Leading or trailing spaces are allowed on a Linux server, but if using a Windows server, it will show an error message.

On the Linux server, this file can be created successfully.
On the Windows server, it returns the following page and fails to create the file:
In my opinion, for consistency, we need to maintain the same behavior across different OS servers.

That's impossible, naturally Windows and Linux are different, eg: the?file.txt, the*file.txt, the<file.txt, con, com1 .........

lunny · 2024-09-18T04:08:03Z

It looks like I can still create a file name with trailing whitespace.

Based on this comment, I have changed the approach to not prevent users from creating spaces in the file name. Instead, a warning message will be shown if there are trailing or leading spaces, and an error message will be displayed if the file cannot be created on the server OS.

I think whether or not to process spaces in the backend can be proposed for discussion.

Leading or trailing spaces are allowed on a Linux server, but if using a Windows server, it will show an error message.

On the Linux server, this file can be created successfully.
On the Windows server, it returns the following page and fails to create the file:
In my opinion, for consistency, we need to maintain the same behavior across different OS servers.

That's impossible, naturally Windows and Linux are different, eg: the?file.txt, the*file.txt, the<file.txt, con, com1 .........

Yes. But no warning or error was displayed.

charles7668 · 2024-09-18T05:53:59Z

It looks like I can still create a file name with trailing whitespace.

Based on this comment, I have changed the approach to not prevent users from creating spaces in the file name. Instead, a warning message will be shown if there are trailing or leading spaces, and an error message will be displayed if the file cannot be created on the server OS.
I think whether or not to process spaces in the backend can be proposed for discussion.
Leading or trailing spaces are allowed on a Linux server, but if using a Windows server, it will show an error message.

On the Linux server, this file can be created successfully.
On the Windows server, it returns the following page and fails to create the file:
In my opinion, for consistency, we need to maintain the same behavior across different OS servers.

That's impossible, naturally Windows and Linux are different, eg: the?file.txt, the*file.txt, the<file.txt, con, com1 .........

Yes. But no warning or error was displayed.

I referred to GitHub's behavior, which only checks when typing '/'. I have now fixed it to check after every typing.

* giteaofficial/main: Notify the user when the file path contains leading or trailing spaces and fix the error message for invalid file names. (go-gitea#31507) Fix wrong status of `Set up Job` when first step is skipped (go-gitea#32120) Fix bug when deleting a migrated branch (go-gitea#32075) Include collaboration repositories on dashboard source/forks/mirrors list (go-gitea#31946) Display head branch more comfortable on pull request view (go-gitea#32000) Truncate commit message during Discord webhook push events (go-gitea#31970) Fix template bug of pull request view (go-gitea#32072)

GiteaBot · 2024-09-26T04:50:37Z

I was unable to create a backport for 1.22. @charles7668, please send one manually. 🍵

go run ./contrib/backport 31507
...  // fix git conflicts if any
go run ./contrib/backport --continue

lunny · 2024-09-26T21:32:19Z

I think this can be kept v1.23 only.

pull-request-size bot added the size/XS label Jun 27, 2024

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 27, 2024

github-actions bot added the modifies/go Pull requests that update Go code label Jun 27, 2024

silverwind approved these changes Jun 27, 2024

View reviewed changes

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 Jun 27, 2024

wxiaoguang reviewed Jun 27, 2024

View reviewed changes

silverwind self-requested a review June 27, 2024 13:49

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

delvh approved these changes Jun 28, 2024

View reviewed changes

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 Jun 28, 2024

charles7668 added 3 commits June 30, 2024 07:59

Fix trimming of leading and trailing spaces when adding a new file

f34a0b9

Add a check for leading or trailing spaces in the parent directory on…

8ddd550

… the "Add New File" page.

Remove parent path when typing a '..' path.

2543f15

charles7668 force-pushed the fix/new-file-space branch from 0c3e581 to 2543f15 Compare June 29, 2024 23:59

pull-request-size bot added size/M and removed size/XS labels Jun 29, 2024

github-actions bot added the modifies/js label Jun 30, 2024

Update Warning message

3cf7472

charles7668 added 3 commits July 19, 2024 11:08

Change .style.display to showElem and hideElem

6d29c32

Prevent the query non-space-related warning div.

df0d7c1

Prevent the use of '/' with no value.

6071df1

Merge remote-tracking branch 'src/main' into fix/new-file-space

7a3ffb2

# Conflicts: # web_src/js/features/repo-editor.ts

charles7668 marked this pull request as ready for review September 10, 2024 05:45

Merge branch 'main' into fix/new-file-space

338cbf6

Fix check for trailing or leading spaces while typing.

df0b1a2

lunny approved these changes Sep 21, 2024

View reviewed changes

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 Sep 21, 2024

lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 24, 2024

Merge branch 'main' into fix/new-file-space

8393fad

github-actions bot added the modifies/frontend label Sep 24, 2024

lunny added the backport/v1.22 This PR should be backported to Gitea 1.22 label Sep 24, 2024

lunny enabled auto-merge (squash) September 24, 2024 18:09

Merge branch 'main' into fix/new-file-space

6d105cd

lunny merged commit 3269b04 into go-gitea:main Sep 24, 2024
26 checks passed

GiteaBot added this to the 1.24.0 milestone Sep 24, 2024

GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 24, 2024

lunny modified the milestones: 1.24.0, 1.23.0 Sep 25, 2024

GiteaBot added the backport/manual No power to the bots! Create your backport yourself! label Sep 26, 2024

lunny removed the backport/v1.22 This PR should be backported to Gitea 1.22 label Sep 26, 2024

go-gitea locked as resolved and limited conversation to collaborators Dec 24, 2024

charles7668 deleted the fix/new-file-space branch March 21, 2025 10:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notify the user when the file path contains leading or trailing spaces and fix the error message for invalid file names. #31507

Notify the user when the file path contains leading or trailing spaces and fix the error message for invalid file names. #31507

charles7668 commented Jun 27, 2024

silverwind left a comment

wxiaoguang Jun 27, 2024

This comment was marked as resolved.

This comment was marked as resolved.

silverwind Jun 27, 2024

wxiaoguang Jun 27, 2024

wxiaoguang Jun 28, 2024

charles7668 Jun 28, 2024 •

edited

Loading

silverwind Jun 28, 2024 •

edited

Loading

delvh Jun 28, 2024

charles7668 Jun 30, 2024 •

edited

Loading

delvh Jun 28, 2024

delvh commented Jul 18, 2024

wxiaoguang commented Jul 18, 2024 •

edited

Loading

charles7668 commented Jul 19, 2024

wxiaoguang commented Jul 19, 2024

charles7668 commented Sep 10, 2024

lunny commented Sep 18, 2024

charles7668 commented Sep 18, 2024 •

edited

Loading

lunny commented Sep 18, 2024

charles7668 commented Sep 18, 2024

GiteaBot commented Sep 26, 2024

lunny commented Sep 26, 2024

Notify the user when the file path contains leading or trailing spaces and fix the error message for invalid file names. #31507

Notify the user when the file path contains leading or trailing spaces and fix the error message for invalid file names. #31507

Conversation

charles7668 commented Jun 27, 2024

silverwind left a comment

Choose a reason for hiding this comment

wxiaoguang Jun 27, 2024

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

silverwind Jun 27, 2024

Choose a reason for hiding this comment

wxiaoguang Jun 27, 2024

Choose a reason for hiding this comment

wxiaoguang Jun 28, 2024

Choose a reason for hiding this comment

charles7668 Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

silverwind Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

delvh Jun 28, 2024

Choose a reason for hiding this comment

charles7668 Jun 30, 2024 • edited Loading

Choose a reason for hiding this comment

delvh Jun 28, 2024

Choose a reason for hiding this comment

delvh commented Jul 18, 2024

wxiaoguang commented Jul 18, 2024 • edited Loading

charles7668 commented Jul 19, 2024

wxiaoguang commented Jul 19, 2024

charles7668 commented Sep 10, 2024

lunny commented Sep 18, 2024

charles7668 commented Sep 18, 2024 • edited Loading

lunny commented Sep 18, 2024

charles7668 commented Sep 18, 2024

GiteaBot commented Sep 26, 2024

lunny commented Sep 26, 2024

charles7668 Jun 28, 2024 •

edited

Loading

silverwind Jun 28, 2024 •

edited

Loading

charles7668 Jun 30, 2024 •

edited

Loading

wxiaoguang commented Jul 18, 2024 •

edited

Loading

charles7668 commented Sep 18, 2024 •

edited

Loading