Re: [web-nfc] reg-name ABNF is unspecified (#350)

Thanks for adding, and thanks for @beaufortfrancois pinging, as this slipped past due to the holidays (and holiday deadlines :D)

Overall, I think the approach in having an explicit validation step is good, in that it makes it much clearer (and easier to write tests for interop issues).

> 1. If input is not a USVString or it is empty, or its length exceeds 255 bytes, return false.

Is 255 bytes a limit from NFC-RTD? Or is this trying to limit for domain names? If it's trying to limit for domain names, I believe you want the 'size check' to be after Step 5. Prior to that, you're dealing in USVString, which corresponds to Unicode scalar values, which means 'bytes' vs' characters' is messy, to say the least! It also means that IDNs that are < 255 bytes in U-Label form but > 255 bytes in A-Label form would be incorrectly permitted.

> 2. Let domain be the input from the start of input up to but excluding the last occurrence of U+003A (:), or null if that is not found.

I don't have access to the NFC-RTD specification, so I'm going based on how I read Step 7 (so hopefully this makes sense). My understanding is that `type` MUST NOT contain `'U+003A (:)`, that this comes from the NFC-RTD specification, and so there's no risk of a type being `domain.example:foo:bar` with the intended _domain_ of `domain.example` and the intended _type_ of `foo:bar`, right?

Apologies if this is something answered in NFC-RTD, I'm not used to Web specifications making normative dependencies on pay-to-access specs.

> Let asciiDomain be the result of running domain to ASCII on domain.

You've omitted the optional boolean beStrict, which means it defaults to false. This permits certain [implementation-dependent characters](https://unicode.org/reports/tr46/#UseSTD3ASCIIRules). It's unclear if that was intended here - that is, the relationship between the *external type* and the current origin are unclear me, but I would suspect that NFC-RTD makes a normative dependency on DNS, which means that beStrict should be true, even if UAs allow the current domain to contain a `U+005F (_) LOW LINE (underbar)`. My general advice is "be strict in what you accept" - if you're validating caller textual input or NFC tag input (versus the current origin/domain), then omitting `_` seems entirely reasonable.


The only other bit I would flag, and I fully admit I'm not an expert here and so you'll want to find someone better versed in the Web Platform idioms, is where/how an error should be signaled for an unsupported `recordType`. Right now, if I understand correctly, I can create a 'valid' `NDEFRecord`  via `NDEFRecordInit`, but whose `recordType` is invalid/unparsable. That error is not actually signaled back to the consumer until they try [`NDEFWriter.push`](https://w3c.github.io/web-nfc/#the-push-method), which invokes the [`create NDEF message`](https://w3c.github.io/web-nfc/#dfn-create-ndef-message) algorithm, which invokes [`create NDEF record`](https://w3c.github.io/web-nfc/#dfn-create-ndef-record), which invokes [`validate external type`](https://w3c.github.io/web-nfc/#dfn-validate-external-type).

Because you [expose the constructor](https://w3c.github.io/web-nfc/#idl-index) for `NDEFRecord`, should you be parsing/validating the type then? The URL class, for better or worse, has setters which do the parsing/validation. I'm not sure why the `NDEFRecord` type is read-only (maybe that's the current best practice), but it seems that you at least want to validate during that constructor as well, to ensure a client only encounters well-formed `NDEFRecord`s

-- 
GitHub Notification of comment by sleevi
Please view or discuss this issue at https://github.com/w3c/web-nfc/issues/350#issuecomment-570312505 using your GitHub account

Received on Thursday, 2 January 2020 19:18:49 UTC