Last Comment Bug 476678 - Rich clients unable to update bugs need security token included in bug xml
: Rich clients unable to update bugs need security token included in bug xml
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Creating/Changing Bugs (show other bugs)
: 3.2.2
: All All
: -- major (vote)
: Bugzilla 3.2
Assigned To: Greg Hendricks
: default-qa
Mentors:
Depends on: 26257
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-03 10:38 PST by Robert Elves
Modified: 2009-02-05 11:18 PST (History)
8 users (show)
LpSolit: approval+
LpSolit: approval3.2+
LpSolit: blocking3.2.3+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (772 bytes, patch)
2009-02-03 12:30 PST, Max Kanat-Alexander
LpSolit: review-
gregaryh: review+
Details | Diff | Review
V2 (2.29 KB, patch)
2009-02-05 09:12 PST, Greg Hendricks
LpSolit: review+
Details | Diff | Review
Checked In Version (2.30 KB, patch)
2009-02-05 10:42 PST, Greg Hendricks
no flags Details | Diff | Review

Description Robert Elves 2009-02-03 10:38:09 PST
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.5) Gecko/2008120121 Firefox/3.0.5
Build Identifier: 

The new security token (bug#26257) is included in html output but not in the xml for bugs (show_bug.cgi with ctype=xml). This is required to support bugzilla clients that need to edit bugs but don't parse the html pages.

Reproducible: Always
Comment 1 Frédéric Buclin 2009-02-03 10:40:09 PST
We first have to make sure one cannot use this token to abuse the user.
Comment 2 Robert Elves 2009-02-03 12:09:03 PST
Any idea on when this fix would be included in a release?  This is a real blocker for bugzilla clients and the work around is expensive (extra bandwidth reqd to load html before submitting changes).
Comment 3 Max Kanat-Alexander 2009-02-03 12:20:38 PST
FWIW, I CC'ed Mik and mentioned this and I didn't get any response from Eclipse:

  https://bugzilla.mozilla.org/show_bug.cgi?id=26257#c104

Igor (from Deskzilla) says that he always loads show_bug.cgi.

We could possibly present a token in the XML.
Comment 4 Max Kanat-Alexander 2009-02-03 12:30:58 PST
Created attachment 360348 [details] [diff] [review]
v1

Here, this is no more dangerous than having the token in the HTML on show_bug.cgi, where it could be parsed out anyway.
Comment 5 Max Kanat-Alexander 2009-02-03 12:33:32 PST
Note that if the bug is updated in between when you load this token and when you submit it, though, the token will become invalid (since it depends on delta_ts). I assume that's nothing new for rich clients, though, if they're already dealing with midair collisions.

The long-term solution for this is to have Bug.update in XML-RPC.
Comment 6 Robert Elves 2009-02-03 13:08:15 PST
Thanks for pointing that out Max, I see your comment and that Mik is on the cc. We appreciate the consideration and if you could you could put me on the cc for bugs of this sort in the future that would be great as I'm the lead on Mylyn's Bugzilla integration.  

You are correct, Mylyn already handles the case of mid-air collisions so we should be fine.

+1 for the Bug.update.
Comment 7 Max Kanat-Alexander 2009-02-03 13:16:06 PST
(In reply to comment #6)
> We appreciate the consideration and if you could you could put me on the cc for
> bugs of this sort in the future that would be great as I'm the lead on Mylyn's
> Bugzilla integration.  

  Okay, great to know. :-) Will do.
Comment 8 Igor Sereda 2009-02-03 13:44:48 PST
Max, thanks for keeping me CCed on this stuff :)

You are right, Deskzilla doesn't need token in the XML because every time it calls process_bug.cgi, it first loads show_bug.cgi and gets the token from there.

Actually, I would advise every remote client to do the same. Because in fact, the main point is not to get the token, but to get all the form inputs that are being sent to the script by the browser when user hits Submit Changes -- so Deskzilla can send them as well even if it doesn't "know" them. 

Remember, this is not API, so you can't assume anything. Better act as a browser, and you won't risk inadvertently changing something because you didn't include in HTTP POST some form data that your program doesn't know about.

Gee, gotta stop helping competitors ;)

Igor
Comment 9 Robert Elves 2009-02-03 14:18:04 PST
(In reply to comment #8)

The Mylyn Bugzilla Connector at one point did scrape the html but has now adopted the use of Bugzilla's xml and rdf output to improve support for highly customized Bugzilla presentations/templates. This does however open us up to problems of this nature when the xml doesn't have all the data necessary for submission. The minute the web service api has the update functionality Max mentioned, we'll be eager adopters!  :)
Comment 10 Mik Kersten 2009-02-03 16:05:18 PST
Thanks Igor.  We are always happy to lend a hand back to you as well, and have you reuse or refer to any parts of our open client as needed.

Max: My apologies for not noticing the CC.  I had disabled my Mozilla repository in Mylyn because we were in the process of fixing other Bugzilla 3.2 related incompatibilities.  Will be seeing CCs again from now on.

We are in a tough position right now, because Eclipse Galileo SR2 is being released in 3 weeks, so we have to figure out what fix to put in place for that.  That version of our Bugzilla client will see something like 2-4 million downloads and be in active use for at least a year.  We are also dealing with the new reality that some large open source repositories' usage is dominated by the Mylyn Bugzilla client, not the Web UI, as visible from user agent entries in the http logs.

So, it sounds like we have two options now:
1) We always retrieve the entire HTML on submission, and screenscrape the token 
2) We code the solution against this patch, and continue relying on our asynchronous retrieval of XML contents

(1) is problematic because it will double our network traffic when retrieving contents, and because it could break with the dozens of customized Bugzilla installs that we support (e.g., RedHat).  I assume that for (2) we would need a Bugzilla 3.2.3 release to happen.  Is this feasible?
Comment 11 Max Kanat-Alexander 2009-02-03 21:50:56 PST
(In reply to comment #10)
> (1) is problematic because it will double our network traffic when retrieving
> contents, and because it could break with the dozens of customized Bugzilla
> installs that we support (e.g., RedHat).  I assume that for (2) we would need a
> Bugzilla 3.2.3 release to happen.  Is this feasible?

  Yeah, I'd like to have another release in about a month or so. We still have a few security issues to patch up, and it'd be nice to do another development release after the hard freeze. Too many releases in close succession put an unnecessary burden on sysadmins, though, so I do want to wait at least a month. In the mean time people are free to apply this patch to their installation (or whatever version we come up with that passes review).
Comment 12 Frédéric Buclin 2009-02-04 10:28:52 PST
Comment on attachment 360348 [details] [diff] [review]
v1

The token shouldn't be passed to the target installation when moving bugs, as it will appear in plain text in a bug comment. Also, importxml.pl doesn't know what to do with this field.

So you should display the token only if show_bug.cgi is called, and add "token" to @fieldlist. This way, the "field" and "excludefield" parameters passed to show_bug.cgi will still work correctly.
Comment 13 Frédéric Buclin 2009-02-04 15:48:32 PST
I suppose everybody wants this bug to be fixed in 3.2.3.
Comment 14 Frédéric Buclin 2009-02-04 16:11:44 PST
Note that 2.22.x and 3.0.x are not affected, as bug 26257 only landed on the 3.2 branch.
Comment 15 Robert Elves 2009-02-04 16:20:49 PST
(In reply to comment #13)
> I suppose everybody wants this bug to be fixed in 3.2.3.

+1 Thanks! :)
Comment 16 Greg Hendricks 2009-02-05 09:12:02 PST
Created attachment 360737 [details] [diff] [review]
V2

importxml can safely ignore the token.
Comment 17 Tiago Mello [:timello] 2009-02-05 10:18:11 PST
That would be nice =).
Comment 18 Max Kanat-Alexander 2009-02-05 10:23:10 PST
Comment on attachment 360737 [details] [diff] [review]
V2

>+      [%# This is here so automated clients can still use process_bug.cgi %]
>+      [% IF displayfields.token %]
>+      <token>[% issue_hash_token([bug.id, bug.delta_ts]) FILTER xml %]</token>
>+      [% END %]

  Nit: indentation.

  Also, I think we should only generate this IF user.id, since logged out users can't use it.
Comment 19 Frédéric Buclin 2009-02-05 10:26:10 PST
Comment on attachment 360737 [details] [diff] [review]
V2

>Index: template/en/default/bug/show.xml.tmpl

>+      [% IF displayfields.token %]
>+      <token>[% issue_hash_token([bug.id, bug.delta_ts]) FILTER xml %]</token>
>+      [% END %]

Please fix the indentation on checkin. Otherwise works great. r=LpSolit


It's fun. Now you can get just a working token with a URL of the form:

 http://.../show_bug.cgi?ctype=xml&id=132456&field=token

The bandwidth cannot me smaller than that, Mylyn guys. ;)
Comment 20 Frédéric Buclin 2009-02-05 10:30:13 PST
(In reply to comment #18)
>   Also, I think we should only generate this IF user.id, since logged out users
> can't use it.

Ah yes, please add && user.id in the IF block.
Comment 21 Max Kanat-Alexander 2009-02-05 10:38:12 PST
Comment on attachment 360737 [details] [diff] [review]
V2

However, for the benefit of installations that want to use this patch before we release the next version, please post the complete corrected patch here.
Comment 22 Greg Hendricks 2009-02-05 10:42:20 PST
Created attachment 360755 [details] [diff] [review]
Checked In Version

    Checking in show_bug.cgi;
    /cvsroot/mozilla/webtools/bugzilla/show_bug.cgi,v  <--  show_bug.cgi
    new revision: 1.53.2.2; previous revision: 1.53.2.1
    done
    Checking in importxml.pl;
    /cvsroot/mozilla/webtools/bugzilla/importxml.pl,v  <--  importxml.pl
    new revision: 1.82.2.4; previous revision: 1.82.2.3
    done
    Checking in template/en/default/bug/show.xml.tmpl;
    /cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/show.xml.tmpl,v  <--  show.xml.tmpl

TIP:
    Checking in importxml.pl;
    /cvsroot/mozilla/webtools/bugzilla/importxml.pl,v  <--  importxml.pl
    new revision: 1.89; previous revision: 1.88
    done
    Checking in show_bug.cgi;
    /cvsroot/mozilla/webtools/bugzilla/show_bug.cgi,v  <--  show_bug.cgi
    new revision: 1.57; previous revision: 1.56
    done
    Checking in template/en/default/bug/show.xml.tmpl;
    /cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/show.xml.tmpl,v  <--  show.xml.tmpl
    new revision: 1.27; previous revision: 1.26
    done
    new revision: 1.23.2.3; previous revision: 1.23.2.2
    done
Comment 23 Robert Elves 2009-02-05 11:18:06 PST
(In reply to comment #19)
> It's fun. Now you can get just a working token with a URL of the form:
> 
>  http://.../show_bug.cgi?ctype=xml&id=132456&field=token
> 
> The bandwidth cannot me smaller than that, Mylyn guys. ;)

Excellent!  Thanks to everyone for their efforts here.

Note You need to log in before you can comment on or make changes to this bug.