Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discourage use of fragment in URI #1100

Merged
merged 3 commits into from
Sep 10, 2018
Merged

Discourage use of fragment in URI #1100

merged 3 commits into from
Sep 10, 2018

Conversation

lexaknyazev
Copy link
Member

Since we treat image.uri and buffer.uri as "URI" by RFC3986, we should explicitly discourage cases like this:

{
  "images": [
    {
      "uri": "image#2.png"
    }
  ]
}

When filename contains # symbol, it must be encoded as a part of path:

{
  "images": [
    {
      "uri": "image%232.png"
    }
  ]
}

@donmccurdy
Copy link
Contributor

This may be mixing terms slightly — the fragment identifier is only one example of a broader case. Unless there's something specific about fragment URIs that presents a problem?

Bad:

  • image#2.png
  • image?2.png

Fine:

  • image.png#v=2
  • image.png?v=2

I think what we should clarify is that authoring tools — not client implementations — are responsible for correct URI-encoding. If a ? or # is in the URI, it should be treated as a query or fragment parameter respectively by the client.

@lexaknyazev
Copy link
Member Author

Unless there's something specific about fragment URIs that presents a problem?

I'd say that while query parameters may be fine for web usage, fragment URIs make zero sense for images and binary buffers.

If a ? or # is in the URI, it should be treated as a query or fragment parameter respectively by the client.

Agree in general. This will work for URI schemes that support query and fragment parameters, but this could be a problem for relative URIs that meant to be used in different contexts, e.g., using the same glTF asset on web and on desktop: query and fragment parameters don't exist for file:// scheme (RFC 8089).


Maybe, we should also refine language around "relative", because, by RFC 3986 definition, relative URIs can have absolute path. Consider something like this:

{
  "images": [
    {
      "uri": "/image.png"
    }
  ]
}

@donmccurdy
Copy link
Contributor

I'd say that while query parameters may be fine for web usage, fragment URIs make zero sense for images and binary buffers.

Fragment URIs are not passed to the server, but they are passed to Service Workers (JS workers that run apart from the main thread, and often handle resource caching). That's an unlikely use case, I admit, but since we want to support query parameters anyway I think it would be ideal to treat both the same way.

Maybe, we should also refine language around "relative", because, by RFC 3986 definition, relative URIs can have absolute path.

+1, good point.

@lexaknyazev
Copy link
Member Author

Fragment URIs are not passed to the server, but they are passed to Service Workers...

Fair point, let's treat query and fragment alike.

@lexaknyazev
Copy link
Member Author

lexaknyazev commented Sep 14, 2017

@donmccurdy
Please review updated URI section.


Client implementations are required to support only embedded resources and relative external references (in a sense of [RFC3986](https://tools.ietf.org/html/rfc3986#section-4.2)). Clients are free to support other schemes (such as `http://`) depending on expected usage.
- **Relative references** as defined by [RFC 3986, Section 4.2](https://tools.ietf.org/html/rfc3986#section-4.2) with additional restriction that only `path-noscheme` part is allowed (i.e., path must not start with `/`, query and fragment parts aren't allowed). All reserved characters must be percent-encoded.
Copy link
Contributor

@donmccurdy donmccurdy Sep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite clear on how to interpret this — we're saying that these things (absolute URI schemes, query and fragment parameters, ...) are invalid glTF, but (according to implementation note below) that clients can choose to support them anyway?

Or do you mean to say, clients must support at least relative URLs that don't include query/fragment, but may also choose to support more? And therefore authoring a glTF with query/fragment is not invalid, just less portable?

Copy link
Member Author

@lexaknyazev lexaknyazev Sep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one:

Or do you mean to say, clients must support at least relative URLs that don't include query/fragment, but may also choose to support more? And therefore authoring a glTF with query/fragment is not invalid, just less portable?

However, I think that usage of query/fragment should issue validation notice. Could you refine PR language so that intention is clear?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about:

  • Relative URI paths — or path-noscheme as defined by RFC 3986, Section 4.2 — without scheme, authority, or parameters. Reserved characters must be percent-encoded, per RFC 3986, Section 2.2.

    Implementation Note: Clients can optionally support additional URI components. For example http:// or file:// schemes, authorities/hostnames, absolute paths, and query or fragment parameters. Assets containing these additional URI components may be less portable.

@alteous
Copy link

alteous commented Sep 15, 2017

In the Rust gltf crate we sometimes return a special #bin fragment, meaning the BIN section binary glTF. Is this sort of behaviour discouraged?

@lexaknyazev
Copy link
Member Author

@alteous
This section applies to JSON contents, it doesn't define runtime behavior.
However, I can see an issue with returning #bin. Let's say that some application uses custom URI resolver that relies on fragments, while GLB asset has both internal BIN section and an external binary buffer which has #bin as its buffer.uri. In such case (probably extremely rare), it may be hard to avoid ambiguity.

@pjcozzi
Copy link
Member

pjcozzi commented Sep 15, 2017

I think what we should clarify is that authoring tools — not client implementations — are responsible for correct URI-encoding.

Spec language is up to you guys, but I agree with the idea.

Merge when ready.

@lexaknyazev
Copy link
Member Author

@donmccurdy @bghgary
OK to merge?

@bghgary
Copy link
Contributor

bghgary commented Sep 18, 2017

This looks fine to me.

Speaking of URIs, should we also disallow referencing parent files (e.g. using ../../..)? This was brought to my attention during a security pass.

@donmccurdy
Copy link
Contributor

... should we also disallow referencing parent files (e.g. using ../../..)?

It should not be part of the two cases clients must support, I think, and should be considered "less portable". But i'm not sure about disallowing it entirely.

@lexaknyazev
Copy link
Member Author

We should definitely add a note about security concerns (e.g., loading an asset with parent paths from untrusted source). I'm not sure about disallowing such URIs on spec level.

Also related: should we encourage normalization on export (e.g., dir1/./dir2/../tex.png to dir1/tex.png)?

@bghgary
Copy link
Contributor

bghgary commented Sep 18, 2017

should we encourage normalization on export (e.g., dir1/./dir2/../tex.png to dir1/tex.png)?

In other words, canonicalized paths. I say yes, especially if caching is happening using this URI.

@emackey
Copy link
Member

emackey commented Aug 30, 2018

Looks like this got a green light but was never actually merged. Still OK to merge?

@lexaknyazev
Copy link
Member Author

@emackey We can merge this as is, or enhance it with language about URI normalization first.

@emackey
Copy link
Member

emackey commented Sep 10, 2018

I think this is good to go. If anyone wants to change the language further, feel free to open a new PR.

@emackey emackey merged commit 415c8ea into master Sep 10, 2018
@emackey emackey deleted the lexaknyazev-patch-1 branch September 10, 2018 14:05
@lexaknyazev lexaknyazev mentioned this pull request Oct 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants