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

Fix sandbox being left enabled if an exception is thrown while rendering #2282

Merged
merged 1 commit into from
Dec 13, 2016
Merged

Fix sandbox being left enabled if an exception is thrown while rendering #2282

merged 1 commit into from
Dec 13, 2016

Conversation

CarsonF
Copy link
Contributor

@CarsonF CarsonF commented Dec 5, 2016

What's happening for me:

  • An exception is thrown while rendering inside the sandbox
  • Render exception response
  • WebProfiler toolbar tries to include a file
  • Sandbox is still enabled, so a SecurityException is thrown (which hides the real error)

@@ -1402,6 +1402,12 @@ function twig_include(Twig_Environment $env, $context, $template, $variables = a

throw $e;
}
} catch (\Exception $e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the same should be down when catching Throwable for PHP 7 too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does PHP 5 choke on catch (Throwable $e)?

Should that be before or after catching Exception? It probably doesn't matter in this case. I just remember reading somewhere that to make code compatible with PHP 5 Throwable should be caught first...

@stof
Copy link
Member

stof commented Dec 6, 2016

could you write a test covering this ?

@CarsonF
Copy link
Contributor Author

CarsonF commented Dec 6, 2016

could you write a test covering this ?

Since this involves the include function and the sandbox I'm not sure where I should add the test?
SandboxTest?

@fabpot
Copy link
Contributor

fabpot commented Dec 13, 2016

@CarsonF Yes, SandboxTest seems to make more sense

@CarsonF
Copy link
Contributor Author

CarsonF commented Dec 13, 2016

@fabpot I don't think those CS changes should be made in this PR, right?

@fabpot
Copy link
Contributor

fabpot commented Dec 13, 2016

Indeed, you can ignore them :)

@@ -1406,6 +1406,18 @@ function twig_include(Twig_Environment $env, $context, $template, $variables = a

throw $e;
}
} catch (\Throwable $e) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\ should be removed here and below as we are not using namespaces and we are still PHP 5.2 compatible :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, of course.

@@ -34,6 +34,7 @@ protected function setUp()
'1_basic' => '{% if obj.foo %}{{ obj.foo|upper }}{% endif %}',
'1_layout' => '{% block content %}{% endblock %}',
'1_child' => "{% extends \"1_layout\" %}\n{% block content %}\n{{ \"a\"|json_encode }}\n{% endblock %}",
'1_include' => '{{ include("1_basic1", sandboxed=true) }}',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change "1_basic1" to "unknown" or something similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well that would throw Twig_Error_Loader and disabling the sandbox is already checked for that exception.
https://github.com/twigphp/twig/blob/1.x/lib/Twig/Extension/Core.php#L1402-L1405

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm

@fabpot
Copy link
Contributor

fabpot commented Dec 13, 2016

Thank you @CarsonF.

@fabpot fabpot merged commit 171a1d4 into twigphp:1.x Dec 13, 2016
fabpot added a commit that referenced this pull request Dec 13, 2016
…ile rendering (CarsonF)

This PR was merged into the 1.x branch.

Discussion
----------

Fix sandbox being left enabled if an exception is thrown while rendering

What's happening for me:
- An exception is thrown while rendering inside the sandbox
- Render exception response
- WebProfiler toolbar tries to include a file
- Sandbox is still enabled, so a SecurityException is thrown (which hides the real error)

Commits
-------

171a1d4 Fix sandbox being left enabled if an exception is thrown while rendering with include function
@CarsonF CarsonF deleted the bugfix/exception-in-sandbox branch December 13, 2016 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants