-
Notifications
You must be signed in to change notification settings - Fork 34
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
Fix 738 log addon version #771
Conversation
342c6d1
to
86b8e6e
Compare
Pull Request Test Coverage Report for Build 7222
💛 - Coveralls |
Gecko struct { | ||
ID string `json:"id"` | ||
} `json:"gecko"` | ||
} `json:"browser_specific_settings"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think applications
is an alias for browser_specific_settings
. In addition, this prop is optional.
Is it OK to not log the add-on ID all the time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more information:
- the
version
should be mandatory - the GUID (same value as the Gecko ID) is somehow inserted in the signature of a signed XPI so I suppose there is a way to always get the GUI somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
applications
is an alias forbrowser_specific_settings
. In addition, this prop is optional.
TIL! I didn't see that key on MDN or in the webext spec, but these pages suggest Fx <48 required it:
- https://stackoverflow.com/questions/37127330/unrecognized-manifest-key-applications-warning-for-google-chrome
- https://support.mozilla.org/en-US/questions/1217302
so it probably predates standardization and we might have to sign addons with that key.
Is it OK to not log the add-on ID all the time?
Yes, I have a few test cases for partial matches and XPIs signed in other modes (privileged, hotfix, etc.) probably won't include the an ID or might use the other key.
8b4f491 adds test cases for:
- a manifest with
applications
set to an empty obj - a manifest with
applications.gecko
set to an empty obj - a manifest with
applications.gecko.id
set to an empty str - a manifest with
applications.gecko.id
set to an addon ID - a manifest with a version and both IDs set (in which case we'll prefer the browser settings ID)
Once we have some logs, I'll try to confirm we can correlate them with signed addons. If not, we might need pass the data out to the "signing operation succeeded" lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more information:
1. the `version` should be mandatory
Hmm, extractAddonIDAndVersionFromWebextManifest
could return an error and we could log it as a warning instead of info level.
If other XPI types that don't include a manifest, we won't log warnings. If they do, we might log some extra warnings but that'd be fine.
2. the GUID (same value as the Gecko ID) is somehow inserted in the signature of a signed XPI so I suppose there is a way to _always_ get the GUI somewhere?
Does addons-server generate that and send it as Options.ID?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does addons-server generate that and send it as Options.ID?
I think so, yes: https://github.com/mozilla/addons-server/blob/80f526e84084a776f9f379301e11250202f9a8ab/src/olympia/lib/crypto/signing.py#L77
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r+wc
}, | ||
{ | ||
name: "version with applications.gecko.id and browser_specific_settings.gecko.id both set in JSON prefers browser_specific_settings id", | ||
manifestBytes: []byte("{\"applications\":{\"gecko\":{\"id\":\"[email protected]\"}},\"browser_specific_settings\":{\"gecko\":{\"id\":\"{5ae54d6f-bcb2-48ec-b98c-7a19e983283f}\"}},\"version\":\"1.2.3\"}"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addons-linter won't accept a manifest with the two props but it's good to be defensive here.
8b4f491
to
8ca13e5
Compare
shaves ~3s off go test -v -coverprofile coverage.out -covermode=count -run TestSignData
shaves ~3s off xpi tests
so zip utils recognize them
8ca13e5
to
1451831
Compare
Thanks Will! Follow up items from comments and original issue:
Warnings return errors if we want to do more linting in autograph and other web extension signers pass for legitimate uses. |
fix #738
Changes:
PASSINGTESTCASES
tovalidSignerConfigs
andFAILINGTESTCASES
toinvalidSignerConfigs
to avoid confusion with other vars namedtestcases
unzip -l
).zip
to theomni.ja
files so zip tools recognize them as zipsomni.ja
fixturesTestSign{Data,File}
test cases in parallel saved ~6s on local testing of the packagemanifest.json
top-levelversion
field and.browser_specific_settings.gecko.id
field usingextractGeckoIDAndVersionFromWebextManifest
, which assumes the version field is a string per the MDN docs. This was the laziest option to fix log signed XPI or addon version number #738 since it doesn't modify the repack func return values or the sign file interface.