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

Add SourcePolicyInterface to selectively enable the Sandbox based on a template's Source #3893

Merged
merged 1 commit into from
Dec 19, 2023

Conversation

YSaxon
Copy link
Contributor

@YSaxon YSaxon commented Oct 17, 2023

This is needed to patch some downstream vulnerabilities which I won't describe here.
I wrote @fabpot an email about this with more details a few weeks ago.

Generally the Sandbox can be enabled for a given template in either of two ways:

  • Globally
  • Including the template from another template in which we use the sandbox tag or parameter

This pull request adds a third way

  • Using a SourcePolicy to selectively sandbox templates based on their Source object

@YSaxon YSaxon changed the title Add SourcePolicyInterface to optionally and additively enable the Sandbox based on the template Source Add SourcePolicyInterface to selectively enable the Sandbox based on a template's Source Oct 17, 2023
src/Extension/SandboxExtension.php Outdated Show resolved Hide resolved
src/Extension/SandboxExtension.php Outdated Show resolved Hide resolved
src/Sandbox/SourcePolicyInterface.php Outdated Show resolved Hide resolved
src/Extension/SandboxExtension.php Outdated Show resolved Hide resolved
src/Extension/SandboxExtension.php Outdated Show resolved Hide resolved
*/
interface SourcePolicyInterface
{
public function enableSandbox(?Source $source): bool;
Copy link
Member

Choose a reason for hiding this comment

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

should we even support null in this interface ? I don't know how an implementation would take a decision in this case. We should rather handle that case in the SandboxExtension, calling the source policy only when we have a source (which should always be the case in calls done in compiled templates rather than custom callers anyway)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't entirely sure under what circumstances Source could actually be null, but my thought was to allow the implementer to decide how to handle the null cases - essentially a question of whether to whitelist acceptable Sources (and block nulls) or blacklist unacceptable ones (and accept nulls)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But in any case I just added commit da79bdc as an implementation of what you said

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Post rebase, that's now commit cbbf38b)

@YSaxon
Copy link
Contributor Author

YSaxon commented Oct 27, 2023

I did a rebase onto the squash commit from my other pull request to solve a merge conflict.

@YSaxon
Copy link
Contributor Author

YSaxon commented Oct 30, 2023

@stof

@YSaxon
Copy link
Contributor Author

YSaxon commented Nov 2, 2023

@stof, I believe this is ready for review, having addressed the points you raised

@YSaxon
Copy link
Contributor Author

YSaxon commented Nov 21, 2023

@fabpot

@YSaxon
Copy link
Contributor Author

YSaxon commented Dec 18, 2023

@fabpot just a friendly reminder that I'm still waiting for a merge here so I can fix the downstream vulns. I believe the only "X" is a mistaken fabbot.io request to change things in existing code.

@fabpot
Copy link
Contributor

fabpot commented Dec 19, 2023

Thank you @YSaxon.

@fabpot fabpot merged commit a4974b2 into twigphp:2.x Dec 19, 2023
5 of 6 checks passed
@YSaxon
Copy link
Contributor Author

YSaxon commented Dec 20, 2023

Thanks @fabpot

Could we get a minor version release for this on 2.x?

@fabpot
Copy link
Contributor

fabpot commented Dec 22, 2023

@YSaxon Done now

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