Provide backwards compatibility with 7.x-1.x implementations

It really makes no sense to only pass the security token via POST while the module and callback are extracted from the URL. Ultimately, this really just creates confusing code for anyone doing 7.x-2.x implementations. It's better to just keep it all together in the JS code that initializes the POST data.

Furthermore, in 7.x-2.x, the URL is specially only ever used in GET requests (in 7.x-1.x everything was GET). This is now used for implementing things like modals that allow just the content of a page to be returned via JSON using the normal menu router in Drupal. Because GET requests are just like a user requesting a page in a browser, it doesn't require the generated token for validating the "callback".

I'm marking "won't fix" because 7.x-2.x was very specifically designed this way for security reasons. It ensures that previous 1.x implementations are upgraded properly to utilize the new token and POST request methods (as the previous GET implementations wouldn't work anyway).

This creates definition and solidarity between what is returned based on which request method is used. That is why there was a major version bump, because it does break backwards compatibility and isn't compatible with 7.x-1.x.

This will be the first time a simple 5 line patch of mine is rejected because it gives users backward compatibility with a 7.x-1.x version of a module, while not making a single compromise on 7.x-2.x

I'm sorry you feel this way. This isn't personal.

A consequence of your doctrine is that all users of the JS module not only need to upgrade their PHP but also their Javascripts.

As stated prior, this was partly done per the security review (not by me, but rather a separate individual/member of the Drupal security team). By making the request methods explicit as well as forcing POST parameters for the module/callback/token, this ensures that requests are secure by default.

If we were to make the 7.x-2.x backwards-compatible with the 7.x-1.x branch, we'd essentially be allowing people to "choose" which version of this module they'd want to install (which also leads to the difficulty of supporting both versions for both of our modules).

Because the 7.x-1.x branch is considered severely insecure, one could (in theory) install it by accident (especially considering the 7.x-2.x is still under development/review before a new release is made). Believe me, I wasn't fond of the idea at first either, but given the security reasons/implications presented to me when developing the 7.x-2.x branch (for a high profile client btw which has rigorous security policies), I really had no choice in the matter.

Previously people's javascript didn't have to know whether it was going to be used in a JS-module context or not.

Exactly. Previously, people's JS was unaware of this module at all. If one looked at the JS, no one would suspect that there was some "special" logic that was used on the back-end. Now, one has to explicitly define that their JS supports this module (which allows people who read the code to track down this module faster); i.e.: there is no more "magic" happening, instead it allows for something along the lines of "pass these extra parameters for the JS module because we actively support it". Code that clearly shows what is being done is far preferable in d.o's coding standards.

By rejecting this patch an unnecessary upgrade burden is placed on all the developers and site-builder using it.

No, it is not "unnecessary"; it is very necessary... hence the major version bump to 7.x-2.x. I didn't design the 7.x-1.x version of the module. It was built out of necessity (faster AJAX requests), sure, but there were also some very serious flaws with its original design.

The 7.x-2.x version is a complete rewrite and API change of the module (basically from the ground up). It is not a simple "bug fix/new features" branch. I did try to keep some familiarity and similarities with the 7.x-1.x branch, but the majority of it is nothing like it was (internally at least). Modules sometimes undergo some really strong scrutiny (especially when backed by client needs) and are ultimately changed as a result. It happens, but that's why we get paid to do what we do (or at least I do).

And it is quite significant as the current documentation doesn't mention any of this -- it took me a long debugging session to the get to the bottom of this.

I will agree and this is indeed a fair assessment. The documentation for the 7.x-2.x branch needs significant improvement. I documented in the code of the module quite extensively. Now it is more of a matter of consolidating that into a single area as well as providing a proper 7.x-1.x to 7.x-2.x upgrade path. Unfortunately, I have had other priories (namely recently moving as well as maintaining other projects) that have prevented me from completing a rather menial (yet highly important) task. FWIW, I would be far more receptive to patches that fix the documentation rather than trying to get an antiquated "feature" from 7.x-1.x back.

I'm all for security where it is needed, but many simple AJAX use-cases do not require rigorous security, permissions and the complexity that comes with it. Mostly we just want to post some totally insensitive data from the browser to the server and have that processed in a super-lean manner, without tokens, bells and whistles that make this module harder to use and can possibly slow it down, thereby defeating what this wonderful module set out to do in the first place.

These statements are just not entirely fair or accurate. Many times the most invasive and potentially damaging attacks come from some of the most obscure places (hidden elements like image/script paths or arbitrary code that executes an invisible AJAX request). Developers are often unaware of just how damaging an AJAX request can potentially be, especially if it's already bypassing more of core's bootstrap process (as this module does).

In 7.x-1.x there were absolutely no checkpoints in place at all. A request could, in theory, do whatever it wanted. Couple that with a poorly designed back-end callback (i.e. processing the raw request and then invoking a DB query for instance based on passed parameters, for instance) could allow an attacker unlimited access to the DB with a cleverly formed AJAX request.

Now, I'm not saying that your module does this (in fact, I really do not know). However, I think you're missing the point of why this has been implemented and done this way: ensure, by default (out-of-the-box), that this module is secure.

If, for instance, a module just needs to provide an extra speed boost because their callback just returns some sort of static/API data (doesn't touch the Drupal DB), then they can certainly opt-out of this security by disabling the token and xss parameters of their callback (js.api.php (line 56)).

What about part 2 of the patch, the one-line change to restore messages output with drupal_set_message() -- is that also rejected?

Yes because this is part of the new js_deliver_json callback, which js.js relies on because the request is never run through core's ajax API (not that it was in 7.x-1.x either though... so curious why you would need it here).

In 7.x-2.x, callbacks can now specify a delivery callback parameter, if needed (which is sounds like you might want).

---

I really hope you don't take me closing this issue personally. It just has more to do with providing a better, cleaner, more secure API that allows for module customization based on needs rather than assumptions. By default the 7.x-2.x branch should always allow for easy, fast and secure implementations of JS callbacks.

When it comes to upgrading existing code, I can see how this might be more of an issue, but I did my best to make sure there are things in place to make it less painful. Hopefully I've outlined a few that you can utilize? I know it's not quite "documentation" per se, but it's a start. This is why I opened that issue in the first place, to see where the pain points are. I'm certainly open to making this process better, but I cannot nor will not sacrifice the security aspect of the 7.x-2.x branch.

I didn't know that 7.x-1.x was considered "severely insecure" to the point that you wanted people to stop using it. The project page perhaps hints at this, but doesn't go quite so far as making that statement. And we didn't get a security alert for 7.x-1.x. Or did I miss it?

No, you didn't miss it. Technically, there was never an "official" report. Also any report would have been rather ambiguous as this module is an API an every module's implementation is different. The entire concept of the module was considered insecure (mainly because there was no token system in place to validate a request). In reality, once 7.x-2.x is out, we'll likely do some sort of "official" statement here.

The process that lead to developing the 7.x-2.x branch was more along the lines of:

1. Client needed faster AJAX requests.
2. We decided to use this module but the client's security policy prevented using it because it was too insecure.
3. I implemented a complete rewrite with the supervision of a couple co-workers who provided security reviews (and who are subsequently also on the security team).
4. Lack of documentation of these API changes and upgrade path has lead to this issue.

Ultimately, yes. I would like to get the 7.x-2.x branch relatively stable so it can be released and 7.x-1.x is just a distant memory. However, that goal is still in process and not yet ready until more things like this can be flushed out.

I look forward to your documentation patches :)