Skip to content
This repository has been archived by the owner on Nov 13, 2024. It is now read-only.

Work around with HTTPSE breaking ruleset format changes #3

Merged
merged 1 commit into from
Feb 15, 2018
Merged

Work around with HTTPSE breaking ruleset format changes #3

merged 1 commit into from
Feb 15, 2018

Conversation

cschanaj
Copy link
Contributor

@cschanaj cschanaj commented Jan 11, 2018

@Bisaloo
Copy link
Contributor

Bisaloo commented Feb 12, 2018

@cschanaj, could you use GitHub auto-closing feature please? It works across folders

https://help.github.com/articles/closing-issues-using-keywords/

Also @diracdeltas, not sure if you were automatically pinged on this.

@diracdeltas
Copy link
Member

Thanks, I did not see this. Will look at it today. Does this address brave/browser-laptop#10976 (comment) ?

@diracdeltas diracdeltas self-requested a review February 12, 2018 15:45
const levelup = require('level')
const rmDir = require('./util').rmDir
const exec = require('child_process').exec

const xpiVersion = '5.2.21' // Manually update this to latest version
const xpiVersion = '2017.12.6' // Manually update this to latest version
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please also bump the version in package.json to 6.0.0. this creates a new ruleset download in S3 instead of overwriting the current one (safer in case the new one has breaking changes).

}

console.log('Writing httpse.json')
fs.writeFileSync('./out/httpse.json', JSON.stringify(rulesets), 'utf8')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we still need to create httpse.json since it's used on the Brave desktop browser (only mobile uses leveldb)

@@ -33,84 +32,40 @@ const downloadRulesets = (dir, cb) => {
}

const buildDataFiles = () => {
// Manually exclude sites that are broken until they are fixed in the next
// HTTPS Everywhere release.
const exclusions = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't want to delete these exclusions unless all of these rulesets are verified to be fixed in HTTPS Everywhere upstream

@cschanaj
Copy link
Contributor Author

cschanaj commented Feb 13, 2018

@diracdeltas I've made corresponding changes in my PR. Do I need update the xpiVersion (to 2018.1.29 or even latest) as well? It has been 2 months after 2017.12.6 was released.

@cschanaj cschanaj changed the title Https everywhere 2017.12.6 Work around with HTTPSE breaking ruleset format changes Feb 13, 2018
@diracdeltas
Copy link
Member

@cschanaj would be great to update it to 2018.1.29. we avoid latest because we want to pin the version number.

@cschanaj
Copy link
Contributor Author

cschanaj commented Feb 13, 2018

@diracdeltas please check. thanks!

PS. The original JSON format is not that straight forward...

{
   "ruleset":{
      "$":{
         "name":"101domain.com (partial)",
         "default_off":"failed ruleset test", // gone
         "f":"101domain.com.xml" // gone
      },
      "exclusion":[
         {
            "$":{
               "pattern":"^http://(?:portuguese|spanish)\\.101domain\\.com/+(?!css/|favicon\\.ico|images/)"
            }
         }
      ],
      "rule":[
         {
            "$":{
               "from":"^http://russian\\.101domain\\.com/[^?]*",
               "to":"https://www.101domain.ru/"
            }
         },
         {
            "$":{
               "from":"^http:",
               "to":"https:"
            }
         }
      ]
   }
}

}

for (const ruleset of rulesets) {
if (!ruleset.default_off && !ruleset.platform) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the original code include default_off and platform ruleset in httpse.json, which is gone now.

'Cisco.com (partial)': 'breaks http://www.cisco.com/c/m/en_us/training-events/events-webinars/techwise-tv/listings.html',
'GQ.com (partial)': 'mixed content on gq.com',
'Where 2 Get It (partial)': 'maps missing on http://us.coopertire.com/Customer-Care/Dealer-Locator.aspx?form=locator_search&addressline=92346',
'Thompson Hotels.com (partial)': 'missing stylesheets on http://www.thompsonhotels.com/'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for going through and re-mapping these!

diracdeltas added a commit to brave/browser-laptop that referenced this pull request Feb 14, 2018
Needed for brave/https-everywhere-builder#3

Note that this is a breaking change from HTTPS Everywhere 5.x's ruleset
format.
@diracdeltas
Copy link
Member

diracdeltas commented Feb 14, 2018

@cschanaj yup, there's no reason to have the HTTPSE.json be as complicated as it used to be. i opened brave/browser-laptop#13133 so you can simplify it somewhat by flattening out the $ properties.

so the new schema can be:

{
   "ruleset":{
      "name": "101domain.com (partial)",
      "exclusion":[
         {
               "pattern":"^http://(?:portuguese|spanish)\\.101domain\\.com/+(?!css/|favicon\\.ico|images/)"
         }
      ],
      "rule":[
         {
               "from":"^http://russian\\.101domain\\.com/[^?]*",
               "to":"https://www.101domain.ru/"
         },
         {
               "from":"^http:",
               "to":"https:"
         }
      ]
   }
}
`

})
}

if (ruleset.securecookie) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't have plans to use securecookie in browser-laptop so this can be omitted

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for submitting a PR upstream!! I have remove the securecookie properties and simplified the schema in 31d9b8f.

@diracdeltas
Copy link
Member

diracdeltas commented Feb 15, 2018

This works great, thank you! Please run rm -r node_modules/ and npm install so that it updates package-lock.json. Then commit the new package-lock.json file.

@cschanaj
Copy link
Contributor Author

@diracdeltas done. thanks!!

diracdeltas
diracdeltas previously approved these changes Feb 15, 2018
@diracdeltas
Copy link
Member

@bsclifton this can be squashed and merged :). once that is done, please run npm run build and npm run upload to upload it to the S3 test bucket. I'll ping you again once it's ready for upload to prod. (https://github.com/brave/https-everywhere-builder/blob/master/scripts/uploadDataFiles.js#L44)

- Update preloadHTTPSE.js
- Update package.json
- Bump version in package.json to 6.0.0
- Update scripts/preloadHTTPSE.json
- Update to HTTPS Everywhere 2018.1.29
- Changes as per npm  run lint
- Update scripts/preloadHTTPSE.js
- Simplify HTTPSE.json schema
- Update package-lock.json
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased / squashed per comments by @diracdeltas 😄

@bsclifton bsclifton merged commit 272d8c4 into brave:master Feb 15, 2018
@bsclifton
Copy link
Member

@diracdeltas built + uploaded 👍

@cschanaj cschanaj deleted the https-everywhere-2017.12.6 branch February 15, 2018 23:51
diracdeltas added a commit to brave/browser-laptop that referenced this pull request Feb 16, 2018
Needed for brave/https-everywhere-builder#3

Note that this is a breaking change from HTTPS Everywhere 5.x's ruleset
format.
@bsclifton
Copy link
Member

bsclifton pushed a commit to brave/browser-laptop that referenced this pull request Feb 16, 2018
Needed for brave/https-everywhere-builder#3

Note that this is a breaking change from HTTPS Everywhere 5.x's ruleset
format.
ryanml pushed a commit to ryanml/browser-laptop that referenced this pull request Feb 27, 2018
Needed for brave/https-everywhere-builder#3

Note that this is a breaking change from HTTPS Everywhere 5.x's ruleset
format.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants