-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add support for signing ES requests #6
base: master
Are you sure you want to change the base?
Conversation
return {requestHeaders: details.requestHeaders}; | ||
} | ||
}, | ||
{urls: ["<all_urls>"]}, | ||
["blocking", "requestHeaders"]); | ||
|
||
const isUrlValid = (url) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move these const declarations to the top of the file please? I feel it would make it more readable
var canonicalRequestHash = CryptoJS.SHA256(canonicalRequest); | ||
log('Canonical Request Hash: ' + canonicalRequestHash); | ||
|
||
var service = 'es'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indents
@@ -13,11 +13,14 @@ | |||
"webRequestBlocking", | |||
"*://*.ft.com/", | |||
"*://*.herokuapp.com/", | |||
"*://next-elastic.ft.com/*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indents
"storage" | ||
], | ||
"background": { | ||
"scripts": [ | ||
"background.js" | ||
"public/background.js", | ||
"lib/hmac-sha256.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need to be added to the Makefile for the build
and publish
commands. The lib folder wont exist at the moment.
@@ -13,11 +13,14 @@ | |||
"webRequestBlocking", | |||
"*://*.ft.com/", | |||
"*://*.herokuapp.com/", | |||
"*://next-elastic.ft.com/*", | |||
"*://search-next-elasticsearch-[a-z1-9]+.(eu-west-1|us-east-1).es.amazonaws.com/*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manifests do not support regex for url matches, instead they have their own limit pattern matching https://developer.chrome.com/extensions/match_patterns
I think we will have to support this in the same way that we support herokuapp.com
domains with the additional check in background.js https://github.com/GlynnPhillips/vary-headers/pull/6/files#diff-f9b12bd72f40c9b8ed144484f0f12a5fR47https://github.com/GlynnPhillips/vary-headers/pull/6/files#diff-f9b12bd72f40c9b8ed144484f0f12a5fR47
@@ -1,5 +1,6 @@ | |||
build: | |||
webpack | |||
webpack popup.js public/popup.js | |||
webpack background.js public/background.js | |||
|
|||
package: build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add the new icons directory to this package command please
cp -f -a ./{public,icons,popup.html,background.js,styles.css} ./publish
* licensed under https://github.com/carsales/aws-request-signer/blob/c835ca69e74a5b5e7d6eee7aaab8bbd7fbcdff4d/LICENSE | ||
* with many thanks to John Jones :+1: | ||
*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate that this file is mainly a copy from elsewhere but could we please update it to make the code style in the rest of the repo. This is partly my bad for not yet adding proper liniting
Another new thing from this PR is that this runs Webpack on the background JS as well as the popup JS.