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

Improve error message when an add-on contains a bad file/folder name #2043

Closed
Pentarctagon opened this Issue Sep 25, 2017 · 9 comments

Comments

Projects
None yet
9 participants
@Pentarctagon
Member

Pentarctagon commented Sep 25, 2017

Currently when the add-ons server rejects an add-on due to having an invalid character in a folder or file name, all it returns is:
Add-on rejected: The add-on contains an illegal file or directory name. File or directory names may not contain whitespace or any of the following characters: '/ \ : ~'

Since it knows that the file name has one of those invalid characters, the error message should also tell what those files are, rather than just leaving the user to hunt them down themselves. As an example, fmac's add-on Era of Second Chances encountered this issue, and as a result it was required to search though all 801 files to find only a few that had a whitespace in the file name(download).

@Arcanister

This comment has been minimized.

Show comment
Hide comment
@Arcanister

Arcanister Sep 26, 2017

Contributor

Server implementation doesn't have to hunt all filenames down though, it could reject the add-on after finding first non-conforming file name. Do you think it would be OK if it does that and outputs only first bad file name?

Does the server message mean that all other characters such as control \1 to \037 are allowed in filenames?

A side note: It's not that hard to search for filenames, anyway.
In GNU/Linux, OS X and other UNIX-like environments, user can use a command such as:

find Addon_Name -name '*[!A-Za-z0-9_.+@-]*'

A Windows user can install cygwin or apply CMD and type something like:

dir /a /s /b Addon_Name | findstr /i /r /c:"[^a-z0-9_.+@\\:-]"

Thought this version wont work from default userdata path name, since it contains space.

This version takes it into account and checks only last path name component:

 dir /a /s /b "C:\Users\Your Login Name\Documents\My Games\Wesnoth1.13\data" | findstr /e /r /c:"[\\][^\\]*[^\\A-Za-z0-9_.+@-][^\\]*"

A command like that will scan all installed add-ons for incorrect filenames. It will also notice names having unicode characters outside of ASCII. It could be done to report only characters which Wesnoth addon server actually rejects, if necessary, but it is good practice to avoid special characters in resource names. Of course path should be replaced with path you want to check or simply omitted if you cd there first.

Users of newer versions of Windows can use PowerShell:

get-childitem -recurse | where {$_.Name -match ".*[^a-z0-9_.+@-].*"} | select -expand fullname

A file manager with search capabilities can also be used.

For example, in Windows Explorer you could type something like "*~*.*" and then "* *.*" in search box including quotes. Colons and slashes are prohibited by OS itself anyway, so there is should be no need to check for them.

Contributor

Arcanister commented Sep 26, 2017

Server implementation doesn't have to hunt all filenames down though, it could reject the add-on after finding first non-conforming file name. Do you think it would be OK if it does that and outputs only first bad file name?

Does the server message mean that all other characters such as control \1 to \037 are allowed in filenames?

A side note: It's not that hard to search for filenames, anyway.
In GNU/Linux, OS X and other UNIX-like environments, user can use a command such as:

find Addon_Name -name '*[!A-Za-z0-9_.+@-]*'

A Windows user can install cygwin or apply CMD and type something like:

dir /a /s /b Addon_Name | findstr /i /r /c:"[^a-z0-9_.+@\\:-]"

Thought this version wont work from default userdata path name, since it contains space.

This version takes it into account and checks only last path name component:

 dir /a /s /b "C:\Users\Your Login Name\Documents\My Games\Wesnoth1.13\data" | findstr /e /r /c:"[\\][^\\]*[^\\A-Za-z0-9_.+@-][^\\]*"

A command like that will scan all installed add-ons for incorrect filenames. It will also notice names having unicode characters outside of ASCII. It could be done to report only characters which Wesnoth addon server actually rejects, if necessary, but it is good practice to avoid special characters in resource names. Of course path should be replaced with path you want to check or simply omitted if you cd there first.

Users of newer versions of Windows can use PowerShell:

get-childitem -recurse | where {$_.Name -match ".*[^a-z0-9_.+@-].*"} | select -expand fullname

A file manager with search capabilities can also be used.

For example, in Windows Explorer you could type something like "*~*.*" and then "* *.*" in search box including quotes. Colons and slashes are prohibited by OS itself anyway, so there is should be no need to check for them.

@jyrkive

This comment has been minimized.

Show comment
Hide comment
@jyrkive

jyrkive Sep 26, 2017

Member

@Arcanister I bet there are many UMC authors who don't have the first idea about how to find files with command line.

Member

jyrkive commented Sep 26, 2017

@Arcanister I bet there are many UMC authors who don't have the first idea about how to find files with command line.

@jyrkive

This comment has been minimized.

Show comment
Hide comment
@jyrkive

jyrkive Sep 26, 2017

Member

My point was that they have no idea what they need to write to the search box.

Member

jyrkive commented Sep 26, 2017

My point was that they have no idea what they need to write to the search box.

shikadiqueen added a commit to shikadiqueen/wesnoth that referenced this issue Sep 26, 2017

campaignd: Send uploading clients a list of illegal names when any ar…
…e found

(See issue #2043.)

It wouldn't be proper to stuff the full list into the conventional error
response, so in order to provide clients with the means to report it in
a better fashion in the future (a la WML load errors report dialog, with
a fancy scrolling box and an option to copy the report to clipboard and
stuff), we're attaching it as an extra_data attribute in the [error]
response. Naturally, only clients that are aware of its existence can
handle it, so this whole patch is completely unsuitable for 1.12.

Still, some level of backwards compatibility is retained -- the [error]
continues to be perfectly functional for incompatible clients (e.g.
1.13.10), and the details are simply missing for them.

The list would currently look like this:

  Test_Addon_420/~illegalname
  Test_Addon_420/testdir/illegal name 2
  Test_Addon_420/testdir/illegal~dir~name/

The required client-side changes for handling the extra_data attribute
and displaying its contents are included in this patch, but the GUI
changes are very rudimentary and should be considered more of a
proof-of-concept than a final solution, which I'll leave to someone who
actually groks GUI2 as it is right now and is an active developer in
the first place, which I am not.

Also note that the only reason the second parameter for
check_names_legal() is optional is that the function is also used
client-side by addons_client::install_addon(), which really doesn't need
to waste time building a full report of invalid names -- add-ons
containing those aren't supposed to make it to the server in the first
place, and the client-side check only exists as an extra safeguard.
@sevu

This comment has been minimized.

Show comment
Hide comment
@sevu

sevu Sep 28, 2017

Member

Windows users don't use any shell. But you can add it somewhere in the wiki.

Member

sevu commented Sep 28, 2017

Windows users don't use any shell. But you can add it somewhere in the wiki.

@Pentarctagon

This comment has been minimized.

Show comment
Hide comment
@Pentarctagon

Pentarctagon Sep 28, 2017

Member

@Arcanister Not to say that those commands aren't useful, but I think you are vastly overestimating the number of people who know cmd.exe or powershell even exist.

Member

Pentarctagon commented Sep 28, 2017

@Arcanister Not to say that those commands aren't useful, but I think you are vastly overestimating the number of people who know cmd.exe or powershell even exist.

shikadiqueen added a commit to shikadiqueen/wesnoth that referenced this issue Oct 5, 2017

campaignd: Send uploading clients a list of illegal names when any ar…
…e found

(See issue #2043.)

It wouldn't be proper to stuff the full list into the conventional error
response, so in order to provide clients with the means to report it in
a better fashion in the future (a la WML load errors report dialog, with
a fancy scrolling box and an option to copy the report to clipboard and
stuff), we're attaching it as an extra_data attribute in the [error]
response. Naturally, only clients that are aware of its existence can
handle it, so this whole patch is completely unsuitable for 1.12.

Still, some level of backwards compatibility is retained -- the [error]
continues to be perfectly functional for incompatible clients (e.g.
1.13.10), and the details are simply missing for them.

The list would currently look like this:

  Test_Addon_420/~illegalname
  Test_Addon_420/testdir/illegal name 2
  Test_Addon_420/testdir/illegal~dir~name/

The required client-side changes for handling the extra_data attribute
and displaying its contents are included in this patch, but the GUI
changes are very rudimentary and should be considered more of a
proof-of-concept than a final solution, which I'll leave to someone who
actually groks GUI2 as it is right now and is an active developer in
the first place, which I am not.

Also note that the only reason the second parameter for
check_names_legal() is optional is that the function is also used
client-side by addons_client::install_addon(), which really doesn't need
to waste time building a full report of invalid names -- add-ons
containing those aren't supposed to make it to the server in the first
place, and the client-side check only exists as an extra safeguard.

Vultraz added a commit that referenced this issue Oct 5, 2017

campaignd: Send uploading clients a list of illegal names when any ar…
…e found

(See issue #2043.)

It wouldn't be proper to stuff the full list into the conventional error
response, so in order to provide clients with the means to report it in
a better fashion in the future (a la WML load errors report dialog, with
a fancy scrolling box and an option to copy the report to clipboard and
stuff), we're attaching it as an extra_data attribute in the [error]
response. Naturally, only clients that are aware of its existence can
handle it, so this whole patch is completely unsuitable for 1.12.

Still, some level of backwards compatibility is retained -- the [error]
continues to be perfectly functional for incompatible clients (e.g.
1.13.10), and the details are simply missing for them.

The list would currently look like this:

  Test_Addon_420/~illegalname
  Test_Addon_420/testdir/illegal name 2
  Test_Addon_420/testdir/illegal~dir~name/

The required client-side changes for handling the extra_data attribute
and displaying its contents are included in this patch, but the GUI
changes are very rudimentary and should be considered more of a
proof-of-concept than a final solution, which I'll leave to someone who
actually groks GUI2 as it is right now and is an active developer in
the first place, which I am not.

Also note that the only reason the second parameter for
check_names_legal() is optional is that the function is also used
client-side by addons_client::install_addon(), which really doesn't need
to waste time building a full report of invalid names -- add-ons
containing those aren't supposed to make it to the server in the first
place, and the client-side check only exists as an extra safeguard.
@gfgtdf

This comment has been minimized.

Show comment
Hide comment
@gfgtdf

gfgtdf Dec 26, 2017

Contributor

is this fixed?

Contributor

gfgtdf commented Dec 26, 2017

is this fixed?

@Pentarctagon

This comment has been minimized.

Show comment
Hide comment
@Pentarctagon
Member

Pentarctagon commented Dec 28, 2017

@GregoryLundberg

This comment has been minimized.

Show comment
Hide comment
@GregoryLundberg

GregoryLundberg Dec 28, 2017

Contributor

I can see it now:

What is the ''' character? Does the list include ' and .

While technically correct, a quoted list and terminating period are visually confusing. I'd suggest a line-break and showing only the characters and no punctuation.

Contributor

GregoryLundberg commented Dec 28, 2017

I can see it now:

What is the ''' character? Does the list include ' and .

While technically correct, a quoted list and terminating period are visually confusing. I'd suggest a line-break and showing only the characters and no punctuation.

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Dec 29, 2017

Member

Agreed.

Member

CelticMinstrel commented Dec 29, 2017

Agreed.

GregoryLundberg added a commit that referenced this issue Jan 6, 2018

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