632820 - No longer able to edit Personas on getpersonas.com

Status: VERIFIED FIXED

(Websites Graveyard :: getpersonas.com, defect)

Description

[Deb Richardson [:dria] (plz NEEDINFO)]

Since the system update yesterday, attempting to edit a Persona only brings up a blank page rather than the editing UI.  This is a significant issue because we strongly encourage Personas authors to edit existing Personas rather than submitting a duplicate with minor changes in order to keep the review queue down.

If you need login credentials and a persona to test, just ping me in IRC, email, or IM.

Comment 2

[Matthew N. [:MattN]]

Here is the error log snippet from my dev box:

[error] [client 127.0.0.1] PHP Notice:  Undefined variable: tos_tmpl in /blah/getpersonas.com/trunk/server/upload_forms.php on line 99, referer: https://getpersonas.com.localhost/en-US/gallery/
[error] [client 127.0.0.1] PHP Warning:  include() [<a href='function.include'>function.include</a>]: Filename cannot be empty in /blah/getpersonas.com/trunk/server/upload_forms.php on line 99, referer: https://getpersonas.com/en-US/gallery/
[error] [client 127.0.0.1] PHP Warning:  include() [<a href='function.include'>function.include</a>]: Failed opening '' for inclusion (include_path='.::/blah/getpersonas.com/trunk/server') in /blah/getpersonas.com/trunk/server/upload_forms.php on line 99, referer: https://getpersonas.com/en-US/gallery/

Comment 3

[Matthew N. [:MattN]]

I'm just testing out a fix now.  I'll post a patch shortly.

Comment 4

[Wil Clouser [:clouserw]]

Thanks Matthew

Comment 5

[Matthew N. [:MattN]]

Attachment: v.1 don't check nonce on initial loading of edit screen

I changed the code to not check the nonce on initial loading of the edit screen. Is there any security issue with this?  The nonce is still checked on submission of the form.

Comment 6

[Wil Clouser [:clouserw]]

Comment on attachment 511065 [details] [diff] [review]
v.1 don't check nonce on initial loading of edit screen

If you look at line 32 you can see that id can come from GET or POST.  It would be better to check $id here, which already looks in either.  I'll r- for that, but [e-mail] or :telliott can review patches faster than I can

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 511065 [details] [diff] [review]
v.1 don't check nonce on initial loading of edit screen

(In reply to comment #6)
> Comment on attachment 511065 [details] [diff] [review]
> v.1 don't check nonce on initial loading of edit screen
> 
> If you look at line 32 you can see that id can come from GET or POST.  It would
> be better to check $id here, which already looks in either.  I'll r- for that,
> but [e-mail] or :telliott can review patches faster than I can

I intentionally didn't use $id since I DO want to check the nonce when id is submitted over POST or is null but not when id is set via GET.  When we get a failed nonce we just include the form again and so this check needs to be far enough in the code so that it has the values to fill in the form (ie. $upload_submitted and $categories).

Comment 8

[Barry Chen]

Comment on attachment 511065 [details] [diff] [review]
v.1 don't check nonce on initial loading of edit screen

Since the error is the missing template path, could establish a path to include depending if it's an add or edit.  And move the get categories line up, of course.

I could bypass the nonce check by adding the id to the form action url since that && in the if will short circuit.

Comment 9

[Matthew N. [:MattN]]

Attachment: v.2 don't check nonce on initial loading of edit screen (using GET)

(In reply to comment #8)
> Comment on attachment 511065 [details] [diff] [review]
> v.1 don't check nonce on initial loading of edit screen
> 
> Since the error is the missing template path, could establish a path to include
> depending if it's an add or edit.  And move the get categories line up, of
> course.

The output in the PHP error log are warnings and are not really the main problem.  It's really that "exit" is being hit since $user->validate_nonce() is false when clicking the edit link.

I'm not sure that there would be much benefit in having two templates at this point as I don't really know how they would differ.

> I could bypass the nonce check by adding the id to the form action url since
> that && in the if will short circuit.

True. I switched to validating the nonce whenever POST is used.

Comment 10

[Barry Chen]

(In reply to comment #9)
> (In reply to comment #8)
> > Since the error is the missing template path, could establish a path to include
> > depending if it's an add or edit.  And move the get categories line up, of
> > course.
> 
> I'm not sure that there would be much benefit in having two templates at this
> point as I don't really know how they would differ.

The edit process doesn't include the Term of Service step.  So I meant that the template path variable should point to the correct template, which is what your patch's first line is doing.

Checking the nonce only on POST works for me.

Comment 11

[Matthew N. [:MattN]]

r82393

To verify:
* Make sure that the edit page for a persona now loads and that edits submitted show up in the edit review queue.
* Verify that the nonce is checked in the following cases by modifying the nonce hidden input and making sure that the website doesn't proceed to the next step after submission.
** From the TOS page when submitting a new persona
** From the upload page when submitting a new persona
** From the upload page when editing a persona.

Comment 12

[Wil Clouser [:clouserw]]

QA: If you can verify this on stage, I can try to get it out today.

Comment 13

[Vishal Kamdar [:vish_moz]]

Verified, FIXED.
1. 'edit' link opens up the correct page
2. edited personas show up in the edit review queue
3. Modifying the nonce value on the following pages does not let you continue to the next step:
 a. upload a new persona
 b. edit a persona

Isn't the Terms of Service page same as the upload page which is https://personas.stage.mozilla.com/en-US/upload ?

Comment 14

[Matthew N. [:MattN]]

(In reply to comment #13)
> Isn't the Terms of Service page same as the upload page which is
> https://personas.stage.mozilla.com/en-US/upload ?

Yes, I just wanted to make sure both steps (accepting TOS & persona details) of the upload process would be verified (making sure the nonce was checked).

Comment 15

[Wil Clouser [:clouserw]]

this fix is live