1034535 - Strip some elements in the shipt-it form

Status: RESOLVED FIXED

(Release Engineering :: Release Automation: Other, defect, P3)

Description

[Sylvestre Ledru [:Sylvestre]]

Attachment: 0001-Strip-some-elements-of-the-form.patch

During yesterday build (30b7), I added a trailing whitespace (bad copy and paste). This broke the build.

The attached patch strip some inputs of the form to make sure that won't happen again.

Comment 2

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8450878 [details] [diff] [review]
0001-Strip-some-elements-of-the-form.patch

I'll defer to the ship-it gurus.

Comment 3

[Rail Aliiev [:rail]]

Comment on attachment 8450878 [details] [diff] [review]
0001-Strip-some-elements-of-the-form.patch

Review of attachment 8450878 [details] [diff] [review]:
-----------------------------------------------------------------

Leaving this for bhearsum.

BTW, can we use form validation instead of stripping characters (what may not help in some cases). I remember once we submitted a revision with some Unicode characters at the end.

Comment 4

[[e-mail] (:bhearsum)]

Comment on attachment 8450878 [details] [diff] [review]
0001-Strip-some-elements-of-the-form.patch

Review of attachment 8450878 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Rail Aliiev [:rail] from comment #3)
> Comment on attachment 8450878 [details] [diff] [review]
> 0001-Strip-some-elements-of-the-form.patch
> 
> Review of attachment 8450878 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Leaving this for bhearsum.
> 
> BTW, can we use form validation instead of stripping characters (what may
> not help in some cases). I remember once we submitted a revision with some
> Unicode characters at the end.

Yeah, I remember that. That was specifically with the revision field IIRC. I think it would be good to have better validation, but I don't think we should block this on that.

::: kickoff/model.py
@@ +44,3 @@
>          self.buildNumber = buildNumber
> +        self.branch = branch.strip()
> +        self.mozillaRevision = mozillaRevision.strip()

Is there a reason you're only stripping these 3 fields? It seems like any <input type="text"> could be vulnerable to the same issues. Probably the l10n changesests field as well...

Comment 5

[Sylvestre Ledru [:Sylvestre]]

(In reply to Ben Hearsum [:bhearsum] from comment #4)
> Comment on attachment 8450878 [details] [diff] [review]
> 0001-Strip-some-elements-of-the-form.patch
> 
> Review of attachment 8450878 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Rail Aliiev [:rail] from comment #3)
> > Comment on attachment 8450878 [details] [diff] [review]
> > 0001-Strip-some-elements-of-the-form.patch
> > 
> > Review of attachment 8450878 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Leaving this for bhearsum.
> > 
> > BTW, can we use form validation instead of stripping characters (what may
> > not help in some cases). I remember once we submitted a revision with some
> > Unicode characters at the end.
> 
> Yeah, I remember that. That was specifically with the revision field IIRC. I
> think it would be good to have better validation, but I don't think we
> should block this on that.
A regexp checking the size + hexadecimal ?


> ::: kickoff/model.py
> @@ +44,3 @@
> >          self.buildNumber = buildNumber
> > +        self.branch = branch.strip()
> > +        self.mozillaRevision = mozillaRevision.strip()
> 
> Is there a reason you're only stripping these 3 fields? It seems like any
> <input type="text"> could be vulnerable to the same issues. Probably the
> l10n changesests field as well...
I already fixed the l10n changeset ( cf cset c98e6e75b7ae78338a4fcb18dcb88c074e605077 )
Other are integer (for example, the build number), so, automagic casting I guess

Comment 6

[[e-mail] (:bhearsum)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #5)
> (In reply to Ben Hearsum [:bhearsum] from comment #4)
> > Comment on attachment 8450878 [details] [diff] [review]
> > 0001-Strip-some-elements-of-the-form.patch
> > 
> > Review of attachment 8450878 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > (In reply to Rail Aliiev [:rail] from comment #3)
> > > Comment on attachment 8450878 [details] [diff] [review]
> > > 0001-Strip-some-elements-of-the-form.patch
> > > 
> > > Review of attachment 8450878 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Leaving this for bhearsum.
> > > 
> > > BTW, can we use form validation instead of stripping characters (what may
> > > not help in some cases). I remember once we submitted a revision with some
> > > Unicode characters at the end.
> > 
> > Yeah, I remember that. That was specifically with the revision field IIRC. I
> > think it would be good to have better validation, but I don't think we
> > should block this on that.
> A regexp checking the size + hexadecimal ?

Probably...but that might be best off in another bug.

Comment 7

[[e-mail] (:bhearsum)]

Are you going to check this is in, Sylvestre?

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Do I have the permission ? :)

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Merged. Sorry. I forgot to add Bug: and r=. I was testing the push and I cannot force the update
http://git.mozilla.org/?p=build/release-kickoff.git;a=commit;h=001a10a4926c58ee1664c1438229b5fd8119a59c

Comment 10

[[e-mail] (:bhearsum)]

This still needs to be deployed before it takes effect. I'll do that sometime in the near future, don't have time to right now.

Comment 11

[[e-mail] (:bhearsum)]

This was pushed to production today.