Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#2171] Action context access from error templates for routes-defined 404 actions #1231

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sant0s
Copy link
Contributor

@sant0s sant0s commented Apr 30, 2018

Lighthouse ticket

https://play.lighthouseapp.com/projects/57987-play-framework/tickets/2171

Framework version

1.5.3

Platform

Ubuntu 18.04, Oracle JDK 1.8.0_201

Description

For 404 actions declared in the routes file, action context items (e.g. controllers.Secure.Security.isConnected()) are not available to error templates and exceptions are thrown when they try to access them, resulting in HTTP 500 responses instead of HTTP 404 ones.

Without access to the session, HTTP 404 pages become more limited than other pages. This may cause the web app not be consistent across all pages.

Example: all the pages in a web app feature a navbar that displays "log in" if the user is logged out or "log out" if the user is logged in based on controllers.Secure.Security.isConnected(). The navbar may also display links to different web app actions depending on whether the user is logged in or not. All works well while a logged in user navigates across different pages, but as soon as a routes-defined 404 page is accessed, the navbar cannot be displayed like the other pages, cause there's no access to session information.

Actual behaviour reproduction steps

  1. Create a brand new Play! 1.5.3 app

  2. Add the secure module dependency (play -> secure) in conf/dependencies.yml and execute play dependencies

  3. Include the following code in app/views/errors/404.html and app/views/main.html:

<p>isConnected: ${controllers.Secure.Security.isConnected()}</p>
<p>Connected: ${controllers.Secure.Security.connected()}</p>
  1. In the routes file, add a route for a URL (1) and the 404 action. Example:

* /notfound 404

  1. Start the app

  2. Open a browser at the root URL (http://localhost:9000)

  3. The returned page successfully displays isConnected/Connected

  4. Point the browser at http://localhost:9000/notfound

  5. An HTTP 500 is returned and logs show a NPE:

Error during the 500 response generation

Execution exception (In {module:secure}/app/controllers/Secure.java around line 167)
NullPointerException occurred : null

play.exceptions.JavaExecutionException
	at play.templates.BaseTemplate.throwException(BaseTemplate.java:87)
	at play.templates.GroovyTemplate.internalRender(GroovyTemplate.java:307)
	at play.templates.Template.render(Template.java:28)
	at play.templates.GroovyTemplate.render(GroovyTemplate.java:230)
	at play.server.PlayHandler.serve500(PlayHandler.java:809)
	at Invocation.HTTP Request(Play!)
Caused by: java.lang.NullPointerException
	at controllers.Secure$Security.isConnected(Secure.java:167)
	at /app/views/errors/500.html.(line:9)
	at play.templates.GroovyTemplate.internalRender(GroovyTemplate.java:282)
	... 4 more

Expected behaviour

Session information should be available everywhere so web apps can be consistent across all pages.

Technical details

In class PlayHandler, the action context is reset in the init method, making it unavailable in templates.

@sant0s
Copy link
Contributor Author

sant0s commented Apr 11, 2019

@xael-fry For some reason, ticket 2170/PR 1230 went through in Play! 1.5.2, but related ticket 2171/PR 1231 (this one) didn't. I've just rebased this pull request's branch onto the latest master and re-tested successfully i.e. checked that the issues reported in the ticket show up/do not show up before/after this fix is applied. Could this pull request be included in the next Play! release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant