Closed Bug 476678 Opened 15 years ago Closed 15 years ago

Rich clients unable to update bugs need security token included in bug xml

Categories

(Bugzilla :: Creating/Changing Bugs, defect)

3.2.2
defect
Not set
major

Tracking

()

RESOLVED FIXED
Bugzilla 3.2

People

(Reporter: rob.elves, Assigned: gregaryh)

References

Details

Attachments

(1 file, 2 obsolete files)

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
We first have to make sure one cannot use this token to abuse the user.
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → 3.2.2
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).
Severity: normal → blocker
Summary: Add new security token to bug xml output → Rich clients unable to update bugs need security token included in bug xml
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.
Assignee: query-and-buglist → create-and-change
Severity: blocker → major
Status: UNCONFIRMED → NEW
Component: Query/Bug List → Creating/Changing Bugs
Ever confirmed: true
Attached patch v1 (obsolete) — Splinter Review
Here, this is no more dangerous than having the token in the HTML on show_bug.cgi, where it could be parsed out anyway.
Assignee: create-and-change → mkanat
Status: NEW → ASSIGNED
Attachment #360348 - Flags: review?(LpSolit)
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.
Target Milestone: --- → Bugzilla 3.2
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.
(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.
Attachment #360348 - Flags: review+
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
(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!  :)
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?
(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).
Attachment #360348 - Flags: review?(LpSolit) → review-
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.
I suppose everybody wants this bug to be fixed in 3.2.3.
Flags: blocking3.2.3+
Note that 2.22.x and 3.0.x are not affected, as bug 26257 only landed on the 3.2 branch.
Depends on: 26257
(In reply to comment #13)
> I suppose everybody wants this bug to be fixed in 3.2.3.

+1 Thanks! :)
Attached patch V2 (obsolete) — Splinter Review
importxml can safely ignore the token.
Assignee: mkanat → ghendricks
Attachment #360348 - Attachment is obsolete: true
Attachment #360737 - Flags: review?(LpSolit)
That would be nice =).
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 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. ;)
Attachment #360737 - Flags: review?(LpSolit) → review+
Flags: approval3.2+
Flags: approval+
(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 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.
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
Attachment #360737 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
(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.
You need to log in before you can comment on or make changes to this bug.