Skip to content
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.

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

Add support for signons.txt format #36

Closed
wants to merge 1 commit into from

Conversation

cskrisz
Copy link

@cskrisz cskrisz commented Jul 20, 2018

No description provided.

@unode
Copy link
Owner

unode commented Jul 21, 2018

Thanks for this, though is anyone still using Firefox < 3 ? 😄

A quick glance shows no major issues. There are 2 signons.txt formats (here and here), are you covering only the first version?

I'll do a closer review and merge once I have some time.

@cskrisz
Copy link
Author

cskrisz commented Jul 22, 2018

My primary target was signons3.txt but I also implemented the other versions since they are not much different.

Copy link
Owner

@unode unode left a comment

Choose a reason for hiding this comment

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

All else looks good to merge.

file_id = '#2' + chr(ord('b') + self._version)
assert lines[0] == file_id

record_size = 3 + self._version
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add a comment here detailing that with a new version of signons.txt one additional line with data was added to every record.
Also please add comments with links to http://kb.mozillazine.org/Signons.txt http://kb.mozillazine.org/Signons2.txt and http://kb.mozillazine.org/Signons3.txt for future reference.

@@ -382,6 +413,9 @@ def load_libnss(self):
# If this succeeds libnss was loaded
self.NSS = self.find_nss(locations, nssname)

if os.name == 'nt' and not (hasattr(self.NSS, "PR_ErrorToName") and hasattr(self.NSS, "PR_ErrorToString")):
Copy link
Owner

Choose a reason for hiding this comment

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

Is NSPR only needed on Windows. Do we need workarounds on other platforms too?

engines = [
(JsonCredentials, {}),
(SqliteCredentials, {}),
(TextCredentials, {'version': 1}),
Copy link
Owner

Choose a reason for hiding this comment

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

The order goes from newer to older (JSON is Firefox 32+), minor thing but should we try version 3 before 1 and 2?

@unode unode force-pushed the master branch 3 times, most recently from f15c99d to 6a0c69b Compare December 10, 2018 04:05
@unode
Copy link
Owner

unode commented Oct 4, 2019

As Firefox 3 is almost 10 years old I'm closing this as it adds unnecessary dependencies and workarounds.
I'll make a mention in the README to this pull request in case anyone has this requirement.

@unode unode closed this Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants