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

0.9.1 downloads instead of displays #107

Open
kanwei opened this issue Oct 8, 2014 · 20 comments
Open

0.9.1 downloads instead of displays #107

kanwei opened this issue Oct 8, 2014 · 20 comments

Comments

@kanwei
Copy link
Contributor

kanwei commented Oct 8, 2014

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
Copy link
Contributor

yogthos commented Oct 9, 2014

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
Copy link
Contributor

yogthos commented Oct 9, 2014

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
Copy link
Contributor

yogthos commented Oct 15, 2014

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

@kanwei
Copy link
Contributor Author

kanwei commented Oct 27, 2014

Nope, still doing the same thing..

@yogthos
Copy link
Contributor

yogthos commented Oct 27, 2014

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
Copy link

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
Copy link
Contributor

yogthos commented Nov 24, 2014

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
Copy link

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
Copy link
Contributor

yogthos commented Nov 26, 2014

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
Copy link

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
Copy link
Contributor

yogthos commented Nov 26, 2014

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
Copy link
Contributor Author

kanwei commented Nov 26, 2014

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

@yogthos
Copy link
Contributor

yogthos commented Nov 26, 2014

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
Copy link
Contributor

yogthos commented Nov 26, 2014

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
Copy link

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
Copy link

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
Copy link

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
Copy link

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
Copy link
Contributor

yogthos commented Dec 12, 2014

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

@yogthos
Copy link
Contributor

yogthos commented Dec 12, 2014

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

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

6 participants