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

Credential Management API issue in Chrome 51 ~ 56 #3238

Merged
merged 3 commits into from Mar 16, 2017

Conversation

Projects
None yet
4 participants
@tjwudi
Contributor

tjwudi commented Mar 10, 2017

In Chrome 51 ~ 56, you cannot use credential set at a.d.com in b.d.com. This was fixed in Chrome 57.

Using credentials across different subdomains were not enforced by the spec. The spec used the term MAY but not MUST. But Chrome chose to support this in Chrome 57, so I'd say it was a bug in versions prior to 57. Let's annotate Chrome 51 ~ 56 as partially supported then.

Credential Management API issue in Chrome 51 ~ 56
In Chrome 51 ~ 56, you cannot use credential set at a.d.com in b.d.com. This was fixed in Chrome 57.

Using credentials across different subdomains were not enforced by the spec. The spec used the term MAY but not MUST. But Chrome chose to support this in Chrome 57, so I'd say it was a bug in versions prior to 57. Let's annotate Chrome 51 ~ 56 as partially supported then.
@agektmr

This comment has been minimized.

Show comment
Hide comment
@agektmr

agektmr Mar 11, 2017

Contributor

I'm hesitant to call this a bug as the spec change happened preceding our change IIRC.
What do you think @vabr-g ?

Contributor

agektmr commented Mar 11, 2017

I'm hesitant to call this a bug as the spec change happened preceding our change IIRC.
What do you think @vabr-g ?

@tjwudi

This comment has been minimized.

Show comment
Hide comment
@tjwudi

tjwudi Mar 11, 2017

Contributor

I agree that this might not be a bug by definition, but definitely something worth mentioning so people can be aware of it. But it looks like other than "partially support" there is no better way to annotate Cr51~56's discrepancy with Cr57. I might be wrong though :)

Contributor

tjwudi commented Mar 11, 2017

I agree that this might not be a bug by definition, but definitely something worth mentioning so people can be aware of it. But it looks like other than "partially support" there is no better way to annotate Cr51~56's discrepancy with Cr57. I might be wrong though :)

@agektmr

This comment has been minimized.

Show comment
Hide comment
@agektmr

agektmr Mar 14, 2017

Contributor

I don't think marking this as a bug is good idea, but I agree that we lack documentation around this. Actually I'm planning one under Web Fundamentals as part of FAQ for CM API. I also think noting this in MDN makes sense.

@jpmedley can we work together to address this in MDN? We have a few notable updates around cross origin CM API calls.

Contributor

agektmr commented Mar 14, 2017

I don't think marking this as a bug is good idea, but I agree that we lack documentation around this. Actually I'm planning one under Web Fundamentals as part of FAQ for CM API. I also think noting this in MDN makes sense.

@jpmedley can we work together to address this in MDN? We have a few notable updates around cross origin CM API calls.

@vabr-g

This comment has been minimized.

Show comment
Hide comment
@vabr-g

vabr-g Mar 14, 2017

I agree with @agektmr that this was not a bug. The spec uses "MAY" intentionally to make this an optional feature. Chrome decided (https://crbug.com/666340) to change how it implements the spec, but it did not implement less of it before.
I am not sure how important it is to note this change, and I have no good ideas about how to note this. Given that 57 is now the stable Chrome, and that Chrome automatically updates, a negligible amount of users will be affected by the pre-57 behaviour soon.

vabr-g commented Mar 14, 2017

I agree with @agektmr that this was not a bug. The spec uses "MAY" intentionally to make this an optional feature. Chrome decided (https://crbug.com/666340) to change how it implements the spec, but it did not implement less of it before.
I am not sure how important it is to note this change, and I have no good ideas about how to note this. Given that 57 is now the stable Chrome, and that Chrome automatically updates, a negligible amount of users will be affected by the pre-57 behaviour soon.

@agektmr

This comment has been minimized.

Show comment
Hide comment
@agektmr

agektmr Mar 14, 2017

Contributor

Luckily I had a chance to chat with @jpmedley in person today about this and he's looking into documenting this in MDN. I will prepare one for Web Fundamentals as well.

Contributor

agektmr commented Mar 14, 2017

Luckily I had a chance to chat with @jpmedley in person today about this and he's looking into documenting this in MDN. I will prepare one for Web Fundamentals as well.

@tjwudi

This comment has been minimized.

Show comment
Hide comment
@tjwudi

tjwudi Mar 14, 2017

Contributor

Sounds good! Thanks for following up.

I think I can change "partially support" back to support, and add this as a note_by_num (general note) in caniuse.com. Does this sound good to you guys?

Contributor

tjwudi commented Mar 14, 2017

Sounds good! Thanks for following up.

I think I can change "partially support" back to support, and add this as a note_by_num (general note) in caniuse.com. Does this sound good to you guys?

@vabr-g

This comment has been minimized.

Show comment
Hide comment
@vabr-g

vabr-g Mar 14, 2017

Sounds good to me. Thanks!

vabr-g commented Mar 14, 2017

Sounds good to me. Thanks!

@tjwudi

This comment has been minimized.

Show comment
Hide comment
@tjwudi

tjwudi Mar 14, 2017

Contributor

@Fyrd Can you help reviewing this PR? =)

Contributor

tjwudi commented Mar 14, 2017

@Fyrd Can you help reviewing this PR? =)

@Fyrd Fyrd merged commit 7b94b55 into Fyrd:master Mar 16, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Fyrd

This comment has been minimized.

Show comment
Hide comment
@Fyrd

Fyrd Mar 16, 2017

Owner

Looks good, thanks!

Owner

Fyrd commented Mar 16, 2017

Looks good, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment