jersey
  1. jersey
  2. JERSEY-1535

Jersey unconditionally wraps Javascript into a JSON-P callback

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0-m09
    • Fix Version/s: 2.0
    • Component/s: media
    • Labels:
      None

      Description

      We're making a pure-JAXRS application, therefore serving JS static files from JAX-RS resources, something like the following:

        @GET
        @Path("bootstrap/js/bootstrap.min.js")
        @Produces("application/javascript")
        public Response bootstrapMinJs() {
          return getClass().getResourceAsStream("/com/twitter/bootstrap/js/bootstrap.min.js");
        }
      

      Jersey wraps the file content into callback(, producing invalid JavaScript:

      callback(var foo = "bar"; window.alert(foo);)
      

      Looking at the code, JsonWithPaddingInterceptor also replaces the Content-Type with application/json, which is plain wrong: it's about treating some JSON payload as if it were a JS object literal and passing it as argument to a JS function, in other words producing JavaScript from JSON, not the other way around.

      The wrapping should be conditioned to the presence of the @JSONP annotation so as to not rewrite (and break) JavaScript code. It could maybe also only trigger if the media type is application/json, replacing it with application/javascript (and not the other way around as it does today).

      Workaround: use @Produces("text/plain") to avoid triggering the JsonWithPaddingInterceptor, as browsers actually don't care about the Content-Type anyway, as long as it's loaded through a <script src=""></script>.

        Activity

        Hide
        t.broyer added a comment -

        I see this has been set as 2.x-backlog, which I understand as being about fixing the interceptor; but can it at least be disabled in 2.0?
        In all honestly, shipping the interceptor in its current state in 2.0 would be unacceptable.

        Furthermore, the @Produces("text/plain") workaround won't work if you want to also set X-Content-Type-Options: nosniff (see https://code.google.com/p/chromium/issues/detail?id=180007#c11) in which case you'll have to use a really ugly workaround (add an interceptor with a lower priority than JsonWithPaddingInterceptor to change the mimetype to a dummy one so that the JsonWithPaddingInterceptor rewriting is not triggered, and another interceptor with a higher priority than than JsonWithPaddingInterceptor to reset the mimetype to JS.

        Show
        t.broyer added a comment - I see this has been set as 2.x-backlog, which I understand as being about fixing the interceptor; but can it at least be disabled in 2.0? In all honestly, shipping the interceptor in its current state in 2.0 would be unacceptable. Furthermore, the @Produces("text/plain") workaround won't work if you want to also set X-Content-Type-Options: nosniff (see https://code.google.com/p/chromium/issues/detail?id=180007#c11 ) in which case you'll have to use a really ugly workaround (add an interceptor with a lower priority than JsonWithPaddingInterceptor to change the mimetype to a dummy one so that the JsonWithPaddingInterceptor rewriting is not triggered, and another interceptor with a higher priority than than JsonWithPaddingInterceptor to reset the mimetype to JS.
        Hide
        t.broyer added a comment -

        Pull-request (temporarily) removing the feature: https://github.com/jersey/jersey/pull/13

        Show
        t.broyer added a comment - Pull-request (temporarily) removing the feature: https://github.com/jersey/jersey/pull/13
        Hide
        Marek Potociar added a comment -

        I do not understand - why you want to disable the interceptor? The feature is not mandatory. I.e. you do not need to register the interceptor or use it in your code.

        Show
        Marek Potociar added a comment - I do not understand - why you want to disable the interceptor? The feature is not mandatory. I.e. you do not need to register the interceptor or use it in your code.
        Hide
        Marek Potociar added a comment -

        Moving to 2.0 for investigation.

        Show
        Marek Potociar added a comment - Moving to 2.0 for investigation.
        Hide
        Michal Gajdos added a comment -

        We'll change the code to activate the interceptor only for methods annotated with @JSONP.

        Show
        Michal Gajdos added a comment - We'll change the code to activate the interceptor only for methods annotated with @JSONP .
        Hide
        t.broyer added a comment -

        My immediate problem is that the interceptor is registered in org.glassfish.jersey.server.ServerBinder and it's triggered for any @Produces("application/javascript") resource, therefore corrupting those scripts. In its current form, it's not optional; hence my proposal to simply remove it until it works in an acceptable way.

        IMO, the proper fix would be to mandate the use of @JSONP on the resource.

        Another issue with the interceptor (when you actually want to use it, which is not my case) is that it wraps (what it assumes is) JSON into callback( and ) to turn it into JavaScript (note that it might break, as some codepoints allowed in JSON are invalid in JavaScript, but that's yet another story), but it changes the Content-Type of the response from JavaScript to application/json, therefore serving the script with an inappropriate media type. I suppose this is done so that the response entity will be encoded with a JSON MessageBodyWriter, but then there should be another interceptor to set the Content-Type back to JavaScript afterwards, or something like that.

        Anyway, thanks for reconsidering the resolution of this bug.

        Show
        t.broyer added a comment - My immediate problem is that the interceptor is registered in org.glassfish.jersey.server.ServerBinder and it's triggered for any @Produces("application/javascript") resource, therefore corrupting those scripts. In its current form, it's not optional; hence my proposal to simply remove it until it works in an acceptable way. IMO, the proper fix would be to mandate the use of @JSONP on the resource. Another issue with the interceptor (when you actually want to use it, which is not my case) is that it wraps (what it assumes is) JSON into callback( and ) to turn it into JavaScript (note that it might break, as some codepoints allowed in JSON are invalid in JavaScript, but that's yet another story), but it changes the Content-Type of the response from JavaScript to application/json , therefore serving the script with an inappropriate media type. I suppose this is done so that the response entity will be encoded with a JSON MessageBodyWriter , but then there should be another interceptor to set the Content-Type back to JavaScript afterwards, or something like that. Anyway, thanks for reconsidering the resolution of this bug.
        Hide
        Michal Gajdos added a comment -

        Yes, you're right about the interceptor not being optional. We agreed to trigger the logic of the interceptor only if @JSONP annotation is present on the resource. Thanks for your notes, I'll use your points when fixing this issue.

        Note: The interceptor does not actually change the Content-Type of the response, it's just an info for underlying MessageBodyWriter}}s as you stated (it should not be reflected to the {{Response or have you seen corrupted Content-Type header in the response?).

        Show
        Michal Gajdos added a comment - Yes, you're right about the interceptor not being optional. We agreed to trigger the logic of the interceptor only if @JSONP annotation is present on the resource. Thanks for your notes, I'll use your points when fixing this issue. Note: The interceptor does not actually change the Content-Type of the response, it's just an info for underlying MessageBodyWriter}}s as you stated (it should not be reflected to the {{Response or have you seen corrupted Content-Type header in the response?).
        Hide
        t.broyer added a comment -

        > or have you seen corrupted Content-Type header in the response?

        Ah, honestly I can't remember…
        Corrupting the JavaScript was enough to upset me that I don't think I even checked the headers and I might simply have misread/misunderstood the interceptor's code.

        Show
        t.broyer added a comment - > or have you seen corrupted Content-Type header in the response? Ah, honestly I can't remember… Corrupting the JavaScript was enough to upset me that I don't think I even checked the headers and I might simply have misread/misunderstood the interceptor's code.
        Hide
        t.broyer added a comment -

        It ended up being a rather easy fix: https://github.com/jersey/jersey/pull/14

        Note that there might be an issue with character-encoding the way the interceptor is coded, which I didn't try to fix (what matters to me is that the interceptor doesn't corrupt my JS responses).

        Show
        t.broyer added a comment - It ended up being a rather easy fix: https://github.com/jersey/jersey/pull/14 Note that there might be an issue with character-encoding the way the interceptor is coded, which I didn't try to fix (what matters to me is that the interceptor doesn't corrupt my JS responses).

          People

          • Assignee:
            Michal Gajdos
            Reporter:
            t.broyer
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - Not Specified
              Not Specified
              Remaining:
              Remaining Estimate - 0 minutes
              0m
              Logged:
              Time Spent - 1 hour
              1h