brinchj / RndPhrase

RndPhrase: Auto-generated secure passwords.
BSD 2-Clause "Simplified" License
21 stars 4 forks source link

Implement Opera app store code review feedback #15

Closed Munter closed 8 years ago

Munter commented 8 years ago

Refs #14

The last two unchecked boxes I can't check. I've added my comments as to why.

These changes have not been tested, so I recommend a proper test run before doing a new release with them merged in. Especially the seed preference migration is important to verify works correctly

brinchj commented 8 years ago

I can confirm that the modal.html and modal.js files were infact build artifacts. They are not present in a new build, so you do not need to investigate that further.

I also figured out what was going on with the comments being inserted into our HTML. Basically, in order to preserve comments in the processed Javascript, gcc was invoked with -C, however that also preserves the comments of some "always included by standard" C code, which looks very weird, especially so in HTML files.

I am disabling comment preservation for now, as it is not that important to include our comments in the generated Javascript anyway.

brinchj commented 8 years ago

I found a few typos in the code changes, but besides from that I think this is enough to fix our issues. Thanks a lot! :+1:

brinchj commented 8 years ago

I have committed a few local changes, although I do not expect any overlap, so you should have no trouble rebasing on top of them (hopefully).

Munter commented 8 years ago

I fixed the bugs you caught in the code review

Munter commented 8 years ago

Note that I am using chrome.storage.sync rather than chrome.storage.local. I thought it would be a nice addition to have updates propagate to your other logged in chrome instances. If this presents a security risk we might want to change that bit

brinchj commented 8 years ago

Is the synchronization optional? If so, it sounds like a decent feature and it should not pose a large risk as it is just the seed, after all.

Munter commented 8 years ago

I think synchronization will always happen between logged in instances of chrome. I'm not sure there is a user controlled setting for it. The docs say that it's a security risk if you don't encrypt the data you put in it, which we do.

brinchj commented 8 years ago

The seed is currently only migrated from the localStorage to the storage API when loading the options page.

Preferably, this should happen in background.js, when loading the preferences during initialization for a website, as the addon would otherwise produce incorrect passwords until the user loads the options page.

Would it be possible to inject the migration code into background.js too? (If the two places cannot easily share code, I think copying the two lines of migration is acceptable)