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 allowList functionality to Twig security policy #1293

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

LukeTowers
Copy link
Member

No description provided.

@bennothommo
Copy link
Member

bennothommo commented Jan 23, 2025

@LukeTowers the docs state that people can put data to the session, as well forgetting data and flushing the entire session, as per https://wintercms.com/docs/v1.2/markup/this/session.

TBH, I'd be more included to remove this.session entirely, or replacing the App::make('session') call here with a limited proxy class that doesn't expose the Session class directly.

@LukeTowers
Copy link
Member Author

@bennothommo in that case then I'll add those methods in but leave the rest blocked. I'm not opposed to a potential proxy class but I'm not sure how much better that'll be than the allowList approach

@bennothommo
Copy link
Member

bennothommo commented Jan 23, 2025

@LukeTowers I feel the Proxy class might be "cleaner", rather than having methods allowed several levels deep in a policy where it could be perhaps forgotten. Ideally I'd like a proxy class for the controller too.

@LukeTowers
Copy link
Member Author

eh, possibly; my main argument against a proxy class is what else is going to use said proxy class? If it's a proxy just for the sake of locking down that interface and is only used in that one case but requires a whole separate file & class for the functionality then I'd argue that it'd be more of a pain to maintain that vs maintaining the centralized security policy for Twig as a whole.

@bennothommo
Copy link
Member

@LukeTowers we could always use an anonymous class ;)

$this->getTwig()->addGlobal('this', [
    // ...
    'session' => new class {
        public function get($key)
        {
            // ...
        }

        public function has($key)
        {
            // ...
        }
    }
    // ...
]);

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.

2 participants