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

feat: add prefix filtering and loop detection for local images #83

Merged
merged 3 commits into from
Oct 5, 2022

Conversation

ascorbic
Copy link

@ascorbic ascorbic commented Oct 5, 2022

Adds support for a localPrefix option, ensuring that local images can only be served from a specific path. Additionally, adds a header to prevent infinite loops where the source image matches the ipx server. This is done by adding an x-ipx-subrequest header to source image requests. If this header is detected then it means there's a request loop.

@ascorbic ascorbic requested a review from a team October 5, 2022 10:55
@github-actions github-actions bot added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Oct 5, 2022
@ascorbic ascorbic changed the title feat: add prefix filtering for local images feat: add prefix filtering and loop detection for local images Oct 5, 2022
Copy link

@nickytonline nickytonline left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple of questions.

return {
statusCode: 400,
body: 'Invalid source image path',
headers: plainText

Choose a reason for hiding this comment

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

I guess for all the spots with plain text headers we always want these responses as text, i.e. no scenario where it would make sense as JSON?

Copy link
Author

Choose a reason for hiding this comment

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

No, these are for humans

const isLocal = !id.startsWith('http://') && !id.startsWith('https://')
if (isLocal) {
const url = new URL(event.rawUrl)
url.pathname = id
if (localPrefix && !url.pathname.startsWith(localPrefix)) {

Choose a reason for hiding this comment

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

We're giving the option to pass in a localPrefix to IPX for scenarios like Next.js that have a dedicated folder for static images correct?

Copy link
Author

@ascorbic ascorbic Oct 5, 2022

Choose a reason for hiding this comment

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

Yes, exactly

Copy link

@ericapisani ericapisani left a comment

Choose a reason for hiding this comment

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

This looks like it should do the trick! 🚀

@kodiakhq kodiakhq bot merged commit f44db69 into main Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants