browserpass / browserpass-legacy

Legacy Browserpass repo, development is now happening at:
https://github.com/browserpass/browserpass-extension
MIT License
999 stars 80 forks source link

Issues with packaging browserpass for Debian #245

Closed meskes closed 6 years ago

meskes commented 6 years ago

I'm running a selfbuild extension on Linux system with Firefox and Chromium with latest app. Whenever I click on the extension symbol it shows me which URL it searches for, but never returns anything. As long as the popup is active it only shows that turning circle symbol but never any password entry. And also no error message, just nothing.

I've double checked the app version, it is up-to-date.

Any idea how to debug? Or even better, any idea what might be going on?

erayd commented 6 years ago

Check the developer console for both the popup window, and the background page. Do you see an error message in the console?

meskes commented 6 years ago

Not sure if I used it correctly, but I do see this:

TypeError: Cannot read property 'autoSubmit' of undefined

Whether I have auto submit configured or not does not make a difference.

maximbaz commented 6 years ago

Do you see in which file this happens, maybe even a line number?

I suspect it's this line:

https://github.com/dannyvankooten/browserpass/blob/22e8af03308ca489beeb51f6bf66714b14240740/chrome/script.browserify.js#L41

erayd commented 6 years ago

@maximbaz I haven't been able to reproduce this issue, but the line you've flagged is an issue - it's trying to access an object which may sometimes be uninitialised.

I've just opened PR #246 to ensure that the settings are always present - I don't know whether that's enough to fix this bug, as I've not been able to reproduce it here.

maximbaz commented 6 years ago

I was unable to reproduce it too, I like the idea in #246 - merged.

@meskes could you try to reproduce the issue with the latest master?

meskes commented 6 years ago

Current master works a little bit longer. If I access e.g. github it now shows me my github entry, but as soon as I click on it I get the neverending circle again. New error message is:

content.html:1 Error in event handler for (unknown): TypeError: Cannot read property 'error' of undefined at chrome-extension://cbfnbkdnigbahbkcikfppmaonggdbcpa/script.js:314:20

The line in questions is the "if (response.error)" in

function getLoginData() { searching = true; logins = resultLogins = []; m.redraw();

chrome.runtime.sendMessage( { action: "login", entry: this, urlDuringSearch: urlDuringSearch }, function(response) { if (response.error) { return resetWithError(response.error); } window.close(); } ); }

maximbaz commented 6 years ago

Hmm, something is very wrong with your extension. Just curious, does it reproduce with extension in the webstore? Could you try to clone the repo from scratch, and then build the extension with $ make command?

meskes commented 6 years ago

No, it does not. I'm working on the Debian package for it and am trying to figure out why it worked correctly with 2.0.11 and 2.0.13 but not with 2.0.18. The build process is different because Debian does not have browserify, but only browserify-lite. I cannot see any functional differences in the js files, though.

Also, I did try copying the 2.0.18 files from firefox.zip to the location the package resides in, to no avail, some problem. However, this obviously was before the patch.

erayd commented 6 years ago

@meskes

The build process is different because Debian does not have browserify, but only browserify-lite.

Are you able to explain how you have changed the build process? If the 'official' builds are working, but yours is not, that might give us a clue as to where the problem might be.

If a reference would be helpful, I can confirm that builds via docker (see CONTRIBUTING.md) are working correctly against the current master.

erayd commented 6 years ago

@meskes One other thing that may be relevant if you're building the browserify bits differently: as of 2.0.18, options.js is now also generated via browserify (from options.browserify.js). This required a makefile change, so if you're using a custom makefile, perhaps that change was missed?

meskes commented 6 years ago

The sources are in https://salsa.debian.org/webext-team/browserpass

The makefile is debian/rules.

Chances are I missed something, but then why didn't copying the files from the zip file help?

erayd commented 6 years ago

The makefile seems OK. I'll try building from your sources and see what happens, hang on.

erayd commented 6 years ago

I can confirm that building from the master branch of the debian repo you have linked above works normally, and results in a usable extension. However, there is a unit test failure when building the host app, and that failure is also present for the master branch of this repo - I've opened #248 for this.

If you run the 2.0.18 browser extension that you have built, but use the host app binary from the github release, does it work correctly?

steve@neith /tmp/browserpass $ docker run --rm -v "$(pwd)":/browserpass browserpass-dev
yarn
yarn install v0.23.3
[1/4] Resolving packages...
[2/4] Fetching packages...
[3/4] Linking dependencies...
[4/4] Building fresh packages...
Done in 5.01s.
dep ensure
node_modules/.bin/prettier --write chrome/styles.css chrome/script.browserify.js chrome/otp.js chrome/otp.css chrome/options.browserify.js chrome/inject_otp.js chrome/inject.js chrome/background.browserify.js
chrome/styles.css 322ms
chrome/script.browserify.js 565ms
chrome/otp.js 73ms
chrome/otp.css 55ms
chrome/options.browserify.js 97ms
chrome/inject_otp.js 18ms
chrome/inject.js 106ms
chrome/background.browserify.js 103ms
node_modules/.bin/browserify chrome/background.browserify.js -o chrome/background.js
node_modules/.bin/browserify chrome/script.browserify.js -o chrome/script.js
node_modules/.bin/browserify chrome/options.browserify.js -o chrome/options.js
cp chrome/host.json chrome-host.json
cp firefox/host.json firefox-host.json
cp chrome/policy.json chrome-policy.json
cp chrome/{*.html,*.css,*.js,*.png,*.svg} firefox/
go build -o browserpass ./cmd/browserpass
go test
PASS
ok      github.com/dannyvankooten/browserpass   0.003s
go test ./pass
--- FAIL: TestDefaultStorePath (0.00s)
        disk_test.go:16: user: Current not implemented on linux/amd64
        disk_test.go:27: 1: '/root/.password-store' does not match ''
FAIL
FAIL    github.com/dannyvankooten/browserpass/pass      0.016s
make: *** [Makefile:62: test] Error 1
meskes commented 6 years ago

Nope, same effect. Hmm, it seems like you build from my source but not with my makefile, right?

erayd commented 6 years ago

I built using the default makefile (i.e. the makefile from the master branch root). Does yours live elsewhere?

maximbaz commented 6 years ago

Unit test fails because it cannot retrieve the "current user" in the docker environment, I don't think that is relevant to this issue.

@meskes in order not to deal with browserify or other dependencies, just grab the browserpass-src.tar.gz from the releases, that archive is built specifically for package maintainers. However firefox.zip also has pre-compiled javascript sources, so it should work just as well...

erayd commented 6 years ago

@meskes I've diffed your master against 2.0.18 of this repo - the only difference is the presence of a debian/ subdirectory, and I can't find a custom makefile in there. Everything else is identical.

Could you please advise how you're building this?

maximbaz commented 6 years ago

@erayd you missed above:

The makefile is debian/rules

meskes commented 6 years ago

@erayd the makefile is called debian/rules @maximbaz the problem with testing from your tarball now is that it does not contain the latest patches, so chances are I run into #246 again

erayd commented 6 years ago

Oops - I did indeed miss that. Thanks :-).

maximbaz commented 6 years ago

Here you go, based on the current master: release.zip

meskes commented 6 years ago

this seems to work, let's see where the difference is ...

meskes commented 6 years ago

It seems to be background.js

maximbaz commented 6 years ago

It became to be "browserified" only recently 😉 So... problem solved?

P.S. Will you please submit a PR to the README.md when it will be possible for Debian users to install browserpass via official package management tools?

erayd commented 6 years ago

@maximbaz Might be worth rolling 2.0.19, if the changes in master have solved a packaging problem. That way the debian package version will be properly in sync with the repo release tags. Otherwise if someone reports a problem, and the debian version of 2.0.18 contains more than what's actually in the 2.0.18 release...

maximbaz commented 6 years ago

Will do once @meskes confirms that the issue is solved and this ticket can be closed 👍

meskes commented 6 years ago

@maximbaz problem is not solved, I just noticed that it works with your background.js but not with mine, why is a different matter

As for the package, it is already available since 2.0.11, but I can surely do a PR for that

maximbaz commented 6 years ago

background.js is an generated script via browserify, if you don't want to use the pre-generated one from browserpass-src.tar.gz or firefox.zip, double check how you are generating the file.

meskes commented 6 years ago

browserify-lite $$(pwd)/chrome/background.browserify.js --outfile $$(pwd)/chrome/background.js

If it's just browserify-lite that's not working correctly, it's not an issue here, but if it's coming from someplace else ...

maximbaz commented 6 years ago

Just curious, why not use pre-generated sources and avoid having troubles with browserify and other dependencies alltogether? It is also a burden for you to maintain, for example a week ago background.js did not require compilation, now it does, but browserpass-src.tar.gz contained and will contain sources that work "out of the box" with no extra effort required.

meskes commented 6 years ago

The idea behind this is to compile everything from sources and by doing so getting the latest version of the dependencies as well.

maximbaz commented 6 years ago

That's not very good, we have yarn.lock and Gopkg.lock to make sure everyone is using the same versions of everything to avoid issues that cannot be reproduced by everyone.

If a dependency needs to be updated, I'd much prefer seeing a PR that updates the lock files, and then everyone is using the code that is built using the same version of tools.

meskes commented 6 years ago

Yeah, that's the difference between the software development and the distribution approach. There is actually a long thread on debian-devel about finding ways to make both work at the same time. Historically the problem was solved by introducing shared libraries. Anyway, I'm just trying to cope

maximbaz commented 6 years ago

6b5b236 updated all dependencies, so you can feel better at using the pre-compiled sources 😉

The decision is up to you of course, but I'm afraid I won't be able to support multiple build configurations, if a bug report would say "it works fine with the official release or when built from master, but doesn't work via the debian package", I'm afraid I would just have to close those bug reports with "use the official builds".

meskes commented 6 years ago

Fair enough, I'd prefer using your build system, too, but unfortunately it's not (yet) possible in Debian. Anyway, thanks for your help. If I find something not related to the Debian build itself, I'll open a new ticket.

erayd commented 6 years ago

@meskes Is using the docker build environment an option for you? That will still compile completely from source.

meskes commented 6 years ago

@erayd Actually no, the basic idea is that everything is buildable within the distro, so all dependencies and all toold should be packaged. There is some discussion that this has to be changed, but no action yet.

maximbaz commented 6 years ago

You could argue that using browserpass-src.tar.gz is okay because all it takes to make that archive is to download dependencies and to run browserify, and browserify is just a concatenation of scripts in one big script 😉 There's no binary code in there, everything is sources.

maximbaz commented 6 years ago

I'll close because it seems there's nothing else we can do here on our side.