noir-clojure / lib-noir

A set of libraries for ring apps, including stateful sessions.
Eclipse Public License 1.0
483 stars 47 forks source link

0.9.1 downloads instead of displays #107

Open kanwei opened 9 years ago

kanwei commented 9 years ago

I've got a pretty simple setup:

(def app (-> all-routes
             (middleware/app-handler :session-options {:cookie-name "dd-session"
                                                      :store (cookie-store {:key (:cookie-key configs)})})
             middleware-json/wrap-json-body
             middleware-json/wrap-json-response
             jsonp/wrap-json-with-padding
             middleware-json/wrap-json-params
             wrap-params))

However, after upgrading to 0.9.1 webpages get downloaded instead of displayed as html.

yogthos commented 9 years ago

One big change in 0.9.1 is that the app-handler uses ring-defaults now. The site-defaults configuration is used when none is specified. This might be conflicting with your middleware.

As a side note, the preferred way to add middleware to app-handler is by putting it in a vector using the :middleware key. This way the middleware will be applied after the default middleware.

yogthos commented 9 years ago

could you try initializing the handler as follows and see if you still get the same issue:

(def app
  (app-handler
   [all-routes]
   :session-options {:cookie-name "dd-session"
                     :store (cookie-store {:key (:cookie-key configs)})}
   :middleware
   [middleware-json/wrap-json-body
    middleware-json/wrap-json-response
    jsonp/wrap-json-with-padding
    middleware-json/wrap-json-params
    wrap-params]))
yogthos commented 9 years ago

@kanwei are you still seeing the issue, or can we close it?

kanwei commented 9 years ago

Nope, still doing the same thing..

yogthos commented 9 years ago

I can't seem to reproduce the issue using the luminus template, could you possibly make a sample project that exhibits it to help with debugging.

adamneilson commented 9 years ago

I have a very similar issue. After running lein ancient on my Luminus based project I discovered that lib-noir was out of date and upgraded from 0.8.3 to 0.9.4. After doing this I discovered that none of the POSTs worked on my site as 0.9.4 seemed to switch Ring-Anti-Forgery on by default and I was getting a <h1>Invalid anti-forgery token</h1> response without any logging on the server-side.

I figured the added security was a good thing™ and so set about adding {% csrf-token %} tags to some of the forms on the site for testing.

Having done this I have found that the forms submit, they are accepted through the anti-forgery filter and get correctly forwarded on to the API but on the return journey the browser downloads the response rather than rendering it. As though a content-disposition header has been added or maybe Content-Type header has been stripped?

Rolling back to 0.8.3 makes this problem go away.

Here's my app-handler...

(def app (app-handler
             ;;  application routes 
             [home-routes
              other-routes
              ;<<===== LASTLY ======>>
              app-routes]
           ;; add custom middleware here
           :middleware [middleware/log-request
                        middleware/template-error-page
                        ; force SSL when we're running from production
                        middleware/with-force-ssl
                        ; are we on speaking terms with this IP?
                        middleware/block-ip-address?]

           ;; add access rules here
           :access-rules []
           ;; serialize/deserialize the following data formats. Available formats:
           ;; :json :json-kw :yaml :yaml-kw :edn :yaml-in-html
           :formats [:json-kw :edn]
           :session-options {:cookie-name "my-app"
                             :store (ring.middleware.session.cookie/cookie-store)}))
yogthos commented 9 years ago

The handler changed a bit to use ring-defaults to initialize the stock middleware, here's what it looks like currently. The custom middleware has now been moved to its own nsamespace as well here. Apps created using the latest template don't appear to exhibit the issue, so it does sound like the problem is with how the middleware is initialized.

devasiajoseph commented 9 years ago

Not sure whether related to this, but after upgrading to 0.9.4 and when I return (status 200 "Hello world") (status is from noir.response) It prompts me to download the content. Shouldn't it be displaying "Hello world"? I see the content type returned is "octet-stream". I am using luminus template. What am I missing?

yogthos commented 9 years ago

It sounds like all the issues relate to content type not being set correctly. Are you using the latest version of the template. When I create a new project using lein new luminus test-app I'm not able to reproduce this issue. Are you experiencing this a problem with the latest version of lein and the luminus template?

devasiajoseph commented 9 years ago

Yes, I tested this by creating a new project lein new luminus test-app. The function looks like this (defn test-page [] (response/status 200 "Hello world")) response is from [noir.response :as response]. Lein version is 2.5.0

yogthos commented 9 years ago

It looks like you have to set the content type explicitly with ring-defaults:

(response/content-type "text/plain; charset=utf-8" (response/status 200 "Hello world"))
kanwei commented 9 years ago

Is this an issue with ring-defaults then? Seems a bit insane to have this not working by default ;)

yogthos commented 9 years ago

I'm just looking through what it's doing, and it looks to be setting content-type wrapper to true for site-defaults here. Perhaps the behavior changed there. Have to investigate further.

Note that returning a string will work as expected:

(GET "/" [] "Hello world")

However, when you use response/status the result becomes a map, and since the map has no content type set, it appears to default to one that's not very useful.

yogthos commented 9 years ago

This is where the octet-stream content type comes from:

(defn content-type-response
  "Adds a content-type header to response. See: wrap-content-type."
  {:arglists '([response request] [response request options])
   :added "1.2"}
  [resp req & [opts]]
  (if (get-header resp "Content-Type")
    resp
    (let [mime-type (ext-mime-type (:uri req) (:mime-types opts))]
      (content-type resp (or mime-type "application/octet-stream")))))

https://github.com/ring-clojure/ring/blob/a39825e6d1050f915ebffefaf8c3ae4ac1434adf/ring-core/src/ring/middleware/content_type.clj#L14

devasiajoseph commented 9 years ago

Another issue is, I am not able to serve cache manifest file in resources/public. I see the extension "appcache" is mapped to "text/cache-manifest" in https://github.com/ring-clojure/ring/blob/master/ring-core/src/ring/util/mime_type.clj#L10 . I see (wrap-file-info) https://github.com/ring-clojure/ring/blob/master/ring-core/src/ring/middleware/file_info.clj#L56 is supposed to be handling content types. Currently I am using lib-noir content type setting to serve this file. Wondering anyone else had this issue?

weavejester commented 9 years ago

The site-default configuration makes some compromises to convenience in the name of security. In this case, I believe you're being hit by this subset of the configuration:

{:security  {:content-type-options :nosniff}
 :responses {:content-types true}}

The :content-type-options key tells the browser not to second-guess the content type, but to accept whatever the server returns. The :content-types key adds the content-type middleware, which defaults to application/octet-stream if no content-type is supplied in the response, and it cannot guess the content-type from the extension.

What this means is that you need to ensure that your responses have content-types, otherwise you'll run into problems. You can always turn the above options off, but it's usually better to treat problems than ignore them, and responses should really have valid content-types.

Compojure will treat string responses as text/html, and Compojure 1.2 is able to guess the mimetypes of file and resource responses in many cases. However, map responses will be sent as is, so you'll need to attach a content-type manually.

@devasiajoseph, your problem might be that you're using Ring 1.3.1, while the appcache extension was only added to the defaults in 1.3.2.

Frozenlock commented 9 years ago

I have the same problem here.

What's weird is that it doesn't happen in a simple lein run. It only happens once everything is uberjarred and (from what I can tell) for a single page: the homepage "/".

Every other page works fine, but "/" will always download, no matter what I put in it. Even something as simple as this doesn't work:

(c/defroutes home
  (c/GET "/" [] "hello world"))

Curl returns this:

HTTP/1.1 200 OK
Date: Fri, 12 Dec 2014 01:39:07 GMT
X-Content-Type-Options: nosniff
X-XSS-Protection: 1; mode=block
Last-Modified: Fri, 12 Dec 2014 01:38:12 GMT
Content-Length: 0
Content-Type: application/octet-stream
Server: Jetty(7.x.y-SNAPSHOT)

As you can see, no content at all.

Frozenlock commented 9 years ago

Ok, after talking with @weavejester, he suggested I enter

[ring/ring-core "1.3.2"]

explicitly in my dependencies. It worked!

Apparently, one of my other deps was overriding the ring-core version.

yogthos commented 9 years ago

ah let me update ring dependency in lib-noir and make a new release

yogthos commented 9 years ago

@Frozenlock ok all the lib-noir dependencies should be at latest versions