Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ npm install

### Build

#### Including depdencies
#### Including dependencies

For testing the extension in the browser, you'll need to ensure that the 3rd party dependencies have been copied to the
source directory. This is performed by the following grunt task:
Expand Down Expand Up @@ -103,7 +103,7 @@ In both cases the extension will intercept the URL, remove the identifier and pa
existing tabs/windows open to the requested URL, the URL is handled as normal. If one already exists it will open to
that existing tab instead. In the case of `refreshsametab.morphic.org`, it will also trigger the page to reload.

For example, `http://opensametab.morphic.org/en.wikipedia.org/wiki/URL` will open `https://en.wikipedia.org/wiki/URL`.
For example, `http://opensametab.morphic.org/redirect/https%3A%2F%2Fen.wikipedia.org%2Fwiki%2FURL` will open `https://en.wikipedia.org/wiki/URL`.

#### Caveats

Expand Down
22 changes: 15 additions & 7 deletions src/background/openURLs.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@
const openURLs = {};

openURLs.openTab = async (url, refresh) => {
const urlObj = new URL(url);
const queryURL = urlObj.href.replace(urlObj.protocol, "*:");
const queryURL = url.href.replace(url.protocol, "*:");

let [matchedTab] = await browser.tabs.query({url: queryURL});
let [loadingTab] = await browser.tabs.query({"status": "loading", "windowId": browser.windows.WINDOW_ID_CURRENT});
Expand All @@ -37,7 +36,7 @@ openURLs.openTab = async (url, refresh) => {
browser.tabs.reload(matchedTab.id);
}
} else {
browser.tabs.update(loadingTab.id, {url});
browser.tabs.update(loadingTab.id, {url: url.href});
}
};

Expand All @@ -53,12 +52,21 @@ openURLs.getFilter = identifiers => {
};

openURLs.handleRequest = details => {
const refresh = details.url.indexOf(openURLs.identifiers.refreshTab) >= 0;
const url = details.url.replace(`${openURLs.identifiers[refresh ? "refreshTab" : "openTab"]}/`, "");
const url = new URL(details.url);
const refresh = url.hostname.indexOf(openURLs.identifiers.refreshTab) >= 0;
// URL will look like: "http://opensametab.morphic.org/redirect/https%3A%2F%2Fexample.com%2F"
const destination = url.pathname.replace(/^\/(redirect\/)?(.*)/, (match, c1, c2) => decodeURIComponent(c2));

openURLs.openTab(url, refresh);
let success;
try {
const destinationUrl = new URL(destination);
openURLs.openTab(destinationUrl, refresh);
success = true;
} catch (e) {
success = false;
}
Comment on lines +61 to +69

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It appears that this try/catch block is to guard against malformed URLs used for the destinationUrl. e.g. refresh/https%3A%2F%2Fexample.com%2F. However, openURLs is an async function. So I don't believe that it will wait to make sure that it doesn't throw an error. You could use await on openURLs but will need to convert openURLs.handleRequest into an async function as well. According to webRequest.onBeforeRequest the handler can return a promise that resolves with a BlockingResponse. You'll also have to test that the polyfill can handle this feature for use in Chrome.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

YEs. In fact, that's why I moved the url parsing out of openURLs.

However, I've now moved the call outside of the try block to make my intention clearer.


return {cancel: true};
return {cancel: success};
};

openURLs.bindListener = () => {
Expand Down
16 changes: 8 additions & 8 deletions tests/js/openURLsTests.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@
blankTab: {
id: 1
},
url: "https://actual.org/url",
url: new URL("https://actual.org/url"),
queryURL: "*://actual.org/url"
};

Expand Down Expand Up @@ -134,7 +134,7 @@
jqUnit.assertTrue("Tab isn't highlighted", browser.tabs.highlight.notCalled);
jqUnit.assertTrue("Loading tab isnt' removed", browser.tabs.remove.notCalled);
jqUnit.assertTrue("Tab isn't reloaded", browser.tabs.reload.notCalled);
let isUpdatedCalled = browser.tabs.update.calledOnceWithExactly(loadingTab.id, {url: openTabTestsProps.url});
let isUpdatedCalled = browser.tabs.update.calledOnceWithExactly(loadingTab.id, {url: openTabTestsProps.url.href});
jqUnit.assertTrue("Loading tab is updated with correct URL", isUpdatedCalled);
}

Expand All @@ -151,29 +151,29 @@
const handlestRequestTestCases = [{
name: "Open same tab",
details: {
url: "https://opensametab.morphic.org/actual.org/url"
url: "https://opensametab.morphic.org/redirect/https%3A%2F%2Factual.org/url"
},
expected: {
response: {cancel: true},
args: ["https://actual.org/url", false]
args: [new URL("https://actual.org/url"), false]
}
}, {
name: "Refresh tab",
details: {
url: "http://refreshsametab.morphic.org/actual.org/url"
url: "http://refreshsametab.morphic.org/redirect/http%3A%2F%2Factual.org/url"
},
expected: {
response: {cancel: true},
args: ["http://actual.org/url", true]
args: [new URL("http://actual.org/url"), true]
}
}, {
name: "Unfiltered URL",
details: {
url: "http://morphic.org/actual.org/url"
url: "http://morphic.org/redirect/https%3A%2F%2Factual.org/url"
},
expected: {
response: {cancel: true},
args: ["http://morphic.org/actual.org/url", false]
args: [new URL("http://morphic.org/redirect/https%3A%2F%2Factual.org/url"), false]
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Need test cases for when the response is {cancel: false}.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done

}];

Expand Down