Closed Bug 570786 Opened 14 years ago Closed 13 years ago

add splinter extension to bugzilla

Categories

(bugzilla.mozilla.org :: Splinter, defect)

Product:
bugzilla.mozilla.org
Component:
Splinter
Version:
Production
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: jmaher, Assigned: dkl)

References

Details

(Whiteboard: [bmo4.0-resolved])

Attachments

(1 file, 5 obsolete files)

Patch to make Splinter work with 4.0 bmo code (v1)
47.96 KB, patch
reed
: review-
Details | Diff | Splinter Review 
Here are some steps to get splinter installed.  It is pretty painless and not very intrusive to a bugzilla installation.

1) read about splinter here: http://blog.fishsoup.net/2009/09/23/splinter-patch-review/
2) pull code from git://git.fishsoup.net/splinter ('git clone git://git.fishsoup.net/splinter')
3) in the splinter directory, read the README (http://git.fishsoup.net/cgit/splinter/tree/README)
4) cd splinter;make install BUGZILLA_ROOT=/path/to/bugzilla/installation
 * this will install to bugzillaroot/extensions/splinter/*, pretty self contained
5) in the bugzilla admin interface, edit the parameters for "splinter patch review", and change splinter base to '/review'
6) we just need to rewrite the rules in the webserver.  In my local configs, I am using apache, and added to my bugzilla VirtualHost rules:
RewriteEngine On
RewriteRule ^review(\?(.*))? page.cgi?id=splinter.html&$2 [L]
* NOTE I changed the regex from the parameters page: '^review(?(.*))?' -> '^review(\?(.*))?'
7) visit a bug that has a patch and there is a 'Review' link that works end to end for doing a review
(In reply to comment #0)
> 5) in the bugzilla admin interface, edit the parameters for "splinter patch
> review", and change splinter base to '/review'
> 6) we just need to rewrite the rules in the webserver.  In my local configs, I
> am using apache, and added to my bugzilla VirtualHost rules:
> RewriteEngine On
> RewriteRule ^review(\?(.*))? page.cgi?id=splinter.html&$2 [L]
> * NOTE I changed the regex from the parameters page: '^review(?(.*))?' ->
> '^review(\?(.*))?'

These aren't necessarily required, no? Can't you just work fine with page.cgi?id=splinter.html?
correct.  steps 5-7 are optional.  I guess if we find this tool to be widely used and there are complaints about the url format we can look into doing a rewrite rule.

Thanks for the clarification question.
When is the next bugzilla down time that y'all can add this extension into bugzilla? Will it go on a staging server first, or will it go directly live?  I'm not entirely sure what the process for changes like this is.

Thanks!
(In reply to comment #3)
> When is the next bugzilla down time that y'all can add this extension into
> bugzilla? Will it go on a staging server first, or will it go directly live? 
> I'm not entirely sure what the process for changes like this is.

Doubtful that we'll take it before we move to Bugzilla 3.6 at the minimum. Also, it'll need to staged and tested, and I want it put through a security review.
> (In reply to comment #3)
> Doubtful that we'll take it before we move to Bugzilla 3.6 at the minimum.
> Also, it'll need to staged and tested, and I want it put through a security
> review.

When do you estimate we'd take 3.6, and get the other things done?
(In reply to comment #5)
> When do you estimate we'd take 3.6, and get the other things done?

Not sure. Gerv has done an initial porting of our custom code to Bugzilla 3.6, and it's available for testing at https://bugzilla-stage-tip.mozilla.org/, but nothing much has happened since that. Really, it depends on when Dave will have time to help test stuff and see when an upgrade could be scheduled.

Once bmo is on 3.6, then the other stuff can begin.
Depends on: bmo-upgrade-3.6
Why can't we deploy this on top of 3.4?  I can't tell from this bug when 3.6 will be deployed, which makes me believe that it's not Soon.
Bugzilla 3.6 is getting deployed as soon as I have time to test it, which isn't going to happen before the end of Q2.  (but that's not that far off).  I'm hoping to spend some time working on it this next week.

There's no point spending the effort needed to deploy this on 3.4 when it's getting upgraded to 3.6 as soon as I have that much time to spend on it anyway.
I guess the question would be - shaver, if we can give you a hard target of 3.6 in 2 weeks, is it OK if we wait and focus on 3.6?  Or is it critical enough to spend the time on getting this in 3.4?
Just to provide a more complete picture, Bugzilla 3.6 is staged already, but we had a "time spent" issue with the upgrade (it took over 6 hours to run the database upgrade, which we don't like).  We have some tweaks to the mysql configuration to try to make the upgrade go faster, but I haven't had time to try again yet.  As soon as that revised test run completes, we'll schedule a downtime for it (and if it winds up being an 8 hour downtime at that point, so be it, but I'd like to avoid being down that long if we can).
So, this morning, I noticed we have upgraded to bugzilla 3.6.1, very awesome.  I also noticed that the splinter diff is evidently not working.  Were issues encountered or has the extension not been added, or what?

If there's anything Joel or I could help you with, please let us know.
It hasn't been added yet.  I was suggested to me that it should have a security audit before it goes in production.  I was intending to get it onto the staging server in the next day or so and then let the security guys play with it, however, I don't seem to be able to get it to work.  Does it work with Bugzilla 3.6?  After installing it, making sure there was no "disabled" file within its directory, running checksetup.pl, and restarting apache, I still have no "Splinter Patch Review" section in the params, and there's no "Review" link on the patches.
(In reply to comment #12)
> It hasn't been added yet.  I was suggested to me that it should have a security
> audit before it goes in production.  I was intending to get it onto the staging
> server in the next day or so and then let the security guys play with it,
> however, I don't seem to be able to get it to work.  Does it work with Bugzilla
> 3.6?  After installing it, making sure there was no "disabled" file within its
> directory, running checksetup.pl, and restarting apache, I still have no
> "Splinter Patch Review" section in the params, and there's no "Review" link on
> the patches.

I have not tested with 3.6. Existing deployments of Splinter are with 3.4. If it's not working at all and not showing up in params, then presumably something has changed with how Bugzilla extensions work and Splinter will have to be adapted accordingly. Doesn't sound hard for someone experienced with Bugzilla, but I'm certainly not going to have time to look at it any time in the forseeable future.
These would have been pretty great questions to ask before we spent weeks waiting for it to be deployed, including the 3.6 upgrade.

When there are requirements added, like security reviews, I think it's pretty reasonable for them to be EXPLICITLY listed in the bug when they're "suggested to you", rather than waiting for people to get impatient (again) and inquire after the status of their requests.  Why wasn't a security review requested when this bug was first filed, more than a month ago?  It's been available in a test setup for many weeks, during which time people could have been reviewing it -- if review requires it to be running, and can't be started by inspecting the source linked from the first comment.

Can you please outline, exactly, what steps are required before this improvement is deployed?  Are you going to update it to 3.6 compatibility, as the second para of comment 8 indicates, or do you need someone else to do that? (Let me restate how unhappy I am that the 3.6 requirement wasn't stated when the bug was first posted, such that Clint or Joel could have taken care of it ages ago.)
The extension package format changed in Bugzilla 3.6.  Conveniently enough, there's a conversion script in contrib that'll convert from the old format to the new format.

I ran that on it, and now it shows up. :)  However, it still doesn't work.  Clicking the "Review" link gives me an indefinite "Loading...", and I get a js console error:

$ is not defined
https://bugzilla-stage-tip.mozilla.org/page.cgi?id=splinter.html&bug=577081&attachment=456154
Line 316
Dave: please get on the phone with Joel or Clint and resolve this stuff in real-time.
Well, the security review wasn't going to take very long in theory.  It's a very small extension, it's not like it'll take weeks to do.  I didn't think of that being a blocker to getting this deployed (unless they ended up finding something, but that's probably easy to fix, too, given the size of it).

I kinda figured that compatibility with Bugzilla 3.6 was a given requirement when it was stated we were waiting for the 3.6 upgrade to deploy it.
(In reply to comment #17)
> I kinda figured that compatibility with Bugzilla 3.6 was a given requirement
> when it was stated we were waiting for the 3.6 upgrade to deploy it.

Not to mention we were planning to upgrade to 3.6 anyway, so even if we had deployed it to 3.4 before upgrading, the upgrade would have broken it.
(In reply to comment #14)
> These would have been pretty great questions to ask before we spent weeks
> waiting for it to be deployed, including the 3.6 upgrade.
...
> (Let me restate how unhappy I am that the 3.6 requirement wasn't stated when
> the bug was first posted, such that Clint or Joel could have taken care of it
> ages ago.)

I outlined in comment #4 the steps needed before this could go live. They haven't changed.

* Finish upgrade to Bugzilla 3.6 (way more important than this one extension)
* Stage and test splinter (on 3.6)
* Security review

There are basically three people who do bmo code work (justdave, gerv, and me), and our time was much better spent doing the Bugzilla 3.6 upgrade during the lull of the summit where it wouldn't cause as much of a disruption due to the extended downtime.

We'll get this extension on bmo and working soon, but we ask that you are patient, as we've been dealing with the usual fallout from a bmo upgrade, so our time has been dedicated to other things.
I don't understand: if the security review isn't a blocker, then what does "before it goes in production" mean?  This whole bug is about getting it into production.

Apropos "kinda figured", you only stated that it had to wait for 3.6 on June 30 (prior it was just "doubtful", and not from the person who was deciding when it would happen).  That's 3 weeks that could have been used to get it working with 3.6 already, and in comment 10 you said "it's getting upgraded to 3.6 as soon as I have that much time to spend on it anyway", which sounds like *you're* doing the upgrade.

"There's no point spending the effort needed to deploy this on 3.4" would be true if we it didn't mean that we could have had better code review tools a month ago, and counting.
ok, I've found the issue, it's pathnames to the js files from the templates, since the directory structure of the interior of the extension package changed, those need to be changed to match.  I should have this working shortly.
(In reply to comment #20)
> I don't understand: if the security review isn't a blocker, then what does
> "before it goes in production" mean?  This whole bug is about getting it into
> production.

I personally consider the security review a blocker. I'd rather not have random extensions running on our production bugzilla without some type of audit.
(In reply to comment #20)
> That's 3 weeks that could have been used to get it working with 3.6 already

Which only makes sense if we had b.m.o itself working as 3.6 already.  You have to have a Bugzilla 3.6 on the staging server to test on before you can test it.  Getting 3.6 completely working on the staging server is what we were spending our time on.
Adding in security now.  Long bug but would like some security eyes on this.
OK, the failure to load jquery was resolved (it was just a path change), however, I hit another roadblock.  We have attachments hosted on a separate domain name in order to prevent cookie-stealing by code in attachments.  XmlHttpRequest won't cross domains.  This means you can't load the attachment via ajax without special headers on the target server (which we can do easily) and code within the extension to specifically handle the redirect (because XmlHttpRequest doesn't follow them), which I'll probably need a little help with.  jmaher's already working on this, so we'll hopefully have it going really soon.
(In reply to comment #25)
> OK, the failure to load jquery was resolved (it was just a path change),
> however, I hit another roadblock.  We have attachments hosted on a separate
> domain name in order to prevent cookie-stealing by code in attachments. 
> XmlHttpRequest won't cross domains.  This means you can't load the attachment
> via ajax without special headers on the target server (which we can do easily)
> and code within the extension to specifically handle the redirect (because
> XmlHttpRequest doesn't follow them), which I'll probably need a little help
> with.  jmaher's already working on this, so we'll hopefully have it going
> really soon.

Oh, hrrm, we have a local hack on bugzilla.gnome.org, that I convinced myself is safe, but is highly ugly. It's also not entirely safe in the future if Splinter was ever modified to linkify random URLs in the text of the patch. Plus it's a splinter-specific hack to the core sources of Bugzilla, which isn't good.

=== modified file 'attachment.cgi'
--- attachment.cgi	2009-11-12 08:16:33 +0000
+++ attachment.cgi	2009-11-12 08:18:28 +0000
@@ -261,7 +261,14 @@
 sub view {
     my $attachment;
 
-    if (use_attachbase()) {
+    # Temporary hack for splinter - we can't redirect to attachbase because that
+    # will trigger cross-site-scripting protections, so we look at the referrer
+    # to exclude splinter-generated requests from the redirection.
+    my $urlbase = Bugzilla->params->{'urlbase'};
+    my $sslbase = Bugzilla->params->{'sslbase'};
+    my $splinter_regexp = $sslbase ? qr/^(\Q$urlbase\E|\Q$sslbase\E)review/ : qr/^\Q$urlbase\Ereview/;
+
+    if (use_attachbase() && $cgi->referer !~ /$splinter_regexp/) {
         $attachment = validateID(undef, 1);
         # Replace %bugid% by the ID of the bug the attachment belongs to, if present.
         my $attachbase = Bugzilla->params->{'attachment_base'};

It's been a while, but I think what I considered the correct fix was to make Bugzilla recognize that the request for the attachment was a request for the attachment *with* the text/plain content type (by URL parameter or header), enforce that the response actually had that content type, and exclude such requests from the redirect. 

Sounds like you have a some direction of your own already though.
  It is not safe to use Referrer headers or Content-Type for security. (The first because they are not reliable and the second because some browsers ignore the content-type [like IE6] if the data "looks" like some other form of data.)

  The actual proper solution would be to provide splinter the attachment's data on the server side, instead of getting it via Ajax.
  For those interested in the original entire discussion around the security aspects of putting attachments in a different domain (and why other approaches are not secure), see bug 38862 and bug 472206.
(In reply to comment #27)
>   It is not safe to use Referrer headers or Content-Type for security. (The
> first because they are not reliable and the second because some browsers ignore
> the content-type [like IE6] if the data "looks" like some other form of data.)

First is a bit vague :-). Referrer is obviously not *authentication*. The second, well, yes, if you care about IE6 and it does that, then that would be an issue with delivering any attachment directly from the main URL.

>   The actual proper solution would be to provide splinter the attachment's data
> on the server side, instead of getting it via Ajax.

Not sure exactly what you mean by that. It's "AJAX" any way you cut it. One thing you could do is modify splinter to remove:

 excludefield: 'attachmentdata'

From its request to show_bug.cgi?ctype=xml and then decode the attachments from that. But if the bug has multiple large non-patch attachments, or lots of obsolete patches that could slow things a lot. Maybe better, is adding some way either in the Bugzilla core or in the splinter extension to get the contents of a single attachment in an xml or xmlprpc response. You'd need to have JS base64-decoding code in splinter, but that shouldn't be hard to write or find.
(In reply to comment #29)
> >   The actual proper solution would be to provide splinter the attachment's data
> > on the server side, instead of getting it via Ajax.
> 
> Not sure exactly what you mean by that. It's "AJAX" any way you cut it.

  I mean redesign splinter so that it gets its data as part of a template on the server side (like the current PatchReader does) instead of getting it via JS.
(In reply to comment #30)
> (In reply to comment #29)
> > >   The actual proper solution would be to provide splinter the attachment's data
> > > on the server side, instead of getting it via Ajax.
> > 
> > Not sure exactly what you mean by that. It's "AJAX" any way you cut it.
> 
>   I mean redesign splinter so that it gets its data as part of a template on
> the server side (like the current PatchReader does) instead of getting it via
> JS.

You could obviously write a patch review extension for Bugzilla that was written more along the lines of the Bugzilla core with extensive server side templating and Perl code. That would not be Splinter.

You could also, I suppose, put everything that Splinter currently gets via AJAX into a couple of enormous JS variables inside the <script/> tag in 
splinter.html.tmpl.in.

The first sounds like waste of resources to me if people are reasonably happy with the current Splinter extension. Not my resources though, so, <shrug>.

The second, well, it would work. Would make things look a little worse on page load, likely, but not enormously. Doesn't actually seem easier or better to me than adding get_attachment_data() (or whatever the appropriate name is, haven't thought it through) to WSSplinter.pm.
Security group here. Below is the link to file a security review request. Please provide all the necessary information as soon as possible. Even if the review doesn't end up taking much time we still need to schedule it in with many other requests and tasks.  Thanks!

https://intranet.mozilla.org/SecurityReview/ReviewRequest

I'll try and keep good tags on this comment train, but please file a bug for the sec review.
(In reply to comment #32)
> Below is the link to file a security review request.

Filed as bug 578573
I have been having a lot of trouble getting the ajax call to follow the redirect due to what appears to be cross site scripting issues (I have configured my server to add the Access-Control-Allow-Origin header.)  

I also noticed splinter runs JQuery 1.3.1 instead of 1.4.2.  Not sure if there is a specific reason for that other than what was available when it was written originally.

Would it make sense to maybe get the attachment data from bugzilla as indicated in Comment 31 ?
> You could also, I suppose, put everything that Splinter currently gets via AJAX
> into a couple of enormous JS variables inside the <script/> tag in 
> splinter.html.tmpl.in.

  That would be simpler than trying to work around the cross-site issues, I think.

> The second, well, it would work. Would make things look a little worse on page
> load, likely, but not enormously.

  Why would page load be affected? Just because we'd be retrieving the data before page load?
  Also, you could just get the attachment data and information via the WebServices, which also support JSON-RPC in 3.6:

http://www.bugzilla.org/docs/3.6/en/html/api/Bugzilla/WebService/Bug.html#attachments
  Oh wait, nevermind. You can't get attachment data via that WebServices method in 3.6. Sorry.
(In reply to comment #34)
> I have been having a lot of trouble getting the ajax call to follow the
> redirect due to what appears to be cross site scripting issues (I have
> configured my server to add the Access-Control-Allow-Origin header.)  

As far as I know, cross-sites requests are completely forbidden for XmlHttpRequest. A page cannot make a XmlHttpRequest against a different site than the one it was served from.

> I also noticed splinter runs JQuery 1.3.1 instead of 1.4.2.  Not sure if there
> is a specific reason for that other than what was available when it was written
> originally.
> 
> Would it make sense to maybe get the attachment data from bugzilla as indicated
> in Comment 31 ?

I think adding the extra method to WSSplinter.pm would be a small and reasonable way to handle this problem. You probably can just use a string return, rather than worrying about extending xmlRpc.js to handle base64. Since splinter just handles the result as a Unicode string anyways, returning the original binary data for the patch with the encoding preserved doesn't have any advantages I can see.
(In reply to comment #35)
> > You could also, I suppose, put everything that Splinter currently gets via AJAX
> > into a couple of enormous JS variables inside the <script/> tag in 
> > splinter.html.tmpl.in.
> 
>   That would be simpler than trying to work around the cross-site issues, I
> think.
> 
> > The second, well, it would work. Would make things look a little worse on page
> > load, likely, but not enormously.
> 
>   Why would page load be affected? Just because we'd be retrieving the data
> before page load?

Patches can be quite large, especially if the patch contains base-64'ed binary data for images or similar. Splinter provides feedback while loading the attachment data, which wouldn't work properly if the attachment data was all in the page anyways. (You might be able to order things in the page, but counting on when different <script> tags are parsed compared to incremental page loading and display isn't a science I'm very comfortable with.)
The security concerns have to do with sending cookies and other bits for attachments that actually are executable by the browser, e.g. text/html.  Splinter doesn't care about that, right?  It only ever wants to see the source.  So why not just add something to attachment.  Right now attachment.cgi redirects to bugxxxxx site when fetching the attachment, but it doesn't for something like https://bugzilla.mozilla.org/attachment.cgi?id=454148&action=edit.  So why not just add "action=raw", have it force a content type of text/plain, and be done with it?  That seems like a trivial thing to add -- would it not solve the problem here?
(In reply to comment #40)
> The security concerns have to do with sending cookies and other bits for
> attachments that actually are executable by the browser, e.g. text/html. 
> Splinter doesn't care about that, right?  It only ever wants to see the source.
>  So why not just add something to attachment.  Right now attachment.cgi
> redirects to bugxxxxx site when fetching the attachment, but it doesn't for
> something like
> https://bugzilla.mozilla.org/attachment.cgi?id=454148&action=edit.  So why not
> just add "action=raw", have it force a content type of text/plain, and be done
> with it?  That seems like a trivial thing to add -- would it not solve the
> problem here?

That was basically my suggestion in comment 26 - attachment.cgi already supports an URL parameter of 'content_type=<whatever>' so it seemed to me that if that is reliable, and the passed in content_type is in a whitelist, then the redirection could be skipped. Max in comment 27 didn't like that idea.
  Yeah, I just don't like the idea of allowing people to circumvent the security protection. There could be some combo attack that is possible with the circumvention that isn't possible without it.
I didn't realize that it already supported a content_type param -- what could happen if it's text/plain?  If the browser has a bug where rendering text/plain causes a cross-site scripting attack to occur, we've probably already lost :-)
(In reply to comment #43)
> If the browser has a bug where rendering text/plain
> causes a cross-site scripting attack to occur, we've probably already lost :-)

Welcome to the world of Internet Explorer.
IE doesn't honour the server's content-type.  if you send html as text/plain, it'll render it as text/html.
Not everybody who uses Bugzilla uses Firefox (sadly). Just as our webdev team has to support IE for all our (mostly Firefox-centric) websites, so do we. Firefox isn't the only user of bmo.
XmlHttpRequest *can* make a request to a different domain as long as the target domain's webserver returns a full set of Access-Control-* headers that authorize the request.  The problem here appears to be that if the result of an XmlHttpRequest is a redirect to another domain, it tosses the result and throws an error, leaving the script no way to find out what the target of the redirect is.  Which means in theory, we should be able to load the attachment *if* we knew the URL to it.  For public bugs, this is easy.  Add "bug{bugid}." to the front of the domain name.  I have this working on bugzilla-stage-tip now, by hacking the URL as such.  For bugs that are secured, we need to have a one-time authtoken passed as part of the URL, and that authtoken is retrieved by hitting the normal attachment URL on the primary domain, and following the redirect (which includes the authtoken as part of the redirected URL).

So in order to make this work and still preserve the cross-domain protection, what we really need to do is provide some way other than a redirect for an ajax call to retrieve one of these one-time tokens to use in generating the URL.
oh, it turns out you actually need all four Access-Control-* headers to make it work, passing only Access-Control-Allow-Origin doesn't work.

Here's the working apache config off bugzilla-stage-tip:

     SetEnvIf Host ^bug\d+\.bugzilla-stage-tip\.mozilla\.org$ USE_CORS
     Header set Access-Control-Allow-Origin https://bugzilla-stage-tip.mozilla.org env=USE_CORS
     Header set Access-Control-Allow-Methods GET env=USE_CORS
     Header set Access-Control-Allow-Headers x-requested-with env=USE_CORS
     Header set Access-Control-Max-Age 60

And a demo URL:

https://bugzilla-stage-tip.mozilla.org/page.cgi?id=splinter.html&bug=577081&attachment=456154
and that demo URL appears to work in Namoroka, but not in Minefield or Safari.
I don't have a problem with blocking IE6 from the advanced code review interface.  I dunno who can dig up the stats, but if IE6 users are more than 2% of the patch comment submissions, I'll give Reed $100 USD.
(In reply to comment #46)
> IE doesn't honour the server's content-type.  if you send html as text/plain,
> it'll render it as text/html.

Sure, so just prevent any IE UA's from getting any attachments via the "raw" interface.  If someone's compromised a system enough that they're able to change IE's UA entirely, then they have bigger problems...
So I think the safest alternative here is to get the attachment data availability via the webservice backported, and then have Splinter use that to retrieve the attachment.  Any volunteers to take that on?  That's probably more coding than I have time to deal with right now.
I started on that today.  I agree, it seems like the most reasonable solution.
These changes are going to determine how splinter interacts with bugzilla, correct? If that is the case, I'll hold of on the security review of the demo app until it is setup completely. I have already taken a look at the code though.
this patch is hacked to not use the attachment ID, we are waiting for a fix in bug 579514 so we can just access the attachment data from the attachment object.

But this is the code and plumbing that we will need to get an attachment data blob from bugzilla into splinter without using ajax.
Comment on attachment 457981 [details] [diff] [review]
grab attachment data from the server instead of ajax (WIP)

Pretty sure this would allow anybody to view any attachment they wanted to... You need to make sure the user is permitted to access the attachment before just handing over the data.
Attachment #457981 - Flags: review-
updated patch with security concerns addressed.  Also added a comment that would be beneficial if bug 579914 lands.
Attachment #457981 - Attachment is obsolete: true
Attachment #458759 - Flags: review?(reed)
(In reply to comment #46)
> IE doesn't honour the server's content-type.  if you send html as text/plain,
> it'll render it as text/html.

Adding

X-Content-Type-Options: nosniff

should prevent that for IE8 and subsequent, if that proves helpful somehow.
(Do sites generally filter text/plain user-posted content to avoid XSS attacks?  That is a new one on me, though I knew of the sniffing behaviour.)
(In reply to comment #60)
> (Do sites generally filter text/plain user-posted content to avoid XSS attacks?
>  That is a new one on me, though I knew of the sniffing behaviour.)

  Well, it depends on the context, I think. If we're talking about arbitrary uploads that are then displayed to users (like attachments), the usual solution is just like ours--to display them on a different domain so that even if they are harmful they can't do anything too bad. It's not just limited to text attachments, either--theoretically any attachment could somehow be dangerous in a way that's hard to predict, and at least personally I didn't feel comfortable whitelisting anything purely by MIME type, since there are so many variables involved.
What are the next steps on this bug?  Is there more work that needs to be done on bugzilla or the extension?
Comment on attachment 458759 [details] [diff] [review]
grab attachment data from the server instead of ajax (1.0)

>+     my $attachid = $params->{attachment_id};
>+     $attachid = "1";

Leftover from debugging, I assume?
updated patch to remove a debugging call that I had '$attach = "1";'
Attachment #458759 - Attachment is obsolete: true
Attachment #458759 - Flags: review?(reed)
Attachment #461249 - Flags: review?(reed)
Comment on attachment 461249 [details] [diff] [review]
grab attachment data from the server instead of ajax (1.1)

>=== modified file 'extensions/Splinter/lib/WSSplinter.pm'
>+     my $attachid = $params->{attachment_id};
>+
>+     my ($data) = 
>+        $dbh->selectrow_array('SELECT attach_data.thedata
>+                                 FROM attach_data
>+                            LEFT JOIN attachments
>+                                   ON attachments.attach_id = attach_data.id
>+                                WHERE attachments.attach_id = ?',
>+                                undef, $attachid );

This query returns a taint error when you run it.  Importing detaint_natural from Bugzilla::Util and running it on $attachid before running the query seems to fix it.

This is now live on bugzilla-stage-tip with that change made.

At the top:
-use Bugzilla::Util qw(trim);
+use Bugzilla::Util qw(trim detaint_natural);

And just before the above query:

+     my $attachid = $params->{attachment_id};
+     detaint_natural($attachid);
Attachment #461249 - Flags: review?(reed) → review-
This is an updated patch with the feedback to use the detaint_natural from comment #65.  Also this addresses an issue where the patch is not able to display via splinter.  In that scenario it displays and error message and provides a link to view it in the attachment viewer that has and still does exist in bugzilla (as noted in https://bugzilla.mozilla.org/show_bug.cgi?id=578573#c5)
Attachment #461249 - Attachment is obsolete: true
Attachment #463533 - Flags: review?(justdave)
OK, I thought this patch was applied already, but checking it again, I do see subtle differences between this and what's deployed on staging...  so I backed out what's there and applied this patch to staging, should be ready to go again.  Sorry for the delay, didn't realize it was different until the comments on bug 578573 pointed it out.
updated patch to include Bugzilla:Error and add a better return value for functions that we could access with XMLRPC calls.

:justdave, can you get this loaded up on bugzilla stage and I will confirm it is on there.
Attachment #463533 - Attachment is obsolete: true
Attachment #473052 - Flags: review?(justdave)
Attachment #463533 - Flags: review?(justdave)
OK, latest patch here has been applied on bugzilla-stage-tip.
just a quick look at this and I see that I get this error when clicking on an attachment:
Failed to retrieve attachment: Application failed during request deserialization: no element found at line 1, column 0, byte -1 at /usr/lib/perl5/vendor_perl/5.8.8/i386-linux-thread-multi/XML/Parser.pm line 187

did any libraries change on this system?
(In reply to comment #70)
> just a quick look at this and I see that I get this error when clicking on an
> attachment:
> Failed to retrieve attachment: Application failed during request
> deserialization: no element found at line 1, column 0, byte -1 at
> /usr/lib/perl5/vendor_perl/5.8.8/i386-linux-thread-multi/XML/Parser.pm line 187
> 
> did any libraries change on this system?

Yeah, stuff got upgraded, and the new version of SOAP::Lite is apparently busted under mod_perl.  I just downrevved it.  Apparently still doesn't work, though.  Now I'm getting "Failed to retrieve attachment: Sorry, you are not authorized to access this attachment." instead of the deserializer error.
Looks like this missed Q2/Q3 -- what needs to be done to have it happen in Q4?
The code needs to work.  When it works, and security signs off on it (and security signs off on it in bug 578573) we'll push it out.
Any movement here?  I can't access bug 578573, so don't know if there's anything going on there..
Nope, unless something has changed, the code doesn't work and all security testing stopped until the new code works.
Is source auditing still going on, at least?  (I'm not sure why that bug is security-sensitive, since it's not about a deployed site, but I'll ask there.)
(In reply to comment #76)
> Is source auditing still going on, at least?  (I'm not sure why that bug is
> security-sensitive, since it's not about a deployed site, but I'll ask there.)

All review has stopped. The code review did take place at the beginning and there will be a few more tests within the app itself. I don't anticipate this will take long to complete once it works.
(In reply to comment #74)
> Any movement here?  I can't access bug 578573, so don't know if there's
> anything going on there..

Added you to the bug.  I'll be happy to add any others that want access.
Here is a bzr bundle from a branch I made off of the current bmo/4.0 effort. 

I did a few things so far:

1. Refactored some of the filenames to be less redundant.
2. Use jsonrpc instead of xmlrpc for all ajax calls which removed alot
of code from splinter.flat.js
3. Various changes in the extension code to work with newer 4.0 API.

This is working well on my local instance of 4.0. Since it is set up to use attachment statuses that bugzilla.gnome.org uses but bmo does not, it still needs some changes to work with review flags or other bmo type attachment status.

It also still uses show_bug.cgi?ctype=xml to get the current bug data so some more work is needed to make it fully jsonrpc. 

Let me know what you think so far.

Dave
Assignee: nobody → dkl
Status: NEW → ASSIGNED
Attachment #500477 - Flags: review?(reed)
Comment on attachment 500477 [details] [diff] [review]
Patch to make Splinter work with 4.0 bmo code (v1)

Haven't actually tested this yet (will do once the below comments are addressed), but here's my initial read-through of the code. Overall, it looks pretty great, and I'm very happy to see xmlrpc stuff replaced with jsonrpc (woo!). After we get this all ready, we should upstream to Owen's git repo or open a new bzr repo for this under bzr.m.o or something...

>     return "<a href='" . html_quote(get_review_url($bug, $attach_id)) . "'>$link_text</a>";

HTML quoting a URL? Wouldn't you use something like url_encode() (if that exists)?

> sub add_review_links_to_email {

Ugh, this entire function gives me the willies. Do we really have to use regexps to add this stuff? No way to add hooks that would make this better? Just looks so fragile to me.

>+    # if https://bugzilla.mozilla.org/show_bug.cgi?id=579514 lands we can just do
>+    # return $attachment->data; instad of all the stuff below;

That bug landed for 4.0. Can you update the code so we don't have to do all that stuff?

>+  Note that saving drafts requires the your browser to have support
>+  for the "DOM Storage" standard. At time of writing, this is
>+  available only in a few very recent browsers, like Firefox
>+  3.5.

Firefox 3.5 is not recent. This documentation needs a lot of work, especially with all of its git-isms that aren't as relevant in the Mozilla world (though, some Mozilla groups do use git quite a lot).

> var UPDATE_ATTACHMENT_SUCCESS = /<title>\s*Changes\s+Submitted/;

I guess we still have to do such hacks due to the lack of an API way to update attachments?

>+        var callData = { 
>+            "method": "Splinter.get_attachment", 
>+            "params": [ { "attachment_id": id } ],
>+            "id" : 1 

Why passing "id" of 1 here? Is that just a default that's replaced later?

>+                        .html("Please use another <a href=\"attachment.cgi?id="+id+"&action=edit\">review tool</a>.")

Does .html() convert & to &amp;?

># Begin bundle

What's in the bundle? The original code?
Attachment #500477 - Flags: review?(reed) → review-
Attachment #473052 - Attachment is obsolete: true
Attachment #473052 - Flags: review?(justdave)
(In reply to comment #80)
> Comment on attachment 500477 [details] [diff] [review]
> Patch to make Splinter work with 4.0 bmo code (v1)
> 
> Haven't actually tested this yet (will do once the below comments are
> addressed), but here's my initial read-through of the code. Overall, it looks
> pretty great, and I'm very happy to see xmlrpc stuff replaced with jsonrpc
> (woo!). After we get this all ready, we should upstream to Owen's git repo or
> open a new bzr repo for this under bzr.m.o or something...

Yes jsonrpc is better for working in javascript that having to create xmlrpc bindings which Owen was doing in splinter.flat.js. I also think I might update the part of the javascript code that gets the bug data via show_bug.cgi?ctype=xml to use Bug.get, Bug.comments, and Bug.attachments instead. I will if one way is better than the other.

I will address your comments as we work to modernize the current implementation. Most of it is as-is from before so I haven't had a chance to go through everything and optimize where possible. I just wanted to get it mostly working in it's current state. Also I am not too familiar with jQuery so I am 
learning some of it's syntax as I go.

One of the remaining questions is how to handle attachment style statuses in bmo. We could just have a standard flag UI displayed in the review page itself and allow the flags to be set from there such as setting to review+ or review-.
Of course we will also need a way to update attachment flags via the jsonrpc API which I don't believe is currently possible. 
 
> What's in the bundle? The original code?

I bundle is basically like a normal diff except it is a diff between two branches in Bazaar context. A bundle is nice especially when there have been files renamed which standard diff doesn't handle so well. To apply the bundle 
just go to your bmo/4.0 checkout and do

bzr merge <bundle_file>

Dave
Just as a question, have you thought about converting it from jQuery to YUI? Then users wouldn't have to download a whole new framework when using Splinter, and you could probably re-use existing Bugzilla code.
  Gerv: Thanks for the info, but we've already picked YUI, and the consensus that I've read seems to be that YUI is much better for designing large applications than jQuery is. So the comparative benefits of the two libraries aren't really in question here--the point is that there would be advantages to Splinter using YUI because Bugzilla already uses YUI.
> Yes jsonrpc is better for working in javascript that having to create xmlrpc
> bindings which Owen was doing in splinter.flat.js. I also think I might update
> the part of the javascript code that gets the bug data via
> show_bug.cgi?ctype=xml to use Bug.get, Bug.comments, and Bug.attachments
> instead. I will if one way is better than the other.

In principle, that's the right thing to do, but if it requires making 3 requests rather than 1, then it might not be desireable from a performance standpoint...

> Of course we will also need a way to update attachment flags via the jsonrpc
> API which I don't believe is currently possible. 

You could tie into the BzAPI for now. Or you could do what the BzAPI does on the back end, and just submit directly to the CGI and screen-scrape the result :-|

Or you could write a patch to add this function to the JSON-RPC API ;-)

Gerv
(In reply to comment #82)
> Just as a question, have you thought about converting it from jQuery to YUI?
> Then users wouldn't have to download a whole new framework when using Splinter,
> and you could probably re-use existing Bugzilla code.

Yes, I definitely thought about this and I think moving to YUI would be good for future work. As I stated before, getting it working in its current form was my first goal and the Splinter extension includes the jQuery lib in it's web directory. Which will eventually have the problem of keeping that file updated with the latest security fixes and bug fixes from the jQuery project. I don't think it will be much work at all to convert to YUI as it doesn't seem like any complex jQuery features are being used. 

Dave
(In reply to comment #85)
> In principle, that's the right thing to do, but if it requires making 3
> requests rather than 1, then it might not be desireable from a performance
> standpoint...

I think I might try to take Max's advices and just make page.cgi?id=splinter.html load the bug, comment, and attachment data at page load time. I don't really at this point see the reason to make a jsonrpc call to do that.
 
> You could tie into the BzAPI for now. Or you could do what the BzAPI does on
> the back end, and just submit directly to the CGI and screen-scrape the result
> :-|
> 
> Or you could write a patch to add this function to the JSON-RPC API ;-)

I think that extending the webservices API to allow updating of flags will be the best choice going forward. At Red Hat we had a local customization where we had Bugzilla/WebServices/Flag.pm that had Flag.get and a Flag.update methods.
I could upstream those and we could use those for the extension. An even better solution would be extending Bug.update to allow updating flags so the extension could add a comment and update the flags in a single transaction (one email).

Dave
Blocks: bmo-upgrade
FYI, I tested Splinter at https://bugzilla-stage-tip.mozilla.org/page.cgi?id=splinter.html&bug=577532&attachment=497375, and it doesn't work at all. Nothing happens.
I looked at https://bugzilla-stage-tip.mozilla.org/page.cgi?id=splinter.html&bug=577532&attachment=497375 and saw that for each file there was no diff.  I tried to view the attachment and ended up in a security exception state.

I suspect that using bugzilla-stage-tip is causing a redirect and a cert mismatch.
(In reply to comment #88)
> FYI, I tested Splinter at
> https://bugzilla-stage-tip.mozilla.org/page.cgi?id=splinter.html&bug=577532&attachment=497375,
> and it doesn't work at all. Nothing happens.

Is there an error in the javascript console?

Dave
(In reply to comment #89)
> I looked at
> https://bugzilla-stage-tip.mozilla.org/page.cgi?id=splinter.html&bug=577532&attachment=497375
> and saw that for each file there was no diff.  I tried to view the attachment
> and ended up in a security exception state.
> 
> I suspect that using bugzilla-stage-tip is causing a redirect and a cert
> mismatch.

Yeah I see the same issue with today's Minefield. I am also getting the following error in addition to the cert warning:

Error: lastHunk is undefined
Source File: https://bugzilla-stage-tip.mozilla.org//extensions/Splinter/web/splinter.js
Line: 1901

Are you also seeing that? I cannot see right away why this is failing. I tried this again with ff4b10 and it works perfectly with not errors/warnings. I may have to get some javascript person to look at this with me to see if there is something I am doing that is non-compliant and the latest builds of ff4 point it out.

Dave
(In reply to comment #89)
> I looked at
> https://bugzilla-stage-tip.mozilla.org/page.cgi?id=splinter.html&bug=577532&attachment=497375
> and saw that for each file there was no diff.  I tried to view the attachment
> and ended up in a security exception state.
> 
> I suspect that using bugzilla-stage-tip is causing a redirect and a cert
> mismatch.

Okay, I have pushed some changes to staging now that fix this problem for me. Please try your tests again.

Thanks for the feedback.
Dave
i've noticed that if you're reviewing a patch which updates two files with the same filename (but in different directories), only the first file is visible.

eg. https://bugzilla-stage-tip.mozilla.org/page.cgi?id=splinter.html&bug=577532&attachment=497375

Bugzilla/Attachment.pm works, but Bugzilla/Config/Attachment.pm does not; probably due to a duplicate id (both are "switch-Attachment.pm").
it's also only showing the first hunk from multi-hunk/same-file patches.
Ok committed some new changes that should show up on staging soon:

1. Fixed issue where some patch hunks getting truncated.
2. Fixed issue displaying previous review comments in the patch hunks.
3. Added attachment information to top of page such as description, date, and creator.
4. Fixed issue in the navigations links where two files having the same base name would conflict.

Thanks for the feedback so far!
Dave
Cool! A few pieces of feedback:

* It's not obvious that you can comment on pieces of the patch by double-clicking on them. I didn't figure it out, which means that nobody (or at least very few people) will figure it out.

* It's a little annoying to have to click on the different file names to see those patch pieces.

* I had no idea that the Overview comment would be merged; I thought that was the only place I could comment.
(In reply to comment #96)
> Cool! A few pieces of feedback:
> 
> * It's not obvious that you can comment on pieces of the patch by
> double-clicking on them. I didn't figure it out, which means that nobody (or at
> least very few people) will figure it out.

I will add some help text to the top of each page to direct people what to do. Also there is an extensive help page that should be more obvious at the top of the page as well.

> * It's a little annoying to have to click on the different file names to see
> those patch pieces.

I can have a default view where it renders each of the files in a full length page. The navigation links at the top would then just jump down to the proper file section. 
 
> * I had no idea that the Overview comment would be merged; I thought that was
> the only place I could comment.

Again would help to have some text at the top with a simple explanation.

Will work on these.
Dave
(In reply to comment #96)
> Cool! A few pieces of feedback:
> 
> * It's not obvious that you can comment on pieces of the patch by
> double-clicking on them. I didn't figure it out, which means that nobody (or at
> least very few people) will figure it out.

I have added some quick help notes to the main page to explain some basic actions.
 
> * I had no idea that the Overview comment would be merged; I thought that was
> the only place I could comment.

Same as above.

Please let me know what you think.

Dave
seems that a long line of code is not viewable.  We don't have horizontal scrolling and don't display text >60 characters per line?
(In reply to comment #99)
> seems that a long line of code is not viewable.  We don't have horizontal
> scrolling and don't display text >60 characters per line?

There's no hard limit, it's determined by how much text fits on the screen - so you can shrink the font size using browser controls and see more. Certainly not ideal, especially if you have a narrow screen. Adding horizontal scrollbars wouldn't work since it would break the flow of being able to read the rest of the diff - it doesn't work if you have to scroll to see old and new.

Possible approaches:

 - Wrap long lines
 - Pop-up hover with the full content of the lines

Doesn't really seem appropriate to have non-blocking issues discussed on this bug. There's a bugzilla component on bugzilla.gnome.org.

 https://bugzilla.gnome.org/browse.cgi?product=splinter

with some of the existing known issues. Don't know if that's an appropriate place long-term if there's a more actively developed version than my version elsewhere.
(In reply to comment #100)
> (In reply to comment #99)
> > seems that a long line of code is not viewable.  We don't have horizontal
> > scrolling and don't display text >60 characters per line?
> 
> There's no hard limit, it's determined by how much text fits on the screen - so
> you can shrink the font size using browser controls and see more. Certainly not
> ideal, especially if you have a narrow screen. Adding horizontal scrollbars
> wouldn't work since it would break the flow of being able to read the rest of
> the diff - it doesn't work if you have to scroll to see old and new.
> 
> Possible approaches:
> 
>  - Wrap long lines
>  - Pop-up hover with the full content of the lines

I think the latter would work better IMO as wrapping may make the patch difficult to read. I can experiment with the hover idea and see how it is.
 
> Doesn't really seem appropriate to have non-blocking issues discussed on this
> bug. There's a bugzilla component on bugzilla.gnome.org.
> 
>  https://bugzilla.gnome.org/browse.cgi?product=splinter
> 
> with some of the existing known issues. Don't know if that's an appropriate
> place long-term if there's a more actively developed version than my version
> elsewhere.

We don't really have a bug bucket for extensions at BMO either so I am not sure where the best place for bugs will be going forward. Our version of the extension is certainly not the standard either so I wouldn't say move all of the bugs out of BGO to somewhere else. Once I get the new extension is the upstream extension repo where I can point people to, I can start taking over some of the bugs at BGO and resolve them. Then point people to the upstream extension if they want to get the changes.

Dave
(In reply to comment #91)
> (In reply to comment #89)
> > I looked at
> > https://bugzilla-stage-tip.mozilla.org/page.cgi?id=splinter.html&bug=577532&attachment=497375
> > and saw that for each file there was no diff.  I tried to view the attachment
> > and ended up in a security exception state.
> > 
> > I suspect that using bugzilla-stage-tip is causing a redirect and a cert
> > mismatch.
> 
> Yeah I see the same issue with today's Minefield. I am also getting the
> following error in addition to the cert warning:
> 
> Error: lastHunk is undefined
> Source File:
> https://bugzilla-stage-tip.mozilla.org//extensions/Splinter/web/splinter.js
> Line: 1901
> 
> Are you also seeing that? I cannot see right away why this is failing. I tried
> this again with ff4b10 and it works perfectly with not errors/warnings. I may
> have to get some javascript person to look at this with me to see if there is
> something I am doing that is non-compliant and the latest builds of ff4 point
> it out.

Not sure if you figured a workaround for this, but I think it's a straight-up regexp engine bug - I just filed bug 635417

(may be a duplicate didn't see anything in a quick look.) Surprised Splinter works even as well as it does with that hitting one if its core regular expressions.

(I have a port of the splinter test case framework now to FF4 spidermonkey, will push to GNOME git shortly)
(In reply to comment #102)
> Not sure if you figured a workaround for this, but I think it's a straight-up
> regexp engine bug - I just filed bug 635417
> 
> (may be a duplicate didn't see anything in a quick look.) Surprised Splinter
> works even as well as it does with that hitting one if its core regular
> expressions.
> 
> (I have a port of the splinter test case framework now to FF4 spidermonkey,
> will push to GNOME git shortly)

Yes I encountered the problem as well and you beat me to filing the bug report. The workaround for me was to split the larger regexes into two separate smaller ones. Such as:

Before:
Patch.HUNK_RE = /^@@[ \t]+-(\d+),(\d+)[ \t]+\+(\d+),(\d+)[ \t]+@@(.*)\n((?:[ +\\-].*\n)*)/mg;

After:
Patch.HUNK_START_RE =/^@@[ \t]+-(\d+),(\d+)[ \t]+\+(\d+),(\d+)[ \t]+@@(.*)\n/mg;
Patch.HUNK_RE = /((?:[ +\\-].*\n)*)/mg;

And them made the necessary changes in the looping code that uses them. I did the same for the HUNK_RE in the Review class as well.

Dave
(In reply to comment #99)
> seems that a long line of code is not viewable.  We don't have horizontal
> scrolling and don't display text >60 characters per line?

  BTW, Bugzilla already has a CSS class available for wrapping <pre> text, if you want to use it. (It's what we use on comments and on the current PatchReader system.)
Checked in fix for line wrapping. Will show up in the next code drop to staging environment.

Dave
Whiteboard: [bmo4.0-resolved]
Links from the details and diff pages to the review would be nice...
Depends on: 652665
Depends on: 652664
No longer depends on: 652664
(In reply to comment #107)
> Links from the details and diff pages to the review would be nice...

Please file this as a separate bug report against the bugzilla.mozilla.org/Splinter product/component. 

I want to close this bug out as the Splinter extension is now live on BMO. Please file any problems/requests as new bug reports.

dkl
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Component: Bugzilla: Other b.m.o Issues → Splinter
OS: Linux → All
Product: mozilla.org → bugzilla.mozilla.org
QA Contact: general → splinter
Hardware: x86 → All
Resolution: --- → FIXED
Version: other → Current
(In reply to comment #107)
> Links from the details and diff pages to the review would be nice...

I have added that to the latest revision in the repository. So it should be in the next code update for production soon.

dkl
Is there known ETA for releasing?
(In reply to comment #110)
> Is there known ETA for releasing?

Can you be more detailed on your question? Are you asking about a particular fix or about the release of the Splinter extension in general? Splinter has been working on bugzilla.mozilla.org for quite a while now and the Splinter extension code is available for anyone who wants it at

https://wiki.mozilla.org/Bugzilla:Addons#Bugzilla_Extensions

dkl
> 
> Can you be more detailed on your question? Are you asking about a particular
> fix or about the release of the Splinter extension in general? Splinter has

release of the Splinter extension in general

> https://wiki.mozilla.org/Bugzilla:Addons#Bugzilla_Extensions
> 

there is Splinter for 4.0. What about 3.6?

Thanks, Jiri
(In reply to comment #112)
> there is Splinter for 4.0. What about 3.6?

It will work for 3.6 as well unchanged. At least it tested fine with BMO's version of 3.6 before we upgraded since we almost released it earlier than the 4.0 upgrade. I realize I should either copy the code over to an extensions/3.6 directory as well or document it on the wiki.

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

Attachment

General

Creator:
Created:
Updated:
Size: