A complete re-write of the ABNF parser.

[ldthomas]

This is a complete rewrite of the ABNF parser in abnf.ts. It uses a fixed, pre-generated ABNF grammar object instead of re-generating the object each time a message is parsed. It runs approximately twice as fast as the original parser, is more accurate in URI validation and removes the dependencies on uri-js and valid-url.

Notes:

• packages/siwe/package.json - removed uri-js and valid-url dependencies.
  • note that a global search indicated that uri-js was never used anyway
  • suggest bumping the version to 2.4.0 but versioning is your prerogative
• packages/siwe-parser/package.json - removed uri-js and valid-url dependencies. Added a script for re-generating the fixed ABNF grammar object, packages/siwe-parser/siwe-grammar.js, from the ABNF grammar, packages/siwe-parser/siwe-abnf.txt. npm run apg needs to be run each time a change is made to the ABNF grammar.
  • suggest bumping the version to 2.2.0 but versioning is your prerogative
• even after deleting the dependence on uri-js and valid-url, the package-lock.json files still contain dozens of references to them. Deleting the package-lock.json files and re-generating them from scratch cleans them up considerably. However, I'm aware there may be unintended consequences from that maneuver. Just a suggestion if you want to clean them up.
• packages/siwe/client.ts - contains a workaround for

import { ParsedMessage, parseIntegerNumber } from '@spruceid/siwe-parser';

• as mentioned in issue #177, in my environment a build of the cloned version of this repository fails to find '@spruceid/siwe-parser'. I've not found a fix for this problem yet.
• a large number of unit tests have been added for testing the accuracy of parsing both the message URI and the resource URIs

[ldthomas]

I think I’m done here except for

1. I’ve included a workaround in client.ts. For importing isUri. I had to point to the specific directory and file. I’ll need some advice on how to fix that.
2. I’ve not used your JSON object file methodology for the unit tests. As is mentioned previously, I’m willing to familiarize myself with that and do the (tedious) work of moving all of the test data into the proper JSON format. But I’d like to finish here for now and leave that to a later PR.

[ldthomas]

Trace: After looking at it, redesigning the trace module and publishing a new release of apg-js is way too much work for a feature that is not necessary for the operation of siwe and probably wouldn’t get much use anyway. The code is still there in comments if someone really wants to use the trace feature to debug a message.

Date times: In the process of analyzing the validation of the message objects in SiweMessage I’ve noticed that there is an asymmetry between the way the messages and message objects are currently validated. The messages are validated by parsing them but the parser does not check the semantics of the date times. RFC 3339 defines the syntax of the date time but much of the semantics is simply mentioned in comments. For example, the syntax of the date-month is simply 2 integers but the semantics requires the number to be in the range 1-12. In the case of a message object the semantics are validated with the regex ISO8601. I’ve copied this regex to the parser callbacks.ts and there I use it to validate the date time semantics while parsing. I’ve added unit tests in siwe/lib/objects.test.ts to cover both syntax and semantic date time errors.

validateMessage(): I’ve removed this function altogether and replaced it by parsing the “stringified” version of the message object. In this way, there is complete symmetry between the validation of a message and a message object. (Incidentally, I also fixed a small bug in the toMessage() function so that the case of an empty statement is handled according to spec.)

From the root lint and test both run without errors. I hope this latest commit works for you. If so then I can move on to updating the unit testing to your JSON object methodology.

[ldthomas]

My apologies for an oversight. I’m still getting myself up to speed with the different ways a message and message object are handled in the SiweMessage constructor. I missed a bracket paring and ended up parsing the message twice. Taking a more careful look at what validateMessage() does, the only thing is can see that it is checking that the parser doesn’t is the validation of the date-times. isValidISO8601Date() does a little more than just the regex. On the other hand, there are some message fields that the parser validates that validateMessage() does not:

• scheme
• statement
• Chain ID

So in this latest commit, I’ve moved the complete date-time validation into the parser’s callback functions and fixed the parsing/validation in the message/object blocks so that there is no redundancy. My intention here now is that all fields are both syntactically and semantically correct if the parser succeeds.

BTW siwe/lib/utils.ts isValidISO8601Date(), which I copied into the parser’s callback functions has a bug. Line 67

if (inputDate) {

should read

if (inputMatch) {

[ldthomas]

Thanks. Missed the debugging code left in this test.

[ldthomas]

Hi @sbihel. Anything else you need? From my end I'm done here unless there is something else required for a merge. I'm waiting on that before I begin working on converting the unit tests to your JSON file methodology.

[sbihel]

Thank you, it looks great. In addition to the clean-up of SiweErrorType:

BTW siwe/lib/utils.ts isValidISO8601Date(), which I copied into the parser’s callback functions has a bug

Would you mind fixing it as part of this PR please?

before I begin working on converting the unit tests to your JSON file methodology

Thank you for doing that, you can just move the tests from objects.test.ts and t-spec.test.ts -- the other tests are a bit too "low-level" and won't provide much value to other implementations.

[ldthomas]

The fix to siwe/lib/utils.ts is done.

Converted siwe/lib/objects.test.ts to JSON file methodology.

I had already converted all of the siwe-parser/lib/t-*.test.ts to the JSON methodology and they are all included in unit.test.ts. If others are only interested in the specification tests they only need to use the valid-specification.json file and ignore the others. But I think having all of the unit tests available is nice for a quick check that anytime a change is made it hasn’t broken anything.

I really don’t want to mess with the SiweErrorType enum. The siwe-parser, past and present, never uses them. They only seem to be used in the siwe client and since I don’t really know what is going on there I really need to stay out of it. However, looking into that I did notice a big problem in client.test.ts. I’m not great with async programming so I’m not going to attempt to fix it but have a look at this code:

(n, test) => { try { new SiweMessage(test); } catch (error) { expect(Object.values(SiweErrorType).includes(error)); } } It can never fail and really does no testing at all. 1) If for some reason SiweMessage() unexpectedly succeeds, the test will just fall through without announcing failure. 2) expect(true) and expect(false), without any expect matchers, both succeed. So no matter what the value of Object.values(SiweErrorType).includes(error) the test succeeds. Not to mention that SiweMessage() doesn’t throw SiweErrorType anymore. My suggestion is to just replace this with an expect().toThrow() test. But I really don’t know what is going on with the tests that use async so I’m not going to touch that. Just pointing it out.