oasis-tcs / odata-abnf

OASIS OData TC: Supporting an ABNF for OData URLs, headers, and literal data values
https://github.com/oasis-tcs/odata-abnf
Other
11 stars 5 forks source link

Validating ABNF with an ABNF parser fails for some ODATA URLs #99

Open thewahome opened 1 year ago

thewahome commented 1 year ago

On Graph Explorer, we use these construction rules together with an ABNF parser to validate URLs entered by our users have the correct syntax.

We have some URLs being marked as invalid even though they are. For instance,

https://graph.microsoft.com/v1.0/sites/m365x55726300.sharepoint.com,f2102bda-0e14-4fc5-bbe1-e65a13cd91a4,6be9dd5c-0640-4465-b955-1528279a0227/lists/7ec9d64f-732f-483b-b1d0-5abb149869f1?$expand=drive,items

We also have some that only pass when a trailing slash is added: https://github.com/microsoftgraph/microsoft-graph-explorer-v4/blob/898e7dc7d78d59a173e0783c2a60a9385d40d5c6/src/modules/validation/abnf.spec.ts#L32

HeikoTheissen commented 1 year ago

The apg-js parser is too greedy and gobbles up the resourcePath as part of the serviceRoot.

ralfhandl commented 1 year ago

AFAIK that can't be avoided for key-as-segment URLs.

@thewahome does the parser accept the "traditional" URL

https://graph.microsoft.com/v1.0/sites('m365x55726300.sharepoint.com,f2102bda-0e14-4fc5-bbe1-e65a13cd91a4,6be9dd5c-0640-4465-b955-1528279a0227')/lists('7ec9d64f-732f-483b-b1d0-5abb149869f1')?$expand=drive,items

HeikoTheissen commented 1 year ago

https://graph.microsoft.com/v1.0/sites('m365x55726300.sharepoint.com,f2102bda-0e14-4fc5-bbe1-e65a13cd91a4,6be9dd5c-0640-4465-b955-1528279a0227')/lists('7ec9d64f-732f-483b-b1d0-5abb149869f1')?$expand=drive,items

The apg-js parser is still too greedy:

Issue 99 fails at 190: https://graph.microsoft.com/v1.0/sites('m365x55726300.sharepoint.com,f2102bda-0e14-4fc5-bbe1-e65a13cd91a4,6be9dd5c-0640-4465-b955-1528279a0227')/lists('7ec9d64f-732f-483b-b1d0-5abb149869f1')?$expand=drive,items
odataUri: https://graph.microsoft.com/v1.0/sites('m365x55726300.sharepoint.com,f2102bda-0e14-4fc5-bbe1-e65a13cd91a4,6be9dd5c-0640-4465-b955-1528279a0227')/lists
.serviceRoot: https://graph.microsoft.com/v1.0/sites('m365x55726300.sharepoint.com,f2102bda-0e14-4fc5-bbe1-e65a13cd91a4,6be9dd5c-0640-4465-b955-1528279a0227')/
..host: graph.microsoft.com
...reg-name: graph.microsoft.com
..segment-nz: v1.0
..segment-nz: sites('m365x55726300.sharepoint.com,f2102bda-0e14-4fc5-bbe1-e65a13cd91a4,6be9dd5c-0640-4465-b955-1528279a0227')
..segment-nz: lists('7ec9d64f-732f-483b-b1d0-5abb149869f1')
.odataRelativeUri: lists
...
ralfhandl commented 1 year ago

Maybe we should reduce the ABNF to URLs relative to the service root.

thewahome commented 1 year ago

The apg-js parser is still too greedy

@HeikoTheissen should I use a different parser?

HeikoTheissen commented 1 year ago

The apg-js parser is still too greedy

@HeikoTheissen should I use a different parser?

No sure whether the problem is with the parser or with the ABNF rules. (Sounds like theoretical computer science, not my area of expertise.) @ralfhandl, can you answer this?

ralfhandl commented 1 year ago

The ABNF rules in this repository are documentation intended for human readers. They are not intended for generating a ready-to-use URL parser.

Many rule alternatives require the reader to know the underlying entity data model. This is especially true for URLs using key-as-segment notation: /foo/bar/baz could address

For client-side validation of URLs you'd need either a two-step approach where a context-unaware parser chunks up the URL into "segment: foo, segment: bar, segment: baz", and a model-aware validator that decides what each segment means. Or a model-aware parser that starts parsing left and then chooses the next rule alternatives based on what it has recognized so far and matched against the model.

There's an open source project for building such a parser, see https://github.com/oasis-open/odata-rapid/tree/main/tools/odataUri, with contributors mainly from Microsoft. Maybe you can join that effort and use it for the Graph Explorer.

ralfhandl commented 5 months ago

See also https://github.com/oasis-tcs/odata-specs/issues/377