1142526 - Treat the innerHTML attribute as the string value

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::L10n, defect)

Description

[Staś Małolepszy :stas]

When bug 994357 lands, we will be able to use HTML in regular translation values.  While bug 1027117 is about removing innerHTML from .properties, we can also improve the security of existing translations by treating innerHTML attributes as the canonical value of the string.

Comment 1

[Staś Małolepszy :stas]

Attachment: Patch

Something like this.  The patch is based on my DOM overlays branch for bug 994357.  Zibi, what say you?

Comment 2

[GitHub Autolander Bot]

Attachment: [gaia] stasm:1142526-innerHTML-value > mozilla-b2g:master

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8576693 [details] [diff] [review]
Patch

Review of attachment 8576693 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, that sounds good. We should also file a bug to migrate all uses of innerHTML to normal value, but that's lower priority.

Would it make sense to add a deprecation notice here?

Comment 4

[Staś Małolepszy :stas]

Comment on attachment 8576701 [details] [review]
[gaia] stasm:1142526-innerHTML-value > mozilla-b2g:master

https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=2dd503651c08166bc0a6b5b917ff0571fde87ae6

I added the deprecation warning, too.

Comment 5

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/685c5c174ff9e4a9358c05050cecd276defe4b52

Comment 6

[Staś Małolepszy :stas]

L10n.js: https://github.com/l20n/l20n.js/commit/f8eb6e94281030e62c7c7cb333c95040869d30b9

Comment 7

[James Lal [:lightsofapollo]]

https://github.com/mozilla-b2g/gaia/commit/68a2f9c80cab97551a3e555cd15773a683365f68

backed out due to infra issues (I am going to reland)

Comment 8

[Staś Małolepszy :stas]

James, is there a bug tracking the infra issues we ran into after this landed?

Comment 9

[Staś Małolepszy :stas]

(In reply to Staś Małolepszy :stas from comment #8)
> James, is there a bug tracking the infra issues we ran into after this
> landed?

Hey James, do you have any updates about the infra problems we ran into here?

Comment 10

[Staś Małolepszy :stas]

I reverted this in the l10n.js upstream repo:

https://github.com/l20n/l20n.js/commit/1309309510a8e74a884cf3ff791e66ce39e71611

Comment 11

[James Lal [:lightsofapollo]]

Clearing NI here things should be good to land (and I believe have now)

Comment 12

[Staś Małolepszy :stas]

James, I meant your comment 7 in which you backed out this patch citing infra issues.  Is this PR good to re-land now?  I believe the problem was one of the buildbots running an old (pre-36) version of Gecko which suffers from bug 1084502.

Comment 13

[GitHub Autolander Bot]

Attachment: [gaia] stasm:1142526-innerHTML-value-relanding > mozilla-b2g:master

Comment 14

[Staś Małolepszy :stas]

Comment on attachment 8598054 [details] [review]
[gaia] stasm:1142526-innerHTML-value-relanding > mozilla-b2g:master

Carrying over the r+.

Comment 15

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/e897f4d24208e5428beaeb9b5d38dda27e366d3c

Comment 16

[Staś Małolepszy :stas]

Re-landed in l20n.js: https://github.com/l20n/l20n.js/commit/0f2411692cd6eb40a5aa42742adc51de26c0a16a

Comment 17

[Staś Małolepszy :stas]

Removing the old ni? request on :lightsofapollo.