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

Added decorator exclude_xframe_options_header #40

Merged
merged 1 commit into from
Jan 15, 2021

Conversation

jkfran
Copy link
Contributor

@jkfran jkfran commented Jan 14, 2021

Done

  • Added decorator exclude_xframe_options_header to avoid the security header X-Frame-Options with the value SAMEORIGIN

QA

QA this change with canonical/snapcraft.io#3337

@nottrobin
Copy link
Contributor

So the interface for using this feature is:

@exclude_xframe_options_header
def my_view():
    # ...

Another option would be something like:

def my_view():
    # ...
    response = flask.render_template("template")
    response.headers["x-frame-options"] = "allowall"
    return response

And then we would write the after_request function to only define x-frame-options if it didn't already exist. This feels a little more intuitive to me, but it has the downside that allowall is in fact not a valid option for x-frame-options - it only works because setting x-frame-options to an invalid option will cause browsers to simply ignore the header, which has the same effect.

So I can't quite decide what is best here.

@jkfran
Copy link
Contributor Author

jkfran commented Jan 14, 2021

Another option would be something like:

def my_view():
    # ...
    response = flask.render_template("template")
    response.headers["x-frame-options"] = "allowall"
    return response

This was my first choice until I searched what is the default value for x-frame-options and discovering that there isn't and the header shouldn't be set then. I don't like setting an invalid value just to avoid the issue, I like the decorator, I'm also open to new ideas.

As an improvement of the current change, we can mix these two options so if we need to set the values DENY or ALLOW-FROM ... this decorator is not needed because the header is already defined.

@nottrobin
Copy link
Contributor

Yes I agree with all that. Let's do both =)

Copy link
Contributor

@nottrobin nottrobin left a comment

Choose a reason for hiding this comment

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

Approved, assuming you put in the conditional =)

@jkfran jkfran force-pushed the exclude_xframe_options_header branch from cb0695e to 905a057 Compare January 15, 2021 09:29
@jkfran
Copy link
Contributor Author

jkfran commented Jan 15, 2021

Approved, assuming you put in the conditional =)

I did! 😃

@jkfran jkfran merged commit 9110d86 into canonical:master Jan 15, 2021
@jkfran jkfran deleted the exclude_xframe_options_header branch January 15, 2021 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants