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

Embeddable mode #213 #772

Closed
wants to merge 24 commits into from
Closed

Conversation

aymanterra
Copy link
Contributor

No description provided.

…le_mode

Revert "embeddable mode 1st commit"
@ukutaht
Copy link
Contributor

ukutaht commented Mar 3, 2021

Wonderful. Really great job @aymanterra and @bradkane. Thank you so much!!

I am testing this out now and will be trying to get it across the finish line this week. Will probably pop my questions and comments in here as I go.

First, I made a pretty big change to how shared links work so that it uses a separate cookie from all the other session stuff we do. This allows us to have a SameSite=None; Secure cookie for shared links while being more strict for normal authenticated sessions.

This PR requires some rework in the plugs and routes and such to align with the new cookie stuff. I am happy to continue the work on this. The second thing I would probably want to change is moving the embeddable field from the site record down to the shared_link record. I think that's a more natural scope for the field. Let me know if you see any issues with that.

I did have a question about X-Frame-Origin - why restrict it at all? Generally speaking I get it, we don't want anyone to embed our settings page for example, because it opens up a clickjacking attack vector. But what if we disabled X-Frame-Option on the embedded endpoints and kept it everywhere else? I can't think of any attacks it would open up and it would allow embeds to work cross origin as I understand it. I might be missing something but I'll test this approach out.

@bradkane
Copy link
Contributor

bradkane commented Mar 3, 2021

@ukutaht So glad you are (generally) pleased with the changes made. I agree with you that @aymanterra did a great job! I think your comments and suggestions are all great improvements. These are areas where deep knowledge of the architecture and code can make our work even better and it is terrific that you are adding your limited time to this effort as well. Let us know how we can help.

@ukutaht ukutaht mentioned this pull request Mar 9, 2021
5 tasks
@ukutaht
Copy link
Contributor

ukutaht commented Mar 10, 2021

Closed in favour of #812

@ukutaht ukutaht closed this Mar 10, 2021
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.

3 participants