Skip to content

loader: reject remote file URL hosts#8715

Open
immanuwell wants to merge 1 commit into
open-policy-agent:mainfrom
immanuwell:fix/file-url-host-validation
Open

loader: reject remote file URL hosts#8715
immanuwell wants to merge 1 commit into
open-policy-agent:mainfrom
immanuwell:fix/file-url-host-validation

Conversation

@immanuwell
Copy link
Copy Markdown

file://server/... was being treated like a local path because OPA dropped the file URL host and used only url.Path

That can rewrite the user input into a different local file path, which is pretty sketchy

This change only accepts local file URLs:

  • file:///...
  • file://localhost/...

It rejects other file URL hosts and adds regression tests

Repro:

  1. Create a file:
    printf '{"x":1}\n' > data.json
  2. Before this fix, this could load the local file even though the URL host is server:
    opa eval --data "file://server$(pwd)/data.json" 'data.x'
  3. After this fix, it fails fast with:
    file URL host must be empty or localhost

Signed-off-by: immanuwell <pchpr.00@list.ru>
Copy link
Copy Markdown
Member

@anderseknert anderseknert left a comment

Choose a reason for hiding this comment

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

TIL. Looks good to me, although I think the error message could probably be more helpful. Will leave it up for others to have a say as there could be some concerns about breaking someone's deployments I suppose.

Comment thread internal/file/url/url.go
}

if url.Host != "" && url.Host != "localhost" {
return "", fmt.Errorf("file URL host must be empty or localhost: %v", path)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd bet that 9 times out of 10, this is just someone who missed a / after file:// and not someone who tried to provide a hostname or even knew that was possible. Perhaps the error message should better help them along?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants