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

Support configurable frame ancestors for embeds #571

Open
aduth opened this issue Dec 21, 2023 · 2 comments
Open

Support configurable frame ancestors for embeds #571

aduth opened this issue Dec 21, 2023 · 2 comments
Labels
feature request A suggestion for a feature or enhancement

Comments

@aduth
Copy link

aduth commented Dec 21, 2023

Is your feature request related to a problem? Please describe

Currently, it is only possible to configure Lookbook to allow embeds from the same origin or for all external sites.

Ideally, it'd be possible to configure specific hosts from which Lookbook can be embedded, using the frame-ancestors Content-Security-Policy.

The existing option for preview_embeds.policy controls X-Frame-Options which, as noted at the MDN article, is largely obsolete in favor of this frame-ancestors CSP directive.

It's also not possible for us to manually add a policy revision to add this CSP directive, since Lookbook disables the content security policy altogether. We've found a way to work around this in our project, but it involves creating a middleware to modify the raw response headers, which is less than ideal.

Describe the solution you'd like

A new option like config.lookbook.preview_embeds.frame_ancestors that accepts an array of sources to be included in a frame-ancestors CSP directive controlled by Lookbook. If present, it may make sense for this to either take precedent over the X-Frame-Options behavior controlled by preview_embeds.policy, or set the value of preview_embeds.policy to ALLOWALL.

Alternatively, if Lookbook were not to disable the content security policy outright and instead modify it as necessary for its own operation (as discussed in #202), it would allow a downstream project to define their own frame-ancestors policy.

@aduth aduth added the feature request A suggestion for a feature or enhancement label Dec 21, 2023
@allmarkedup
Copy link
Collaborator

Hey @aduth, thanks for this and many apologies for taking so long to reply.

I have to say my knowledge of CSPs is still pretty limited (despite your help in #202) but what you are suggesting makes total sense.

My gut reaction is to go down the preview_embeds.frame_ancestors config option route as I still feel that keeping parity with the way that ViewComponent disables the CSP is probably still preferable. I'm also a little nervous about adding more complex policy configuration code when I don't feel like I have a lot of expertise in this area.

I'm pretty short of time for working on Lookbook at the moment but I'll try to have a proper think about this as soon as I can. As the config option route is probably simpler I may see if I can give that a go - I'll let you know once I have any code ready so you can take a look at it and see if it will work for you.

@aduth
Copy link
Author

aduth commented Feb 7, 2024

Hi @allmarkedup, totally understand if you don't have the bandwidth for this at the moment! Fortunately our workaround is serving us just fine for the time being, but we'll look forward to this functionality being supported out of the box. Happy to review or provide feedback for any changes or ideas you come up with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request A suggestion for a feature or enhancement
Projects
None yet
Development

No branches or pull requests

2 participants