Skip to content
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

Convert http-USO to https-USO to enable updating #1800

Closed
wants to merge 1 commit into from

Conversation

Rob--W
Copy link

@Rob--W Rob--W commented Sep 6, 2013

Several scripts are stuck at an old version because their downloadURL points to the http-version of USO instead of https, while extension.greasemonkey.requireSecureUpdates is set to true by default.

Since it is known in advance that USO is able to server content over https, we can safely convert all USO downloadURLs to the https-version.

The limitation that has been fixed affects over 6k scripts:

Several scripts are stuck at an old version because their downloadURL
points to the http-version of USO instead of https, while
extension.greasemonkey.requireSecureUpdates is set to true by default.

Since it is known in advance that USO is able to server content over
https, we can safely convert all USO downloadURLs to the https-version.
arantius added a commit to arantius/greasemonkey that referenced this pull request Sep 27, 2013
@arantius
Copy link
Collaborator

Thanks for your suggestion! I decided to implement this by using the setters for those properties.

@arantius arantius closed this Sep 27, 2013
@Rob--W
Copy link
Author

Rob--W commented Sep 27, 2013

@arantius Unfortunately, the feature has not been implemented correctly. I followed the following steps:

  1. I checked out from your master branch
  2. I built the xpi (build.sh) and installed the new version in a new Firefox profile.
  3. I went to USO and installed an old version of a script from USO that had a http-update/download URL
  4. I navigated to the list of installed user script (at the add-ons tab).
  5. I right-clicked on the just-installed user script, and noticed that the "Find updates" context menu is still disabled.

What I expected was that the "Find updates" context menu entry was available, and on click it should get the latest version of the user script from USO.

You've fixed the behavior for new scripts, but old scripts are still affected. That's why I added a special case-check for USO to your source code.

@arantius
Copy link
Collaborator

Can you confirm exactly which script you were testing with?

@arantius arantius reopened this Sep 27, 2013
@Rob--W
Copy link
Author

Rob--W commented Sep 27, 2013

@arantius

I've tested the feature with http://userscripts.org/scripts/version/112070/619679.user.js (released in July, see
http://userscripts.org/scripts/versions/112070 for the release history). On 19 August, I replaced the URL with a https-URL (http://userscripts.org/scripts/diff/112070/629085)

To verify the bug, you can take any script that has an old release with a http-@downloadURL (ideally, the release needs to be old, so you can see that the script is actually being updated.).

arantius added a commit to arantius/greasemonkey that referenced this pull request Sep 27, 2013
@arantius
Copy link
Collaborator

Confirmed that specifically an "old version" from us.o does not behave as expected. Thanks for the testing. I missed a few references to the private property being set directly, and have now fixed them.

@Rob--W
Copy link
Author

Rob--W commented Sep 27, 2013

Confirmed to work as expected.

I can still see a few references to _downloadURL in the source code, you might want to check these as well:

modules/addons4.js:286:  var rs = new RemoteScript(this._script._downloadURL);
modules/parseScript.js:112:        scriptRequire._downloadURL = reqUri.spec;
modules/parseScript.js:146:        scriptResource._downloadURL = resUri.spec;
modules/scriptIcon.js:47:    var resUri = GM_util.uriFromUrl(this._script._downloadURL);
modules/scriptIcon.js:48:    this._downloadURL = GM_util.uriFromUrl(value, resUri).spec;
modules/scriptDependency.js:9:  this._downloadURL = null;
modules/scriptDependency.js:40:  return '' + (this._downloadURL || '');
modules/script.js:41:  this._downloadURL = null;
modules/script.js:144:function Script_getDownloadUrl() { return this._downloadURL; });
modules/script.js:146:function Script_setDownloadUrl(aVal) { this._downloadURL = usoFix(aVal); });
content/config.js:230:          script.textContent, GM_util.uriFromUrl(script._downloadURL));

@arantius
Copy link
Collaborator

Thanks!

There is indeed two other missed references in there. The rest are other objects with properties of the same name (or the actual setter/getter impl), which is why I missed any to begin with. Leaving this open to remember to come back and fix those too.

arantius added a commit to arantius/greasemonkey that referenced this pull request Oct 2, 2013
@arantius arantius closed this Oct 2, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants