Allows dashes in OTA distribution names

RESOLVED FIXED in Firefox 55

Status

()

Firefox for Android
Distributions
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mkaply, Assigned: mkaply)

Tracking

54 Branch
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
OTA distribution names currently only allow letters and numbers.

We should at least allow hyphens.
Comment hidden (mozreview-request)
(In reply to Mike Kaply [:mkaply] from comment #0)
> OTA distribution names currently only allow letters and numbers.
> 
> We should at least allow hyphens.

Why?  And why not the "standard" identifier set [a-zA-Z0-9_-], then?
(In reply to Nick Alexander :nalexander from comment #2)
> (In reply to Mike Kaply [:mkaply] from comment #0)
> > OTA distribution names currently only allow letters and numbers.
> > 
> > We should at least allow hyphens.
> 
> Why?  And why not the "standard" identifier set [a-zA-Z0-9_-], then?

Sorry, I see that the bug title and the patch do not agree, and you expanded this in the patch.

Comment 4

a year ago
mozreview-review
Comment on attachment 8858886 [details]
Bug 1357121 - Allow dashes and underscore in OTA distribution name.

https://reviewboard.mozilla.org/r/130872/#review133448

::: mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java:881
(Diff revision 1)
>              return null;
>          }
>  
>          // We restrict here to avoid injection attacks. After all,
>          // we're downloading a distribution payload based on intent input.
> -        if (!content.matches("^[a-zA-Z0-9]+$")) {
> +        if (!content.matches("^[a-zA-Z0-9-_]+$")) {

I can't find a link, but I think this hyphen needs to be escaped _or_ be the final character of the set.
Attachment #8858886 - Flags: review?(nalexander)
(Assignee)

Comment 5

a year ago
> Why?  And why not the "standard" identifier set [a-zA-Z0-9_-], then?

I didn't know there was a standard :). I'll fix that.

As far as the "why", it's because we're having to do some device specific distribution ID specific configs, and it seems silly to have to make them foo001 instead of foo-001 and match the distribution ID.

I added underscores simply because that seemed logical to allow that as well.

In most cases like this, you allow numbers, letter, dashes and underscores.
Comment hidden (mozreview-request)

Comment 7

a year ago
mozreview-review
Comment on attachment 8858886 [details]
Bug 1357121 - Allow dashes and underscore in OTA distribution name.

https://reviewboard.mozilla.org/r/130872/#review133452

The "matching" part was what I wanted to hear.  Ta!
Attachment #8858886 - Flags: review?(nalexander) → review+

Comment 8

a year ago
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/bcbab8f89ee7
Allow dashes and underscore in OTA distribution name. r=nalexander

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bcbab8f89ee7
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.