Skip to content

Commit

Permalink
validator: Add some limited developer-ID validation
Browse files Browse the repository at this point in the history
  • Loading branch information
@ximion
ximion committed Feb 21, 2024
1 parent 09fd889 commit faa48f6
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 3 deletions.
6 changes: 6 additions & 0 deletions src/as-validator-issue-tag.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -603,6 +603,12 @@ AsValidatorIssueTag as_validator_issue_tag_list[] = {
"Consider adding a unique ID."),
},

{ "developer-id-invalid",
AS_ISSUE_SEVERITY_WARNING,
N_("The developer-ID is invalid. It should be an rDNS string identifying the developer, or a Fediverse handle. "
"It must also only contain lowercase ASCII letters, numbers and punctuation."),
},

{ "unknown-desktop-id",
AS_ISSUE_SEVERITY_ERROR,
N_("The set value is not an identifier for a desktop environment as registered with Freedesktop.org."),
Expand Down
28 changes: 25 additions & 3 deletions src/as-validator.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1488,11 +1488,33 @@ as_validator_check_references (AsValidator *validator, xmlNode *node)
static void
as_validator_check_developer (AsValidator *validator, xmlNode *node)
{
g_autofree gchar *devp_id = NULL;
g_autofree gchar *devid = NULL;

devp_id = as_xml_get_prop_value (node, "id");
if (devp_id == NULL)
devid = as_xml_get_prop_value (node, "id");
if (devid == NULL) {
as_validator_add_issue (validator, node, "developer-id-missing", NULL);
} else {
if (g_strstr_len (devid, -1, "@") == NULL && g_strstr_len (devid, -1, ".") == NULL)
as_validator_add_issue (validator, node, "developer-id-invalid", NULL);

for (guint i = 0; devid[i] != '\0'; ++i) {
if (g_ascii_isalpha (devid[i]) && !g_ascii_islower (devid[i])) {
as_validator_add_issue (validator,
node,
"developer-id-invalid",
devid);
break;
}

if (!g_ascii_isalnum (devid[i]) && !g_ascii_ispunct (devid[i])) {
as_validator_add_issue (validator,
node,
"developer-id-invalid",
devid);
break;
}
}
}

for (xmlNode *iter = node->children; iter != NULL; iter = iter->next) {
if (iter->type != XML_ELEMENT_NODE)
Expand Down

6 comments on commit faa48f6

@barthalion
Copy link

Choose a reason for hiding this comment

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

Can this be added to the demotion allowlist, please?

@barthalion
Copy link

Choose a reason for hiding this comment

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

…and on an unrelated note, I wouldn't mind if description-has-plaintext-url could also be demoted. I can submit PRs for both.

@ximion
Copy link
Owner Author

@ximion ximion commented on faa48f6 Feb 21, 2024

Choose a reason for hiding this comment

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

Why would you want to do that? Both are "must not" wordings in the specification. If there are false-positives, we should fix the validator instead...

@barthalion
Copy link

Choose a reason for hiding this comment

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

It's a breaking change. We cannot be breaking builds for developers forever. We will need to find apps which will explode on that, report issues, and announce a reasonable cut-off date.

@barthalion
Copy link

Choose a reason for hiding this comment

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

As for the URL because it's already used in the wild to point users to documentation not fitting existing types, usually when users is expected to do something on the host for the app to work properly, or purchase assets from a game store.

@ximion
Copy link
Owner Author

@ximion ximion commented on faa48f6 Feb 22, 2024

Choose a reason for hiding this comment

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

It's a breaking change. We cannot be breaking builds for developers forever.

It's not though - this will not trigger unless you actually have a developer tag, so if you have developer_name, you'll be fine. It will become a huge issue though if we do not validate this according to spec immediately, because then people will do whatever and we will make them fail validation later once the check is added. Furthermore, the spec is clear on this, and so are the Flathub guidelines - follow them, and you're safe.

In addition, I ran this against thousands of metainfo files overnight (general compose and generator run, also for a different test) and this tag was emitted exactly 0 times.

As for the URL thing: It will result in a plain, unclickable URL in the description text, which is ugly and usually not the intent of the application author. It also, from experience, annoys designers very much. If I relax this check, I'll have bug reports about adding it back within a week, I guarantee it.
So, we either add a proper <url/> style element to descriptions to address this properly, or we don't and people add URLs to the appropriate place (within url blocks) and reference them from the text.

Please sign in to comment.