From 260c46181d18f6e43d0a7a1e00a34211e21bc11c Mon Sep 17 00:00:00 2001 From: Piotr Sarnacki Date: Tue, 29 Jan 2013 03:13:01 +0100 Subject: [PATCH] Add hack to allow handling redirect to logs on the client properly. This hack is temporary and should be removed when we find better solution. TL;DR: we can't handle redirects to S3 using CORS, so in case we want to get logs from S3 without additional requests to API, we need to return status that will not be automatically redirected (in this case 204 seems like the best answer). Longer rationale: Old logs are hosted on S3 now and in case log is not available in the database, we would like to redirect to the archived log. Although S3 support CORS, our use case breaks on some browsers: * when request is triggered to /jobs/:id/log and log is archived, api returns 302 redirect, Location header points to the log on S3 * browser transparently redirects to given url, but it sets Origin to null, for security reasons * "Origin: null" is ok, because we allow every origin by setting AllowedOrigin to "*" * S3 returns "Access-Control-Allow-Origin: null" header, which breaks some browsers (I confirmed it for webkit based browsers) In order to fix this, S3 would need to return * in Access-Control-Allow-Origin header or we would need to tell the browser to not follow redirect. Both solutions are not achiveable. Another option would be to return log information in job payload - we could send log_url field which should be either log url on amazon or null, but in such case we would need to query artifacts table in each job request. This is something that should be avoided as archived logs are not frequently requested - slowing down every request to get info for it would be a waste. --- lib/travis/api/app/endpoint/jobs.rb | 10 +++++++++- spec/integration/v2/jobs_spec.rb | 9 +++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/lib/travis/api/app/endpoint/jobs.rb b/lib/travis/api/app/endpoint/jobs.rb index ed554324..3759316c 100644 --- a/lib/travis/api/app/endpoint/jobs.rb +++ b/lib/travis/api/app/endpoint/jobs.rb @@ -14,7 +14,15 @@ class Travis::Api::App get '/:job_id/log' do resource = service(:find_artifact, params).run if !resource || resource.archived? - redirect archive_url("/jobs/#{params[:job_id]}/log.txt"), 307 + archived_log_path = archive_url("/jobs/#{params[:job_id]}/log.txt") + + if params[:cors_hax] + status 204 + headers['Access-Control-Expose-Headers'] = 'Location' + headers['Location'] = archived_log_path + else + redirect archived_log_path, 307 + end else respond_with resource end diff --git a/spec/integration/v2/jobs_spec.rb b/spec/integration/v2/jobs_spec.rb index fa617d15..6df25881 100644 --- a/spec/integration/v2/jobs_spec.rb +++ b/spec/integration/v2/jobs_spec.rb @@ -40,5 +40,14 @@ describe 'Jobs' do response.should redirect_to("https://s3.amazonaws.com/archive.travis-ci.org/jobs/#{job.id}/log.txt") end end + + context 'with cors_hax param' do + it 'renders No Content response with location of the archived log' do + job.log.destroy + response = get "/jobs/#{job.id}/log.txt?cors_hax=true", {}, headers + response.status.should == 204 + response.headers['Location'].should == "https://s3.amazonaws.com/archive.travis-ci.org/jobs/#{job.id}/log.txt" + end + end end end