Prevent Directory Traversal in YAML Content Validation

Siddhant-K-code commented 5 months ago

Description

This PR fixes a vulnerability in the YAML content validation process. The changes ensure that paths containing ../ are not allowed, preventing directory traversal attacks.

References

fixes #199

Review Checklist

[x] I have clicked on "allow edits by maintainers".
[ ] I have added documentation for new/changed functionality in this PR or in a PR to openfga.dev [Provide a link to any relevant PRs in the references section above]
[x] The correct base branch is being used, if not main
[ ] I have added tests to validate that the change in functionality is working as expected

ewanharris commented 5 months ago

Thanks for the PR @Siddhant-K-code, I've just pushed #229 which should fix the CI failure we're seeing.

If you're also comfortable with handling the Go side of this fix, would you mind adding a test? It would go inside this file (as our tests are generated from the yaml files it will be automatically added to the Go suite so would need a fix there too)

Siddhant-K-code commented 5 months ago

@ewanharris I've committed d086abf to add the tests, is that the only thing required, do I need to run any script to generate test files?

Also, will rebase my PR after #229 is merged.

ewanharris commented 5 months ago

Thanks for adding that! As we've added the test it's now being ran against the Go language package, would you mind also applying the fix over there?

I'm also wondering now whether a check for ../ is enough:

We should also consider Windows path separators (i.e. ..\\) as I don't believe we have a requirement to use posix separators in our API validation
I think if we're truly wanting to round this out, we also need to handle replacing URI encoded characters to prevent someone writing %2e%2e%2fname.fga (I believe Node's fs doesn't handle this but I'm not sure if the APIs we use in VS Code and potentially other environments do).

Siddhant-K-code commented 5 months ago

Thanks for adding that! As we've added the test it's now being ran against the Go language package, would you mind also applying the fix over there?

Will need to explore that. Can you point to any code reference, anyway I would also check it out.

I'm also wondering now whether a check for ../ is enough:

We should also consider Windows path separators (i.e. ..\\) as I don't believe we have a requirement to use posix separators in our API validation

I think if we're truly wanting to round this out, we also need to handle replacing URI encoded characters to prevent someone writing %2e%2e%2fname.fga (I believe Node's fs doesn't handle this but I'm not sure if the APIs we use in VS Code and potentially other environments do).

Yeah, there could be many cases like this 🫠. It would be tricky to handle all.

ewanharris commented 5 months ago

The Go package lives in this same repo under pkg/go, the code block we'd need to add the check to is here.

For the Windows/encoded characters piece, I think if we did something like below that should tick off the different variants, (I also added an absolute path check as that was also been raised as part of this).

// replace URI encoded characters
let filename = file.value.replace(/%2e/ig, ".");
filename = filename.replace(/%2f/ig, "/");
filename = filename.replace(/%5c/ig, "\\");

// error if the filename indicates a potential directory traversal or absolute path
if (filename.includes("../") || filename.includes("..\\" ) || filename.startsWith("/")) {
    // error
}

Siddhant-K-code commented 5 months ago

Tried some things in 00aa1a4

Siddhant-K-code commented 5 months ago

Ahh, tests are failing now ;_;

Will debug it later 🫠

Siddhant-K-code commented 5 months ago

There seem to be more problems in this. 🤔 Will check it after my dinner

Siddhant-K-code commented 5 months ago

@ewanharris I am not sure, how are other tests are getting affected 🤔 Unrelated changes functions' tests also failing, can you help me out?

Siddhant-K-code commented 5 months ago

Thanks a lot, @ewanharris for helping me out, I've tested the tests, and builds in my Gitpod workspace, and it seems to be passing now. Requires final approval to run the GHA workflows.

Siddhant-K-code commented 5 months ago

ahhh yehahhhh, loved the green CI :)

ewanharris commented 5 months ago

I just pushed to the branch in order to squash the commit, thanks again for working through this @Siddhant-K-code, it's great to see your contributions!

Siddhant-K-code commented 5 months ago

Thank you so much, @ewanharris ! I really appreciate your help and your kind words. It's been a pleasure contributing to all the OpenFGA projects, and I'm glad to have your support. Looking forward to more future collaborations ❤️

openfga / language