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

Delegate Content-Type verification solely to contentTypeMatches() #252

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

blakestoddard
Copy link
Contributor

@blakestoddard blakestoddard commented Oct 16, 2020

Image content returned from the initial request will have its Content-Type verified at https://github.com/willnorris/imageproxy/blob/main/imageproxy.go#L243. Prefilling the Accept header with the list of accepted Content-Type's has proven troublesome for some of our customers as some web servers have odd behaviors like:

  • returning an error if you provide a list of Content-Types in Accept
  • returning an error if you provide a Content-Type in Accept that the server does not know about (like image, which is not valid normally, but some web servers return content using that Content-Type 🙃)

The addition of Accept-Language is similar -- we've found some servers that will return an error if no Accept-Language header is supplied. Wack a mole!

Some customer-proxied files have been hosted on servers that will kick back errors if we a) provide a list of Accept'ed Content-Types, or b) provide a Content-Type in the Accept list that the server does not know about.
Some financial institutions will return an error via text/html if a request is made without an Accept-Language header.
@codecov
Copy link

codecov bot commented Oct 16, 2020

Codecov Report

Merging #252 (6ed3117) into main (c08b3c5) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #252   +/-   ##
=======================================
  Coverage   87.27%   87.27%           
=======================================
  Files           6        6           
  Lines         503      503           
=======================================
  Hits          439      439           
  Misses         36       36           
  Partials       28       28           
Impacted Files Coverage Δ
imageproxy.go 79.79% <100.00%> (ø)

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 c08b3c5...6ed3117. Read the comment docs.

@blakestoddard
Copy link
Contributor Author

Whoops, forgot that there was a PR for this branch. I'll pull that last commit into a different branch -- we're just disabling SVG all together since we can't properly sanitize it and security researchers keep using it to submit reports.

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.

None yet

1 participant