b2g include.js must stringify params before sending to browserid internal api

VERIFIED FIXED in Firefox 19

Status

()

Core
Identity
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: jedp, Assigned: jedp)

Tracking

Trunk
mozilla20
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-basecamp:+, firefox19 fixed, firefox20 fixed, b2g18 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

optional arguments sent to internal_api.get are being ignored.  This breaks unverifiedEmail and forceIssuer.

problem: gecko does not allow objects to cross the blood-brain barrier into userland JS; they must be strings.

fix: stringify those babies

This is a basecamp blocker; i'll have a patch in a jiffy
For those of us that don't understand the context of this component:

Is this server-side only? Or is there client-side work?

They are getting quite critical for calling blocker now (even to the point where some "gashes" are getting minused), so I think I need a better understanding why this is a blocker. At this point, it needs to be "I will jump off a bridge if I don't fix this bug." If it's something off the old criteria for blocker, then I'd nom this for tracking-b2g18.

Updated

5 years ago
status-firefox18: fixed → ---
status-firefox19: fixed → ---

Comment 2

5 years ago
The client and server components between Marketplace and Persona won't work without this bug fix.

The issue wasn't visible until we integrated the pieces.
Created attachment 694588 [details]
stringify objects

Objects must be stringified before being sent to userland js
Attachment #694588 - Flags: review?(benadida)
(In reply to Jason Smith [:jsmith] from comment #1)
> For those of us that don't understand the context of this component:
> 
> Is this server-side only? Or is there client-side work?
> 
> They are getting quite critical for calling blocker now (even to the point
> where some "gashes" are getting minused), so I think I need a better
> understanding why this is a blocker. At this point, it needs to be "I will
> jump off a bridge if I don't fix this bug." If it's something off the old
> criteria for blocker, then I'd nom this for tracking-b2g18.

Yup, I understand.  I wouldn't have said blocker unless I meant it.

This is a bug in the client side code that was not evident until we were in a position to test all the extensions we've made for marketplace.

Updated

5 years ago
Component: Server: Identity → Identity
Product: Mozilla Services → Core
Version: unspecified → Trunk
Pushed the corresponding fix to persona internal api in our b2g branch:
https://github.com/mozilla/browserid/commit/a5eade72a6451e4861cf7f6270085fe2f141effc

Redeployed notoriousb2g.personatest.org
Ah, I can confirm this bug with my test page:

http://www.se.rit.edu/~jds2501/personagetest.html

The persona dialog appears to be getting hung on "connecting to persona.."

Updated

5 years ago
Keywords: testcase
(In reply to Jason Smith [:jsmith] from comment #6)
> Ah, I can confirm this bug with my test page:
> 
> http://www.se.rit.edu/~jds2501/personagetest.html
> 
> The persona dialog appears to be getting hung on "connecting to persona.."

Hi, Jason, 

Your test page isn't actually exercising the issue that this patch fixes.  This makes allowUnverified and forceIssuer work.  Your test page is using the deprecated get() method, not the observer api that marketplace is using.

You can use this or copy from it: http://people.mozilla.org/~jparsons/test_b2g.html

You'll know the patch is landed and working when you can:

- enter 'b2g2pac.personatest.org' in "Force Issuer" and Request with Force Issuer
- check 'allow unverified' and Request Unverified OK

I'm adding these to my marionette tests, so this will all be part of automated testing
Oh, then I misread this bug a bit then.
Keywords: testcase
(In reply to Jason Smith [:jsmith] from comment #8)
> Oh, then I misread this bug a bit then.

Filed bug 823761 about this.
Comment on attachment 694588 [details]
stringify objects

good to go!
Attachment #694588 - Flags: review?(benadida) → review+
Created attachment 694619 [details] [diff] [review]
stringify objects

r=benadida
Attachment #694588 - Attachment is obsolete: true
Keywords: checkin-needed
Comment on attachment 694619 [details] [diff] [review]
stringify objects

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug didn't manifest until integration testing could be done between client code and hosted JS

User impact if declined: Marketplace won't work :(

Testing completed (on m-c, etc.): Local interactive testing by two separate people

Risk to taking this patch (and alternatives if risky): Very low

String or UUID changes made by this patch: None
Attachment #694619 - Flags: approval-mozilla-beta?
If we know this is a blocker btw then just + the bug and you can get auto-uplift.

Ben - Can you do that?
done
blocking-basecamp: ? → +
auto-uplift - is that also available for the human spirit?  or is it only b2g.
(In reply to Jed Parsons [:jparsons] from comment #15)
> auto-uplift - is that also available for the human spirit?  or is it only
> b2g.

Only for b2g. Oh and...you are looking to uplift now to the b2g 18 branch btw, not beta.
(In reply to Jason Smith [:jsmith] from comment #16)
> (In reply to Jed Parsons [:jparsons] from comment #15)
> > auto-uplift - is that also available for the human spirit?  or is it only
> > b2g.
> 
> Only for b2g. Oh and...you are looking to uplift now to the b2g 18 branch
> btw, not beta.

I was specifically thinking of the latest builds - aren't those made more or less nightly from beta?
(In reply to Jed Parsons [:jparsons] from comment #17)
> (In reply to Jason Smith [:jsmith] from comment #16)
> > (In reply to Jed Parsons [:jparsons] from comment #15)
> > > auto-uplift - is that also available for the human spirit?  or is it only
> > > b2g.
> > 
> > Only for b2g. Oh and...you are looking to uplift now to the b2g 18 branch
> > btw, not beta.
> 
> I was specifically thinking of the latest builds - aren't those made more or
> less nightly from beta?

Not anymore. They are now primarily made off of a specialized branch called b2g18.

See http://hg.mozilla.org/releases/mozilla-b2g18.
(In reply to Jason Smith [:jsmith] from comment #18)
> (In reply to Jed Parsons [:jparsons] from comment #17)
> > (In reply to Jason Smith [:jsmith] from comment #16)
> > > (In reply to Jed Parsons [:jparsons] from comment #15)
> > > > auto-uplift - is that also available for the human spirit?  or is it only
> > > > b2g.
> > > 
> > > Only for b2g. Oh and...you are looking to uplift now to the b2g 18 branch
> > > btw, not beta.
> > 
> > I was specifically thinking of the latest builds - aren't those made more or
> > less nightly from beta?
> 
> Not anymore. They are now primarily made off of a specialized branch called
> b2g18.
> 
> See http://hg.mozilla.org/releases/mozilla-b2g18.

Wow, I completely missed that memo.  Is there a mailing list I need to be on to find this sort of thing out?  Where is this kind of change announced?
Comment on attachment 694619 [details] [diff] [review]
stringify objects

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug was revealed when integration with external services was performed

User impact if declined: Marketplace authentication will not work :(

Testing completed: Local testing with two users; marionette tests

Risk to taking this patch (and alternatives if risky): Very low

String or UUID changes made by this patch: none
Attachment #694619 - Flags: approval-mozilla-b2g18?
OS: Mac OS X → All
Hardware: x86 → All
Blocks: 794634
Can we land this on mozilla-central in preparation for uplift to Aurora/B2G18? Beta (desktop/mobile) doesn't require this fix.
(In reply to Alex Keybl [:akeybl] from comment #21)
> Can we land this on mozilla-central in preparation for uplift to
> Aurora/B2G18? Beta (desktop/mobile) doesn't require this fix.

Alex, yes.

If beta doesn't need it, shall i remove the request for beta uplift?

Many thanks
j

Updated

5 years ago
Attachment #694619 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #694619 - Attachment is patch: true
Comment on attachment 694619 [details] [diff] [review]
stringify objects

This doesn't need separate b2g18 approval since it's blocking-basecamp.
Attachment #694619 - Flags: approval-mozilla-b2g18?
https://hg.mozilla.org/integration/mozilla-inbound/rev/827adb196982
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/827adb196982
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20

Updated

5 years ago
Keywords: verifyme
QA Contact: jsmith
https://hg.mozilla.org/releases/mozilla-aurora/rev/a41baaf884d0
https://hg.mozilla.org/releases/mozilla-b2g18/rev/517c056f2582
status-b2g18: --- → fixed
status-firefox19: --- → fixed
status-firefox20: --- → fixed
Confirmed fixed with two different approaches:

1. Using http://www.se.rit.edu/~jds2501/persona_observer_test.html - this should technically fail, as the icon I am using here in the arguments is not hosted over https. Before this patch, no error was ever fired. Now we get the expected error.

2. Marketplace dev testing with their definition of privacy policy and TOS - we now get the privacy policy and TOS to show up for marketplace dev's specific privacy policy and TOS, which means those args were accepted correctly

So this indicates that the original intention of the bug about args getting ignored is no longer occurring, so that makes this bug marked verified.

However! Unverified emails definitely still doesn't work with this patch. I've tried comment 7's test cases, but both did not work for me with a brand new email that I have not used previously.

Expected - If allowing unverified emails is set, I'd expect that I could still complete a login with a brand new account.

Actual - I'm still forced to do a verification right now. So what my expectation of an unverified email flow is definitely isn't working.

Jed - Any thoughts?
Status: RESOLVED → VERIFIED

Updated

5 years ago
Flags: needinfo?(jparsons)
Actually, disregard. Mistake on my behalf. I saw that I forgot to hit the unverified email checkbox when testing this.
Flags: needinfo?(jparsons)

Updated

5 years ago
Keywords: verifyme
(In reply to Jason Smith [:jsmith] from comment #28)
> Actually, disregard. Mistake on my behalf. I saw that I forgot to hit the
> unverified email checkbox when testing this.

Right - I added the checkbox so you can ensure that calling the api without the allow-unverified flag set doesn't somehow return you an assertion.  So with the checkbox unchecked, you should be able to test that it does not give you an assertion; with the checkbox, it should work.
You need to log in before you can comment on or make changes to this bug.