Feature #12898: Handle GET /issues/context_menu parameters nicely to prevent returning error 500 to crawlers - Redmine

Actions

Copy link

Feature #12898

closed

Handle GET /issues/context_menu parameters nicely to prevent returning error 500 to crawlers

Added by Björn Peemöller over 11 years ago. Updated over 11 years ago.

Status:

Closed

Priority:

Normal

Assignee:

Jean-Philippe Lang

Category:

Code cleanup/refactoring

Target version:

2.3.0

Start date:

Due date:

% Done:

Estimated time:

Resolution:

Fixed

Description

When I navigate to the URL /issues/context_menu in my browser, I get an error 500.

The airbrake notification I get points to line 36 in file app/views/context_menus/issues.html.erb:

ERROR MESSAGE:

NoMethodError: undefined method `include?' for nil:NilClass

WHERE:

context_menus#issues

[PROJECT_ROOT]/app/views/context_menus/issues.html.erb:36

[PROJECT_ROOT]/app/controllers/context_menus_controller.rb:71

URL:
http://<my-domain>/redmine/issues/context_menu

BACKTRACE:
[PROJECT_ROOT]/app/views/context_menus/issues.html.erb:36

[GEM_ROOT]/gems/actionpack-3.2.11/lib/action_view/template.rb:145

[GEM_ROOT]/gems/actionpack-3.2.11/lib/action_view/template.rb:145

...

I do not know in which circumstances this URL is used, but as this error is reproducible by retrieving the URL directly, it might be worth a fix.

My configuration:

Environment:
  Redmine version                          2.2.1.stable
  Ruby version                             1.8.7 (i486-linux)
  Rails version                            3.2.11
  Environment                              production
  Database adapter                         PostgreSQL
Redmine plugins:
  redmine_wiki_extensions                  0.6.0

Actions

Copy link

Updated by Etienne Massip over 11 years ago

Status changed from New to Closed
Resolution set to Invalid

This URL is called when right-clicking the issues list and is expecting a ids[]=<issue id> parameter matching each selected issue (try /issues/context_menu?ids[]=10&ids[]=11 or whatever ids).

It is not meant to be called directly.

Actions

Copy link

Updated by Björn Peemöller over 11 years ago

Status changed from Closed to Reopened

Okay, I understand that. But I think returning a status code 400 indicating a bad request (or something similar) would be much better than relying on the Ruby code to crash. Therefore, I'm reopening this issue.

Actions

Copy link

Updated by Etienne Massip over 11 years ago

Björn Peemöller wrote:

Okay, I understand that. But I think returning a status code 400 indicating a bad request (or something similar) would be much better than relying on the Ruby code to crash. Therefore, I'm reopening this issue.

Why would you care, this URL is intended for internal use only?

Actions

Copy link

Updated by Björn Peemöller over 11 years ago

Etienne Massip wrote:

Why would you care, this URL is intended for internal use only?

Because I think that the API accessible from the outside world (in this case, accessible URLs) should be stable in the sense that even wrong parameters should not lead to a runtime error.

By the way, I found out that the Google Bot first asked for this URL, which is given in the initialization of the context menu:

<script type="text/javascript">
//<![CDATA[
contextMenuInit('/redmine/issues/context_menu')
//]]>
</script>

I only noticed this because Airbrake automatically notifies me about internal server errors. If you do not intend to change this, I can configure Airbrake to conceal this error, of course.

Actions

Copy link

Updated by Etienne Massip over 11 years ago

Tracker changed from Defect to Feature
Subject changed from Get-Request of URL /issues/context_menu causes Error 500 to Handle GET /issues/context_menu parameters nicely to prevent returning error 500 to crawlers
Category set to Code cleanup/refactoring
Target version set to Candidate for next major release
Resolution deleted (~~Invalid~~)

Björn Peemöller wrote:

Because I think that the API accessible from the outside world (in this case, accessible URLs) should be stable in the sense that even wrong parameters should not lead to a runtime error.

Agreed but this is not part of an API.

I only noticed this because Airbrake automatically notifies me about internal server errors. If you do not intend to change this, I can configure Airbrake to conceal this error, of course.

No, I simply need a good reason, you just gave me one.
I set it as a FR since it's not exactly a defect to me (it's working as expected), fixer is free to switch it back to Defect.

Actions

Copy link