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

Status: RESOLVED FIXED

(Bugzilla :: WebService, defect)

Description

[Gervase Markham [:gerv]]

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

[Frédéric Buclin]

I can only reproduce in trunk. 4.4.1 and older are all fine.

Comment 2

[Frédéric Buclin]

I found the problem. Bug.possible_updates no longer returns update_token, which explains the error message (missing token).

Comment 3

[Frédéric Buclin]

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?

Comment 4

[:glob ✱]

(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.

Comment 5

[David Lawrence [:dkl]]

Working on this now.

dkl

Comment 6

[David Lawrence [:dkl]]

Attachment: 927497_1.patch

Comment 7

[Frédéric Buclin]

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.

Comment 8

[David Lawrence [:dkl]]

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

Comment 9

[David Lawrence [:dkl]]

Attachment: 927497_2.patch

Comment 10

[Frédéric Buclin]

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?

Comment 11

[David Lawrence [:dkl]]

(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

Comment 12

[David Lawrence [:dkl]]

Attachment: 927497_3.patch

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

Comment 13

[Frédéric Buclin]

Comment on attachment 821290 [details] [diff] [review]
927497_3.patch

r=LpSolit

Comment 14

[David Lawrence [:dkl]]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk
modified Bugzilla/WebService/Bug.pm
Committed revision 8793.