Closed Bug 1355490 Opened 7 years ago Closed 7 years ago

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

Categories

(bugzilla.mozilla.org :: Extensions, defect, P1)

Product:
bugzilla.mozilla.org
Component:
Extensions
Version:
Production
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: MattN, Assigned: dylan)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

1355490_4.patch
3.47 KB, patch
glob
: review+
Details | Diff | Splinter Review 
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: nobody → dylan
Priority: -- → P1
Depends on: 1350909
Attached patch 1355490_1.patch (obsolete) — — Splinter Review 
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 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>?
(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.
weird brain-o, "generate_api_token login" => "generate_api_token variable"
Attached patch 1355490_2.patch (obsolete) — — Splinter Review 
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 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+
(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.
Attached patch 1355490_4.patch — — Splinter Review 

        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+

    
    

    
  
To git@github.com:mozilla-bteam/bmo.git
   8be2b9a..318651c  master -> master
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Component: Extensions: Other → Extensions

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: