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

Sanitize URL pattern from rootPath #785

Merged
merged 1 commit into from Apr 16, 2015

Conversation

Projects
None yet
7 participants
@Toilal
Contributor

Toilal commented Nov 20, 2014

This is related to #784

@coveralls

This comment has been minimized.

coveralls commented Nov 20, 2014

Coverage Status

Coverage remained the same when pulling 6833104 on Toilal:rootPath into be79100 on dropwizard:master.

@@ -117,8 +117,8 @@ shutdownGracePeriod 30 seconds The maxi
to cleanly shutdown before forcibly terminating them.
allowedMethods ``GET``, ``POST``, ``PUT``, ``DELETE``, The set of allowed HTTP methods. Others will be rejected with a
``HEAD``, ``OPTIONS``, ``PATCH`` 405 Method Not Allowed response.
rootPath ``/`` The URL pattern relative to ``applicationContextPath`` from which
the JAX-RS resources will be served.
rootPath ``/*`` The URL Pattern relative to ``applicationContextPath`` from which

This comment has been minimized.

@joschi

joschi Nov 24, 2014

Member

Why did you capitalize "pattern"?

rootPath ``/`` The URL pattern relative to ``applicationContextPath`` from which
the JAX-RS resources will be served.
rootPath ``/*`` The URL Pattern relative to ``applicationContextPath`` from which
the JAX-RS resources will be served. It must end with a wildcard.
====================== =============================================== =============================================================================

This comment has been minimized.

@joschi

joschi Nov 24, 2014

Member

The trailing wildcard is automatically removed (see https://github.com/dropwizard/dropwizard/blob/v0.8.0-rc1/dropwizard-jersey/src/main/java/io/dropwizard/jersey/DropwizardResourceConfig.java#L149-151), so this is only a visual "helper" so that it looks more like an URL pattern people know from the Java Servlet API.

This comment has been minimized.

@Toilal

Toilal Nov 24, 2014

Contributor

It doesn't work when setting / as the rootPath, nor anything else not ending with *. Dropwizard is starting, logs seems OK, but jersey allways throws 404 error.

I thought it was an error in docs, but from what you are telling me, it's a bug and what's in the documentation is valid and should work.

@@ -393,13 +393,13 @@ Serving Assets
Either your application or your static assets can be served from the root path, but
not both. The latter is useful when using Dropwizard to back a Javascript
application. To enable it, move your application to a sub-URL.
application. To enable it, move your jersey resources to a sub-URL.

This comment has been minimized.

@joschi

joschi Nov 24, 2014

Member

Please capitalize "Jersey".

This comment has been minimized.

@skamille

skamille Jan 12, 2015

Member

@Toilal did you get this note?

This comment has been minimized.

@Toilal

Toilal Jan 12, 2015

Contributor

Thanks @skamille to remind me this not, i'll push the change now.

@Toilal

This comment has been minimized.

Contributor

Toilal commented Nov 30, 2014

@joschi

* is removed from rootPath parameter for DropWizard endpoints.

But DropwizardResourceConfig#getUrlPattern() is called by AbstractServerFactory#createAppServlet(). This Url pattern is the unmodified one, loaded by rootPath parameter.

So the default path in docs /, and any other path without any ending /* will pass validation, but servlet won't be registered properly in Jersey.

I'll update my pull request to fix this issue instead of modifying the docs to make it work.

@Toilal Toilal force-pushed the Toilal:rootPath branch from 6833104 to 67fe6b5 Nov 30, 2014

@coveralls

This comment has been minimized.

coveralls commented Nov 30, 2014

Coverage Status

Coverage increased (+0.02%) when pulling 67fe6b5 on Toilal:rootPath into 07fe33c on dropwizard:master.

@Toilal Toilal changed the title from Documentation enhancements to Sanitize URL pattern from rootPath Nov 30, 2014

@Toilal Toilal force-pushed the Toilal:rootPath branch 2 times, most recently from 9972d10 to 3fbfd15 Jan 12, 2015

@coveralls

This comment has been minimized.

coveralls commented Jan 12, 2015

Coverage Status

Coverage increased (+0.01%) when pulling 3fbfd15 on Toilal:rootPath into 8a1d9ca on dropwizard:master.

@coveralls

This comment has been minimized.

coveralls commented Jan 12, 2015

Coverage Status

Coverage increased (+0.01%) when pulling 3fbfd15 on Toilal:rootPath into 8a1d9ca on dropwizard:master.

@Toilal

This comment has been minimized.

Contributor

Toilal commented Jan 22, 2015

Is it plan to merge this one ?

@jplock

This comment has been minimized.

Member

jplock commented Mar 9, 2015

Is this still an issue now that DW 0.8 has been released?

@Toilal

This comment has been minimized.

Contributor

Toilal commented Mar 9, 2015

I just gave a try on my webapp with DW 0.8, and issue is still there.

My configuration is

server:
  type: simple
  rootPath: /api/*
  applicationContextPath: /
  adminContextPath: /admin
  connector:
    type: http
    port: 35180

When setting rootPath: /api/, resources are not loaded. (Adding * is the current workaround).

@jplock

This comment has been minimized.

Member

jplock commented Mar 14, 2015

@joschi do you have any thoughts on merging this one in?

@carlo-rtr

This comment has been minimized.

Member

carlo-rtr commented Apr 15, 2015

I took a look, and it seems the trailing * is required when providing a the path spec to Handler.addServlet, similar to this guide here: https://wiki.eclipse.org/Jetty/Tutorial/Embedding_Jetty

@@ -459,7 +459,14 @@ protected Handler createAppServlet(Server server,
handler.addFilter(holder, "/*", EnumSet.allOf(DispatcherType.class));
}
if (jerseyContainer != null) {
jersey.setUrlPattern(jerseyRootPath);
String urlPattern = jerseyRootPath;
if (!urlPattern.endsWith("/")) {

This comment has been minimized.

@carlo-rtr

carlo-rtr Apr 15, 2015

Member

Unless I'm reading this wrong, if the user provide a rootPath ending with * this will add another /, then it will add another *. We shouldn't add the slash if it already ends with *

This comment has been minimized.

@Toilal

Toilal Apr 16, 2015

Contributor

👍

I've rebased on current master, and fixed this point.

@carlo-rtr carlo-rtr self-assigned this Apr 15, 2015

@Toilal Toilal force-pushed the Toilal:rootPath branch from 3fbfd15 to 116471a Apr 16, 2015

@carlo-rtr

This comment has been minimized.

Member

carlo-rtr commented Apr 16, 2015

Thank you.

carlo-rtr added a commit that referenced this pull request Apr 16, 2015

Merge pull request #785 from Toilal/rootPath
Sanitize URL pattern from rootPath

@carlo-rtr carlo-rtr merged commit f42f8c6 into dropwizard:master Apr 16, 2015

@jplock jplock added this to the 0.9.0 milestone Apr 16, 2015

@arteam arteam added the bug label Apr 26, 2015

@Toilal Toilal deleted the Toilal:rootPath branch May 21, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment