Add message to NotFound exception

[stefansundin]

Hi. I am currently monkey-patching this in my own project, but I think it would be useful for everyone, so I'm submitting this PR.

Add request method and path to the NotFound exception message. This is useful for exception reporting tools.

As an example, this is what Airbrake looks before this change. I can still get the URL but I have to click in to each report. It is very hard to get a nice overview.

And this is what it looks like after:

Let me know what you think!

[jkowens]

Looks like a great improvement to me 👍🏻

[BerkhanBerkdemir]

I believe the documentation also needs an update after this improvement like

...raise a NotFound exception with the path as message.

https://github.com/stefansundin/sinatra/blob/243dca5c40650269cec922ed8decfbf6f5eea83a/lib/sinatra/base.rb#L1044-L1048

[raxoft]

I wonder who really thought this is a great change. Instead of providing the fixed size message body which corresponds to the error so the user can immediately see what was wrong you have turned this into an echo engine so people can ddos not only your incoming bandwidth but also the outcoming one. Great. 🤦

And while the OP could have simply used the not_found handler to get this functionality, without monkey patching anything, you have now forced this on everyone who was happy with the prior behavior, so they will now have to work around this. Double facepalm.

Sorry for being negative, but this really didn't please me at all.

[raxoft]

Seriously, guys, can we talk about getting this reverted? @zzak? @namusyaka? There are so many things wrong with this. To name a few:

• First of all, echoing back unverified content in this date and age when XSS attacks are being fought all around seems pretty stupid, to put it mildly. Until you fixed it later in Add escaping to the static 404 page. #1645, this didn't even escape things properly.
• You want the "not found" case to be handled as quickly as possible, so you don't waste extra resources on those, especially when being scanned by tons of bots with random probes etc. You have now slowed this by creating strings of semi-arbitrary length and sending them back, wasting more resources along your entire infrastructure. The people who will ddos you with cache poisoning attacks will be very pleased.
• With the app sitting back behind some front end proxy servers, which may alter the URLs, including adding some private information which perhaps shouldn't be revealed, you have now allowed this to be echoed back to the world. Wonderful.
• The fact that you imposed this on everyone by default without warning is sad, too. Seeing how many specs it broke this should have been marked as a change which breaks the prior contract.
• Last but not least, this was entirely pointless change in the first place. The OP could have used this in his app to get the very same result (adding whatever escaping he wanted, eventually):

  not_found do
    content_type :text
    "#{env['REQUEST_METHOD']} #{env['REQUEST_URI']}"
  end

So, where do we move from here? I am willing to prepare a pull request which reverts this and fixes the specs, too. But I would like to know first that it has a chance of being accepted.

And sorry for not objecting earlier. We seldom upgrade our gems when there is no need and sinatra was mature and stable enough as it was... I found this only when starting on a new app and using the latest versions of everything.

[dentarg]

I've felt this pain too, would be onboard with a change (maybe optional?), but I think rack does the same? https://github.com/rack/rack/blob/a7747ec32e9971649257e838b0b49b9588945107/lib/rack/urlmap.rb#L79

[raxoft]

I think rack does the same?

Good point, but IMO the context is entirely different. I mean, rack does report that only when it doesn't find the app responsible for some path mapping, which in practice should happen only while you are setting up and testing the stack, but never in production. Whereas sinatra does that now for every path which is not found, unconditionally, even if everything is set up as it should be.

Edit: You may compare it to the not found and error handlers while in development. I am grateful that sinatra does that in dev mode, as it helps seeing the request and everything. But echoing the path back in production is wrong, IMO.

[dentarg]

@raxoft I think you should open a PR changing this, I support it

[jkowens]

@raxoft is a PR incoming?

[raxoft]

Definitely. Sorry for the wait. I know it should be just few lines in the end, but I have been very busy with other things recently, and this week is not any better. I'll do my best to get back to this ASAP. Thanks.

[raxoft]

Finally got around to creating the PR. Sorry for the long wait and thanks for looking into this.