Short URL link give "The token is not valid" error

RESOLVED FIXED

Status

()

bugzilla.mozilla.org
Extensions: Other
P1
normal
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: MattN, Assigned: dylan)

Tracking

({regression})

Production
regression

Details

Attachments

(1 attachment, 2 obsolete attachments)

STR: 
1) Load https://mzl.la/2p3j0zu (which I shortened from bit.ly directly)
2) Click the "Short URL" link in the footer

Expected result:
A short URL appears in the text box

Actual result:
"The token is not valid. It could be because you loaded this page more than 3 days ago." appears in the search even though I just loaded the page. Doing a refresh doesn't help either.

This worked 2 weeks ago (on a similar search)
(Assignee)

Updated

11 months ago
Assignee: nobody → dylan
Priority: -- → P1
(Assignee)

Updated

11 months ago
Depends on: 1350909
(Assignee)

Comment 1

11 months ago
Created attachment 8857195 [details] [diff] [review]
1355490_1.patch

In bug 1350909 we moved around the BUGZILLA variable to be stored as json in the <head> element, which seemed to work except for the Bitly extension, which wants to be able to change generate_api_key. 

This patch moves that code around to a <meta> element, after the 'header-start' hook is called. Bitly works with this change.
Attachment #8857195 - Flags: review?(glob)

Comment 2

11 months ago
Comment on attachment 8857195 [details] [diff] [review]
1355490_1.patch

Review of attachment 8857195 [details] [diff] [review]:
-----------------------------------------------------------------

::: template/en/default/global/header.html.tmpl
@@ +148,5 @@
> +        IF generate_api_token;
> +            js_BUGZILLA.api_token = get_api_token();
> +        END; 
> +    %]
> +    <meta id='bugzilla-global' name='bugzilla-global' data-bugzilla='[% json_encode(js_BUGZILLA) FILTER html %]'>

<meta> has a special meaning in html (it sets http headers); replace this with <div>
Attachment #8857195 - Flags: review?(glob) → review-
(In reply to Byron Jones ‹:glob› from comment #2)
> Comment on attachment 8857195 [details] [diff] [review]
> 1355490_1.patch
> 
> Review of attachment 8857195 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: template/en/default/global/header.html.tmpl
> @@ +148,5 @@
> > +        IF generate_api_token;
> > +            js_BUGZILLA.api_token = get_api_token();
> > +        END; 
> > +    %]
> > +    <meta id='bugzilla-global' name='bugzilla-global' data-bugzilla='[% json_encode(js_BUGZILLA) FILTER html %]'>
> 
> <meta> has a special meaning in html (it sets http headers);

You're thinking of the http-equiv attribute on <meta>

> replace this with <div>

Why not just an attribute on <html> or <body>?
(Assignee)

Comment 4

11 months ago
(In reply to Matthew N. [:MattN] (behind on bugmail; PM if requests are blocking you) from comment #3)
> (In reply to Byron Jones ‹:glob› from comment #2)
> > Comment on attachment 8857195 [details] [diff] [review]
> > 1355490_1.patch
> > 
> > Review of attachment 8857195 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: template/en/default/global/header.html.tmpl
> > @@ +148,5 @@
> > > +        IF generate_api_token;
> > > +            js_BUGZILLA.api_token = get_api_token();
> > > +        END; 
> > > +    %]
> > > +    <meta id='bugzilla-global' name='bugzilla-global' data-bugzilla='[% json_encode(js_BUGZILLA) FILTER html %]'>
> > 
> > <meta> has a special meaning in html (it sets http headers);
> 
> You're thinking of the http-equiv attribute on <meta>
> 
> > replace this with <div>
> 
> Why not just an attribute on <html> or <body>?
previously it was on <head> but that is too early for other parts of the template code to change the generate_api_token login,
and as part of <body> it is too late for the scripts in <head> to see it -- at least when I experimented with it in firefox.
(Assignee)

Comment 5

11 months ago
weird brain-o, "generate_api_token login" => "generate_api_token variable"
(Assignee)

Comment 6

11 months ago
Created attachment 8857650 [details] [diff] [review]
1355490_2.patch

Using <template> is a bit nicer.

<template> is allowed in <head>.

Tests in Edge and IE as well, IE required a css hack to hide <template> but otherwise it works.
Attachment #8857195 - Attachment is obsolete: true
Attachment #8857650 - Flags: review?(glob)

Comment 7

10 months ago
Comment on attachment 8857650 [details] [diff] [review]
1355490_2.patch

Review of attachment 8857650 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob, with fix on commit.

::: js/global.js
@@ +16,4 @@
>  *                 
>  */
>  
> +var BUGZILLA = JSON.parse($('#bugzilla-global').html());

using .html() means you'll get escaped html which may cause problems in future.

use .content on browsers that support it, fall back on unescaping html on IE/older browsers:

var global = document.getElementById('bugzilla-global');
var json = 'content' in global
    ? global.content.textContent
    : $('<div/>').html(global.innerHTML).text();

::: template/en/default/global/header.html.tmpl
@@ +146,5 @@
> +            }
> +        };
> +        IF generate_api_token;
> +            js_BUGZILLA.api_token = get_api_token();
> +        END; 

may as well remove those trailing spaces while touching these lines
Attachment #8857650 - Flags: review?(glob) → review+
(Assignee)

Comment 8

10 months ago
(In reply to Byron Jones ‹:glob› from comment #7)
> var global = document.getElementById('bugzilla-global');
> var json = 'content' in global
>     ? global.content.textContent
>     : $('<div/>').html(global.innerHTML).text();

This is vulnerable to XSS. I realize in context it would only be executed in browsers that don't have <template> but I'd rather never use it, and discourage anyone from ever writing it.

I'm going to re-visit the <meta> approach, which works well everywhere and requires less code, and :ato said was fine.
(Assignee)

Comment 9

10 months ago
Created attachment 8861621 [details] [diff] [review]
1355490_4.patch
Attachment #8857650 - Attachment is obsolete: true
Attachment #8861621 - Flags: review?(glob)
Comment on attachment 8861621 [details] [diff] [review]
1355490_4.patch

Review of attachment 8861621 [details] [diff] [review]:
-----------------------------------------------------------------

my win7 vm is playing up, so i've been unable to test if it works in IE.
r=glob assuming that you've tested in IE and edge.

::: js/global.js
@@ +16,4 @@
>  *                 
>  */
>  
> +var BUGZILLA = $('#bugzilla-global').data("bugzilla");

s/"/'/g for consistency.

::: template/en/default/global/header.html.tmpl
@@ +147,2 @@
>      [% Hook.process("start") %]
> +    [% IF generate_api_token; js_BUGZILLA.api_token = get_api_token(); END %]

don't concatenate multiple lines of code into a single line.

either:

[%
    IF generate_api_token;
        js_BUGZILLA.api_token = get_api_token();
    END;
%]

or:

[% js_BUGZILLA.api_token = get_api_token() IF generate_api_token %]

@@ +147,5 @@
>      [% Hook.process("start") %]
> +    [% IF generate_api_token; js_BUGZILLA.api_token = get_api_token(); END %]
> +
> +    <meta name="bugzilla-global" content="dummy"
> +        id='bugzilla-global' data-bugzilla="[% json_encode(js_BUGZILLA) FILTER html %]">

s/'/"/g for consistency.
Attachment #8861621 - Flags: review?(glob) → review+
(Assignee)

Comment 11

10 months ago
To git@github.com:mozilla-bteam/bmo.git
   8be2b9a..318651c  master -> master
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.