Moodle
  1. Moodle
  2. MDL-32990

box.com/box.net repository using HTTP to access APIs, instead of HTTPS

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.1.6, 2.2.3, 2.3
    • Fix Version/s: 2.1.7, 2.2.4
    • Component/s: Repositories
    • Labels:
    • Testing Instructions:
      Hide
      1. Enable and add a box.net
      2. Go to my private files
      3. Use the file picker to add a file to moodle from box.net
      4. VERIFY: That thumbnails from box.net are displayed in the file picker when browsing
      5. VERIFY: Files can be selected and imported into private files without errors
      6. Save changes
      7. Download a file from private files which you uploaded from box.net
      8. VERIFY: that the file is the same as in box.net and is not corrupted
      Show
      Enable and add a box.net Go to my private files Use the file picker to add a file to moodle from box.net VERIFY: That thumbnails from box.net are displayed in the file picker when browsing VERIFY: Files can be selected and imported into private files without errors Save changes Download a file from private files which you uploaded from box.net VERIFY: that the file is the same as in box.net and is not corrupted
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-32990-master
    • Rank:
      40163

      Description

      We just received the following email from box.com:

      From: Alex Willen <awillen@box.com>
      Subject: HTTP Calls on the Box API

      Hi,

      I'm reaching out to let you know about a potential issue with your application's use of the Box API. We found that your application makes API requests over HTTP, rather than HTTPs. For security, soon we're going to begin enforcing HTTPs not only in our site, but also in our API.

      Previously we allowed HTTP calls to work for free users, although we still pushed for HTTPs calls in all cases, but we'll soon require HTTPs for all users. Starting 6/30/2012, all API calls will have to be made over HTTPs in order to function properly. The changes required from you should minimal - just be sure all your calls are pointing to https://www.box.com.

      We apologize for any inconvenience this causes. If you have any issues in making the change or would like to discuss, please let me know.

      Thanks,
      Alex Willen

      I looked at the boxnet repository code for Moodle 2.2 (stable) and 2.3 (master) and the API references are hard-coded to be "http://box.net/api/1.0". Hopefully the fix is just to change the api calls to use "https://box.com/api/1.0"?

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for reporting that.

          Show
          Michael de Raadt added a comment - Thanks for reporting that.
          Hide
          Jason Fowler added a comment -

          All done, requesting peer review

          Show
          Jason Fowler added a comment - All done, requesting peer review
          Hide
          Dan Poltawski added a comment - - edited
          1. We shouldn't need to do 'CURLOPT_SSL_VERIFYHOST'=>0, 'CURLOPT_SSL_VERIFYPEER'=>0, if box.net is not sending valid SSL certificates then that is a bug at their end. Our stack should be able to verify the certificates and indeed that is why they are enforcing the change to SSL.
          2. The issue description says: 'just be sure all your calls are pointing to https://www.box.com.', I assume they have changed their canonical name away from box.net, so your switch to https://box.net doesn't seem right (this is probably linked to my above point)
          3. Not in production code, please: error_log(print_r($params, true));
          4. Your whitespace change in
            $this->auth_token.'/'.$params['folder_id'];
            

            is incorrect. That is a 'single line' split into two, the indentation is to make that more visible

          5. Are you sure the non-api urls like the thumbnails are supposed to be changed to https (like the thumbnails?)
          Show
          Dan Poltawski added a comment - - edited We shouldn't need to do 'CURLOPT_SSL_VERIFYHOST'=>0, 'CURLOPT_SSL_VERIFYPEER'=>0 , if box.net is not sending valid SSL certificates then that is a bug at their end. Our stack should be able to verify the certificates and indeed that is why they are enforcing the change to SSL. The issue description says: 'just be sure all your calls are pointing to https://www.box.com .', I assume they have changed their canonical name away from box.net, so your switch to https://box.net doesn't seem right (this is probably linked to my above point) Not in production code, please: error_log(print_r($params, true)); Your whitespace change in $ this ->auth_token.'/'.$params['folder_id']; is incorrect. That is a 'single line' split into two, the indentation is to make that more visible Are you sure the non-api urls like the thumbnails are supposed to be changed to https (like the thumbnails?)
          Hide
          Jason Fowler added a comment -

          fixed all that up now Dan, can you just give it another look over please? Then it should be good to go

          Show
          Jason Fowler added a comment - fixed all that up now Dan, can you just give it another look over please? Then it should be good to go
          Hide
          Dan Poltawski added a comment -

          Removing the security issue flag on this as its an improvement really.

          Show
          Dan Poltawski added a comment - Removing the security issue flag on this as its an improvement really.
          Hide
          Dan Poltawski added a comment -

          Thanks,

          I've expanded the testing instructions to verify more explicitly.

          Looks good.

          Show
          Dan Poltawski added a comment - Thanks, I've expanded the testing instructions to verify more explicitly. Looks good.
          Hide
          Jason Fowler added a comment -

          Thanks Dan, pushing for integration now

          Show
          Jason Fowler added a comment - Thanks Dan, pushing for integration now
          Hide
          Aparup Banerjee added a comment -

          Hi,
          just had a look at this :

          • the urls in this file are all over the place. perhaps its worth a slight tidying up.
            inconsistency : 'https://www.box.com/api/1.0/auth/' & 'https://www.box.com/api/1.0/download/' used verbatim but $_box_api_upload_url defined for one use. suggest creating $_box_api_auth_url etc. surely we can centralis the string used everywhere here just a bit more .

          Thats all, with that this should be good for integration.

          Show
          Aparup Banerjee added a comment - Hi, just had a look at this : the urls in this file are all over the place. perhaps its worth a slight tidying up. inconsistency : 'https://www.box.com/api/1.0/auth/' & 'https://www.box.com/api/1.0/download/' used verbatim but $_box_api_upload_url defined for one use. suggest creating $_box_api_auth_url etc. surely we can centralis the string used everywhere here just a bit more . Thats all, with that this should be good for integration.
          Hide
          Jason Fowler added a comment -

          Changes all made now Aparup

          Show
          Jason Fowler added a comment - Changes all made now Aparup
          Hide
          Aparup Banerjee added a comment -

          cool , thats been integrated into master and picked into 22 and 21.

          please test in all branches.

          ps: Jason - theres still some literals used multiple times ie: 'box.com' here and there but thats the general improvement idea in code but you get the idea,thanks. no time, better things to do - so integrated.

          Show
          Aparup Banerjee added a comment - cool , thats been integrated into master and picked into 22 and 21. please test in all branches. ps: Jason - theres still some literals used multiple times ie: 'box.com' here and there but thats the general improvement idea in code but you get the idea,thanks. no time, better things to do - so integrated.
          Hide
          Frédéric Massart added a comment -

          I could not have this working on master (did not test the other ones).

          I followed the test instructions, and tried with an alias/shortcut and with a hard copy. None of them work, Box.net returns a 404 error.

          I don't know if this is related to this patch. But which tickles me is that the original path for the alias/shortcut is: 'Box.net: http://www.box.com/api/1.0/downloadgxrlfou18bjhnhyrxdhqc8r6vv9o2ipe/2451646514' which is not https (expected?). On the other end, they 301 redirect to the https protocol, which leads to a 404.

          Show
          Frédéric Massart added a comment - I could not have this working on master (did not test the other ones). I followed the test instructions, and tried with an alias/shortcut and with a hard copy. None of them work, Box.net returns a 404 error. I don't know if this is related to this patch. But which tickles me is that the original path for the alias/shortcut is: 'Box.net: http://www.box.com/api/1.0/downloadgxrlfou18bjhnhyrxdhqc8r6vv9o2ipe/2451646514 ' which is not https (expected?). On the other end, they 301 redirect to the https protocol, which leads to a 404.
          Hide
          Jason Fowler added a comment -

          thanks for spotting that Fred, I didn't test after Aparup's suggestion, but have found the error and pushed it as a new commit

          Show
          Jason Fowler added a comment - thanks for spotting that Fred, I didn't test after Aparup's suggestion, but have found the error and pushed it as a new commit
          Hide
          Jason Fowler added a comment -

          just did a quick test to make sure it works this time

          Show
          Jason Fowler added a comment - just did a quick test to make sure it works this time
          Hide
          Aparup Banerjee added a comment -

          thanks picked into master, 22 and 21.

          all yours for test Fred.

          Show
          Aparup Banerjee added a comment - thanks picked into master, 22 and 21. all yours for test Fred.
          Hide
          Frédéric Massart added a comment -

          Tested on master, 2.2 and 2.1. All good guys!

          Show
          Frédéric Massart added a comment - Tested on master, 2.2 and 2.1. All good guys!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this has been incorporated to all the weekly builds and also, to Moodle 2.3 Release Candidate 1, yay!

          Many, many thanks for your hard work!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - And this has been incorporated to all the weekly builds and also, to Moodle 2.3 Release Candidate 1, yay! Many, many thanks for your hard work! Ciao

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: