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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Selective route permission to use embeds, fixes #322 in a better way #812

Merged
merged 2 commits into from Jul 17, 2018

Conversation

rigelk
Copy link
Collaborator

@rigelk rigelk commented Jul 15, 2018

note: this time it should work 馃槄

@@ -38,6 +38,8 @@ server {
# resolver_timeout 5s;

add_header Strict-Transport-Security "max-age=63072000; includeSubDomains; preload";
add_header X-Frame-Options DENY;
add_header X-Frame-Options https://peertube.example.com/videos/embed;
Copy link
Contributor

Choose a reason for hiding this comment

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

If people forget to update this, the default position will be to deny embeds. I think the default should be to allow embeds. This header should be applied dynamically in the codebase not in the server configuration.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should be corrected.

@rigelk rigelk force-pushed the nginx/selective-frame-deny branch from c24c18d to d3c5e5c Compare July 15, 2018 20:00
server.ts Outdated
app.use('/services', servicesRouter, helmet.frameguard({
action: 'allow-from',
domain: CONFIG.WEBSERVER.URL
}))
Copy link
Contributor

Choose a reason for hiding this comment

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

The regular HTML embed is served from /embed, this is the oembed part

@rigelk rigelk force-pushed the nginx/selective-frame-deny branch 3 times, most recently from edcf5d6 to e9a49a2 Compare July 16, 2018 07:02
@rigelk rigelk added the Status: Requires Tests Either requires manual test, or writing tests, or both label Jul 16, 2018
@rigelk rigelk force-pushed the nginx/selective-frame-deny branch from e9a49a2 to be9520d Compare July 16, 2018 11:44
@rigelk
Copy link
Collaborator Author

rigelk commented Jul 16, 2018

@rezonant I've added a CSP since helmet makes adding one quite easy. Could you have a look at it?

anmol26s pushed a commit to YunoHost-Apps/peertube_ynh that referenced this pull request Jul 17, 2018
@rigelk rigelk force-pushed the nginx/selective-frame-deny branch from be9520d to 8c3caec Compare July 17, 2018 09:37
@rigelk rigelk merged commit 4bdd947 into Chocobozzz:develop Jul 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Requires Tests Either requires manual test, or writing tests, or both
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants