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: validate external domains #343

Merged
merged 7 commits into from
Jun 29, 2021
Merged

feat: validate external domains #343

merged 7 commits into from
Jun 29, 2021

Conversation

farnabaz
Copy link
Member

Add logic to serve original src if image does not serve from listed domains

Resolves #342

@codecov-commenter
Copy link

codecov-commenter commented Jun 28, 2021

Codecov Report

Merging #343 (f117bd7) into main (4711fee) will decrease coverage by 0.06%.
The diff coverage is 44.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #343      +/-   ##
==========================================
- Coverage   59.36%   59.30%   -0.07%     
==========================================
  Files          26       26              
  Lines         598      602       +4     
  Branches      188      188              
==========================================
+ Hits          355      357       +2     
- Misses        243      245       +2     
Impacted Files Coverage Δ
src/provider.ts 74.28% <ø> (+2.06%) ⬆️
src/runtime/providers/vercel.ts 0.00% <0.00%> (ø)
src/runtime/image.ts 66.37% <25.00%> (-1.52%) ⬇️
src/module.ts 92.85% <50.00%> (-3.30%) ⬇️
src/runtime/providers/ipx.ts 100.00% <100.00%> (+15.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4711fee...f117bd7. Read the comment docs.

@pi0
Copy link
Member

pi0 commented Jun 28, 2021

Thanks for pull-request @farnabaz. I think we shall move this validation out of provider implementation and always validate to externalize URLs when domains option is provided and current URL is not matching.

@atinux
Copy link
Member

atinux commented Jun 28, 2021

On which file/function can he implement the validation @pi0 ?

@pi0
Copy link
Member

pi0 commented Jun 28, 2021

I think here we can add the hostname check:

@farnabaz
Copy link
Member Author

I moved domain validation into resolveImage helper and removed logic from both ipx and vercel providers.

Does it requires other changes @pi0?

@pi0
Copy link
Member

pi0 commented Jun 29, 2021

Changes look good @farnabaz 👌 I'm adding a change to ipx using hostname to use unified method and avoid startsWith check for vercel

pi0 added a commit to unjs/ipx that referenced this pull request Jun 29, 2021
@pi0
Copy link
Member

pi0 commented Jun 29, 2021

  • Updated ipx with hostname check
  • Domains are normalized in module
  • Only do external check if provider exports validateDomains flag enabled for ipx and vercel. This is to avoid breaking prismic and storyblok as they accept full URLs (will probably add a flag for this incompatible providers instead later for now flag is undocumented)

@pi0 pi0 changed the title fix(vercel): validate remote domains feat: validate external domains Jun 29, 2021
@pi0 pi0 merged commit bee0040 into main Jun 29, 2021
@pi0 pi0 deleted the fix/vercel-validate-domains branch June 29, 2021 16:38
procrates pushed a commit to procrates/nuxt-image that referenced this pull request Feb 21, 2023
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.

Vercel Provider does not respect domains list
4 participants