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

add validation to allowed-origins to include path #265

Merged
merged 4 commits into from Jul 7, 2019

Conversation

nicksrandall
Copy link
Contributor

@nicksrandall nicksrandall commented Jun 28, 2019

The use-case here is that we store our assets in AWS S3 and we'd like to restrict access to a certain bucket.

So, instead of starting service using -allowed-origins https://s3.amazonaws.com flag which isn't very useful in our case because this would allow anybody to use our service with any asset on s3, we could instead extend the allowed origins to something like -allowed-origins https://s3.amazonaws.com/some_bucket/.

Then, if a request comes in to fetch image at url https://s3.amazonaws.com/some_bucket/path/image.jpg, we would complete this request because it contains the host and basepath listed in allowed origins.

If the request had asked for https://s3.amazonaws.com/different_bucket/path/image.jpg or https://s3.different-origin.com/some_bucket/path/image.jpg the request would fail because host and basepath did not match an allowed origin.

@Dynom
Copy link
Collaborator

Dynom commented Jul 1, 2019

Hi,

First thanks for your contribution!

A couple of considerations for this PR:

  1. It's missing test-coverage, I'll see if I can chip in on that.
  2. Configuration might require some additional legwork (for safety, one might want to be explicit regarding the ending /, for a situation regarding example.org/foo/ versus example.org/foobar.png). I think we can add something for that to parseOrigins
  3. It's worth noting that configuration becomes slightly more complicated in terms of collisions. Something we might want to add to the documentation (order is significant in the current configuration: *.example.org/,foo.example.org/assets is an easy mistake to make)

@Dynom Dynom self-assigned this Jul 1, 2019
Test contributions
@nicksrandall
Copy link
Contributor Author

nicksrandall commented Jul 1, 2019

@Dynom thanks for your help! I have merged your tests into this PR and they are all passing. Is there anything else I can do to help get this PR merged? I also added some documentation to README to help explain how this logic works.

@Dynom Dynom requested a review from h2non July 2, 2019 08:15
@nicksrandall
Copy link
Contributor Author

I’m not sure if this is automated or not but after this PR gets merged, would it be possible to push a new Docker image to Docker registry?

@nicksrandall
Copy link
Contributor Author

I don't intend to sound rude or impatient as I know that this is an open source project and the maintainers likely have day jobs. That said, would it be possible to estimate when this change might be able to land? I'm trying to plan a release around this feature.

@h2non h2non merged commit 687ef9b into h2non:master Jul 7, 2019
@h2non
Copy link
Owner

h2non commented Jul 7, 2019

New release v1.1.1 is now available. See:
https://github.com/h2non/imaginary/releases/tag/v1.1.1

@Dynom
Copy link
Collaborator

Dynom commented Jul 9, 2019

@nicksrandall no offence taken. I fully understand the frustration, but it's exactly as you said it.

The new Docker image is also available. Enjoy the release and thanks again for your contribution!

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.

None yet

3 participants