fix(web-server): strip scheme, host and port #899

Merged
merged 1 commit into from Apr 23, 2014

2 participants

@fredrikbonander

Absolute URI requests are striped to match files in middlewares.
AbsoluteURIs are use from proxies according to Section
5.1.2

PR to socketio/socket.io#1304 is needed to fully resolve this issue
karma-runner/karma#705

@vojtajina vojtajina commented on an outdated diff Jan 30, 2014
@@ -6,6 +6,7 @@ tmp/*
docs/_build
*.swp
*.swo
+coverage/

Revert this please, it has nothing to do with this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@vojtajina vojtajina commented on an outdated diff Jan 30, 2014
test/unit/middleware/karma.spec.coffee
@@ -81,7 +81,6 @@ describe 'middleware.karma', ->
callHandlerWith '/'
-

Revert these whitespace changes please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@vojtajina vojtajina and 1 other commented on an outdated diff Jan 30, 2014
lib/middleware/karma.js
@@ -37,7 +37,12 @@ var createKarmaMiddleware = function(filesPromise, serveStaticFile,
/* config.basePath */ basePath, /* config.urlRoot */ urlRoot) {
return function(request, response, next) {
- var requestUrl = request.url.replace(/\?.*/, '');
+ var requestUrl = (request.normalizedUrl || request.url).replace(/\?.*/, '');

Why this? request.normalizedUrl is always defined, no?

Added it as a safety if the strip middleware should not be used/present (like during tests)

Can you update the tests and remove this?

I don't like having some code in production code that is there just because of tests. That just feels wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@vojtajina vojtajina and 1 other commented on an outdated diff Feb 14, 2014
lib/middleware/karma.js
@@ -37,7 +37,12 @@ var createKarmaMiddleware = function(filesPromise, serveStaticFile,
/* config.basePath */ basePath, /* config.urlRoot */ urlRoot) {
return function(request, response, next) {
- var requestUrl = request.url.replace(/\?.*/, '');
+ var requestUrl = request.normalizedUrl.replace(/\?.*/, '');
+
+ // Strip scheme, host and port for requests with absolute URI

Remove this, please.

Sorry, remove what? The comment?

Why are you stripping the host/port again in Karma middleware?

Sorry. Merge conflict.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@vojtajina vojtajina commented on the diff Feb 14, 2014
test/unit/middleware/strip_host.spec.coffee
+
+ it 'should strip request with absoluteURI over HTTPS', (done) ->
+ request = new HttpRequestMock('https://karma-runner.github.io/base/a.js?123345')
+ handler request, null, nextSpy
+
+ expect(request.normalizedUrl).to.equal '/base/a.js?123345'
+ expect(nextSpy).to.have.been.called
+ done()
+
+ it 'should return same url as passed one', (done) ->
+ request = new HttpRequestMock('/base/b.js?123345')
+ handler request, null, nextSpy
+
+ expect(request.normalizedUrl).to.equal '/base/b.js?123345'
+ expect(nextSpy).to.have.been.called
+ done()

Can you add the empty line?

What empty line? At the bottom?

yep, it's good now, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@vojtajina vojtajina commented on the diff Feb 14, 2014
test/unit/web-server.spec.coffee
@@ -59,6 +59,11 @@ describe 'web-server', ->
requestPath '/'
+ it 'should serve client.html with a absoluteURI request path ', (done) ->

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@vojtajina vojtajina commented on an outdated diff Feb 14, 2014
test/unit/middleware/karma.spec.coffee
@@ -39,8 +39,13 @@ describe 'middleware.karma', ->
servedFiles = (files) ->
filesDeferred.resolve {included: [], served: files}
+ mockHttpRequest = (urlPath) ->

Can you call it normalizedHttpRequest?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@vojtajina vojtajina merged commit 06a0da0 into karma-runner:master Apr 23, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment