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

Errors are reported to the client instead of ignoring them. #24

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

penpendede
Copy link
Contributor

At our company, we have been using php-cross-domain-proxy quite intensively. In general it works great but when issues occur it does not always behave as expected. To give an example: It sometimes sends a 200 HTTP reply and an empty answer in cases when it cannot connect to the server hosting the data to be proxied.

When a valid answer can be empty this effectively and efficiently breaks client-side error detection. We therefore modified the proxy to improve error handling by making the proxy inform the client about what went wrong instead of silently discarding certain error conditions.

One more aspect: When developing a quite involved application that uses the proxy for accessing a number of sites it can easily happen that you try to access a site that is not yet whitelisted. To easily detect this we found it very helpful to have the proxy report a HTTP response of 403 that reports that the access was denied due to the site not being whitelisted. You immediately see the issue in the browser's developer tools and simply can open the link to see what precisely went wrong (i.e. which site caused the error).

As in our application we find this modification quite useful we suggest to make it

@softius
Copy link
Owner

softius commented Jul 22, 2016

Hi @penpendede,

If I got this correctly you need a way to get feedback in HTML format when errors occurred. How about log such incidents into an access / error file according to environment so that we keep the request / response intact?

Idea is to log everything (debug / info level) in a development environment and log minimal messages in Production env.

@ischas
Copy link
Contributor

ischas commented Jul 22, 2016

Let me answer this for @penpendede since he is a colleague of mine and has it's day off.

Our concern was not about the HTML, but the HTTP-status. The proposed change enables a pass-through of the HTTP-status of the proxied server-data. By now, the proxy doesn't give a proper HTTP-status when the proxied URL returns another HTTP-status but 200.

The second implemented feature (maybe this should have been done in another pull request) is that the proxy returns a message with HTTP-status 200 if the proxied URL isn't whitelisted. It now returns a more precise 403.

@softius
Copy link
Owner

softius commented Jul 22, 2016

That's clear @ischas . Going through your PRs I see many improvements which are welcomed but (as you have mentioned) I would like to introduce them as separated PRs. Hope that's ok with you.

Here is what I see:

  • Return 403 (Forbidden) when proxied URL isn't whitelisted
  • Return 503 (Service Unavailable) when cURL errors are occured
  • Adjust Content-Length for POST Requests
  • HTML Responses for errors

@ischas
Copy link
Contributor

ischas commented Jul 22, 2016

This is fine for me. Since we are in the middle of a new release of one of our products we will can come back to this in a couple of weeks.

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.

3 participants