Closed Bug 1355490 Opened 8 years ago Closed 8 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: 8 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: