Feature #12898

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

Added by Björn Peemöller almost 5 years ago. Updated almost 5 years ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:Jean-Philippe Lang% Done:

0%

Category:Code cleanup/refactoring
Target version:2.3.0
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

Associated revisions

Revision 11226
Added by Jean-Philippe Lang almost 5 years ago

Respond with 404 when params[:ids] is missing (#12898).

History

#1 Updated by Etienne Massip almost 5 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.

#2 Updated by Björn Peemöller almost 5 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.

#3 Updated by Etienne Massip almost 5 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?

#4 Updated by Björn Peemöller almost 5 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.

#5 Updated by Etienne Massip almost 5 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.

#6 Updated by Björn Peemöller almost 5 years ago

Thanks, that sounds good.

#7 Updated by Jean-Philippe Lang almost 5 years ago

  • Status changed from Reopened to Closed
  • Assignee set to Jean-Philippe Lang
  • Target version changed from Candidate for next major release to 2.3.0
  • Resolution set to Fixed

Fixed in r11226. You get a 404 response now.

Also available in: Atom PDF