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

App Name Validation Can Allow Names that Produce an Invalid pyproject.toml #1011

Closed
rmartin16 opened this issue Dec 16, 2022 · 5 comments
Closed
Labels
bug A crash or error in behavior. good first issue Is this your first time contributing? This could be a good place to start!

Comments

@rmartin16
Copy link
Member

rmartin16 commented Dec 16, 2022

Describe the bug

The App name validation can allow characters that are not valid as "bare keys" in the table names of the pyproject.toml file.

For example, an app name of this_is_a_dotless_lowercase_i_ı produces table names like [tool.briefcase.app.this_is_a_dotless_lowercase_i_ı.macOS].

TOML only allows ASCII in "bare key" names. But "quoted keys" can use a much broader set of characters. So, [tool.briefcase.app."this_is_a_dotless_lowercase_i_ı".macOS] becomes valid TOML.

Given that Briefcase is simply using the regex from PEP-508 to validate names, the root of this may stem from much further upstream. Nonetheless, I think the underlying problem is they recommended using re.IGNORECASE; my guess is the underlying case folding is making ı in to a "normal" lowercase i and the the validation succeeds.

Steps to reproduce

Use ı or also İ in the App Name when running briefcase new.

Expected behavior

Such names should be rejected or the TOML should be created properly.

Screenshots

No response

Environment

  • Operating System: pop os 22.04
  • Python version: 3.10.8
  • Software versions:
    • Briefcase: 0.3.12.dev251+g6d29ece1

Logs

briefcase.2022_12_16-15_17_55.dev.log

Additional context

No response

@freakboy3742
Copy link
Member

this_is_a_dotless_lowercase_i_ı is a valid module name in python (as in import this_is_a_dotless_lowercase_i_ı works if there's a this_is_a_dotless_lowercase_i_ı.py on your filesystem), so the app name validation is working; however, it's not valid TOML in a bare state. This suggests to me that we need a "safe toml key" filter that can determine if a string requires quoting (and add quotes if it is), then use that filter in the briefcase template anywhere a user-provided string is used in a key. This should be pretty straightforward - "is this string bare ascii" is a robust check, and we already have the briefcase.integrations.cookiecutter module defining a bunch of filters.

@freakboy3742 freakboy3742 added the good first issue Is this your first time contributing? This could be a good place to start! label Dec 17, 2022
@rmartin16
Copy link
Member Author

hmmm....this sent me down another rabbit hole. I eventually found PEP-3131 and its subsequent docs that outlines the supported non-ASCII characters allowed in "names" in Python. Therefore, ı is certainly valid as part of a name.

However, these rules do not apply for all the tooling used throughout Briefcase. Using this rule, I was only able to build a DMG package; all other workflows choked on the ı character....even web had problems.

Therefore, we may ultimately be better served by revising the validation for this name to actually enforce only ASCII, underscore, or dash.

@freakboy3742
Copy link
Member

I'm not sure a "hard ASCII" requirement is a valid option here. Briefcase aims to be able to wrap any Python module; if ı is a valid character in a module name, we need to be able to support it.

That said - there may be a need to provide an "ASCII-safe" mapping of app name, in the same way that we have module_name as a mapping that is a valid import, class_name that is a conversion to a class, etc. Something like the IDNA encoding might be an option here; we'd then need to audit the places where app name is used to see where "ASCII name" (or quoting of app name) is needed.

@jkomalley
Copy link
Contributor

I'm taking a jab at this during the sprint

@mhsmith
Copy link
Member

mhsmith commented Apr 28, 2023

Fixed by #1223.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A crash or error in behavior. good first issue Is this your first time contributing? This could be a good place to start!
Projects
None yet
Development

No branches or pull requests

4 participants