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

redirectToPost implementation is incompatible with strict Content-Security-Policy #1647

Closed
charukiewicz opened this issue Nov 30, 2019 · 4 comments

Comments

@charukiewicz
Copy link
Contributor

charukiewicz commented Nov 30, 2019

The Problem

Work working with yesod-auth I ran into a strange issue where my /auth/logout path stopped automatically redirecting me to my intended logout page, and instead left me in the intermediary "Continue" state. It turns out that this issue was caused by a Content-Security-Policy header that does not allow unsafe-inline, blocking inline JavaScript and instead throwing an error.

yesod-auth-logout-csp-violation

Turns out that this issue is caused by the current implementation of redirectToPost, which relies on an inline <script> tag.

My Attempted Solution

I tried to refactor redirectToPost, currently implemented as:

redirectToPost :: (MonadHandler m, RedirectUrl (HandlerSite m) url)
               => url
               -> m a
redirectToPost url = do
    urlText <- toTextUrl url
    req <- getRequest
    withUrlRenderer [hamlet|
$newline never
$doctype 5

<html>
    <head>
        <title>Redirecting...
    <body>
        <form id="form" method="post" action=#{urlText}>
            $maybe token <- reqToken req
                <input type=hidden name=#{defaultCsrfParamName} value=#{token}>
            <noscript>
                <p>Javascript has been disabled; please click on the button below to be redirected.
            <input type="submit" value="Continue">
        <script>
          window.onload = function() { document.getElementById('form').submit(); };
|] >>= sendResponse

My approach to eliminate the inline <script> tag was to convert the window.onload line to a widget and load it into the hamlet body. Here's what I tried to do:

redirectToPost :: (MonadHandler m, RedirectUrl (HandlerSite m) url)
               => url
               -> m a
redirectToPost url = do
    urlText <- toTextUrl url
    req <- getRequest
    pc <- widgetToPageContent $ toWidget $ [julius|window.onload = function() { document.getElementById('form').submit(); };|]
    withUrlRenderer [hamlet|
$newline never
$doctype 5

<html>
    <head>
        <title>Redirecting...
    <body>
        <form id="form" method="post" action=#{urlText}>
            $maybe token <- reqToken req
                <input type=hidden name=#{defaultCsrfParamName} value=#{token}>
            <noscript>
                <p>Javascript has been disabled; please click on the button below to be redirected.
            <input type="submit" value="Continue">
        ^{pageBody pc}
|] >>= sendResponse

I believe this approach would work, but toWidget is defined in Yesod.Core.Widget, which depends on Yesod.Core.Handler (the module with the change), so that's not really an option.

Another Possible Solution

A different solution would be to make the GET handler (getLogoutR) delete the user's credentials, with no need for this intermediary page. Currently it is implemented as:

getLogoutR :: AuthHandler master ()
getLogoutR = do
tp <- getRouteToParent
setUltDestReferer' >> redirectToPost (tp LogoutR)

This approach was suggested in #1475 (for reasons unrelated to a CSP), but rejected, for the reason that GET requests should not modify server state.

How to Proceed

I started my first-ever Yesod project a few months ago, and while I very much like the framework, I think one of the project's goals should be to strive for compatibility with modern web standards, especially ones related to security. A strict Content-Security-Policy is one of the examples of that. To quote Mozilla:

The use of this header is the best method to prevent cross-site scripting (XSS) vulnerabilities. Due to the difficulty in retrofitting CSP into existing websites, CSP is mandatory for all new websites and is strongly recommended for all existing high-risk sites.

I understand the argument for not making GET requests modify server state. If that isn't a good option, I am curious to hear if anyone else has thoughts on the approach I outlined above (relying on making the automatic redirect a widget rather than an inline <script>), or if anyone has any thoughts on this issue in general.

@charukiewicz charukiewicz changed the title yesod-core redirectToPost implementation is incompatible with strict Content-Security-Policy redirectToPost implementation is incompatible with strict Content-Security-Policy Nov 30, 2019
@snoyberg
Copy link
Member

snoyberg commented Dec 3, 2019

I don't think Widgets can be relied upon to solve this problem. They may also inject the Javascript into a script tag. Though I suppose you could argue that, if someone enabled that header, then they better also fix the rendering of Javascript to be in a separate file.

So I guess I'm OK with moving to the Widget approach.

Idea for implementation: what if we make a new method in the Yesod typeclass for this, and have redirectToPost call that. Then we can provide a method for people to opt-in to the new behavior if they want.

@jezen
Copy link
Member

jezen commented Dec 11, 2019

@charukiewicz I actually struggled with the same earlier, but it was merged already here: #1620

I wrote more about working with a CSP in Yesod, which you can read here: https://jezenthomas.com/implementing-csp-in-yesod/

You might find it useful.

@charukiewicz
Copy link
Contributor Author

charukiewicz commented Dec 17, 2019

@jezen Thanks for the link and suggesting this approach. This is very helpful. Looks like using a strict-dynamic CSP solves the problem I described in my first post.

@jezen
Copy link
Member

jezen commented Dec 17, 2019

I think that given your immediate issue is resolved, this thread can be closed.

I do agree with your point here:

I think one of the project's goals should be to strive for compatibility with modern web standards, especially ones related to security. A strict Content-Security-Policy is one of the examples of that.

But I think this is a separate (and much larger) conversation to be had.

@jezen jezen closed this as completed Dec 17, 2019
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

No branches or pull requests

3 participants