-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
Target restriction #111
base: master
Are you sure you want to change the base?
Target restriction #111
Conversation
* function `checkRateLimit` - If set, it is called with the origin (string) of the request. If this | ||
function returns a non-empty string, the request is rejected and the string is send to the client. | ||
* boolean `redirectSameOrigin` - If true, requests to URLs from the same origin will not be proxied but redirected. | ||
The primary purpose for this option is to save server resources by delegating the request to the client | ||
(since same-origin requests should always succeed, even without proxying). | ||
* array of strings `requireHeader` - If set, the request must include this header or the API will refuse to proxy. | ||
Recommended if you want to prevent users from using the proxy for normal browsing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please undo the removal of every two spaces at the end (
). These are necessary to force a line break. After your line the Example: ...
is not on a separate line any more.
res.end('The target "' + target + '" was not whitelisted by the operator of this proxy.'); | ||
return; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check can be bypassed if the whitelisted target site has an open redirect. A better place to enforce this is in the proxyRequest
function.
Locally I started with something like this (many months ago), but I never got to finish the implementation.
function proxyRequest(req, res, proxy) {
var location = req.corsAnywhereRequestState.location;
+ // TODO: Add something like this?
+ // For https://github.com/Rob--W/cors-anywhere/issues/67
+ if (req.corsAnywhereRequestState.checkRequestAllowed &&
+ !req.corsAnywhereRequestState.checkRequestAllowed(location)) {
+ res.writeHead(403, 'Forbidden', withCORS({}, req));
+ res.end('The requested resource was blocked by the operator of this proxy.');
+ return;
+ }
The idea behind this is that the validator can be as flexible as anyone wants to, with some default implementation in server.js
that simply looks at the environment variables and the prefix (like you're doing right now).
Matching by prefix is the simplest, but is it sufficient for most use cases? I recall another bug that wanted to match by "file extension".
.expect(403, done); | ||
}); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also add tests where the request to a whitelisted target is redirected. One test for redirection to a whitelisted target, and another test for redirection to a blacklisted target.
The server can now take a list of target servers (in the form of both whitelist and blacklist) it can serve.