-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Conversation
If anyone wants to test this functionality, you can use the following to add a JWK: {"kty":"RSA","n":"zv4NkDYgBL5n1LY_Pl2WWmiHV_IApqtXTXUL6STgUS3G_ZFORA5Qx_7x5TMA9OWeKsN9kgDkpiZWs_cUZCfWLkYeP6MRmtOe-50Wzca99qb8hQfBT81Pnir-hadlISgcuosZFWfEmaEPXHLI47ZgnelDvBcVD8XyOH5qc4umNCZIeFl_cQApv_t7KDo0HRr4fvWqWWhLwfJJU3HPNUr7VHAyEdU6whllOUl3uSEt7zXEfODnxFxUFILUmIXfqFS8c8VcI20l-8QXBijbHRXGTKpAj2Sdu9v5pGL0-5hh3iXISlC3rRpNjwBhdlQSWK9V30Cfp163bOP4WekSt51J5w","e":"AQAB"} Path Prefix: This will make it so that in any instance of Tor Browser, navigating to You can use |
You can also publish rulesets of your own by placing them in |
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.
Just a few suggestions and questions, would be great if they are addressed. The new option page looks nice!
P.S. I didn't dig very deep into the code but the basic functionalities look good to me. Thanks.
<div id="update-channels-list"></div> | ||
<div id="add-update-channel-wrapper"> | ||
<button id="add-update-channel" data-i18n="options_addUpdateChannel"></button> | ||
<input type="text" id="update-channel-name" /> |
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.
Currently, an empty channel name works, but it will cause the form rendered oddly. I guess this should be a required field, since it is showed on the popup and used to help the user identify an update channel
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.
Good point. This is fixed in a0bec3d
update_channel_row_path_prefix.appendChild(update_channel_path_prefix_column_right); | ||
const update_channel_path_prefix = document.createElement('input'); | ||
update_channel_path_prefix.setAttribute("type", "text"); | ||
update_channel_path_prefix.className = "update-channel-path-prefix"; |
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 know that the rulesets are signed, but it might be a good idea to encourage the endpoints to use HTTPS by hard coding the https
protocol here.
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 can envision http
update channels over onion
services, or network administrators who wish to mirror update channels locally by DNS redirection (as is sometimes done with apt
). I'd like to allow update channel operators to choose their own transport layer.
@@ -170,7 +187,9 @@ async function applyStoredRulesets(rulesets_obj){ | |||
const rulesets_jsons = await Promise.all(rulesets_promises); | |||
if(rulesets_jsons.join("").length > 0){ | |||
for(let rulesets_json of rulesets_jsons){ |
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.
It seems to me that EFF's update channels will always have the top priority in rewriting URLs, while other channels are prioritize by insertion order. If the coverage of different channels overlaps, it might not works as the users expected, e.g. some channels might never apply. If this is the case, it might be good to have a way to prioritize the channels (possibly in later PR/ release). Please correct me if I am wrong.
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.
Yes I think prioritization of update channels makes sense, but this will require some re-engineering. This would be a good task for a future PR.
Adding a UX to update channels, as well as functionality to add, remove, or edit update channels.