Community
Participate
Working Groups
Created attachment 124496 [details] Failed Update Build ID: M20080911-1700 Steps To Reproduce: Bugzilla just checked in a fix to prevent CSRF on bug update. Since Mylyn submits directly to process_bug.cgi updates to bugs from within mylyn now fail with the following message: Suspicious Action It looks like you didn't come from the right page. One reason could be that you entered the URL in the address bar of your web browser directly, which should be safe. Another reason could be that you clicked on a URL which redirected you here without your consent. This affects Bugzilla 3.2 and the development branches. More information: See https://bugzilla.mozilla.org/show_bug.cgi?id=26257
Thanks for reporting this. Frank, I've created a test repository: http://mylyn.eclipse.org/bugs321 Looks like a special token is now passed within the html to verify if the edit is allowed. I've commented on the bugzilla bug asking for their recommendation no how rich clients to Bugzilla should go about obtaining this token. It looks like the token may be valid for three days so worse case scenario we detect the failure and retrieve a new token from an html parse. Lets wait and hear what Max recommends.
Filed bug to get token included in bug xml: https://bugzilla.mozilla.org/show_bug.cgi?id=476678
Reed has a patch in progress for this as per comment #154 on https://bugzilla.mozilla.org/show_bug.cgi?id=26257
I've patched our 321 repo to 322 and applied token xml patch. Before I dive into implementation, *Committers, could you please review the proposed solution*: * Update sax xml parser to capture token ** update token on task object immediately (so if user submits open task it has a token) a la: // Security token TaskAttribute attrSecurityToken = taskData.getRoot().getMappedAttribute(BugzillaAttribute.TOKEN.getKey()); if (attrSecurityToken != null && !attrSecurityToken.getValue().equals("")) { //$NON-NLS-1$ task.setAttribute(BugzillaAttribute.TOKEN.getKey(), attrSecurityToken.getValue()); } * Upon submiting updated task ** In doSubmit() of BugzillaTaskEditor, migrate the fresh token from the task to the outgoing taskData (note this doesn't help those using headless) Within BugzillaClient upon posting task data... ** scrapeToken=false , scrapeGroupSecurity=false ** IF no token exists scrapeToken=true ** IF group security required scrapeGroupSecurity=true; scrapeToken=true; (might as well get the latest token to ensure successful submit) * In case of submit error due to stale token, rescrape for new token and re-submit (one time only else propagate error to user)
Looks good. It sounds like the submit will slow down, but only in 3.2.2, and hopefully people will update to 3.2.3.
(In reply to comment #4) > I've patched our 321 repo to 322 and applied token xml patch. > > Before I dive into implementation, *Committers, could you please review the > proposed solution*: > > > * Update sax xml parser to capture token > ** update token on task object immediately (so if user submits open task it has > a token) a la: > > // Security token > TaskAttribute attrSecurityToken = > taskData.getRoot().getMappedAttribute(BugzillaAttribute.TOKEN.getKey()); > if (attrSecurityToken != null && !attrSecurityToken.getValue().equals("")) { > //$NON-NLS-1$ > task.setAttribute(BugzillaAttribute.TOKEN.getKey(), > attrSecurityToken.getValue()); > } > > * Upon submiting updated task > ** In doSubmit() of BugzillaTaskEditor, migrate the fresh token from the task > to the outgoing taskData (note this doesn't help those using headless) > > Within BugzillaClient upon posting task data... > ** scrapeToken=false , scrapeGroupSecurity=false > ** IF no token exists scrapeToken=true > ** IF group security required scrapeGroupSecurity=true; scrapeToken=true; > (might as well get the latest token to ensure successful submit) > > * In case of submit error due to stale token, rescrape for new token and > re-submit (one time only else propagate error to user) > Looks good to me. But one thing I want to know. Why do we need the change in doSubmit() of BugzillaTaskEditor. sax xml parser create the token during getTaskData() and the BugzillaClient set this token in getPairsForExisting()
> But one thing I want to know. Why do we need the change in doSubmit() of > BugzillaTaskEditor. > sax xml parser create the token during getTaskData() and the BugzillaClient set > this token in getPairsForExisting() My understanding is that we always have to sent the latest valid token. In the case of opening a task editor we trigger a background synchronization, but if the task has not changed the retrieved task data is discarded and with it the token. That is why the token needs to be preserved on the task in AbstractRepositoryConnetor.hasTaskChanges(). doSubmit() is overridden to always use the latest known token. Rob, please make sure to document that. > * Upon submiting updated task > ** In doSubmit() of BugzillaTaskEditor, migrate the fresh token from the task to > the outgoing taskData (note this doesn't help those using headless) Headless clients will have the token on retrieved task data or will have to get the token anyways, e.g. for creating new tasks. I don't think anything changes here. The tasks framework tries to be smart about avoiding disk access and UI updates etc. that is why we discard task data if it has not changed. > Within BugzillaClient upon posting task data... > ** scrapeToken=false , scrapeGroupSecurity=false > ** IF no token exists scrapeToken=true > ** IF group security required scrapeGroupSecurity=true; scrapeToken=true; (might > as well get the latest token to ensure successful submit) > > * In case of submit error due to stale token, rescrape for new token and > re-submit (one time only else propagate error to user) Sounds good. Let's see how many cases we missed when this gets implemented :).
Created attachment 124901 [details] Token fix * move web address of patched 321 repository url to mylyn.eclipse.org/bugs322
Looking into the resubmit scenario...
(In reply to comment #2) > Filed bug to get token included in bug xml: > > https://bugzilla.mozilla.org/show_bug.cgi?id=476678 > This bug has been resolved. Please be sure to pick up the latest version of the patch as it was changed before checkin.
(In reply to comment #10) > (In reply to comment #2) > > Filed bug to get token included in bug xml: > > > > https://bugzilla.mozilla.org/show_bug.cgi?id=476678 > > > > This bug has been resolved. Please be sure to pick up the latest version of the > patch as it was changed before checkin. Will do, thanks.
Created attachment 124906 [details] updated 1
Created attachment 124907 [details] mylyn/context/zip
Update: I've discovered a problem whereby submitting a change to a task, followed by a subsequent change, results in an error from Bugzilla about the security token being invalid. I've set up an unpatched 3.2.2 at /bugs322 to test against. Will keep you posted.
Created attachment 125036 [details] Update 2 * Includes fix for double submit error. * Filed bug to fix IRepositoryListener interface granularity: bug#264017 * Patched 3.2.2 now exists at /3.2.3 (to be upgraded to the release once available)
Created attachment 125041 [details] Update 3 Version as committed. To be backported.
Created attachment 125055 [details] patch This pach is only for Bugzilla 3.2.0 where no token is used
Created attachment 125056 [details] mylyn/context/zip
Good catch Frank, please commit this patch, I'll start on the backport.
(In reply to comment #19) > Good catch Frank, please commit this patch, I'll start on the backport. Patch commited
Created attachment 125355 [details] backport patch
Patch looks good. Please avoid refactorings (e.g. renaming fields) when backporting. It is much more time consuming to review a patch when it contains mostly irrelevant changes.
(In reply to comment #22) > Patch looks good. Please avoid refactorings (e.g. renaming fields) when > backporting. It is much more time consuming to review a patch when it contains > mostly irrelevant changes. Thanks for reviewing. Yeah, should have left those out (were in original patch for head). Next time...
Created attachment 125566 [details] fix for second submit (3.0.x) stream Upon a quick second submit failure can occur against 3.2.2 patched. This is due to the background query synch erroneously synchronizing that same task (a fix is in Bugzilla 3.4 for this). This failure will happen consistently when a second submit is attempted on the same task. Two potential work arounds: A) Always get a new token from the html (expensive) B) Implement error handling so that if the submit fails we only then get a new token Attached is a proposed fix using approach B. Steffen, Frank, if you could review that would be great. If we wan't to go with this route, I'll need one of you to update this patch with the German translated "Suspicious Action" string in bugzilla.ui's plugin.xml. I'll also need to apply a similar patch to head.
Created attachment 125568 [details] Correct patch Update
We should also include the following in the backport so that attachments will work (the confirm response message is different): enSetting.addLanguageAttribute("changes_submitted", "added to Bug");
(In reply to comment #26) > We should also include the following in the backport so that attachments will > work (the confirm response message is different): > > enSetting.addLanguageAttribute("changes_submitted", "added to Bug"); > OK but I think that we should an the new language keywords to the plugin.xml to. (In reply to comment #25) > Created an attachment (id=125568) [details] > Correct patch > > Update > This looks good to me.
Created attachment 125645 [details] minor cleanup [3.0.x] Thanks Frank. I'll drum up a patch against head as well.
Created attachment 125655 [details] for m3.0.x branch patch for 3.0.x
Created attachment 125656 [details] for Head patch for head
Patches applied to both head and 3.0.x branch
Created attachment 125963 [details] patch for reassignment on 3.2.x Patch against 3.0.x to enable reassignment on Bugzilla 3.1 and greater
Created attachment 125973 [details] fixes api breakage 3.0.x
Created attachment 125974 [details] api fix head as applied
Mylyn 3.0.5 is now available from the following update site: download.eclipse.org/tools/mylyn/update/e3.4
(In reply to comment #35) > Mylyn 3.0.5 is now available from the following update site: > > download.eclipse.org/tools/mylyn/update/e3.4 and download.eclipse.org/tools/mylyn/update/e3.3
This is now fixed and released.
Forgive me for asking a dumb question, but: if I'm running headlessly, will this fix let me run against Bugzilla 3.2.2, or will I need to wait for the corresponding fix on their end in Bugzilla 3.2.3 and later?
(In reply to comment #38) > Forgive me for asking a dumb question, but: if I'm running headlessly, will this > fix let me run against Bugzilla 3.2.2, or will I need to wait for the > corresponding fix on their end in Bugzilla 3.2.3 and later? Good question. This fix will support both Bugzilla 3.2.2 and 3.2.3 (headless or not). In the case of 3.2.2 you will experience more network traffic (due to having to go get a new token upon submit). If the performance hit is a problem and you can't wait for 3.2.3 simply apply the patch on: 476678: Rich clients unable to update bugs need security token included in bug xml https://bugzilla.mozilla.org/show_bug.cgi?id=476678
