571535 - Last commenter info is pre-filed on comment input box

Status: RESOLVED FIXED

(Developer Engagement :: Mozilla Hacks, task)

Description

[:Felipe Gomes (needinfo for replies!)]

I opened the following link http://hacks.mozilla.org/2010/06/comparing-indexeddb-and-webdatabase/comment-page-1/#comment-94265 and the comment input boxes came pre-filed with the info from one of the last commenters, including the e-mail address (not the very last, but the last with link/email).
The problem persisted after refreshing the page (the info appeared to be coming directly in the page source), and after a shift + refresh the problem went way and I can't reproduce it anymore.

Comment 1

[Rob Arnold [:robarnold]]

I can also see my email address in:
http://hacks.mozilla.org/2010/06/comparing-indexeddb-and-webdatabase/comment-page-1/#comment-93567
when using Chrome and Minefield on fresh restarts.

Comment 3

[John Karahalis [:openjck]]

Bug 888014 confirms that this is still happening.

Comment 5

[John Karahalis [:openjck]]

Lots of great detail in bug 652560.

Comment 6

[David Walsh :davidwalsh]

I'm not familiar with how we cache these pages on production, but it would appear to be a caching issue to me.  Those forms are pre-filled upon page load, so if we can't find a way to prevent incorrectly caching those requests, we should simply stop outputting preview commenter name / email / etc. fields.

Comment 7

[James Bennett [:ubernostrum]]

(In reply to David Walsh :davidwalsh from comment #6)
> I'm not familiar with how we cache these pages on production, but it would
> appear to be a caching issue to me.  Those forms are pre-filled upon page
> load, so if we can't find a way to prevent incorrectly caching those
> requests, we should simply stop outputting preview commenter name / email /
> etc. fields.

At first glance, this sounds like a Vary issue -- if we're varying on cookie, then this shouldn't happen. But I'm not sure how to check what's going on server-side or change it if necessary.

Comment 8

[Luke Crouch [:groovecoder]]

I checked a request and we *do* have Vary: Cookie set ...

Comment 9

[Robert Nyman]

Still happens. Just got this: http://cl.ly/RJjx from Chris Coyier

Comment 10

[Robert Nyman]

Attachment: hacks-comment.png

Comment 11

[Robert Nyman]

See above attachment - this is what I got this morning. I'd like to upgrade this to major. This is a serious trust issue for us - especially taking about integrity, user data and more, and then we have some caching mechanism that displays information for other users.

Comment 12

[Luke Crouch [:groovecoder]]

Maris, can you help us prioritize this security issue on hacks.mozilla.org? David and I or Craig Cook from Websites/WebProd team are the devs who have worked on the hacks.mozilla.org code.

Comment 13

[David Walsh :davidwalsh]

My only thought here to fix this quickly is to stop trying to pre-fill these form fields all together.  No idea if this is a caching issue, plugin issue, etc.

Comment 14

[Craig Cook (:craigcook)]

A commenter's name, email, and URL are saved in cookies that WP reads and echoes back to the browser that has those cookies. This data isn't coming from the database -- true, it's stored there as part of the comment, but that's not what's being displayed; the form is filled by values from cookies.

So I'm almost certain it's a cache issue, rather than WP somehow generating duplicate cookies. Assuming the page is getting cached when it's rendered for a real visitor, and if that visitor has cookies, the HTML would include this info when it gets cached. Because we don't know which particular request will be the one that gets cached, that would explain why this only appears sometimes and is hard to reproduce, but still happens consistently enough to be a big concern.

So the simplest fix, as David said in comment 13, is to just remove the code that echoes the cookie values to prepopulate the fields. At the same time I can add a filter that prevents setting those comment cookies, since we no longer have use for them anyway. If it's never displayed it can never be cached, and it's just a minor inconvenience for repeat commenters (they'll have to fill in the form every time). I suspect those people are relatively few.

I've filed https://github.com/mozilla/mozhacks/pull/18 to remove the display of user data and disable the cookies.

If we get a lot of repeat commenters and we want to continue setting the cookies and prepopulating those fields, the next thing to look into is configuring the caching mechanism so it doesn't cache requests for pages with comment forms, or maybe make some kind of anonymous request to generate the cache so it won't have those cookies. I have no idea how hacks is cached currently so this might be easy or it might be hard. It would only be worth exploring if there's a real benefit to keeping the cookies, or if it turns out to be trivial to not cache comment forms. 

WP Super Cache is a plugin that does pretty smart caching, including not serving cached pages to people with those comment cookies, nor to logged-in users, or those who have viewed private posts (so those don't get cached either). It's been approved by infrasec and we're using it elsewhere, so maybe we should use it on hacks instead of however it's being cached presently. http://wordpress.org/plugins/wp-super-cache/

Comment 15

[Robert Nyman]

Thanks Craig! And yes, disabling seems to be the easiest thing for now. I'd also like to see Persona support get implemented on Hacks, so that way people can have their info remembered.

Will you file a bug to get this deployed, or will that happen automatically?

Comment 16

[Luke Crouch [:groovecoder]]

My bookmarks include URL's for stage and prod chief servers for hacks:

http://genericadm.private.phx1.mozilla.com/chief/hacks.stage

http://genericadm.private.phx1.mozilla.com/chief/hacks.prod

We should be able to push this ourselves?

Comment 17

[Craig Cook (:craigcook)]

I just pushed the changes to stage and production. I've verified that comment cookies are not being set and the fields are not prepopulated after commenting. Marking this resolved but there may still be some old caches being served for a short while so be on the lookout.

Comment 18

[Robert Nyman]

Great, thank you very much!

Comment 19

[Will Kahn-Greene [:willkg] ET needinfo? me]

For bugs that are resolved, we remove the security flag. These haven't had their flag removed, so I'm removing it now.