Skip to content

Resolve "null is not a valid URL" errors caused by a URL parsing bug in Firefox #192

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Resolve "null is not a valid URL" errors caused by a URL parsing bug in Firefox #192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 5 additions & 2 deletions src/background.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ async function updateMatchingPasswordsCount(tabId, forceRefresh = false) {

try {
const tab = await chrome.tabs.get(tabId);
badgeCache.settings.origin = new URL(tab.url).origin;
badgeCache.settings.origin = tab.url && new BrowserpassURL(tab.url).origin;
Copy link
Member

@max-baz max-baz Oct 31, 2019

Choose a reason for hiding this comment

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

I don't like that we have to repeat this tab.url && XXX logic in many places, easy to miss... Maybe we should think of another approach? e.g. always initialize BrowserpassURL object, but make the origin field inside of it be null, or some special value, so that we don't have to check for null everywhere?

Copy link
Collaborator Author

@erayd erayd Oct 31, 2019

Choose a reason for hiding this comment

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

Are you suggesting that BrowserpassURL should accept null as a valid argument? Because that's what your suggestion implies.

Copy link
Member

@max-baz max-baz Oct 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Is it ever null? When I tested this, tab.url was "about:newtab" in Firefox (and "chrome://newtab" in chromium).

Copy link
Collaborator Author

@erayd erayd Oct 31, 2019

Choose a reason for hiding this comment

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

Yes, because that's not the only place we obtain a URL from. In some places we parse the origin property of a previously-parsed URL, which may be null.

Copy link
Collaborator Author

@erayd erayd Oct 31, 2019

Choose a reason for hiding this comment

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

Also, this specific invocation sometimes has null coming from tab.url. This showed up in Firefox debugging.

Copy link
Member

@max-baz max-baz Oct 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

let's rethink it, how about we make settings.origin be not a nullable string, but non-nullable BrowserpassURL object that we don't have to ever reparse? If that requires allowing BrowserpassURL to accept null, we should consider that too :)

} catch (e) {
throw new Error(`Unable to determine domain of the tab with id ${tabId}`);
}
Expand Down Expand Up @@ -590,7 +590,10 @@ async function getFullSettings() {
let originInfo = new BrowserpassURL(settings.tab.url);
settings.host = originInfo.host; // TODO remove this after OTP extension is migrated
settings.origin = originInfo.origin;
} catch (e) {}
} catch (e) {
settings.host = null;
settings.origin = null;
}

return settings;
}
Expand Down
4 changes: 2 additions & 2 deletions src/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ function pathToInfo(path, currentHost) {
function prepareLogins(files, settings) {
const logins = [];
let index = 0;
let origin = new BrowserpassURL(settings.origin);
let origin = settings.origin && new BrowserpassURL(settings.origin);

for (let storeId in files) {
for (let key in files[storeId]) {
Expand All @@ -72,7 +72,7 @@ function prepareLogins(files, settings) {
};

// extract url info from path
let pathInfo = pathToInfo(storeId + "/" + login.login, origin);
let pathInfo = origin && pathToInfo(storeId + "/" + login.login, origin);
if (pathInfo) {
// set assumed host
login.host = pathInfo.port
Expand Down
2 changes: 1 addition & 1 deletion src/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
}
],
"dependencies": {
"@browserpass/url": "^1.1.3",
"@browserpass/url": "^1.1.5",
"chrome-extension-async": "^3.3.2",
"fuzzysort": "^1.1.4",
"idb": "^4.0.3",
Expand Down
2 changes: 1 addition & 1 deletion src/popup/searchinterface.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ function SearchInterface(popup) {
*/
function view(ctl, params) {
var self = this;
var host = new URL(this.popup.settings.origin).host;
var host = this.popup.settings.origin && new URL(this.popup.settings.origin).host;
return m(
"form.part.search",
{
Expand Down
8 changes: 4 additions & 4 deletions src/yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@
# yarn lockfile v1


"@browserpass/url@^1.1.3":
version "1.1.3"
resolved "https://registry.yarnpkg.com/@browserpass/url/-/url-1.1.3.tgz#8b94896198e7032b80c7d82e836d59e42b769669"
integrity sha512-XWhYNZo0BGcyFxkum8g1DeUkFw9QyqcaRTOEinyPSpIubh+aL2VQxbdItitVtS5qkkowqaA5Xt1H2pAqu8ic8w==
"@browserpass/url@^1.1.5":
version "1.1.5"
resolved "https://registry.yarnpkg.com/@browserpass/url/-/url-1.1.5.tgz#5b94c13421a69fc58bcb9b9f214ad6dd8b544319"
integrity sha512-er+AQEOXXgEfJVp38SCVvV3noAvKs+07o8+cGXyexTV7AVpkUSm6jdtFBFn2ijxjomKEmdBJZ8pVajs2Nhm1GA==
dependencies:
punycode "^2.1.1"

Expand Down