rstacruz / sinatra-assetpack

Package your assets transparently in Sinatra.
http://ricostacruz.com/sinatra-assetpack/
MIT License
541 stars 97 forks source link

Rack::Auth::Basic, SSL, package name may results in empty files or cash #68

Open stefang opened 11 years ago

stefang commented 11 years ago

Slightly odd issue.

When I use Rack::Auth::Basic in a Sinatra app (classic) Asset Pack appears to deliver empty files. Works perfectly locally. Remove the Auth::Basic lines and it starts working on Heroku again as expected.

If I remove the Auth the assets compile perfectly. Image asset serving appears unaffected. This is affecting JS and SASS files only.

For reference I'm using the basic example from the Sinatra README

use Rack::Auth::Basic, "Protected Area" do |username, password| username == 'username' && password == 'password' end

Regards, Stefan

j15e commented 11 years ago

Which version are you using?

Could you try 0.1.0 vs 0.0.11?

0.1.0 was released yesterday and introduced at lot of bugfixes

stefang commented 11 years ago

Hi,

"Using sinatra-assetpack (0.1.0)"

Actually only started using it yesterday :-)

j15e commented 11 years ago

Ok, I will have a look at this when I got time, but now right now.

If someone may submit either a failing test or a patch I could have a look at it faster.

(and thanks for reporting issue)

rstacruz commented 11 years ago

I might know why this is. the auth middleware is probably barring access to the CSS/Js files.

On Tuesday, January 15, 2013, Jean-Philippe Doyle wrote:

Ok, I will have a look at this when I got time, but now right now.

If someone may submit either a failing test or a patch I could have a look at it faster.

(and thanks for reporting issue)

— Reply to this email directly or view it on GitHubhttps://github.com/rstacruz/sinatra-assetpack/issues/68#issuecomment-12268641.

rstacruz commented 11 years ago

The way CSS/js combination/compression works is that it simulates GET requests to retrieve each of the CSS/js files it contains. Auth::Basic is preventing this from happening in production, perhaps.

I'd try to make your Auth middleware have an exception to let assets be served without the need for auth.

On Tuesday, January 15, 2013, Rico Notifications wrote:

I might know why this is. the auth middleware is probably barring access to the CSS/Js files.

On Tuesday, January 15, 2013, Jean-Philippe Doyle wrote:

Ok, I will have a look at this when I got time, but now right now.

If someone may submit either a failing test or a patch I could have a look at it faster.

(and thanks for reporting issue)

— Reply to this email directly or view it on GitHubhttps://github.com/rstacruz/sinatra-assetpack/issues/68#issuecomment-12268641.

j15e commented 11 years ago

Thanks for the input @rstacruz !

If we would make a rack middleware (#16) would that help fixing this issue? Maybe fonts not being served are related to this too?

rstacruz commented 11 years ago

Hmmm.

I think asset pack can be used as a sort of middleware by making a Sinatra::Base subclass that has nothing but assetpack. Sinatra apps can be used as middleware.

I do agree though that this process can be streamlined!

On Jan 15, 2013, at 10:26 PM, Jean-Philippe Doyle notifications@github.com wrote:

Thanks for the input @rstacruz https://github.com/rstacruz !

If we would make a rack middleware (#16https://github.com/rstacruz/sinatra-assetpack/issues/16) would that help fixing this issue? Maybe fonts not being served are related to this too?

— Reply to this email directly or view it on GitHubhttps://github.com/rstacruz/sinatra-assetpack/issues/68#issuecomment-12269137.

j15e commented 11 years ago

I would guess it is related to the new rack session built to combine assets here : https://github.com/rstacruz/sinatra-assetpack/blob/master/lib/sinatra/assetpack/package.rb#L96

Why go trought http requests to build comined assets package? I guess this could be done internaly.

rstacruz commented 11 years ago

Because sometimes you may want to package assets that are not managed by AssetPack. Say, JSTs with sinatra-backbone. :-)

Rico Sta. Cruz ricostacruz.com • nadarei.co

T: +63 (908) 864 8125 Skype: rico.sta.cruz Gtalk: rico@ricostacruz.com

On Mon, Jan 21, 2013 at 12:27 PM, Jean-Philippe Doyle < notifications@github.com> wrote:

I would guess it is related to the new rack session built to combine assets here : https://github.com/rstacruz/sinatra-assetpack/blob/master/lib/sinatra/assetpack/package.rb#L96

Why go trought http requests to build comined assets package? I guess this could be done internaly.

— Reply to this email directly or view it on GitHubhttps://github.com/rstacruz/sinatra-assetpack/issues/68#issuecomment-12484228.

j15e commented 11 years ago

Ok thanks @rstacruz, I think we could keep it as a fallback but could deal internally when possible.

ujifgc commented 11 years ago

Is it possible to let the user configure serving and building assets? If so, I would like to work on this.

I suggest using serve '/stylesheets', from: '../assets/stylesheets', with: :rack or serve '/stylesheets', from: '../assets/stylesheets', with: :file with defaulting to :rack.

The code trying to do rack requests is here: https://github.com/rstacruz/sinatra-assetpack/blob/master/lib/sinatra/assetpack/package.rb#L106

j15e commented 11 years ago

There is also the builder which use a rack session to build files with rake :

https://github.com/rstacruz/sinatra-assetpack/blob/master/lib/sinatra/assetpack/builder.rb#L12

Obviously the ideal flow would be to always use file when possible and fallback to a rack request only if file is non-existant. This is currently impossible to do as the way served assets paths are defined is through the routes in class_methods.rb and a lot of core logic if defined there.

I think it was done this way in the first place since it less complex to manage assets routes & assets packaging in the same place. Building them was added on top of it, using the same concept of simply calling routes with a rack session for each defined package & asset.

What we should do is extract the logic defined in the routes so it can be used outside of a request context (ex. to avoid using rack sessions which introduce problems when your app is behind basic auth or SSL or whatnot). This is not an easy task as it would require a complete rewrite of a core part of assetpack. I do plan on doing so when I have the time and contributions are welcome.

j15e commented 11 years ago

In other words, class_methods.rb should be rewrite & split into something like routes.rb & assets.rb.

There is also a lot of code in options.rb that are not really "option" related but more like "serving assets" related (local_file_for, fetch_dynamic_asset, dyn_local_file_for, etc) that should fit into an assets module.

mikehale commented 10 years ago

I ran into this issue because my app requires SSL in production. I was able to work around it by modifying the Rack::Test::Session::default_env: https://gist.github.com/mikehale/11193614