1007724 - [NFC] NDEF Record validation implementation

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Krzysztof Mioduszewski[:tauzen]]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/34.0.1847.131 Safari/537.36

Steps to reproduce:

NDEF record validation for messages which originate from Gaia and Gecko should be implemented. AOSP is performing the following validation: http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/android/4.1.1_r1/android/nfc/NdefRecord.java#864
Similar mechanics should be implemented in Gecko.

Yoshi's suggestion: "it should be done in MozNDEFRecord so it could validate the data structure and throw error/exception when the caller (could be Gaia side or Gecko side NfcContentHelper ) constructs it with invalid data". This requires that MozNDEFRecord will be passed in both directions.

We should also agree on:
1. Handling of invalid NDEF records detected by the device.
2. Way of notifying user that a malformed record was discovered (new UI? reuse of current components?).
3. Handling of invalid NDEF created by an application - I assume that an for the app should be enough, no additional notification for the user required.

Comment 1

[Krzysztof Mioduszewski[:tauzen]]

Additional info from Yoshi, adding for completness:

Also there's one thing different I found when looking into AOSP
MozNDEFRecord allows type, and id could be null, 
http://dxr.mozilla.org/mozilla-central/source/dom/webidl/MozNDEFRecord.webidl?from=MozNDEFRecord.webidl#29
whereas Android NdefRecord will convert them to empty array.
http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/android/4.1.1_r1/android/nfc/NdefRecord.java#517

This should also be taken care when we try to add validateTnf into Gecko.

And we already have one bug for the EMPTY_TNF case in Bug 997576
Anyone would like to implement validateTnf should also check the test case in Bug 997576.

Comment 2

[Krzysztof Mioduszewski[:tauzen]]

One thing which I've noticed is that in case of 'nfc-manager-tech-discovered' message we don't create MozNDEFRecord, this should be also take care of if we would implement the validation in MozNDEFRecord.

Comment 3

[Garner Lee]

One thing of note. Although I like the use of of using interfaces, there's Bug 964190 to move MozNDEFRecord to dictionary types (which also has API benefits). This may conflict with the other bug's goal.

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #2)
> One thing which I've noticed is that in case of
> 'nfc-manager-tech-discovered' message we don't create MozNDEFRecord, this
> should be also take care of if we would implement the validation in
> MozNDEFRecord.

nfc-manager-tech-discovered is a system message fired from Chrome Process, whereas MozNDEFRecord is defined in Content Process, so we cannot call 'new MozNDEFREcord' from Nfc.js.
Therefore you can see we use a plain JS object there.

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: tnf validation

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: marionette tests.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8481141 [details] [diff] [review]
Part 1: tnf validation

>+MozNDEFRecord::ValidateTNF(const MozNDEFRecordOptions& aOptions,
>+                           ErrorResult& aRv)
>+{
>+  // * The TNF field MUST have a value between 0x00 and 0x06.
>+  // * The TNF value MUST NOT be 0x07.
>+  // These two requirements are already handled by WebIDL bindings.
>+
>+  // If the TNF value is 0x00 (Empty), the TYPE_LENGTH, ID_LENGTH, and
>+  // PAYLOAD_LENGTH fields MUST be zero and the TYPE, ID, and PAYLOAD fields
>+  // MUST be omitted from the record.
>+  if ((aOptions.mTnf == TNF::Empty) &&
>+      (aOptions.mType.WasPassed() || aOptions.mId.WasPassed() ||
>+       aOptions.mPayload.WasPassed())) {
>+    NS_WARNING("tnf is empty but type/id/payload is not null.");
>+    aRv.Throw(NS_ERROR_INVALID_ARG);
>+    return false;
>+  }
>+
>+  // If the TNF value is 0x05 (Unknown) or 0x06(Unchanged), the TYPE_LENGTH
>+  // field MUST be 0 and the TYPE field MUST be omitted from the NDEF record.
>+  if ((aOptions.mTnf == TNF::Unknown || aOptions.mTnf == TNF::Unchanged) &&
>+      aOptions.mType.WasPassed()) {
You're not testing that anything is 0 here, but just whether mType was passed, so please update the comment.

>+    aRv.Throw(NS_ERROR_INVALID_ARG);



Could we perhaps throw INVALID_STATE_ERR? That would be from DOM spec, NS_ERROR_INVALID_ARG is Gecko internal

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Olli Pettay [:smaug] from comment #7)
> >+  // If the TNF value is 0x05 (Unknown) or 0x06(Unchanged), the TYPE_LENGTH
> >+  // field MUST be 0 and the TYPE field MUST be omitted from the NDEF record.
> >+  if ((aOptions.mTnf == TNF::Unknown || aOptions.mTnf == TNF::Unchanged) &&
> >+      aOptions.mType.WasPassed()) {
> You're not testing that anything is 0 here, but just whether mType was
> passed, so please update the comment.
>
Yeah, these comments are copy from specification, I'll make them more clear.
 
> >+    aRv.Throw(NS_ERROR_INVALID_ARG);
>  
> Could we perhaps throw INVALID_STATE_ERR? That would be from DOM spec,
> NS_ERROR_INVALID_ARG is Gecko internal

But there's no state in this data structure (NDEFRecord), but I'll check if there are more appropriate DOM error code.

Thanks

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: tnf validation v2.

addressed comments from smaug.
- Update comments
- throw INVALID_STATE

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://hg.mozilla.org/integration/b2g-inbound/rev/7ec42e511e82
https://hg.mozilla.org/integration/b2g-inbound/rev/c29b308ace9c

try: https://tbpl.mozilla.org/?tree=Try&rev=83615a9024d9

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7ec42e511e82
https://hg.mozilla.org/mozilla-central/rev/c29b308ace9c