Restrict gltf URIs to relative paths by default; add --allowAbsolute as fallback to insecure behavior.

[Fordi]

Reference implementation for #673.

• Makes readFile fail by default for files outside of resourceDirectory
• Provides an --allowAbsolute flag / .allowAbsolute option to disable this failure, and to emit a warning on the CLI about insecure gltf buffer URIs.
• Emits a warning when resourceDirectory is undefined and the gltf references an absolute path, but otherwise continues normally.
• Documents the change in README

[javagl]

I tried this out with the example glTF that was given in the issue (adjusted for Windows).

And it seems to resolve the issue: With the current main state, it inlines some password.txt into the target glTF. With the state from this branch, it bails out with the error.

---

Specifying --allow-absolute restores the original behavior.

There's a minor inconsistency here:

• The error message calls it allowAbsolute
• The command line help calls it allow-absolute

Both seem to work (which is likely a side-effect of the yargs argument parser). But it may be better to (not rely on that and) use consistent style in the help and error message. I'd vote for allowAbsolute, which is more consistent with the existing flags (like separateTextures etc).

The allowAbsolute flag is also not mentioned in the "Command-Line Flags" in the README.md. It should be added there as well.

---

I also tried to "break" it 😈 , with obscure combinations of input directories and URL paths. But it seems like the combination of path.relative and startsWith("..") catches all cases 🤞

And it may, in fact, catch too many cases: When the URI is something like
"uri": "../password.txt"
then this is not an absolute URI, but still rejected here. Such URLs are actually legitimate, and not uncommon. They are often used to refer to some common ../texture.png or ../animation.bin that is used by multiple glTFs.

This might not be an issue here: The default behavior should be safe, and gltf-pipeline should not mess around in the file system. If someone has such a file, the allowAbsolute can be used to process it anyway.

EDIT: Conversely, using an absolute path like
"uri": "file:///C:/tmp/password.txt"
in a file like c:/tmp/example.gltf does not complain. It's an absolute path, but resolved to a relative one.

---

This fix currently focusses on the command line usage. If the goal was also to apply this when this is used as a library (where the resourceDirectory might not be present), I'll have to run more tests.

I assume that this was not the intention for now.

---

Possibly somehow related to this:

• Files with external http:// uri refererences can't be procesed with gltf-pipeline #666
• We considered to create some sort of abstraction for the file access. (This doesn't seem to be tracked here, but came up e.g. in glTF model references separate files but no resourceDirectory is supplied Error when upgrading legacy 3d tiles 3d-tiles-tools#109 ). Instead of juggling with paths, the gltf-pipeline should probably receive some function that receives a string and returns a Buffer, and that can resolve either against the file system, or against ... anything else. However, given that the future of gltf-pipeline is not clear, no effort went into that yet.

[Fordi]

And it may, in fact, catch too many cases: When the URI is something like "uri": "../password.txt"

A less aggressive check might be uri.startsWith('file:///') || uri.startsWith('/') before we create a new URL(). I can push that now.

This fix currently focusses on the command line usage. If the goal was also to apply this when this is used as a library (where the resourceDirectory might not be present), I'll have to run more tests.

You're correct; I wasn't thinking about the programmatic case at all. It seems to me that, going forward, we should recommend to the developer to either:

1. Use buffers if they're generating a glTF object for processing, or
2. Set a resourceDirectory if passing in a glTF file from some path context (like the CLI does)

Barring that, to handle existing code that might not already follow those recommendations, the safest change would be to treat !defined(resourceDirectory) the same as allowAbsolute === true. This way, the developer gets the well-known behavior when they don't provide a resourceDirectory. I'll do that, emit a warning, write some tests around it, add docs and update the PR description.

We considered to create some sort of abstraction for the file access. (This doesn't seem to be tracked here, but came up e.g. in CesiumGS/3d-tiles-tools#109 ). Instead of juggling with paths, the gltf-pipeline should probably receive some function that receives a string and returns a Buffer, and that can resolve either against the file system, or against ... anything else. However, given that the future of gltf-pipeline is not clear, no effort went into that yet.

Naming the critical field sourceUrl instead of resourceDirectory, and making the necessary URL arithmetic changes internally would probably go a long way here. That is, the lib consumer wouldn't be responsible for deriving a directory from the source path; they'd just pass in the URL the thing came from; protocol wouldn't matter.

[javagl]

The first review pass contained a few "nitpicks" that have been resolved now.

Beyond that: The primary purpose is to get rid of the "file injection". And once we got gid of that, the PR should be ready to go. Any effort beyond the "least effort fix" here would be hard to justify, in view of #670 . So I do not want to hold up this PR. We should get this fix in soon, and not go overboard.

---

But for completeness, I still want to emphasize how far one could go when reviewing this from the perspective of a pedantic nerd, in terms of functionality and sustainability:

• The idealistic view of generalizing things from files to "arbitrary data sources" would involve a refactoring, not in the scope of this PR.
• The fact that fileURLToPath and pathToFileURL carry a note that they have only been added for node versions <10.12 (!) (with the current requirement in the package JSON being "node": ">=16.0.0) is a prime example of "legacy code". (Are these functions tested? Have they been modified after being extracted from the Node code?...).
• What is currently done for the "absolute" handling in readFile could reasonably be moved into a dedicated function with 10 different tests...
• The mix of path (the library) and URL (the class) always causes trouble. I've read a lot about that, but would not claim to have the "big picture" or "know all caveats" here - probably because I read a lot about that: The different handlings of leading and trailing slashes makes it nearly impossible to get this "right" for all cases
• A URI like "uri": "https://example.com/index.html" is valid, but ... apparently never worked (but this can easily be justified)

---

Back to getting this merged: There currently is a test failure on Windows.

The resourceDirectory here becomes C:\\Develop\\CesiumGS\\gltf-pipeline\\specs\\data\\2.0\\box-textured-separate\\, which counts as "not absolute" based on this check, and when fileURLToPath is called below, it bails out because the protocol is not file:.

If desired, I can try to allocate some time to investigate this and create a fix. (I might have a hard time resisting the urge to throw in some other cleanups with that, but I can try...)

---

An aside: I just opened CesiumGS/3d-tiles-tools#184 - this LFI might also be an issue for the 3D Tiles Tools.

[Fordi]

The resourceDirectory here becomes C:\Develop\CesiumGS\gltf-pipeline\specs\data\2.0\box-textured-separate\, which counts as "not absolute" based on this check, and when fileURLToPath is called below, it bails out because the protocol is not file:.

A URI like "uri": "https://example.com/index.html" is valid, but ... apparently never worked (but this can easily be justified)

I'm gonna be bold here and say both of those are correct behavior - though, fileURLToPath is not called on resourceDirectory; pathToFileURL is. But passing C:\\... into uri should absolutely fail.

The "uri" field is not an OS path; it's a URI. We would not expect a strongly non-URI thing like an absolute Windows path to be at all well-behaved. We also would not expect a lightly non-URI thing like an absolute POSIX path to be perfectly well-behaved - the semantics are similar, but POSIX paths don't need escaped like URIs do.

Meanwhile, the spec only mandates relative URIs and data URLs.

The fact we're allowing absolute URIs at all is allowed by the spec ("Client implementations MAY optionally support additional URI components. For example ... file:// schemes, ... absolute paths ..."), but is flagged for portability.

The fact that fileURLToPath and pathToFileURL carry a note that they have only been added for node versions <10.12 (!) (with the current requirement in the package JSON being "node": ">=16.0.0) is a prime example of "legacy code". (Are these functions tested? Have they been modified after being extracted from the Node code?...).

*blink*

I had to read that a few times before I understood what you meant; I honestly did not notice those weren't the node:url versions of the functions 😭 . Thankfully, they're only ever pulled in by readResources, so the number of things that could break from dropping the ancient duckpunch is just the one, and its the thing I'm looking at.

The idealistic view of generalizing things from files to "arbitrary data sources" would involve a refactoring, not in the scope of this PR.

Understood, and agreed. Wasn't really suggesting it for this PR; just musing on the asides you listed... though...

The mix of path (the library) and URL (the class) always causes trouble. I've read a lot about that, but would not claim to have the "big picture" or "know all caveats" here - probably because I read a lot about that: The different handlings of leading and trailing slashes makes it nearly impossible to get this "right" for all cases

...is exactly the case for planning to switch to a sourceUrl instead of a resourceDirectory. Because new URL('file.json', 'file:///path/to/referencingFile.json') is unambiguous, while new URL('file.json', 'file:///path/to/') and new URL('file.json', 'file:///path/to') look similar, but are a whole path element different.

glTF deals in URIs, so it makes sense to work in that space as much as possible, particularly when doing path arithmetic. This project may never get a chance to get to it, but all the same.

[javagl]

But passing C:\... into uri should absolutely fail.

Maybe I buried that in that wall of text: Of course a "URI" like C://... is not supposed to be supported in any way (as you said, it's not even a URI to begin with). But the specs are creating these with the toAbs helper, at least on Windows:

This is caused by the fileURLToPath. Without that, when toAbs is implemented as

    const toAbs = (uri) => {
      return new URL(uri, pathToFileURL(boxTexturedSeparate2Path)).toString();
    }

then it does use the file:// prefix (and passes that test).

So maybe this point is/was just a side-effect from creating the specs.

(As I said in the initial review: I might have to think through further cases for the library usage. Given that there are two "flavors" for a URI to count as "abolute" - namely, via the leading "/" or the leading "file:///" - one could even consider to just say

// Use a leading slash to let it count as absolute:
gltf.buffers[0].uri = "/box-textured-separate.bin";

But nitpickers could then complain about the missing test case for the leading file:///...)

[Fordi]

Oh! Yeah. Didn't think about that for the test case (I mostly dev inside WSL). I'll adjust.

[javagl]

With the latest state, the tests are passing on Windows as well.

(And I also checked whether the actual issue is still resolved. Although I think that the last changes mainly revolved around the specs (and cleanups like removing that FileUrl), I wanted to make sure that they didn't affect the CLI part...)

So I think this is ready to merge.

(I'm not sure wheher I'm supposed to do this, or whether there is someone who counts more as an "active maintainer". In doubt, I'll just hit 'Merge' soon, assuming that I'm not stepping on anyone's toes...)

[javagl]

Fixed a security issue (with the option to retain the original behavior). Added documentation. Removed legacy code. Added specs. I don't see why anyone would not want to merge this.