'Add me to CC list' feature of bug creation screen gives token error

RESOLVED FIXED in Bugzilla 5.0

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: gerv, Assigned: dkl)

Tracking

({regression})

Bugzilla 5.0
regression
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 2 obsolete attachments)

Steps to reproduce:

* Visit https://landfill.bugzilla.org/bugzilla-tip/enter_bug.cgi?product=FoodReplicator
* Enter "Foo bar baz bug"
* Click "Add me to CC list" on any of the potential duplicates which come up

Expected: added to CC list of bug

Actual: "You submitted changes to process_bug.cgi with an invalid token, which may indicate that someone tried to abuse you, for instance by making you click on a URL which redirected you here without your consent.

Are you sure you want to commit these changes?"

This problem is also visible on BMO.

Gerv

Comment 1

6 years ago
I can only reproduce in trunk. 4.4.1 and older are all fine.
Keywords: regression
Target Milestone: --- → Bugzilla 5.0
Version: unspecified → 4.5

Comment 2

6 years ago
I found the problem. Bug.possible_updates no longer returns update_token, which explains the error message (missing token).
Assignee: create-and-change → webservice
Component: Creating/Changing Bugs → WebService

Comment 3

6 years ago
This is a regression due to bug 569177. update_token has been moved out of _bug_to_hash(), which was called by possible_duplicates(), and moved into get(), which makes it unaccessible to possible_duplicates(). dkl: why this move?
Assignee: webservice → dkl
Status: NEW → ASSIGNED
Depends on: 569177
(In reply to Frédéric Buclin from comment #3)
> dkl: why this move?

as the token is different each time, you can't use it when calculating the etag of a bug.
possible_duplicates() needs to inject update tokens into the response.
(Assignee)

Comment 5

6 years ago
Working on this now.

dkl
(Assignee)

Comment 6

6 years ago
Posted patch 927497_1.patch (obsolete) — Splinter Review
Attachment #818689 - Flags: review?(glob)

Comment 7

6 years ago
Comment on attachment 818689 [details] [diff] [review]
927497_1.patch

>=== modified file 'js/bug.js'

>-                include_fields : [ "id", "summary", "status", "resolution",
>-                                   "update_token" ]
>+                include_fields : [ "id", "summary", "status",
>+                                   "resolution", "last_change_time" ]

Are you saying that in order to get a token, you have to pass last_change_time instead of update_token? This is not documented.



>=== modified file 'process_bug.cgi'

>-my $delta_ts = $cgi->param('delta_ts') || '';
>+my $delta_ts = $cgi->param('delta_ts') || $first_bug->delta_ts;
> 
> if ($delta_ts) {

Now that $delta_ts is always set, this makes the |if ($delta_ts)| check useless. We will always enter this block.
(Assignee)

Comment 8

6 years ago
Comment on attachment 818689 [details] [diff] [review]
927497_1.patch

Going to revise the way I am doing this as discussed with LpSolit on IRC. We should not require for clients to include 'last_change_time' and not 'update_token' for the token to work properly. I will change it to automatically include last_change_time if the user wants update_token and then remove before returning.

Also need to figure out how to do include delta_ts as recent changes in process_bug.cgi have also broken the "Add to cc list" function as we now required delta_ts to be passed as a cgi param. Which previously the URL generated by possible duplicates did not include it.

dkl
Attachment #818689 - Flags: review?(glob)
(Assignee)

Comment 9

6 years ago
Posted patch 927497_2.patch (obsolete) — Splinter Review
Attachment #818689 - Attachment is obsolete: true
Attachment #819225 - Flags: review?(LpSolit)
Comment on attachment 819225 [details] [diff] [review]
927497_2.patch

>=== modified file 'js/bug.js'

>                 include_fields : [ "id", "summary", "status", "resolution",
>+                                   "last_change_time", "update_token" ]

In comment 8, you said we didn't want the caller to be forced to pass "last_change_time", but it's still listed in include_fields... Is that intentional?
(Assignee)

Comment 11

6 years ago
(In reply to Frédéric Buclin from comment #10)
> In comment 8, you said we didn't want the caller to be forced to pass
> "last_change_time", but it's still listed in include_fields... Is that
> intentional?

It is. Until we update process_bug.cgi to no longer need cgi->param('delta_ts') to validate tokens
then I also need to pass in delta_ts as part of the URL when adding a user to the cc list. So 
I need to request that last_change_time be include in the fields returned. 

dkl
(Assignee)

Updated

6 years ago
Depends on: 930013
(Assignee)

Comment 12

6 years ago
When the change in 930013 lands it will no longer be necessary for the possible duplicates code to pass the delta_ts value when adding to the cc list. So I have simplified the patch for this bug that requires the patch in 930013 to work.

dkl
Attachment #819225 - Attachment is obsolete: true
Attachment #819225 - Flags: review?(LpSolit)
Attachment #821290 - Flags: review?(LpSolit)
Comment on attachment 821290 [details] [diff] [review]
927497_3.patch

r=LpSolit
Attachment #821290 - Flags: review?(LpSolit) → review+

Updated

6 years ago
Flags: approval?
Flags: approval? → approval+
(Assignee)

Comment 14

6 years ago
Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/trunk
modified Bugzilla/WebService/Bug.pm
Committed revision 8793.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.